Re: [PATCH v3 6/6] arm/mpu: Enable read/write to protection regions for arm32

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Ayan,

> On 11 Jun 2025, at 15:35, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> 
> Define prepare_selector(), read_protection_region() and
> write_protection_region() for arm32. Also, define
> GENERATE_{READ/WRITE}_PR_REG_OTHERS to access MPU regions from 32 to 255.
> 
> Enable pr_{get/set}_{base/limit}(), region_is_valid() for arm32.
> Enable pr_of_addr() for arm32.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---

Based on your v2 
(https://patchwork.kernel.org/project/xen-devel/patch/20250606164854.1551148-4-ayan.kumar.halder@xxxxxxx/)
 I was imaging something like this:

diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 74e96ca57137..5d324b2d4ca5 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -87,20 +87,28 @@ static void __init __maybe_unused build_assertions(void)
  */
 static void prepare_selector(uint8_t *sel)
 {
-#ifdef CONFIG_ARM_64
     uint8_t cur_sel = *sel;
 
+#ifdef CONFIG_ARM_64
     /*
-     * {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.
+     * {read,write}_protection_region works using the Arm64 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.
      */
     cur_sel &= 0xF0U;
+#else
+    /* Arm32 MPU can use direct access for 0-31 */
+    if ( cur_sel > 31 )
+        cur_sel = 0;
+#endif
     if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
     {
         WRITE_SYSREG(cur_sel, PRSELR_EL2);
         isb();
     }
+
+#ifdef CONFIG_ARM_64
     *sel = *sel & 0xFU;
 #endif
 }
@@ -144,6 +152,12 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
         GENERATE_READ_PR_REG_CASE(29, pr_read);
         GENERATE_READ_PR_REG_CASE(30, pr_read);
         GENERATE_READ_PR_REG_CASE(31, pr_read);
+        case 32 ... 255:
+        {
+            pr->prbar.bits = READ_SYSREG(PRBAR_EL2);
+            pr->prlar.bits = READ_SYSREG(PRLAR_EL2);
+            break;
+        }
 #endif
     default:
         BUG(); /* Can't happen */
@@ -190,6 +204,12 @@ void write_protection_region(const pr_t *pr_write, uint8_t 
sel)
         GENERATE_WRITE_PR_REG_CASE(29, pr_write);
         GENERATE_WRITE_PR_REG_CASE(30, pr_write);
         GENERATE_WRITE_PR_REG_CASE(31, pr_write);
+        case 32 ... 255:
+        {
+            WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2);
+            WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2);
+            break;
+        }
 #endif
     default:
         BUG(); /* Can't happen */


Is it using too ifdefs in your opinion that would benefit the split you do in 
v3?