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?
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?
Cheers,
Luca
