Re: [PATCH v2 2/7] arm/mpu: Provide access to the MPU region from the C code

[Luca Fancellu]

Hi Ayan,

> On 8 Apr 2025, at 15:02, Ayan Kumar Halder <ayank...@amd.com> wrote:
> 
> Hi Luca,
> 
>> 
>> +static void prepare_selector(uint8_t sel)
>> +{
>> +    /*
>> +     * {read,write}_protection_region works using the direct access to the 
>> 0..15
>> +     * regions, so in order to save the isb() overhead, change the 
>> PRSELR_EL2
>> +     * only when needed, so when the upper 4 bits of the selector will 
>> change.
>> +     */
>> +    sel &= 0xF0U;
>> +    if ( READ_SYSREG(PRSELR_EL2) != sel )
>> +    {
>> +        WRITE_SYSREG(sel, PRSELR_EL2);
>> +        isb();
>> +    }
> 
> This needs to be arm64 specific. Refer ARM DDI 0600A.d ID120821
> 
> G1.3.19  PRBAR<n>_EL2, /* I guess this is what you are following */
> 
> Provides access to the base address for the MPU region determined by the 
> value of 'n' and
> PRSELR_EL2.REGION as PRSELR_EL2.REGION<7:4>:n.
> 
> 
> Whereas for arm32 . Refer ARM DDI 0568A.c ID110520
> 
> E2.2.3 HPRBAR<n>,
> 
> Provides access to the base addresses for the first 32 defined EL2 MPU 
> regions.
> 
> /* Here we do not need to use HPRSELR for region selection */
> 
> 
> If you want to make the code similar between arm32 and arm64, then I can 
> suggest you can use these registers.
> 
> G1.3.17 PRBAR_EL2
> 
> Provides access to the base addresses for the EL2 MPU region. 
> PRSELR_EL2.REGION determines
> which MPU region is selected.
> 
> E2.2.2 HPRBAR
> 
> Provides indirect access to the base address of the EL2 MPU region currently 
> defined by
> HPRSELR.
> 
> Let me know if it makes sense.
> 
> - Ayan

Ok I didin’t get that before, what do you think if I modify the code as in this 
diff (not tested):

diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index fe05c8097155..1bc6d7a77296 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -58,19 +58,21 @@ static void __init __maybe_unused build_assertions(void)
     BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
 }
 
-static void prepare_selector(uint8_t sel)
+static void prepare_selector(uint8_t *sel)
 {
+    uint8_t cur_sel = *sel;
     /*
      * {read,write}_protection_region works using the direct access to the 
0..15
      * regions, so in order to save the isb() overhead, change the PRSELR_EL2
      * only when needed, so when the upper 4 bits of the selector will change.
      */
-    sel &= 0xF0U;
-    if ( READ_SYSREG(PRSELR_EL2) != sel )
+    cur_sel &= 0xF0U;
+    if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
     {
-        WRITE_SYSREG(sel, PRSELR_EL2);
+        WRITE_SYSREG(cur_sel, PRSELR_EL2);
         isb();
     }
+    *sel = *sel & 0xFU;
 }
 
 /*
@@ -102,7 +104,7 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
      */
     prepare_selector(sel);
 
-    switch ( sel & 0xFU )
+    switch ( sel )
     {
         GENERATE_READ_PR_REG_CASE(0, pr_read);
         GENERATE_READ_PR_REG_CASE(1, pr_read);
@@ -140,7 +142,7 @@ void write_protection_region(const pr_t *pr_write, uint8_t 
sel)
      */
     prepare_selector(sel);
 
-    switch ( sel & 0xFU )
+    switch ( sel )
     {
         GENERATE_WRITE_PR_REG_CASE(0, pr_write);
         GENERATE_WRITE_PR_REG_CASE(1, pr_write);

And later you will use some #ifdef CONFIG_ARM_32 inside prepare_selector() to 
check
that the code is passing up to the max supported region or panic.
And in {read,write}_protection_region() you could add some #ifdef CONFIG_ARM_32 
to add
the case generators for regions from 16 to 23 since R52 can address them 
directly.

What do you think?

Cheers,
Luca