| Message ID | 20241010140351.309922-6-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable early bootup of AArch64 MPU systems | expand |
Hi Ayan, > /* > * Maps the various sections of Xen (described in xen.lds.S) as different MPU > * regions. > @@ -68,10 +92,11 @@ > * Inputs: > * lr : Address to return to. > * > - * Clobbers x0 - x5 > + * Clobbers x0 - x6 > * > */ > FUNC(enable_boot_cpu_mm) > + mov x6, lr > > /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ > mrs x5, MPUIR_EL2 > @@ -113,7 +138,7 @@ FUNC(enable_boot_cpu_mm) > beq 5f > prepare_xen_region x0, x1, x2, x3, x4, x5 > > -5: > +5: mov lr, x6 Shall these changes to enable_boot_cpu_mm be part of the previous commit? The rest looks good to me: Reviewed-by: Luca Fancellu <luca.fancelllu@arm.com>
On 14/10/2024 20:11, Luca Fancellu wrote: > Hi Ayan, Hi Luca, Sorry I missed something. > >> /* >> * Maps the various sections of Xen (described in xen.lds.S) as different MPU >> * regions. >> @@ -68,10 +92,11 @@ >> * Inputs: >> * lr : Address to return to. >> * >> - * Clobbers x0 - x5 >> + * Clobbers x0 - x6 >> * >> */ >> FUNC(enable_boot_cpu_mm) >> + mov x6, lr >> >> /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ >> mrs x5, MPUIR_EL2 >> @@ -113,7 +138,7 @@ FUNC(enable_boot_cpu_mm) >> beq 5f >> prepare_xen_region x0, x1, x2, x3, x4, x5 bl enable_mpu >> -5: >> +5: mov lr, x6 > Shall these changes to enable_boot_cpu_mm be part of the previous commit? This is why I had to save and restore the LR. - Ayan
Hi, On 10/10/2024 15:03, Ayan Kumar Halder wrote: > After the regions have been created, now we enable the MPU. For this we disable > the background region so that the new memory map created for the regions take > effect. Also, we treat all RW regions as non executable and the data cache is > enabled. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > Changes from :- > > v2 - 1. Extracted from the previous patch into a new one. > > 2. Disabled background region. > > xen/arch/arm/arm64/mpu/head.S | 29 ++++++++++++++++++-- > xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 3 ++ > 2 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S > index 4a21bc815c..e354f4552b 100644 > --- a/xen/arch/arm/arm64/mpu/head.S > +++ b/xen/arch/arm/arm64/mpu/head.S > @@ -61,6 +61,30 @@ > add \xb, \xb, x20 /* x20 - Phys offset */ > .endm > > +/* > + * Enable EL2 MPU and data cache > + * If the Background region is enabled, then the MPU uses the default memory > + * map as the Background region for generating the memory > + * attributes when MPU is disabled. > + * Since the default memory map of the Armv8-R AArch64 architecture is > + * IMPLEMENTATION DEFINED, we intend to turn off the Background region here. > + * > + * Clobbers x0 > + * > + */ > +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? > + dsb sy I still question this use of "dsb sy"... > + msr SCTLR_EL2, x0 > + isb > + > + ret > +END(enable_mpu) > + > /* > * Maps the various sections of Xen (described in xen.lds.S) as different MPU > * regions. > @@ -68,10 +92,11 @@ > * Inputs: > * lr : Address to return to. > * > - * Clobbers x0 - x5 > + * Clobbers x0 - x6 > * > */ > FUNC(enable_boot_cpu_mm) > + mov x6, lr > > /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ > mrs x5, MPUIR_EL2 > @@ -113,7 +138,7 @@ FUNC(enable_boot_cpu_mm) > beq 5f > prepare_xen_region x0, x1, x2, x3, x4, x5 > > -5: > +5: mov lr, x6 > ret > > fail: > diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h > index b0c31a58ec..3769d23c80 100644 > --- a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h > +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h > @@ -15,6 +15,9 @@ > /* MPU Protection Region Selection Register encode */ > #define PRSELR_EL2 S3_4_C6_C2_1 > > +/* Backgroud region enable/disable */ > +#define SCTLR_ELx_BR BIT(17, UL) > + > #endif /* __ASM_ARM_ARM64_MPU_SYSREGS_H */ > > /* Cheers,
On 18/10/2024 23:25, Julien Grall wrote: > Hi, Hi Julien, > > On 10/10/2024 15:03, Ayan Kumar Halder wrote: >> After the regions have been created, now we enable the MPU. For this >> we disable >> the background region so that the new memory map created for the >> regions take >> effect. Also, we treat all RW regions as non executable and the data >> cache is >> enabled. >> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> Changes from :- >> >> v2 - 1. Extracted from the previous patch into a new one. >> >> 2. Disabled background region. >> >> xen/arch/arm/arm64/mpu/head.S | 29 ++++++++++++++++++-- >> xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 3 ++ >> 2 files changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/mpu/head.S >> b/xen/arch/arm/arm64/mpu/head.S >> index 4a21bc815c..e354f4552b 100644 >> --- a/xen/arch/arm/arm64/mpu/head.S >> +++ b/xen/arch/arm/arm64/mpu/head.S >> @@ -61,6 +61,30 @@ >> add \xb, \xb, x20 /* x20 - Phys offset */ >> .endm >> +/* >> + * Enable EL2 MPU and data cache >> + * If the Background region is enabled, then the MPU uses the >> default memory >> + * map as the Background region for generating the memory >> + * attributes when MPU is disabled. >> + * Since the default memory map of the Armv8-R AArch64 architecture is >> + * IMPLEMENTATION DEFINED, we intend to turn off the Background >> region here. >> + * >> + * Clobbers x0 >> + * >> + */ >> +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. - Ayan > >> + msr SCTLR_EL2, x0 >> + isb >> + >> + ret >> +END(enable_mpu) >> + >> /* >> * Maps the various sections of Xen (described in xen.lds.S) as >> different MPU >> * regions. >> @@ -68,10 +92,11 @@ >> * Inputs: >> * lr : Address to return to. >> * >> - * Clobbers x0 - x5 >> + * Clobbers x0 - x6 >> * >> */ >> FUNC(enable_boot_cpu_mm) >> + mov x6, lr >> /* Check if the number of regions exceeded the count >> specified in MPUIR_EL2 */ >> mrs x5, MPUIR_EL2 >> @@ -113,7 +138,7 @@ FUNC(enable_boot_cpu_mm) >> beq 5f >> prepare_xen_region x0, x1, x2, x3, x4, x5 >> -5: >> +5: mov lr, x6 >> ret >> fail: >> diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h >> b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h >> index b0c31a58ec..3769d23c80 100644 >> --- a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h >> +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h >> @@ -15,6 +15,9 @@ >> /* MPU Protection Region Selection Register encode */ >> #define PRSELR_EL2 S3_4_C6_C2_1 >> +/* Backgroud region enable/disable */ >> +#define SCTLR_ELx_BR BIT(17, UL) >> + >> #endif /* __ASM_ARM_ARM64_MPU_SYSREGS_H */ >> /* > > Cheers, > >
Hi Ayan, On 21/10/2024 16:30, Ayan Kumar Halder wrote: > > On 18/10/2024 23:25, Julien Grall wrote: >> Hi, > Hi Julien, >> >> On 10/10/2024 15:03, Ayan Kumar Halder wrote: >>> After the regions have been created, now we enable the MPU. For this >>> we disable >>> the background region so that the new memory map created for the >>> regions take >>> effect. Also, we treat all RW regions as non executable and the data >>> cache is >>> enabled. >>> >>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>> --- >>> Changes from :- >>> >>> v2 - 1. Extracted from the previous patch into a new one. >>> >>> 2. Disabled background region. >>> >>> xen/arch/arm/arm64/mpu/head.S | 29 ++++++++++++++++++-- >>> xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 3 ++ >>> 2 files changed, 30 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/ >>> head.S >>> index 4a21bc815c..e354f4552b 100644 >>> --- a/xen/arch/arm/arm64/mpu/head.S >>> +++ b/xen/arch/arm/arm64/mpu/head.S >>> @@ -61,6 +61,30 @@ >>> add \xb, \xb, x20 /* x20 - Phys offset */ >>> .endm >>> +/* >>> + * Enable EL2 MPU and data cache >>> + * If the Background region is enabled, then the MPU uses the >>> default memory >>> + * map as the Background region for generating the memory >>> + * attributes when MPU is disabled. >>> + * Since the default memory map of the Armv8-R AArch64 architecture is >>> + * IMPLEMENTATION DEFINED, we intend to turn off the Background >>> region here. >>> + * >>> + * Clobbers x0 >>> + * >>> + */ >>> +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. I don't mind to keep it for this reason so long it is documented. Cheers,
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, 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. Cheers, Luca
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,
Hi Julien, > On 21 Oct 2024, at 17:27, Julien Grall <julien@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
Hi Luca, On 22/10/2024 10:41, Luca Fancellu wrote: >> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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? Cheers,
> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 22/10/2024 10:41, Luca Fancellu wrote: >>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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? > > Cheers, > > -- > Julien Grall >
On 22/10/2024 10:56, Luca Fancellu wrote: > > >> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >> >> Hi Luca, >> >> On 22/10/2024 10:41, Luca Fancellu wrote: >>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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...). Cheers,
Hi Julien, > On 22 Oct 2024, at 14:13, Julien Grall <julien@xen.org> wrote: > > > > On 22/10/2024 10:56, Luca Fancellu wrote: >>> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Luca, >>> >>> On 22/10/2024 10:41, Luca Fancellu wrote: >>>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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. msr SCTLR_EL2, x0 isb ret END(enable_mpu) I didn’t understand if your point is that it is not needed or if it is needed but in a different location. > > Cheers, > > -- > Julien Grall >
Hi Luca/Julien, On 22/10/2024 17:31, Luca Fancellu wrote: > Hi Julien, > >> On 22 Oct 2024, at 14:13, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 22/10/2024 10:56, Luca Fancellu wrote: >>>> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Luca, >>>> >>>> On 22/10/2024 10:41, Luca Fancellu wrote: >>>>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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
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@xen.org> wrote: >>> >>> >>> >>> On 22/10/2024 10:56, Luca Fancellu wrote: >>>>> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> Hi Luca, >>>>> >>>>> On 22/10/2024 10:41, Luca Fancellu wrote: >>>>>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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,
On 23/10/2024 17:13, Julien Grall wrote: > > > 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@xen.org> wrote: >>>> >>>> >>>> >>>> On 22/10/2024 10:56, Luca Fancellu wrote: >>>>>> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >>>>>> >>>>>> Hi Luca, >>>>>> >>>>>> On 22/10/2024 10:41, Luca Fancellu wrote: >>>>>>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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 Sorry I fat fingered and pressed send too early. I think this is the part I disagree with. All explicit accesses don't need to be complete (in the sense observed by everyone in the system). They only need to have gone through the permissions check. We also need the change in the MPU registers to be visible to the MPU. For instance, for the MMU, we only need to use "dsb nsh" because the walker is within the non-shareable domain. Personally, I think "sy" is a bit excessive here, but as this is boot code, that's ok. Cheers,
On 23/10/2024 17:18, Julien Grall wrote: > > > On 23/10/2024 17:13, Julien Grall wrote: >> >> >> 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@xen.org> wrote: >>>>> >>>>> >>>>> >>>>> On 22/10/2024 10:56, Luca Fancellu wrote: >>>>>>> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >>>>>>> >>>>>>> Hi Luca, >>>>>>> >>>>>>> On 22/10/2024 10:41, Luca Fancellu wrote: >>>>>>>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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 > > Sorry I fat fingered and pressed send too early. I think this is the > part I disagree with. All explicit accesses don't need to be complete > (in the sense observed by everyone in the system). They only need to > have gone through the permissions check. I think I managed to find again the wording that would justify why a "dsb" is not necessary for the permission checks. From ARM DDI 0487K.a D23-7349: ``` Direct writes using the instructions in Table D22-2 require synchronization before software can rely on the effects of changes to the System registers to affect instructions appearing in program order after the direct write to the System register. Direct writes to these registers are not allowed to affect any instructions appearing in program order before the direct write. ``` I understand the paragraph as enabling the MPU via SCTLR_EL2 will not affect any instruction before hand. Cheers,
Hi Julien, On 23/10/2024 17:30, Julien Grall wrote: > > > On 23/10/2024 17:18, Julien Grall wrote: >> >> >> On 23/10/2024 17:13, Julien Grall wrote: >>> >>> >>> 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@xen.org> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 22/10/2024 10:56, Luca Fancellu wrote: >>>>>>>> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >>>>>>>> >>>>>>>> Hi Luca, >>>>>>>> >>>>>>>> On 22/10/2024 10:41, Luca Fancellu wrote: >>>>>>>>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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 >> >> Sorry I fat fingered and pressed send too early. I think this is the >> part I disagree with. All explicit accesses don't need to be complete >> (in the sense observed by everyone in the system). They only need to >> have gone through the permissions check. > > I think I managed to find again the wording that would justify why a > "dsb" is not necessary for the permission checks. From ARM DDI 0487K.a > D23-7349: > > ``` > Direct writes using the instructions in Table D22-2 require > synchronization before software can rely on the effects > of changes to the System registers to affect instructions appearing in > program order after the direct write to the > System register. Direct writes to these registers are not allowed to > affect any instructions appearing in program order > before the direct write. > ``` > > I understand the paragraph as enabling the MPU via SCTLR_EL2 will not > affect any instruction before hand. Yes, I agree. However, as the line states "Direct writes using the instructions in Table D22-2 require synchronization before software can rely on the effects" This means synchronization is required after the write to SCTLR_EL2. However, this synchronization seems to imply dsb sy + isb. FromARM DDI 0487K.a ID032224 B2-274 "A DSB instruction ordered after a direct write to a System PMU register does not complete until all observers observe the direct write" So, a write to SCTLR_EL2 needs to be followed by a dsb to ensure the write is observed on all the processors (as SCTLR_EL2 and PMU registers belong to Table D22-2, so the behavior should be same). And then it needs to be followed by an ISB to ensure any instruction fetch observes that MPU is enabled. So the code needs to be 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 */ msr SCTLR_EL2, x0 dsb sy /* ensure the writes to SCTLR_EL2 are observed on all the processors */ isb /* any instruction fetch observes that MPU is enabled. So force flush the pre-existing instruction pipeline */ ret END(enable_mpu) This will be similar to what Zephyr does https://github.com/zephyrproject-rtos/zephyr/blob/a30270668d4b90bac932794ef75df12a2b6f6f78/arch/arm/core/mpu/arm_mpu.c#L258 . Let me know if you are ok with the rationale. Also, I would prefer to have 3 orr instructions instead of one for the sake of readability. However, this is not a strong preference so if you feel otherwise, I can change to have a single orr instruction. - Ayan
Hi Ayan, > On 24 Oct 2024, at 09:02, Ayan Kumar Halder <ayankuma@amd.com> wrote: > > Hi Julien, > > On 23/10/2024 17:30, Julien Grall wrote: >> >> >> On 23/10/2024 17:18, Julien Grall wrote: >>> >>> >>> On 23/10/2024 17:13, Julien Grall wrote: >>>> >>>> >>>> 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@xen.org> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 22/10/2024 10:56, Luca Fancellu wrote: >>>>>>>>> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >>>>>>>>> >>>>>>>>> Hi Luca, >>>>>>>>> >>>>>>>>> On 22/10/2024 10:41, Luca Fancellu wrote: >>>>>>>>>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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 >>> >>> Sorry I fat fingered and pressed send too early. I think this is the part I disagree with. All explicit accesses don't need to be complete (in the sense observed by everyone in the system). They only need to have gone through the permissions check. >> >> I think I managed to find again the wording that would justify why a "dsb" is not necessary for the permission checks. From ARM DDI 0487K.a D23-7349: >> >> ``` >> Direct writes using the instructions in Table D22-2 require synchronization before software can rely on the effects >> of changes to the System registers to affect instructions appearing in program order after the direct write to the >> System register. Direct writes to these registers are not allowed to affect any instructions appearing in program order >> before the direct write. >> ``` >> >> I understand the paragraph as enabling the MPU via SCTLR_EL2 will not affect any instruction before hand. > > Yes, I agree. > > However, as the line states > > "Direct writes using the instructions in Table D22-2 require synchronization before software can rely on the effects" > > This means synchronization is required after the write to SCTLR_EL2. > > However, this synchronization seems to imply dsb sy + isb. > > FromARM DDI 0487K.a ID032224 B2-274 > > "A DSB instruction ordered after a direct write to a System PMU register does not complete until all observers observe the direct write" I think this is related only to the System PMU register, not to the registers in table D22-2 (which SCTLR_ELx are part) Anyway we could discuss this in person at the Xen meet-up and write a summary in the thread later. Cheers, Luca
Hi, On 24/10/2024 09:02, Ayan Kumar Halder wrote: > On 23/10/2024 17:30, Julien Grall wrote: >> >> >> On 23/10/2024 17:18, Julien Grall wrote: >>> >>> >>> On 23/10/2024 17:13, Julien Grall wrote: >>>> >>>> >>>> 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@xen.org> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 22/10/2024 10:56, Luca Fancellu wrote: >>>>>>>>> On 22 Oct 2024, at 10:47, Julien Grall <julien@xen.org> wrote: >>>>>>>>> >>>>>>>>> Hi Luca, >>>>>>>>> >>>>>>>>> On 22/10/2024 10:41, Luca Fancellu wrote: >>>>>>>>>>> On 21 Oct 2024, at 17:27, Julien Grall <julien@xen.org> 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 >>> >>> Sorry I fat fingered and pressed send too early. I think this is the >>> part I disagree with. All explicit accesses don't need to be complete >>> (in the sense observed by everyone in the system). They only need to >>> have gone through the permissions check. >> >> I think I managed to find again the wording that would justify why a >> "dsb" is not necessary for the permission checks. From ARM DDI 0487K.a >> D23-7349: >> >> ``` >> Direct writes using the instructions in Table D22-2 require >> synchronization before software can rely on the effects >> of changes to the System registers to affect instructions appearing in >> program order after the direct write to the >> System register. Direct writes to these registers are not allowed to >> affect any instructions appearing in program order >> before the direct write. >> ``` >> >> I understand the paragraph as enabling the MPU via SCTLR_EL2 will not >> affect any instruction before hand. > > Yes, I agree. > > However, as the line states > > "Direct writes using the instructions in Table D22-2 require > synchronization before software can rely on the effects" > > This means synchronization is required after the write to SCTLR_EL2. > > However, this synchronization seems to imply dsb sy + isb. > > FromARM DDI 0487K.a ID032224 B2-274 > > "A DSB instruction ordered after a direct write to a System PMU register > does not complete until all observers observe the direct write" > > So, a write to SCTLR_EL2 needs to be followed by a dsb to ensure the > write is observed on all the processors (as SCTLR_EL2 and PMU registers > belong to Table D22-2, so the behavior should be same). Aside what Luca already said, I don't understand why the write to SCTLR_EL2 needs to be seen by all the processors (which BTW would imply "ish" rather than "sy"). The MPU and SCTLR are per-processor. > > And then it needs to be followed by an ISB to ensure any instruction > fetch observes that MPU is enabled. > > So the code needs to be > > 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 */ > msr SCTLR_EL2, x0 > dsb sy /* ensure the writes to SCTLR_EL2 are > observed on all the processors */ > isb /* any instruction fetch observes > that MPU is enabled. So force flush the pre-existing instruction > pipeline */ > > ret > END(enable_mpu) > > This will be similar to what Zephyr does https://github.com/ > zephyrproject-rtos/zephyr/blob/a30270668d4b90bac932794ef75df12a2b6f6f78/ > arch/arm/core/mpu/arm_mpu.c#L258 . I guess you mean https://github.com/zephyrproject-rtos/zephyr/blob/a30270668d4b90bac932794ef75df12a2b6f6f78/arch/arm/core/mpu/arm_mpu.c#L223? But the reasoning there is seem quite different. AFAIU The "dsb" is to ensure that the changes to the MPU registers are visible. That said, I am not convinced that the "dsb" belongs after. Even if "SCTLR_EL2" may not be fully synchronized until the next "isb", it may happen before and therefore you want the MPU registers to be visible before SCTLR_EL2 is visible. > Let me know if you are ok with the rationale. See above. > > Also, I would prefer to have 3 orr instructions instead of one for the > sake of readability. However, this is not a strong preference so if you > feel otherwise, I can change to have a single orr instruction. I am ok with that. Cheers,
diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S index 4a21bc815c..e354f4552b 100644 --- a/xen/arch/arm/arm64/mpu/head.S +++ b/xen/arch/arm/arm64/mpu/head.S @@ -61,6 +61,30 @@ add \xb, \xb, x20 /* x20 - Phys offset */ .endm +/* + * Enable EL2 MPU and data cache + * If the Background region is enabled, then the MPU uses the default memory + * map as the Background region for generating the memory + * attributes when MPU is disabled. + * Since the default memory map of the Armv8-R AArch64 architecture is + * IMPLEMENTATION DEFINED, we intend to turn off the Background region here. + * + * Clobbers x0 + * + */ +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 + msr SCTLR_EL2, x0 + isb + + ret +END(enable_mpu) + /* * Maps the various sections of Xen (described in xen.lds.S) as different MPU * regions. @@ -68,10 +92,11 @@ * Inputs: * lr : Address to return to. * - * Clobbers x0 - x5 + * Clobbers x0 - x6 * */ FUNC(enable_boot_cpu_mm) + mov x6, lr /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ mrs x5, MPUIR_EL2 @@ -113,7 +138,7 @@ FUNC(enable_boot_cpu_mm) beq 5f prepare_xen_region x0, x1, x2, x3, x4, x5 -5: +5: mov lr, x6 ret fail: diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h index b0c31a58ec..3769d23c80 100644 --- a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h @@ -15,6 +15,9 @@ /* MPU Protection Region Selection Register encode */ #define PRSELR_EL2 S3_4_C6_C2_1 +/* Backgroud region enable/disable */ +#define SCTLR_ELx_BR BIT(17, UL) + #endif /* __ASM_ARM_ARM64_MPU_SYSREGS_H */ /*
After the regions have been created, now we enable the MPU. For this we disable the background region so that the new memory map created for the regions take effect. Also, we treat all RW regions as non executable and the data cache is enabled. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from :- v2 - 1. Extracted from the previous patch into a new one. 2. Disabled background region. xen/arch/arm/arm64/mpu/head.S | 29 ++++++++++++++++++-- xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 3 ++ 2 files changed, 30 insertions(+), 2 deletions(-)