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

[Julien Grall]

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.zh...@arm.com>
Signed-off-by: Wei Chen <wei.c...@arm.com>
---
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