Re: [PATCH v1 2/4] xen/arm: mpu: Define Xen start address for MPU systems

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 28/08/2024 15:49, Julien Grall wrote:
> Hi,
Hi Julien,
> On 23/08/2024 17:31, 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 in this patch, we reuse the existing arm/platforms to store
> > Armv8-R platforms' parameters. And `XEN_START_ADDRESS` is one
> > kind of MPU based platform's parameters. So we define default
> > `XEN_START_ADDRESS` in the MPU specific header file.
> This seems a left-over from as previous work?
Yes, this needs to be removed.
> > We also introduce one Kconfig option for users to override the
> > default Xen start address of selected platform, if they think
> > the default address doesn't suit their scenarios. For this
> > Kconfig option, we use an unaligned address "0xffffffff" as the
> > default value to indicate that users haven't used a customized
> > Xen start address.
> >
> > 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>
> > ---
> >   xen/arch/arm/Kconfig                  | 11 +++++++++++
> >   xen/arch/arm/include/asm/mpu/layout.h | 25 +++++++++++++++++++++++++
> >   2 files changed, 36 insertions(+)
> >   create mode 100644 xen/arch/arm/include/asm/mpu/layout.h
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index 686948cefd..d722ca690c 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -23,6 +23,17 @@ 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
> > +      This option is allowed to set customized address at which Xen 
> > will be
> > +      linked on MPU systems.
> I don't quite understand what you mean by "allowed".
I will rephrase this as ...

"Used to set customized address at which which Xen will be linked

on MPU systems. This address must be aligned to a page size.
0xFFFFFFFF is used as the default value to indicate that user hasn't
customized this address."

> >  This address must be aligned to a page size.> +      Use 0xFFFFFFFF 
> as the default value to indicate that user hasn't
> > +      customized this address, and Xen use use the default value 
> > that has
> > +      been defined in platform files.
> At least in this patch, there is no default set. Instead you will 
> throw an error.
Ack. See above.
> > +
> >   menu "Architecture Features"
> >     choice
> > 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..82a74b8b5b
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/mpu/layout.h
> > @@ -0,0 +1,25 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> Coding style: /* SPDX-... */
Ack
> > +/*
> > + * config_mpu.h: A Linux-style configuration list for Arm MPU systems,
> > + *               only can be included by config.h
> > + */
> I don't understand how this description match the content below. Also, 
> shouldn't you include it from xen/config.h?
My bad. This description is incorrect. I will remove the description as 
it is not needed at this point in time.
> > +
> > +#ifndef __ARM_CONFIG_MPU_H__
> > +#define __ARM_CONFIG_MPU_H__
> This doesn't match the name of the file.
__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 PAGE_SIZE"
> > +#endif
> > +
> > +#define XEN_VIRT_START         _AT(paddr_t, XEN_START_ADDRESS)
> > +
> > +#endif /* __ARM_CONFIG_MPU_H__ */
> > +
> Cheers,
- Ayan