Re: [PATCH v1 1/3] arm/mpu: Introduce MPU memory region map structure

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Michal,

On 06/06/2025 11:13, Orzel, Michal wrote:
> 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.
Now I understand you. However, I can't thing of a better name to make it 
more meaningful.
I have added a comment on top. Is this helpful ?

/* Unlike arm64, there are no reserved 0 bits beyond base in prbar 
register */
> > > > +
> > > > +/* 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].
Ah makes sense now.

- Ayan

> ~Michal