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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Julien,

>> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
>> index 6d336cafbbaf..59ddc46526eb 100644
>> --- a/xen/arch/arm/arm64/mpu/head.S
>> +++ b/xen/arch/arm/arm64/mpu/head.S
>> @@ -40,6 +40,9 @@ FUNC(enable_boot_cpu_mm)
>>      mrs   x5, MPUIR_EL2
>>      and   x5, x5, #NUM_MPU_REGIONS_MASK
>>  +    ldr   x0, =max_mpu_regions
>> +    strb  w5, [x0]
> 
> Below you are handling cache invalidation when writing to the bitmap. But you 
> don't do one here. Is this just an overlook?

oh yes, overlooked, thanks

>> 
>> diff --git a/xen/arch/arm/include/asm/bitmap-op.inc 
>> b/xen/arch/arm/include/asm/bitmap-op.inc
>> new file mode 100644
>> index 000000000000..ea7c8abbf6f0
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/bitmap-op.inc
>> @@ -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.
>> + * Output:
>> + *  tmp1: Address of the word containing the changed bit.
> 
> If the register is used for output, then I think it needs a better name.
> But looking at the code, it is not entirely clear how the output will be used.

So currently it will give the address of the word containing the bit, so that 
we will
pass it to the invalidate cache function.

> 
>> + * Clobbers: tmp1, tmp2, tmp3, tmp4.
>> + */
>> +.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 */
> 
> Style: missing some spaces.
> 
> But is the comment explaining the logic above? If so, I think I would put it 
> right at the beginning to make easier to understand the logic. I would also 
> consider to split in smaller comments on each line e.g.:
> 
> * ... = ... + ...

Ok I’ll do a line by line explanation
>> 
>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc 
>> b/xen/arch/arm/include/asm/mpu/regions.inc
>> index 47868a152662..ba38213ffff1 100644
>> --- a/xen/arch/arm/include/asm/mpu/regions.inc
>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
>> @@ -1,14 +1,42 @@
>>  /* SPDX-License-Identifier: GPL-2.0-only */
>>  +#include <asm/bitmap-op.inc>
>>  #include <asm/mpu.h>
>>  #include <asm/sysregs.h>
>>    /* Backgroud region enable/disable */
>>  #define SCTLR_ELx_BR    BIT(17, UL)
>>  +#define REGION_DISABLED_PRLAR   0x00    /* NS=0 ATTR=000 EN=0 */
>>  #define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
>>  #define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
>>  +#define PRLAR_ELx_EN            0x1
>> +
>> +#ifdef CONFIG_ARM_64
>> +#define XEN_MPUMAP_ENTRY_SHIFT  0x4     /* 16 byte structure */
>> +
>> +.macro store_pair reg1, reg2, dst
>> +    stp \reg1, \reg2, [\dst]
>> +.endm
>> +
>> +.macro invalidate_dcache_one reg
>> +    dc ivac, \reg
>> +.endm
>> +
>> +#else
>> +#define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
>> +
>> +.macro store_pair reg1, reg2, dst
>> +    nop
> 
> Can we use a function that will trigger a fault? So we will remember this 
> will needed to be updated. Maybe re-use the BUG_OPCODE?

Sure, I’ll look into it

> 
>> +.endm
>> +
>> +.macro invalidate_dcache_one reg
>> +    nop
> 
> Same here.
> 
>> +.endm
> > +> +#endif
>> +
>>  /*
>>   * Macro to prepare and set a EL2 MPU memory region.
>>   * We will also create an according MPU memory region entry, which
>> @@ -56,6 +84,27 @@
>>      isb
>>      WRITE_SYSREG_ASM(\prbar, PRBAR_EL2)
>>      WRITE_SYSREG_ASM(\prlar, PRLAR_EL2)
>> +
>> +    /* Load pair into xen_mpumap and invalidate cache */
> 
> To confirm my understanding, xen_mpumap is part of the loaded binary. So we 
> expect the bootloader to have clean the cache for us. Therefore, we
> only need to invalidate the entries afterwards. Is it correct?

yes xen_mpumap is part of the binary, are you suggesting we should use clean 
and invalidate here? So for example boot-wrapper-aarch64 is not
cleaning the cache.

> 
>> +    adr_l \base, xen_mpumap
>> +    add   \base, \base, \sel, LSL #XEN_MPUMAP_ENTRY_SHIFT
>> +    store_pair \prbar, \prlar, \base
> 
> I think you want a comment on top of pr_t to mention the structure
> will not changed and

can you explain better this part, what should I write on top of the typedef 
struct {...} pt_t?

> 
>> +    invalidate_dcache_one \base
> 
> This will invalidate a single line in the data cache. The size depends on the 
> HW, but typically it will be 64 - 128 bytes. Do we have any check
> that will confirm the data will fit in an cache line?

You are right, so we are gonna write 16 bytes at most for Arm64 and (for now) 8 
bytes for Arm32, so I think we will be way below 64 bytes,
shall we have a BUILD_BUG_ON inside build_assertions() in arm/mpu/mm.c to check 
sizeof(pr_t) <= 64?

> 
>> +
>> +    /* Set/clear xen_mpumap_mask bitmap */
>> +    tst   \prlar, #PRLAR_ELx_EN
>> +    bne   2f
>> +    /* Region is disabled, clear the bit in the bitmap */
>> +    bitmap_clear_bit xen_mpumap_mask, \sel, \base, \limit, \prbar, \prlar
>> +    b     3f
>> +
>> +2:
>> +    /* Region is enabled, set the bit in the bitmap */
>> +    bitmap_set_bit xen_mpumap_mask, \sel, \base, \limit, \prbar, \prlar
>> +
>> +3:
>> +    invalidate_dcache_one \base
> 
> You want to a comment to explain what this invalidate does. AFAICT, this is 
> for the bitmap. But given the bitmap will be typically small, wouldn't it 
> better to do it in one go at the end?

Yes this invalidate is a bit overkill because it will invalidate 64-128 bytes 
starting from the address of the last changed word where the bit was.

Should I invalidate everything outside this macro, inside enable_boot_cpu_mm in 
arm64/mpu/head.S instead?

> 
> Same comment here.
> 
>> +
>>      dsb   sy
>>      isb
>>  diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 07c8959f4ee9..ee035a54b942 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -7,9 +7,25 @@
>>  #include <xen/mm.h>
>>  #include <xen/sizes.h>
>>  #include <xen/types.h>
>> +#include <asm/mpu.h>
>>    struct page_info *frame_table;
>>  +/* Maximum number of supported MPU memory regions by the EL2 MPU. */
>> +uint8_t __ro_after_init max_mpu_regions;
>> +
>> +/*
>> + * 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_REGION_NR) \
>> +    __section(".data.page_aligned");
> 
> Why does this need to be page_aligned?

I see from the linker file that this section is aligned to SMP_CACHE_BYTES, 
which is L1_CACHE_BYTES
for Arm, because we are using the invalidate cache I was under the impression 
that it’s better to have it
aligned on the cache line

> 
>> +
>> +/* EL2 Xen MPU memory region mapping table. */
>> +pr_t __section(".data.page_aligned") xen_mpumap[MAX_MPU_REGION_NR];
> 
> I guess for this one this is mandated by the HW?

same reason above, no other reason (I’m aware of).

Cheers,
Luca