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月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. 
I also prefer this way. Frankly, I really think FIXMAP is awkward in MPU
System.

> >
> > But in MPU systems, we can't map physical address to any virtual
> > address. So we want the code that is using FIXMAP_ADDR to return
> > the input physical address in MPU systems. So in MPU version,
> > FIXMAP_ADDR will trim physical address to PAGE alignment. This
> > will return an offset which is similar to MMU version FIXMAP_ADDR.
> > But it's a physical offset got from input physical address, plus
> > to an offset inside page (which is also got from physical address
> > mask with PAGE_MASK). The caller can return the input physical
> > address directly.
> >
> > As pmap depends on FIXAMP, so we disable pmap for Arm with MPU
> > enabled systems.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   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.

> 
> >     select IOMMU_FORCE_PT_SHARE
> >
> >   config ARCH_DEFCONFIG
> > diff --git a/xen/arch/arm/include/asm/config_mpu.h
> b/xen/arch/arm/include/asm/config_mpu.h
> > index 530abb8302..eee60dcffc 100644
> > --- a/xen/arch/arm/include/asm/config_mpu.h
> > +++ b/xen/arch/arm/include/asm/config_mpu.h
> > @@ -24,4 +24,6 @@
> >
> >   #define HYPERVISOR_VIRT_START  XEN_VIRT_START
> >
> > +#define FIXMAP_ADDR(n)         (_AT(paddr_t, n) & (PAGE_MASK))
> > +
> >   #endif /* __ARM_CONFIG_MPU_H__ */
> > diff --git a/xen/arch/arm/include/asm/fixmap.h
> b/xen/arch/arm/include/asm/fixmap.h
> > index d0c9a52c8c..1e338759e9 100644
> > --- a/xen/arch/arm/include/asm/fixmap.h
> > +++ b/xen/arch/arm/include/asm/fixmap.h
> > @@ -7,6 +7,8 @@
> >   #include <xen/acpi.h>
> >   #include <xen/pmap.h>
> >
> > +#ifndef CONFIG_HAS_MPU
> > +
> >   /* Fixmap slots */
> >   #define FIXMAP_CONSOLE  0  /* The primary UART */
> >   #define FIXMAP_MISC     1  /* Ephemeral mappings of hardware */
> > @@ -45,4 +47,27 @@ static inline unsigned int virt_to_fix(vaddr_t vaddr)
> >
> >   #endif /* __ASSEMBLY__ */
> >
> > +#else
> > +
> > +/*
> > + * FIXMAP_ADDR will trim physical address to PAGE alignment.
> > + * This will return an offset which is similar to MMU version
> > + * FIXMAP_ADDR.
> > + * For example:
> > + * EARLY_UART_VIRTUAL_ADDRESS is defined by:
> > + *     (FIXMAP_ADDR(FIXMAP_CONSOLE) + \
> > + *     (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
> > + * With MPU version FIXMAP_CONSOLE and FIXMAP_ADDR definitions,
> > + * EARLY_UART_VIRTUAL_ADDRESS can be restore to
> > + * CONFIG_EARLY_UART_BASE_ADDRESS.
> > + * In this case, we don't need to use #ifdef MPU in the code
> > + * where are using FIXMAP_ADDR to make them to use physical
> > + * address explicitily.
> > + */
> > +#ifdef CONFIG_EARLY_UART_BASE_ADDRESS
> > +#define FIXMAP_CONSOLE         CONFIG_EARLY_UART_BASE_ADDRESS
> > +#endif
> > +
> > +#endif /* CONFIG_HAS_MPU */
> > +
> >   #endif /* __ASM_FIXMAP_H */
> 
> Cheers,
> 
> --
> Julien Grall