|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU
Hi, On 24/10/2024 09:02, Ayan Kumar Halder wrote: On 23/10/2024 17:30, Julien Grall wrote:On 23/10/2024 17:18, Julien Grall wrote:On 23/10/2024 17:13, Julien Grall wrote:On 23/10/2024 17:06, Ayan Kumar Halder wrote:Hi Luca/Julien, On 22/10/2024 17:31, Luca Fancellu wrote:Hi Julien,Uhm… I’m not sure we are on the same page, probably I explained that wrongly, so my point is that:On 22 Oct 2024, at 14:13, Julien Grall <julien@xxxxxxx> wrote: On 22/10/2024 10:56, Luca Fancellu wrote:I’m afraid this is only my deduction, Section C1.4 of the Arm® Architecture Reference Manual Supplement Armv8, for R-profile AArch64 architecture, (DDI 0600B.a) explains what is the background region, it says it implements physical address range(s), so when we disable it, we would like any data access to complete before changing this implementation defined range with the ranges defined by us tweaking PRBAR/PRLAR, am I right?On 22 Oct 2024, at 10:47, Julien Grall <julien@xxxxxxx> wrote: Hi Luca, On 22/10/2024 10:41, Luca Fancellu wrote:TBH, I don't understand this one. Why would disabling the background region requires a dsb + isb? Do you have any pointer in the Armv8-R specification?2) dsb+isb barrier after enabling the MPU, since we are enabling the MPU but also because we are disabling the background regionOn 21 Oct 2024, at 17:27, Julien Grall <julien@xxxxxxx> wrote:My mental model for the ordering is similar to a TLB flush which is: 1/ dsb nsh 2/ tlbi 3/ dsb nsh 4/ isbEnabling the MPU is effectively 2. AFAIK, 1 is only necessary to ensure the update to the page-tables. But it is not a requirement to ensure any data access are completed (otherwise, we would have needed a dsb sy because we don't know how far the access are going...).FUNC_LOCAL(enable_mpu) mrs x0, SCTLR_EL2bic x0, x0, #SCTLR_ELx_BR /* Disable Background region */orr x0, x0, #SCTLR_Axx_ELx_M /* Enable MPU */ orr x0, x0, #SCTLR_Axx_ELx_C /* Enable D-cache */ orr x0, x0, #SCTLR_Axx_ELx_WXN /* Enable WXN */ dsb sy^— This data barrier is needed in order to complete any data access, which guarantees that all explicit memory accesses before this instruction complete, so we can safely turn ON the MPU and disable the background region.I thinkSorry I fat fingered and pressed send too early. I think this is the part I disagree with. All explicit accesses don't need to be complete (in the sense observed by everyone in the system). They only need to have gone through the permissions check.I think I managed to find again the wording that would justify why a "dsb" is not necessary for the permission checks. From ARM DDI 0487K.a D23-7349:```Direct writes using the instructions in Table D22-2 require synchronization before software can rely on the effects of changes to the System registers to affect instructions appearing in program order after the direct write to the System register. Direct writes to these registers are not allowed to affect any instructions appearing in program orderbefore the direct write. ```I understand the paragraph as enabling the MPU via SCTLR_EL2 will not affect any instruction before hand.Yes, I agree. However, as the line states"Direct writes using the instructions in Table D22-2 require synchronization before software can rely on the effects"This means synchronization is required after the write to SCTLR_EL2. However, this synchronization seems to imply dsb sy + isb. FromARM DDI 0487K.a ID032224 B2-274"A DSB instruction ordered after a direct write to a System PMU register does not complete until all observers observe the direct write"So, a write to SCTLR_EL2 needs to be followed by a dsb to ensure the write is observed on all the processors (as SCTLR_EL2 and PMU registers belong to Table D22-2, so the behavior should be same). Aside what Luca already said, I don't understand why the write to SCTLR_EL2 needs to be seen by all the processors (which BTW would imply "ish" rather than "sy"). The MPU and SCTLR are per-processor. And then it needs to be followed by an ISB to ensure any instruction fetch observes that MPU is enabled.So the code needs to be FUNC_LOCAL(enable_mpu) mrs x0, SCTLR_EL2 bic x0, x0, #SCTLR_ELx_BR /* Disable Background region */ orr x0, x0, #SCTLR_Axx_ELx_M /* Enable MPU */ orr x0, x0, #SCTLR_Axx_ELx_C /* Enable D-cache */ orr x0, x0, #SCTLR_Axx_ELx_WXN /* Enable WXN */ msr SCTLR_EL2, x0dsb sy /* ensure the writes to SCTLR_EL2 are observed on all the processors */ isb /* any instruction fetch observes that MPU is enabled. So force flush the pre-existing instruction pipeline */ret END(enable_mpu)This will be similar to what Zephyr does https://github.com/ zephyrproject-rtos/zephyr/blob/a30270668d4b90bac932794ef75df12a2b6f6f78/ arch/arm/core/mpu/arm_mpu.c#L258 . I guess you mean https://github.com/zephyrproject-rtos/zephyr/blob/a30270668d4b90bac932794ef75df12a2b6f6f78/arch/arm/core/mpu/arm_mpu.c#L223? But the reasoning there is seem quite different. AFAIU The "dsb" is to ensure that the changes to the MPU registers are visible. That said, I am not convinced that the "dsb" belongs after. Even if "SCTLR_EL2" may not be fully synchronized until the next "isb", it may happen before and therefore you want the MPU registers to be visible before SCTLR_EL2 is visible. Let me know if you are ok with the rationale. See above. > > Also, I would prefer to have 3 orr instructions instead of one for the sake of readability. However, this is not a strong preference so if you feel otherwise, I can change to have a single orr instruction. I am ok with that. Cheers, -- Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |