Re: [PATCH v3 24/52] xen/mpu: build up start-of-day Xen MPU memory region map

[Julien Grall <julien@xxxxxxx>]

Hi,

On 28/06/2023 11:55, Ayan Kumar Halder wrote:
> On 26/06/2023 04:34, Penny Zheng wrote:
> > CAUTION: This message has originated from an External Source. Please 
> > use proper judgment and caution when opening attachments, clicking 
> > links, or responding to this email.
> >
> > The start-of-day Xen MPU memory region layout shall be like
> > as follows:
> >
> > xen_mpumap[0] : Xen text
> > xen_mpumap[1] : Xen read-only data
> > xen_mpumap[2] : Xen read-only after init data
> > xen_mpumap[3] : Xen read-write data
> > xen_mpumap[4] : Xen BSS
> > xen_mpumap[5] : Xen init text
> > xen_mpumap[6] : Xen init data
> >
> > The layout shall be compliant with what we describe in xen.lds.S,
> > or the codes need adjustment.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> > v3:
> > - cache maintanence for safety when modifying MPU memory mapping table
> > - Hardcode index for all data/text sections
> > - To make sure that alternative instructions are included, use 
> > "_einitext"
> > as the start of the "Init data" section.
> > ---
> < snip>
> > +/*
> > + * Static start-of-day Xen EL2 MPU memory region layout:
> > + *
> > + *     xen_mpumap[0] : Xen text
> > + *     xen_mpumap[1] : Xen read-only data
> > + *     xen_mpumap[2] : Xen read-only after init data
> > + *     xen_mpumap[3] : Xen read-write data
> > + *     xen_mpumap[4] : Xen BSS
> > + *     xen_mpumap[5] : Xen init text
> > + *     xen_mpumap[6] : Xen init data
> > + *
> > + * Clobbers x0 - x6
> > + *
> > + * It shall be compliant with what describes in xen.lds.S, or the below
> > + * codes need adjustment.
> > + */
> > +ENTRY(prepare_early_mappings)
> > +    /* x0: region sel */
> > +    mov   x0, xzr
> > +    /* Xen text section. */
> > +    load_paddr x1, _stext
> > +    load_paddr x2, _etext
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6, 
> > attr_prbar=REGION_TEXT_PRBAR
> > +
> > +    add   x0, x0, #1
> > +    /* Xen read-only data section. */
> > +    load_paddr x1, _srodata
> > +    load_paddr x2, _erodata
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6, 
> > attr_prbar=REGION_RO_PRBAR
> > +
> > +    add   x0, x0, #1
> > +    /* Xen read-only after init data section. */
> > +    load_paddr x1, __ro_after_init_start
> > +    load_paddr x2, __ro_after_init_end
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6
> > +
> > +    add   x0, x0, #1
> > +    /* Xen read-write data section. */
> > +    load_paddr x1, __ro_after_init_end
> > +    load_paddr x2, __init_begin
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6
> > +
> > +    add   x0, x0, #1
> > +    /* Xen BSS section. */
> > +    load_paddr x1, __bss_start
> > +    load_paddr x2, __bss_end
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6
> > +
> > +    add   x0, x0, #1
> > +    /* Xen init text section. */
> > +    load_paddr x1, _sinittext
> > +    load_paddr x2, _einittext
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6, 
> > attr_prbar=REGION_TEXT_PRBAR
> > +
> > +    add   x0, x0, #1
> > +    /* Xen init data section. */
> > +    /*
> > +     * Even though we are not using alternative instructions in MPU yet,
> > +     * we want to use "_einitext" for the start of the "Init data" 
> > section
> > +     * to make sure they are included.
> > +     */
> > +    load_paddr x1, _einittext
> > +    roundup_section x1
> > +    load_paddr x2, __init_end
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, x6
> > +
> > +    /* Ensure any MPU memory mapping table updates made above have 
> > occurred. */
> > +    dsb   nshst
> > +    ret
> > +ENDPROC(prepare_early_mappings)
> Any reason why this is in assembly ?
I am not Penny. But from my understanding, in your approach, you will 
require to disable to switch the disable the MPU for using the new 
sections. While I agree...
> We have implemented it in C 
> https://github.com/Xilinx/xen/blob/d1258f1cefe406a3f91237b8106746c089864651/xen/arch/arm_mpu/mm_mpu.c#L941 , so that it can be common between R82 and R52.
... this means you can share the code. It also means:
  * You can't protect Xen properly from the start
  * You need to flush the cache (not great for performance)
  * You need to be more cautious as the MPU will be disabled for a 
short period of time.
In fact looking at your switch code in setup_protection_regions(), I am 
not convinced you can disable the MPU in C and then call 
set_boot_mpumap(). I think the enable/disable would need to be moved in 
the assembly function. There are potentially more issues.
So overall, I am not convinced of the C/common approach.

Cheers,

--
Julien Grall