|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 24/52] xen/mpu: build up start-of-day Xen MPU memory region map
Hi Ayan, On 28/06/2023 14:22, Ayan Kumar Halder wrote: On 28/06/2023 12:17, Julien Grall wrote:Hi,Hi Julien,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 startYes, I see what you mean. Refer https://github.com/Xilinx/xen/blob/d1258f1cefe406a3f91237b8106746c089864651/xen/arch/arm_mpu/arm32/mpu_v8r.S#L82C7-L82C15 ,I am mapping CONFIG_XEN_START_ADDRESS + 2 MB. But, probably you mean individual sections should be properly mapped from the beginning.* 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.Yes, MPU is disabled when set_boot_mpumap() is invoked. But is this really a security issue ? My second point is not about security, it is about been compliant with the Arm Arm. To quote what you wrote: " To avoid unexpected unaligment access fault during MPU disabled, set_boot_mpumap shall be written in assembly code." What's the guarantee that the compiler will not generate any instructions that could generate an alignment fault? Furthermore, from my understanding, at least on Armv8-A, there are caching problem because you will need to save some registers (for the call to set_boot_mpumap()) on the stack with cache disabled. This means the cache will be bypassed. But you may then restore the registers with the cache enabled (the compiler could decide that it is not necessary to read the stack before hand). So you could read the wrong data if there is a stale cacheline. So overall, I think you want to tightly control the section where the MPU is off. This means writing the logic in assembly rather than C. I mean only a single core is running and it is executing Xen. The MPU is turned off for few cycles.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.disable_mpu(), enable_mpu() are written in assembly. See https://github.com/Xilinx/xen/blob/d1258f1cefe406a3f91237b8106746c089864651/xen/arch/arm_mpu/arm32/mpu_v8r.S#L128 Right, but then you return to C world to then call set_boot_mpumap(). So you have still some part in C even if it is small. Cheers, -- Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |