Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien,

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,
> > > > >
> > > > > > On 22 Oct 2024, at 14:13, Julien Grall <julien@xxxxxxx> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 22/10/2024 10:56, Luca Fancellu wrote:
> > > > > > > > On 22 Oct 2024, at 10:47, Julien Grall <julien@xxxxxxx> wrote:
> > > > > > > >
> > > > > > > > Hi Luca,
> > > > > > > >
> > > > > > > > On 22/10/2024 10:41, Luca Fancellu wrote:
> > > > > > > > > > On 21 Oct 2024, at 17:27, Julien Grall <julien@xxxxxxx> wrote:
> > > > > > > > > 2) dsb+isb barrier after enabling the MPU, since we are 
> > > > > > > > > enabling the MPU but also because we are disabling the 
> > > > > > > > > background region
> > > > > > > > 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?
> > > > > > > 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?
> > > > > > My mental model for the ordering is similar to a TLB flush which is:
> > > > > >    1/ dsb nsh
> > > > > >    2/ tlbi
> > > > > >    3/ dsb nsh
> > > > > >    4/ isb
> > > > > >
> > > > > > Enabling 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...).
> > > > > Uhm… I’m not sure we are on the same page, probably I explained 
> > > > > that wrongly, so my point is that:
> > > > > 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 */
> > > > >      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 think
> > Sorry 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 order
> before 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).
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, x0
    dsb   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 
.
Let me know if you are ok with the rationale.

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.
- Ayan