Re: [PATCH v3 3/6] xen/arm: mpu: Define Xen start address for MPU systems

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 18/10/2024 22:59, Julien Grall wrote:
> Hi,
Hi Julien,

Few clarifications.

> On 10/10/2024 15:03, Ayan Kumar Halder wrote:
> > From: Wei Chen <wei.chen@xxxxxxx>
> >
> > On Armv8-A, Xen has a fixed virtual start address (link address too) 
> > for all
> > Armv8-A platforms. In an MMU based system, Xen can map its loaded 
> > address to
> > this virtual start address. So, on Armv8-A platforms, the Xen start 
> > address does
> > not need to be configurable. But on Armv8-R platforms, there is no 
> > MMU to map
> > loaded address to a fixed virtual address and different platforms 
> > will have very
> > different address space layout. So Xen cannot use a fixed physical 
> > address on
> > MPU based system and need to have it configurable.
> >
> > So, we introduce a Kconfig option for users to set the start address. 
> > The start
> > address needs to be aligned to 4KB. We have a check for this alignment.
> >
> > MPU allows us to define regions which are 64 bits aligned. This 
> > restriction
> > comes from the bitfields of PRBAR, PRLAR (the lower 6 bits are 0 
> > extended to
> > provide the base and limit address of a region). This means that the 
> > start
> > address of Xen needs to be at least 64 bits aligned (as it will 
> > correspond to
> > the start address of memory protection region).
> >
> > As for now Xen on MPU tries to use the same memory alignment 
> > restrictions as it
> > has been for MMU. Unlike MMU where the starting virtual address is 
> > 2MB, Xen on
> > MPU needs the start address to be 4KB (ie page size) aligned.
> >
> > In case if the user forgets to set the start address, then 0xffffffff 
> > is used
> > as default. This is to trigger the error (on alignment check) and 
> > thereby prompt
> > user to set the start address.
> >
> > Also updated config.h so that it includes mpu/layout.h when 
> > CONFIG_MPU is
> > defined.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Jiamei.Xie <jiamei.xie@xxxxxxx>
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > Changes from :-
> >
> > v1 - 1. Fixed some of the coding style issues.
> > 2. Reworded the help message.
> > 3. Updated the commit message.
> >
> > v2 - Added clarification for the use of page and page size.
> >
> >   xen/arch/arm/Kconfig                  | 10 ++++++++
> >   xen/arch/arm/include/asm/config.h     |  4 +++-
> >   xen/arch/arm/include/asm/mpu/layout.h | 33 +++++++++++++++++++++++++++
> >   xen/arch/arm/setup.c                  |  8 +++++++
> >   xen/arch/arm/xen.lds.S                |  6 +++++
> >   5 files changed, 60 insertions(+), 1 deletion(-)
> >   create mode 100644 xen/arch/arm/include/asm/mpu/layout.h
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index ed92eb67cb..15b2e4a227 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -23,6 +23,16 @@ config ARCH_DEFCONFIG
> >       default "arch/arm/configs/arm32_defconfig" if ARM_32
> >       default "arch/arm/configs/arm64_defconfig" if ARM_64
> >   +config XEN_START_ADDRESS
> > +    hex "Xen start address: keep default to use platform defined 
> > address"
> > +    default 0xFFFFFFFF
> > +    depends on MPU
> > +    help
> > +      Used to set customized address at which which Xen will be 
> > linked on MPU
> > +      systems. Must be aligned to 4KB.
> > +      0xFFFFFFFF is used as default value to indicate that user has not
> > +      customized this address.
> > +
> >   menu "Architecture Features"
> >     choice
> > diff --git a/xen/arch/arm/include/asm/config.h 
> > b/xen/arch/arm/include/asm/config.h
> > index a2e22b659d..0a51142efd 100644
> > --- a/xen/arch/arm/include/asm/config.h
> > +++ b/xen/arch/arm/include/asm/config.h
> > @@ -69,8 +69,10 @@
> >   #include <xen/const.h>
> >   #include <xen/page-size.h>
> >   -#ifdef CONFIG_MMU
> > +#if defined(CONFIG_MMU)
> >   #include <asm/mmu/layout.h>
> > +#elif defined(CONFIG_MPU)
> > +#include <asm/mpu/layout.h>
> >   #else
> >   # error "Unknown memory management layout"
> >   #endif
> > diff --git a/xen/arch/arm/include/asm/mpu/layout.h 
> > b/xen/arch/arm/include/asm/mpu/layout.h
> > new file mode 100644
> > index 0000000000..d6d397f4c2
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/mpu/layout.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef __ARM_MPU_LAYOUT_H__
> > +#define __ARM_MPU_LAYOUT_H__
> > +
> > +#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS
> > +
> > +/*
> > + * All MPU platforms need to provide a XEN_START_ADDRESS for linker. 
> > This
> > + * address indicates where Xen image will be loaded and run from. This
> > + * address must be aligned to a PAGE_SIZE.
> > + */
> > +#if (XEN_START_ADDRESS % PAGE_SIZE) != 0
> > +#error "XEN_START_ADDRESS must be aligned to 4KB"
> > +#endif
> > +
> > +/*
> > + * For MPU, XEN's virtual start address is same as the physical 
> > address.
> > + * The reason being MPU treats VA == PA. IOW, it cannot map the 
> > physical
> > + * address to a different fixed virtual address. So, the virtual start
> > + * address is determined by the physical address at which Xen is 
> > loaded.
> > + */
> > +#define XEN_VIRT_START         _AT(paddr_t, XEN_START_ADDRESS)
> > +
> > +#endif /* __ARM_MPU_LAYOUT_H__ */
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 71ebaa77ca..0203771164 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -295,6 +295,14 @@ void asmlinkage __init start_xen(unsigned long 
> > fdt_paddr)
> >       struct domain *d;
> >       int rc, i;
> >   +#ifdef CONFIG_MPU
> > +    /*
> > +     * Unlike MMU, MPU does not use pages for translation. However, 
> > we continue
> > +     * to use PAGE_SIZE to denote 4KB. This is so that the existing 
> > memory
> > +     * management based on pages, continue to work for now.
> > +     */
> > +    BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
> > +#endif
> I think it would be better suited in mm.c or mpu/*.c.
We do not have mpu/*.c at the moment. Also, I am not sure within which 
function in mm.c, should this change be incorporated.
Can we take out this change from the current patch and put it in a later 
series when we support the lateboot of Xen (ie start_xen() onwards) for 
MPU ?
> >       dcache_line_bytes = read_dcache_line_bytes();
> >         percpu_init_areas();
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index bd884664ad..fe4b468cca 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -231,6 +231,12 @@ SECTIONS
> >    */
> >   ASSERT(_start == XEN_VIRT_START, "_start != XEN_VIRT_START")
> >   +/*
> > + * On MPU based platforms, the starting address is to be provided by 
> > user.
> > + * One need to check that it is 4KB aligned.
> > + */
> > +ASSERT(IS_ALIGNED(_start,       4096), "starting address is 
> > misaligned")
> > +
> Shouldn't this be protected with #ifdef CONFIG_MPU? 
yes
> Also, it would probably be useful to start the exact size in the error 
> message.
Do you mean ?

ASSERT(IS_ALIGNED(_start,       4096), "starting address should be 
aligned to 4KB")

- Ayan