On 8 Apr 2025, at 17:33, Ayan Kumar Halder <ayank...@amd.com> wrote:
On 08/04/2025 15:29, Luca Fancellu wrote:
Hi Ayan,
Hi Luca,
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?
I got this diff working as it is for R82. This looks much lesser code
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index fe05c80971..63627c85dc 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -28,23 +28,19 @@ DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGIONS);
/* EL2 Xen MPU memory region mapping table. */
pr_t xen_mpumap[MAX_MPU_REGIONS];
-/* The following are needed for the case generator with num==0 */
-#define PRBAR0_EL2 PRBAR_EL2
-#define PRLAR0_EL2 PRLAR_EL2
-
#define GENERATE_WRITE_PR_REG_CASE(num, pr) \
case num: \
{ \
- WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR##num##_EL2); \
- WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR##num##_EL2); \
+ WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2); \
+ WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2); \
break; \
}
#define GENERATE_READ_PR_REG_CASE(num, pr) \
case num: \
{ \
- pr->prbar.bits = READ_SYSREG(PRBAR##num##_EL2); \
- pr->prlar.bits = READ_SYSREG(PRLAR##num##_EL2); \
+ pr->prbar.bits = READ_SYSREG(PRBAR_EL2); \
+ pr->prlar.bits = READ_SYSREG(PRLAR_EL2); \
break; \
}
@@ -65,7 +61,6 @@ static void prepare_selector(uint8_t sel)
* 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);
Please give it a try to see if it works at your end.
And then, the code can be reduced further as
void read_protection_region(pr_t *pr_read, uint8_t sel)
{
/*
* Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
* make sure PRSELR_EL2 is set, as it determines which MPU region
* is selected.
*/
prepare_selector(sel);
pr_read->prbar.bits = READ_SYSREG(PRBAR_EL2);
pr_read->prlar.bits = READ_SYSREG(PRLAR_EL2);
}
The same can be done for write_protection_region(...)
- Ayan
