[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 1/3] arm/mpu: Introduce MPU memory region map structure
Hi Michal, On 05/06/2025 08:06, Orzel, Michal wrote: On 04/06/2025 19:43, Ayan Kumar Halder wrote:Introduce pr_t typedef which is a structure having the prbar and prlar members, each being structured as the registers of the AArch32 Armv8-R architecture. Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the BASE or LIMIT bitfields in prbar or prlar respectively. Move pr_t definition to common code. Also, enclose xn_0 within ARM64 as it is not present for ARM32. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> --- xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++----- xen/arch/arm/include/asm/arm64/mpu.h | 6 ------ xen/arch/arm/include/asm/mpu.h | 6 ++++++ xen/arch/arm/mpu/mm.c | 2 ++ 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h index f0d4d4055c..ae3b661fde 100644 --- a/xen/arch/arm/include/asm/arm32/mpu.h +++ b/xen/arch/arm/include/asm/arm32/mpu.h @@ -5,11 +5,31 @@#ifndef __ASSEMBLY__ -/* MPU Protection Region */-typedef struct { - uint32_t prbar; - uint32_t prlar; -} pr_t; +#define MPU_REGION_RES0 0x0The name of the macro does not make a lot of sense in AArch32 context and can create a confusion for the reader. I know, but I want to avoid introducing ifdef or have separate implementation (for arm32 and arm64) for the following Refer xen/arch/arm/include/asm/mpu.h static inline void pr_set_base(pr_t *pr, paddr_t base) { pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT); } Let me know your preference. + +/* Hypervisor Protection Region Base Address Register */ +typedef union { + struct { + unsigned int xn:1; /* Execute-Never */ + unsigned int ap_0:1; /* Acess Permission */If you write AP[1] below, I would expect here AP[0] Again same reason as before, let me know if you want to have additional ifdef in pr_of_addr() or separate functions for arm32 and arm64 pr_t pr_of_addr(...) {... prbar = (prbar_t) { .reg = { #ifdef CONFIG_ARM64 .xn_0 = 0, #endif .xn = PAGE_XN_MASK(flags), .ap_0 = 0, .ro = PAGE_RO_MASK(flags) }}; ... } Also s/acess/access/ Ack. + unsigned long ro:1; /* Access Permission AP[1] */ + unsigned int sh:2; /* Sharebility */ + unsigned int res0:1; /* Reserved as 0 */Here your comment for RES0 (which IMO could be just RES0) does not match... I will remove the comment here and .. + unsigned int base:26; /* Base Address */ + } reg; + uint32_t bits; +} prbar_t; + +/* Hypervisor Protection Region Limit Address Register */ +typedef union { + struct { + unsigned int en:1; /* Region enable */ + unsigned int ai:3; /* Memory Attribute Index */ + unsigned int res0:2; /* Reserved 0 by hardware */this one. I don't think you need to explain what RES0 means. here. - Ayan ~Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |