|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 3/4] xen/arm: mpu: Create boot-time MPU protection regions
Hi, On 04/09/2024 13:21, Ayan Kumar Halder wrote: On 28/08/2024 16:01, Julien Grall wrote:On MPU systems, XEN_START_ADDRESS is also the + * address that Xen image should be loaded at. So for initial MPU + * regions setup, we use 2MB for Xen data memory to setup boot + * region, or the create boot regions code below will need adjustment. + */ +#define XEN_START_MEM_SIZE 0x200000See above.Ack.+ +/* + * In boot stage, we will use 1 MPU region: + * Region#0: Normal memory for Xen text + data + bss (2MB) + */ +#define BOOT_NORMAL_REGION_IDX 0x0 + +/* MPU normal memory attributes. */ +#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */ +#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */ + +.macro write_pr, sel, prbar, prlar + msr PRSELR_EL2, \sel + dsb syI am not sure I understand why this is a dsb rather than isb. Can you clarify?ISB is not needed here as the memory protection hasn't been activated yet. The above instruction just selects the memory region and the below two instructions sets the base address and limit for that memory region. After the three instructions, we need an ISB so that the memory protection takes into affect for further instruction fetches. We discussed this on Matrix, I have also had a read of the Arm Arm again (D19.1.2 General behavior of accesses to the AArch64 System registers in ARM DDI 0487J.a). Writing down mainly for keeping track of the decision. In general you need an a context synchronization event (e.g. isb) before the other system registers can start to rely on the change. There are some exceptions though, but AFAICT PRSELR_EL2 doesn't have any. IIUC, the behavior of PRBAR_EL2 and PRLAR_EL2 will change depending on PRSELR_EL2.REGION. Therefore, we need to make sure the new value of PRSELR_EL2 is seen before we write to the two other registers. However, a DSB is needed here as the below two instructions depend on this. So, we definitely want this instruction to complete.Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1 "Protection region attributes"0. ```Writes to MPU registers are only guaranteed to be visible following a Context synchronization event and DSB operation.``` Thus, I infer that DSB is necessary here I read it differently. To me it was more the "dsb" is necessary before the MPU can start using the new region values. But here an 'isb' should be fine. That said, from my experience, certain section of the Arm Arm can be interpreted differently whic means that someone could implement your way. This is also not a hot path, so best to use the most restrictive approach. Therefore I am ok with adding a 'dsb'. If a "dsb" is necessary, "sy" seems to be quite strict.I can use "st" here as I am only interested in stores (ie MSR) to complete.Now the whether we want to restrict it to inner shareable/outer shareable/POU/full system is a difficult decision to make.May be here we can use ishst (stores to complete to inner shareable region). However ....+ msr PRBAR_EL2, \prbar + msr PRLAR_EL2, \prlar + dsb syThis should be visible to outer shareable domain atleast. The reason being one can use the SH[1:0] bits in PRBAR_EL2 to set the region to outer shareable.Thus, the writes to these registers should be visible to outer shareable domain as well. I am a bit confused. SH[1:0] is about how the region will be accessed not up to where should registers are visible. I was expecting that the MPU registers only need to be visible to the MPU itself. For instance, when using the MMU, the translation unit is in the non-shareable domain. So a 'nsh' is sufficient regardless of the shareability of the page/block. This is explicitely written in the Arm Arm (see D5-4929 in ARM DDI 0487H.a) but I can't find a similar section for the MPU yet. Although, I would be a bit surprised if the MPU is not in the non-shareable domain... Maybe this could be clarified with Arm? Anyway, for now, I am open to use 'dsb sy' with a TODO to revisit it. + isb Re-quoting the spec from you previous answer: ``` Writes to MPU registers are only guaranteed to be visible following a Context synchronization event and DSB operation. ```So this suggests that it should be first an 'isb' and then a 'dsb'. Any reason you wrote it the other way around? +.endm + +.section .text.header, "ax", %progbits + +/* + * Static start-of-day EL2 MPU memory layout. + * + * It has a very simple structure, including: + * - 2MB normal memory mappings of xen at XEN_START_ADDRESS, which + * is the address where Xen was loaded by the bootloader. + */Please document a bit more the code and how the registers are used. For an example see the mmu/head.S code.Ack+ENTRY(enable_boot_cpu_mm) + /* Map Xen start memory to a normal memory region. */ + mov x0, #BOOT_NORMAL_REGION_IDX + ldr x1, =XEN_START_ADDRESS + and x1, x1, #MPU_REGION_MASK + mov x3, #PRBAR_NORMAL_MEM + orr x1, x1, x3 + + ldr x2, =XEN_START_ADDRESS + mov x3, #(XEN_START_MEM_SIZE - 1) + add x2, x2, x3 + and x2, x2, #MPU_REGION_MASK + mov x3, #PRLAR_NORMAL_MEM + orr x2, x2, x3 + + /* + * Write to MPU protection region: + * x0 for pr_sel, x1 for prbar x2 for prlar + */ + write_pr x0, x1, x2 + + ret +ENDPROC(enable_boot_cpu_mm)Missing emacs magic.You mean it should be END(enable_boot_cpu_mm) . /* * Local variables: * mode: ASM * indent-tabs-mode: nil * End: */ Correct. diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.hnew file mode 100644 index 0000000000..f5ebca8261 --- /dev/null +++ b/xen/arch/arm/include/asm/mpu/mm.hDo we need this file?In xen/arch/arm/arm64/mpu/head.S, we have #include <asm/mm.h> So, it should pick up this file. OOI, why can't you include asm/arm64/mm.h? Cheers, -- Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |