RE: [PATCH v6 07/11] xen/arm: implement FIXMAP_ADDR for MPU systems

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2022年11月10日 2:30
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand
> Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v6 07/11] xen/arm: implement FIXMAP_ADDR for MPU
> systems
> 
> 
> 
> On 09/11/2022 06:46, Wei Chen wrote:
> > Hi Julien,
> 
> Hi Wei,
> 
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: 2022年11月7日 3:45
> >> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> Bertrand
> >> Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@xxxxxxxx>
> >> Subject: Re: [PATCH v6 07/11] xen/arm: implement FIXMAP_ADDR for MPU
> >> systems
> >>
> >> Hi Wei,
> >>
> >> On 04/11/2022 10:07, Wei Chen wrote:
> >>> FIXMAP is a special virtual address section for Xen to map some
> >>> physical ram or device memory temporarily in initialization for
> >>> MMU systems. FIXMAP_ADDR will return a virtual address by index
> >>> for special purpose phys-to-virt mapping usage. For example,
> >>> FIXMAP_ADDR(FIXMAP_CONSOLE) for early console mapping and
> >>> FIXMAP_ADDR(FIXMAP_MISC) for copy_from_paddr.
> >>
> >> To me, we are bending quite a bit the definition of the fixmap. There
> >> are not many use of the FIXMAP within the code and I think it would
> >> simply be better to abstract the use (or removing it when possible) and
> >> avoid defining FIXMAP_ADDR() & co for MPU.
> >>
> >
> > I agree, if we don't mind to add some CONFIG_HAS_MPU in some generic
> code.
> 
> FAOD, this is not what I had in mind. Instead, it was to provide helper
> which for !HAS_MPU would call fixmap and for HAS_MPU would do the work
> to map the region in the MPU.
> 

Sorry, I am still confused about this comment, did you mean we can provider
Some generic helpers like: early_map_console / eary_map_guest_memory.
For non-MPU system, we still can call fixmap in these callers, but for
MPU system, we have to map the region to MPU region?

> [...]
> 
> >>>    xen/arch/arm/Kconfig                  |  2 +-
> >>>    xen/arch/arm/include/asm/config_mpu.h |  2 ++
> >>>    xen/arch/arm/include/asm/fixmap.h     | 25
> +++++++++++++++++++++++++
> >>>    3 files changed, 28 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> >>> index ac276307d6..1458ffa777 100644
> >>> --- a/xen/arch/arm/Kconfig
> >>> +++ b/xen/arch/arm/Kconfig
> >>> @@ -16,7 +16,7 @@ config ARM
> >>>           select HAS_DEVICE_TREE
> >>>           select HAS_PASSTHROUGH
> >>>           select HAS_PDX
> >>> - select HAS_PMAP
> >>> + select HAS_PMAP if !HAS_MPU
> >>
> >> I can't find any change of mm.c in this series. So surely this will
> >> break the build?
> >
> > Yes, in our internal testing, open PMAP for MPU will cause building
> > failed, except we add some new stubs for MPU system.
> 
> Do you mean you added some stubs for PMAP? If so, I would not expect any
> caller for the pmap() to be used for the MPU. Therefore, why would they
> be necessary?
> 

No, I mean if we want to make pmap can be built successfully for MPU,
we have to implement some stubs like: fix_to_virt, xen_fixmap and
write_pte, to make compiling success. But just as you said, we would
not expect MPU to use any PMAP function, so we have not implemented
them for MPU. Instead we disable PMAP for MPU. 

Cheer,
Wei Chen

> Cheers,
> 
> --
> Julien Grall