[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 18/40] xen/mpu: introduce helper access_protection_region



Hi Penny,

On 13/01/2023 05:28, Penny Zheng wrote:
Each EL2 MPU protection region could be configured using PRBAR<n>_EL2 and
PRLAR<n>_EL2.

This commit introduces a new helper access_protection_region() to access
EL2 MPU protection region, including both read/write operations.

As explained in section G1.3.18 of the reference manual for AArch64v8R,
a set of system register PRBAR<n>_EL2 and PRLAR<n>_EL2 provide access to
the EL2 MPU region which is determined by the value of 'n' and
PRSELR_EL2.REGION as PRSELR_EL2.REGION<7:4>:n.(n = 0, 1, 2, ... , 15)
For example to access regions from 16 to 31:
- Set PRSELR_EL2 to 0b1xxxx
- Region 16 configuration is accessible through PRBAR0_EL2 and PRLAR0_EL2
- Region 17 configuration is accessible through PRBAR1_EL2 and PRLAR1_EL2
- Region 18 configuration is accessible through PRBAR2_EL2 and PRLAR2_EL2
- ...
- Region 31 configuration is accessible through PRBAR15_EL2 and PRLAR15_EL2

Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
---
  xen/arch/arm/mm_mpu.c | 151 ++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 151 insertions(+)

diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
index c9e17ab6da..f2b494449c 100644
--- a/xen/arch/arm/mm_mpu.c
+++ b/xen/arch/arm/mm_mpu.c
@@ -46,6 +46,157 @@ uint64_t __ro_after_init next_transient_region_idx;
  /* Maximum number of supported MPU memory regions by the EL2 MPU. */
  uint64_t __ro_after_init max_xen_mpumap;
+/* Write a MPU protection region */
+#define WRITE_PROTECTION_REGION(sel, pr, prbar_el2, prlar_el2) ({       \
+    uint64_t _sel = sel;                                                \
+    const pr_t *_pr = pr;                                               \
+    asm volatile(                                                       \
+        "msr "__stringify(PRSELR_EL2)", %0;" /* Selects the region */   \

This is an open-coding version of WRITE_SYSREG(). Can we use it instead?

+        "dsb sy;"                                                       \

What is this dsb for? Also is the 'sy' really necessary?

+        "msr "__stringify(prbar_el2)", %1;" /* Write PRBAR<n>_EL2 */    \

WRITE_SYSREG()?

+        "msr "__stringify(prlar_el2)", %2;" /* Write PRLAR<n>_EL2 */    \

WRITE_SYSREG()?

+        "dsb sy;"                                                       \

Same about dsb. But I would consider to move the dsb and selection part outside of the macro. So they could outside of the switch and reduce the code generation.

+        : : "r" (_sel), "r" (_pr->prbar.bits), "r" (_pr->prlar.bits));  \
+})
+
+/* Read a MPU protection region */
+#define READ_PROTECTION_REGION(sel, prbar_el2, prlar_el2) ({            \

My comment on READ_PROTECTION also applies here. But you would want to use READ_SYSREG() for 'mrs'.

+    uint64_t _sel = sel;                                                \
+    pr_t _pr;                                                           \
+    asm volatile(                                                       \
+        "msr "__stringify(PRSELR_EL2)", %2;" /* Selects the region */   \
+        "dsb sy;"                                                       \
+        "mrs %0, "__stringify(prbar_el2)";" /* Read PRBAR<n>_EL2 */     \
+        "mrs %1, "__stringify(prlar_el2)";" /* Read PRLAR<n>_EL2 */     \
+        "dsb sy;"                                                       \
+        : "=r" (_pr.prbar.bits), "=r" (_pr.prlar.bits) : "r" (_sel));   \
+    _pr;                                                                \
+})
+
+/*
+ * Access MPU protection region, including both read/write operations.
+ * Armv8-R AArch64 at most supports 255 MPU protection regions.
+ * See section G1.3.18 of the reference manual for Armv8-R AArch64,
+ * PRBAR<n>_EL2 and PRLAR<n>_EL2 provide access to the EL2 MPU region
+ * determined by the value of 'n' and PRSELR_EL2.REGION as
+ * PRSELR_EL2.REGION<7:4>:n(n = 0, 1, 2, ... , 15)
+ * For example to access regions from 16 to 31 (0b10000 to 0b11111):
+ * - Set PRSELR_EL2 to 0b1xxxx
+ * - Region 16 configuration is accessible through PRBAR0_ELx and PRLAR0_ELx
+ * - Region 17 configuration is accessible through PRBAR1_ELx and PRLAR1_ELx
+ * - Region 18 configuration is accessible through PRBAR2_ELx and PRLAR2_ELx
+ * - ...
+ * - Region 31 configuration is accessible through PRBAR15_ELx and PRLAR15_ELx
+ *
+ * @read: if it is read operation.
+ * @pr_read: mpu protection region returned by read op.
+ * @pr_write: const mpu protection region passed through write op.
+ * @sel: mpu protection region selector
+ */
+static void access_protection_region(bool read, pr_t *pr_read,
+                                     const pr_t *pr_write, uint64_t sel)

I would rather prefer if we introduce two helpers (one for the read operation, the other for the write operation). This would make the code a bit easier to read.

+{
+    switch ( sel & 0xf )
+    {
+    case 0:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR0_EL2, PRLAR0_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR0_EL2, PRLAR0_EL2);
+        break;
+    case 1:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR1_EL2, PRLAR1_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR1_EL2, PRLAR1_EL2);
+        break;
+    case 2:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR2_EL2, PRLAR2_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR2_EL2, PRLAR2_EL2);
+        break;
+    case 3:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR3_EL2, PRLAR3_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR3_EL2, PRLAR3_EL2);
+        break;
+    case 4:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR4_EL2, PRLAR4_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR4_EL2, PRLAR4_EL2);
+        break;
+    case 5:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR5_EL2, PRLAR5_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR5_EL2, PRLAR5_EL2);
+        break;
+    case 6:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR6_EL2, PRLAR6_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR6_EL2, PRLAR6_EL2);
+        break;
+    case 7:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR7_EL2, PRLAR7_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR7_EL2, PRLAR7_EL2);
+        break;
+    case 8:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR8_EL2, PRLAR8_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR8_EL2, PRLAR8_EL2);
+        break;
+    case 9:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR9_EL2, PRLAR9_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR9_EL2, PRLAR9_EL2);
+        break;
+    case 10:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR10_EL2, PRLAR10_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR10_EL2, PRLAR10_EL2);
+        break;
+    case 11:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR11_EL2, PRLAR11_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR11_EL2, PRLAR11_EL2);
+        break;
+    case 12:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR12_EL2, PRLAR12_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR12_EL2, PRLAR12_EL2);
+        break;
+    case 13:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR13_EL2, PRLAR13_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR13_EL2, PRLAR13_EL2);
+        break;
+    case 14:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR14_EL2, PRLAR14_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR14_EL2, PRLAR14_EL2);
+        break;
+    case 15:
+        if ( read )
+            *pr_read = READ_PROTECTION_REGION(sel, PRBAR15_EL2, PRLAR15_EL2);
+        else
+            WRITE_PROTECTION_REGION(sel, pr_write, PRBAR15_EL2, PRLAR15_EL2);
+        break;

What if the caller pass a number higher than 15?

+    }
+}
+
  /* TODO: Implementation on the first usage */
  void dump_hyp_walk(vaddr_t addr)
  {

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.