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

[Julien Grall <julien@xxxxxxx>]

On 21/10/2024 17:24, Luca Fancellu wrote:
> Hi Ayan,
>
> > > > + */
> > > > +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 */
> > > NIT: Can't we have a single "orr" instruction to set all the flags?
> > Yes, I will change this.
> > > > +    dsb   sy
> > > I still question this use of "dsb sy"...
> > Actually I kept this to ensure that all outstanding memory access are completed 
> > before MPU is enabled.
> >
> > However, prepare_xen_region() is invoked before this and it has a 'dsb sy' at 
> > the end.
> >
> > So we can drop this barrier.
> I suggest to keep the barrier here and drop the one in prepare_xen_region 
> instead,
I think the barriers in prepare_xen_region() should be kept because we 
may want to use the helper *after* the MPU is turned on.
> have a look in my branch: 
> https://gitlab.com/xen-project/xen/-/merge_requests/7/diffs?commit_id=f42a2816f9bd95f2f6957887be910cb9acd140b5
>
> During my testing I was having trouble without this barrier.
Was it before or after removing the barriers in prepare_xen_region()? If 
the latter, then I am a bit puzzled why you need it. Did you investigate 
a bit more?
Cheers,

--
Julien Grall