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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien,

On 24/09/2024 17:44, Julien Grall wrote:
> On 23/09/2024 12:22, Ayan Kumar Halder wrote:
> > On 19/09/2024 13:42, Julien Grall wrote:
> > > Hi Ayan,
> > Hi Julien,
> > > On 18/09/2024 19:51, 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.
> > > It would suggest to add some explanation why you want the start 
> > > address to be 4KB aligned.
> > Let me know if this makes sense.
> >
> > "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 4 KB 
> > (ie page size) aligned."
> > Thinking about this a bit more (and based on the discussion in "Re: 
> > [PATCH v1 2/4] xen/arm: mpu: Define Xen start address for MPU 
> > systems"), I think we can keep the 4 KB restriction for now and relax 
> > it later. Let me know what you think.
> I am fine with that.
>
> > > > 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.
> > > >
> > > >   xen/arch/arm/Kconfig                  | 10 ++++++++++
> > > >   xen/arch/arm/include/asm/config.h     |  4 +++-
> > > >   xen/arch/arm/include/asm/mpu/layout.h | 27 
> > > > +++++++++++++++++++++++++++
> > > Looking at this patch again, I don't see any modification in 
> > > xen.lds.S. Is it intended?
> > If we agree with the justification provided before, then this should 
> > be the modification.
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -232,6 +232,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")
> > +
> Ah I wasn't asking to add an ASSERT (although it would be good so long 
> it is protected by CONFIG_MPU). Instead I was expecting XEN_VIRT_START 
> to be replaced. But looking at the code again, I see you define
> XEN_VIRT_START for the MPU.
>
> I think this is a little bit odd to use XEN_VIRT_* defined for the MPU 
> but it would be ok. Is the expectation that all the MPU code should be 
> using XEN_START_ADDRESS? If so, maybe it would be worth add a comment 
> on top of XEN_VIRT_ADDRESS.
I have defined XEN_VIRT_START as XEN_START_ADDRESS , so that the code 
continues to use XEN_VIRT_START (as it does for the MMU) and we avoid 
introducing #if-defs.
Does this comment make sense ?

/*
 * 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)

- Ayan

> Cheers,