Re: [PATCH v4 3/7] arm/mpu: Provide and populate MPU C data structures

[Luca Fancellu <Luca.Fancellu@arm.com>]

Hi Michal,

> On 30 Apr 2025, at 11:57, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 29/04/2025 17:20, Luca Fancellu wrote:
>> Provide some data structure in the C world to track the MPU
>> status, these structures will be filled at boot by the assembly
>> early code with the boot MPU regions and afterwards they will be
>> used at runtime.
>> 
>> Provide methods to update a bitmap created with DECLARE_BITMAP
>> from the assembly code for both Arm32 and Arm64.
>> 
>> Modify Arm64 assembly boot code to reset any unused MPU region,
>> initialise 'max_xen_mpumap' with the number of supported MPU
> IMO this is not a good name because there's nothing there suggesting that this
> variable stores the number. Maybe max_mpu_regions or max_xen_mpumap_regions.

ok I will change it

>> 
>>     /* x0: region sel */
>>     mov   x0, xzr
>>     /* Xen text section. */
>> @@ -74,6 +77,16 @@ FUNC(enable_boot_cpu_mm)
>>     prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
>> #endif
>> 
>> +zero_mpu:
>> +    /* Reset remaining MPU regions */
>> +    cmp   x0, x5
>> +    beq   out_zero_mpu
>> +    mov   x1, #0
>> +    mov   x2, #1
> Shouldn't we mark the region as emtpy (base == limit) when doing region clear?

So the macro takes an exclusive range, inside it will change to inclusive by doing limit-1,
so the region will be empty.

> 
> 
>> @@ -0,0 +1,67 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +/*
>> + * Sets a bit in a bitmap declared by DECLARE_BITMAP, symbol name passed through
>> + * bitmap_symbol.
>> + *
>> + * bitmap_set_bit:      symbol of the bitmap declared by DECLARE_BITMAP
>> + * bit:                 bit number to be set in the bitmap
>> + * tmp1-tmp4:           temporary registers used for the computation
>> + *
>> + * Preserves bit.
> Here you say it is preserved, yet...
> 
>> + * Output:
>> + *  tmp1: Address of the word containing the changed bit.
>> + * Clobbers: bit, tmp1, tmp2, tmp3, tmp4.
> ... here you list is as clobbered.

right, I’ll fix that

> 
>> + */
>> +.macro bitmap_set_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4
>> +    adr_l   \tmp1, \bitmap_symbol
>> +    mov     \tmp2, #(BYTES_PER_LONG - 1)
>> +    mvn     \tmp2, \tmp2
>> +    lsr     \tmp3, \bit, #3
>> +    and     \tmp2, \tmp3, \tmp2
>> +    add     \tmp1, \tmp1, \tmp2                 // bitmap_symbol + (bit/BITS_PER_LONG)*BYTES_PER_LONG
> We don't use // style comments. Please use /* */

sure, I’ll change here and in the rest of the patch

>> 
>> 
>> #define MPU_REGION_SHIFT  6
>> @@ -17,6 +21,7 @@
>> #define NUM_MPU_REGIONS_SHIFT   8
>> #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
>> #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>> +#define MAX_MPU_REGION_NR       255
> Shouldn't you define it using NUM_MPU_REGIONS? It should have the same
> definition as mask.

Maybe I misunderstood your comment in the previous patch, ok I will use:

#define MAX_MPU_REGION_NR      NUM_MPU_REGIONS_MASK

>> 
>> +
>> +.macro store_pair reg1, reg2, dst
>> +        stp \reg1, \reg2, [\dst]
> Why 8 instead of 4 spaces?

I’ll fix 

>> 
>> /*
>>  * Macro to prepare and set a EL2 MPU memory region.
>>  * We will also create an according MPU memory region entry, which
>>  * is a structure of pr_t,  in table \prmap.
>>  *
>>  * sel:         region selector
>> - * base:        reg storing base address
>> - * limit:       reg storing limit address
>> + * tmp1:        reg storing base address
>> + * tmp2:        reg storing limit address
> I think this change is not needed. The parameters should be named base and limit
> because this is what you expect caller to pass. Inside the function, you can do
> whatever you want with these registers and caller does not care as long as you
> mention if they are clobbered or not. Same in C world. You can reuse the
> parameter for a different internal purpose inside a function.

ok I’ll revert back

>> 
>> +
>> +    /* Load pair into xen_mpumap and invalidate cache */
>> +    mov   \tmp1, \sel
>> +    lsl   \tmp1, \tmp1, #XEN_MPUMAP_ENTRY_SHIFT
> You could get rid of these 2 extra instructions and instead do:
> 
>> +    adr_l \tmp2, xen_mpumap
>> +    add   \tmp2, \tmp2, \tmp1
> add \tmp2, \tmp2, \sel, lsl #XEN_MPUMAP_ENTRY_SHIFT

yep, I’ll use it

> which combines everything in one go.
> 
>> +    store_pair \prbar, \prlar, \tmp2
>> +    invalidate_dcache_one \tmp2
>> +
>> +    /* Set/clear xen_mpumap_mask bitmap */
>> +    tst   \prlar, #PRLAR_ELx_EN
>> +    bne   2f
>> +    // Region is disabled, clear the bit in the bitmap
> Comment style, here and elsewhere
> 
>> +    bitmap_clear_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
>> +    b     3f
>> +
>> +2:
>> +    // Region is enabled, set the bit in the bitmap
>> +    bitmap_set_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
> Wouldn't it be better to first clear the entire bitmap before setting up the
> regions (i.e. all regions disabled) and then only have the set part here?

well we are going to set up all region anyway, doing that here will save some time
spent on barriers and cache invalidation, maybe not much, but still…

Cheers,
Luca