Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Wed, 30 Oct 2024 at 09:17, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Ayan,
>
> While I rebased the branch on top of your patches, I saw you’ve changed the 
> number of regions
> mapped at boot time, can I ask why?

I have asked the change. If you look at the layout...

>
> Compared to 
> https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-Penny.Zheng@xxxxxxx/:


... you have two sections with the same permissions:

xen_mpumap[1] : Xen read-only data
xen_mpumap[2] : Xen read-only after init data
xen_mpumap[3] : Xen read-write data

During boot, [2] and [3] will share the same permissions. After boot,
this will be [1] and [2]. Given the number of MPU regions is limited,
this is a bit of a waste.

We also don't want to have a hole in the middle of Xen sections. So
folding seemed to be a good idea.

>
> > +FUNC(enable_boot_cpu_mm)
> > +
> > +    /* Get the number of regions specified in MPUIR_EL2 */
> > +    mrs   x5, MPUIR_EL2
> > +
> > +    /* x0: region sel */
> > +    mov   x0, xzr
> > +    /* Xen text section. */
> > +    ldr   x1, =_stext
> > +    ldr   x2, =_etext
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> > +
> > +    /* Xen read-only data section. */
> > +    ldr   x1, =_srodata
> > +    ldr   x2, =_erodata
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> > +
> > +    /* Xen read-only after init and data section. (RW data) */
> > +    ldr   x1, =__ro_after_init_start
> > +    ldr   x2, =__init_begin
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5
>
>          ^— this, for example, will block Xen to call init_done(void) later, 
> I understand this is earlyboot,
>                but I guess we don’t want to make subsequent changes to this 
> part when introducing the
>                patches to support start_xen()

Can you be a bit more descriptive... What will block?

>
> > +
> > +    /* Xen code section. */
> > +    ldr   x1, =__init_begin
> > +    ldr   x2, =__init_data_begin
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> > +
> > +    /* Xen data and BSS section. */
> > +    ldr   x1, =__init_data_begin
> > +    ldr   x2, =__bss_end
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5
> > +
> > +    ret
> > +
> > +END(enable_boot_cpu_mm)
>
> I suggest to keep exactly the regions as are in Penny’s patch.

See above. Without any details on the exact problem, it is difficult
to agree on your suggestion.

Cheers,