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

[Julien Grall <julien@xxxxxxx>]

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.
We also need the change in the MPU registers to be visible to the MPU.

For instance, for the MMU, we only need to use "dsb nsh" because the 
walker is within the non-shareable domain. Personally, I think "sy" is a 
bit excessive here, but as this is boot code, that's ok.
Cheers,

--
Julien Grall