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

[Julien Grall <julien@xxxxxxx>]

On 21/10/2024 13:40, Ayan Kumar Halder wrote:
> On 18/10/2024 22:59, Julien Grall wrote:
> > > 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.
But we are going to get some and most likely a mpu/mm.c, right? If so, I 
don't see why we can't create one right now. Anyway...
> Also, I am not sure within which > function in mm.c, should this 
change be incorporated.
in arm/mm.c, I could create a new function:

static void __init __maybe_unused build_assertions(void)

> 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 ?
I would say no. The BUILD_BUG_ON() is suited here.

> > >       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")
Yes.

Cheers,

--
Julien Grall