Hi Julien, > On 21 Oct 2024, at 17:27, Julien Grall <jul...@xen.org> 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
