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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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 Julien agreed to this in a previous email as long as we have an 
appropriate comment. So we will keep this as it is.
- Ayan