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

[Julien Grall <julien@xxxxxxx>]

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

> 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.
Sorry I didn't manage to answer yet. But yes, I am ok with the barrier 
for now, but I am not agree on the reasoning used.
Cheers,

--
Julien Grall