|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 3/6] arm/mpu: Provide and populate MPU C data structures
Hi Luca, On 13/05/2025 09:45, Luca Fancellu wrote:
Below you are handling cache invalidation when writing to the bitmap. But you don't do one here. Is this just an overlook?
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. > + adr_l \tmp1, \bitmap_symbol> + mov \tmp2, #(BYTES_PER_LONG - 1)+ * Clobbers: tmp1, tmp2, tmp3, tmp4. + */ +.macro bitmap_set_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4 + 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.: * ... = ... + ... + and \tmp2, \bit, #(BITS_PER_LONG - 1) /* bit offset inside word */ + + ldr \tmp3, [\tmp1] + mov \tmp4, #1 + lsl \tmp4, \tmp4, \tmp2 /* (1 << offset) */ + orr \tmp3, \tmp3, \tmp4 /* set the bit */ + str \tmp3, [\tmp1] +.endm + +/* + * Clears 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. + * Clobbers: tmp1, tmp2, tmp3, tmp4. + */ +.macro bitmap_clear_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 */ + and \tmp2, \bit, #(BITS_PER_LONG - 1) /* bit offset inside word */ + + ldr \tmp3, [\tmp1] + mov \tmp4, #1 + lsl \tmp4, \tmp4, \tmp2 /* (1 << offset) */ + mvn \tmp4, \tmp4 /* ~(1 << offset) */ + and \tmp3, \tmp3, \tmp4 /* clear the bit */ + str \tmp3, [\tmp1] +.endm + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h index bb83f5a5f580..fb93b8b19d24 100644 --- a/xen/arch/arm/include/asm/mpu.h +++ b/xen/arch/arm/include/asm/mpu.h @@ -8,6 +8,10 @@#if defined(CONFIG_ARM_64)# include <asm/arm64/mpu.h> +#elif defined(CONFIG_ARM_32) +# include <asm/arm32/mpu.h> +#else +# error "unknown ARM variant" #endif#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 NUM_MPU_REGIONS_MASK#endif /* __ARM_MPU_H__ */ diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.hindex bfd840fa5d31..409b4dd53dc6 100644 --- a/xen/arch/arm/include/asm/mpu/mm.h +++ b/xen/arch/arm/include/asm/mpu/mm.h @@ -8,9 +8,16 @@ #include <xen/page-size.h> #include <xen/types.h> #include <asm/mm.h> +#include <asm/mpu.h>extern struct page_info *frame_table; +extern uint8_t max_mpu_regions;+ +extern DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR); + +extern pr_t xen_mpumap[MAX_MPU_REGION_NR]; + #define virt_to_maddr(va) ((paddr_t)((vaddr_t)(va) & PADDR_MASK))#ifdef CONFIG_ARM_32diff --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? +.endm + +.macro invalidate_dcache_one reg + nop Same here. +.endm > +> +#endif 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? + 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 + 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? + + /* 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? Same comment here.
Why does this need to be page_aligned? I guess for this one this is mandated by the HW? Cheers, -- Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |