Hi Luca,
On 15/04/2025 00:17, Luca Fancellu wrote:
HI Julien,
On 14 Apr 2025, at 13:12, Julien Grall <jul...@xen.org> wrote:
Hi Luca,
On 11/04/2025 23:56, Luca Fancellu wrote:
Implement the function setup_mpu that will logically track the MPU
regions defined by hardware registers, start introducing data
structures and functions to track the status from the C world.
The xen_mpumap_mask bitmap is used to track which MPU region are
enabled at runtime.
This function is called from setup_mm() which full implementation
will be provided in a later stage.
Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
---
v3 changes:
- Moved PRENR_MASK define to common.
---
---
xen/arch/arm/include/asm/mpu.h | 2 ++
xen/arch/arm/mpu/mm.c | 49 +++++++++++++++++++++++++++++++++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index eba5086cde97..77d0566f9780 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -20,6 +20,8 @@
#define NUM_MPU_REGIONS_MASK (NUM_MPU_REGIONS - 1)
#define MAX_MPU_REGIONS NUM_MPU_REGIONS_MASK
+#define PRENR_MASK GENMASK(31, 0)
+
/* Access permission attributes. */
/* Read/Write at EL2, No Access at EL1/EL0. */
#define AP_RW_EL2 0x0
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 635d1f5a2ba0..e0a40489a7fc 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -14,6 +14,17 @@
struct page_info *frame_table;
+/* Maximum number of supported MPU memory regions by the EL2 MPU. */
+uint8_t __ro_after_init max_xen_mpumap;
Are this variable and ...
+
+/*
+ * Bitmap xen_mpumap_mask is to record the usage of EL2 MPU memory regions.
+ * Bit 0 represents MPU memory region 0, bit 1 represents MPU memory
+ * region 1, ..., and so on.
+ * If a MPU memory region gets enabled, set the according bit to 1.
+ */
+DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGIONS);
... this one meant to be global? If yes, then they need to have a declaration
in the header. If not, then you want to add 'static'.
yes they are meant to be global, I’ll add a declaration in the header.
+
/* EL2 Xen MPU memory region mapping table. */
pr_t xen_mpumap[MAX_MPU_REGIONS];
@@ -222,9 +233,45 @@ pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned
attr)
return region;
}
+/*
+ * The code in this function needs to track the regions programmed in
+ * arm64/mpu/head.S
+ */
+static void __init setup_mpu(void)
+{
+ register_t prenr;
+ unsigned int i = 0;
+
+ /*
+ * MPUIR_EL2.Region[0:7] identifies the number of regions supported by
+ * the EL2 MPU.
+ */
+ max_xen_mpumap = (uint8_t)(READ_SYSREG(MPUIR_EL2) & NUM_MPU_REGIONS_MASK);
+
+ /* PRENR_EL2 has the N bit set if the N region is enabled, N < 32 */
+ prenr = (READ_SYSREG(PRENR_EL2) & PRENR_MASK);
+
+ /*
+ * Set the bitfield for regions enabled in assembly boot-time.
+ * This code works under the assumption that the code in head.S has
+ * allocated and enabled regions below 32 (N < 32).
+
This is a bit fragile. I think it would be better if the bitmap is set by
head.S as we add the regions. Same for ...
So, I was trying to avoid that because in that case we need to place xen_mpumap
out of the BSS and start
manipulating the bitmap from asm, instead I was hoping to use the C code, I
understand that if someone
wants to have more than 31 region as boot region this might break, but it’s
also a bit unlikely?
The 31 is only part of the problem. The other problem is there is at
least one other places in Xen (i.e. as early_fdt_map()) which will also
create an entry in the MPU before setup_mm()/setup_mpu() is called. I am
a bit concerned that the assumption is going to spread and yet we have
no way to programmatically check if this will be broken.
Furthermore, right now, you are hardcoding the slot used and updating
the MPU. But if you had the bitmap updated, you could just look up for a
free slot.
So I was balancing the pros to manipulate everything from the C world against the
cons (boot region > 31).
Is it still your preferred way to handle everything from asm?
Yes. I don't think the change in asm will be large and this would allow
to remove other assumptions (like in the FDT mapping code).
As a side note, I noticed that the MPU entries are not cleared before we
enable the MPU. Is there anything in the boot protocol that guarantee
all the entries will be invalid? If not, then I think we need to clear
the entries.
Otherwise, your current logic doesn't work. That said, I think it would
still be necessary even if we get rid of your logic because we don't
know the content of the MPU entries.
Cheers,
--
Julien Grall
