Re: [PATCH v3 7/7] arm/mpu: Implement setup_mpu for MPU system

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

HI Julien,

> On 14 Apr 2025, at 13:12, Julien Grall <julien@xxxxxxx> 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.fancellu@xxxxxxx>
>> ---
>> 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