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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Julien,

> On 21 Oct 2024, at 17:27, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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.

Sure I agree, given that the current code was only called before enabling the 
MPU I was ok to drop the barrier in prepare_xen_region,
but given that the macro could be reused, it’s safer to keep them both.

> 
>> 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?

I don’t recall unfortunately, I tried to reproduce the issue removing the 
barrier from enable_mpu and adding it into prepare_xen_region only but it’s 
working fine
or at least I’m not able to reproduce the issue I was having, of course I 
wouldn’t remove it from both since it goes against the arm arm, so I think the 
logic
here should be keeping both the barriers:

1) dsb+isb barrier after writing prbar/prlar as the arm arm recommends (in case 
the macro is used with MPU enabled in the future)
2) dsb+isb barrier after enabling the MPU, since we are enabling the MPU but 
also because we are disabling the background region

Cheers,
Luca