[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
On 05/06/2025 15:39, Ayan Kumar Halder wrote: > 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 0x0 >> The 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. I did not mean #ifdef-ing. I was more like suggesting to use a different macro name that would be more meaningful than this one. > >> >>> + >>> +/* 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 I don't understand. My comment was only about changing comment to say /* Access Permission AP[0] */ because below you have a comment with AP[1]. ~Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |