Re: [XEN v4 07/11] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 03/12/2022 18:35, Julien Grall wrote:
> Hi,
Hi Julien,

> On 29/11/2022 14:33, Michal Orzel wrote:
> > > @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu 
> > > *v)
> > >       if ( v == current )
> > >       {
> > >           for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> > > -            printk("   HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
> > > +            printk("   HW_LR[%d]=%" PRIx64 "\n", i, 
> > > gicv3_ich_read_lr(i));
> > 1. We do not usually add spaces between " and PRIx.
> I don't have a strong preference on this one.
ok, I will then keep it as it is.
> > >       }
> > >       else
> > >       {
> > >           for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> > > -            printk("   VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
> > > +            printk("   VCPU_LR[%d]=%" PRIx64 "\n", i, 
> > > v->arch.gic.v3.lr[i]);
> > >       }
> > >   }
> > >
> > > diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h 
> > > b/xen/arch/arm/include/asm/arm32/sysregs.h
> > > index 6841d5de43..22871999af 100644
> > > --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> > > +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> > > @@ -62,6 +62,25 @@
> > >   #define READ_SYSREG(R...)       READ_SYSREG32(R)
> > >   #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
> > >
> > > +/* Wrappers for accessing interrupt controller list registers. */
> > > +#define ICH_LR_REG(index)       ICH_LR ## index ## _EL2
> > > +#define ICH_LRC_REG(index)      ICH_LRC ## index ## _EL2
> > > +
> > > +#define READ_SYSREG_LR(index) ({                            \
> > > +    uint64_t _val;                                          \
> > > +    uint32_t _lrc = READ_CP32(ICH_LRC_REG(index));          \
> > > +    uint32_t _lr = READ_CP32(ICH_LR_REG(index));            \
> > > +                                                            \
> > > +    _val = ((uint64_t) _lrc << 32) | _lr;                   \
> > > +    _val;                                                   \
> > > +})
> > > +
> > > +#define WRITE_SYSREG_LR(v, index) ({                        \
> > > +    uint64_t _val = (v);                                    \
> > > +    WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(index));   \
> > > +    WRITE_CP32(_val >> 32, ICH_LRC_REG(index));             \
> > > +})
> > > +
> > >   /* MVFR2 is not defined on ARMv7 */
> > >   #define MVFR2_MAYBE_UNDEFINED
> > >
> > > diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h 
> > > b/xen/arch/arm/include/asm/arm64/sysregs.h
> > > index 54670084c3..4638999514 100644
> > > --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> > > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> > > @@ -472,6 +472,11 @@
> > >   #define READ_SYSREG(name)     READ_SYSREG64(name)
> > >   #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
> > >
> > > +/* Wrappers for accessing interrupt controller list registers. */
> > > +#define ICH_LR_REG(index)          ICH_LR ## index ## _EL2
> > > +#define WRITE_SYSREG_LR(v, index)  WRITE_SYSREG(v, ICH_LR_REG(index))
> > > +#define READ_SYSREG_LR(index) READ_SYSREG(ICH_LR_REG(index))
> > > +
> > >   #endif /* _ASM_ARM_ARM64_SYSREGS_H */
> > >
> > >   /*
> > > diff --git a/xen/arch/arm/include/asm/cpregs.h 
> > > b/xen/arch/arm/include/asm/cpregs.h
> > > index 6daf2b1a30..b85e811f51 100644
> > > --- a/xen/arch/arm/include/asm/cpregs.h
> > > +++ b/xen/arch/arm/include/asm/cpregs.h
> > > @@ -8,6 +8,8 @@
> > >    * support 32-bit guests.
> > >    */
> > >
> > > +#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, 
> > > crm, opc2
> > 2. As you are using ___CP32 much later in this file, it could be 
> > moved...
> __CP32() is already defined in arm32/sysregs.h which includes 
> cpregs.h. We should not define __CP32() twice and the only reason the 
> compiler doesn't complain is because the definition is the same
The definition is different :-

In xen/arch/arm/include/asm/arm32/sysregs.h

 "#define __CP32(r, coproc, opc1, crn, crm, opc2) coproc, opc1, r, crn, 
crm, opc2"   (Note:- it takes 6 arguments)
And what I have defined in  xen/arch/arm/include/asm/cpregs.h:-

#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, 
opc2 (It takes 5 arguments, also note it has 3 underscores before CP32).
> So one of the two needs to be dropped. Also, I think __CP32(), 
> __CP64(), CP32() and CP64() should be defined together because they 
> are all related.
> However...
>
> > > +
> > >   #define __HSR_CPREG_c0  0
> > >   #define __HSR_CPREG_c1  1
> > >   #define __HSR_CPREG_c2  2
> > > @@ -259,6 +261,48 @@
> > >   #define VBAR            p15,0,c12,c0,0  /* Vector Base Address 
> > > Register */
> > >   #define HVBAR           p15,4,c12,c0,0  /* Hyp. Vector Base 
> > > Address Register */
> > ...here, before first use. The remark I gave in v3 was that the 
> > definition should occur before use,
> > but it does not mean placing the macro at the top of the file.
> >
> > > +/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
> > > +#define __LR0(x)        ___CP32(p15, 4, c12, c12, x)
> > > +#define __LR8(x)        ___CP32(p15, 4, c12, c13, x)
> ... I don't understand why you need to use __CP32 here and everywhere 
> else in this header. In fact I have replaced in my tree all the 
> __CP32(foo) with foo and it still compiles.
I think you mean ___CP32 here (3 underscores).
> So can you explain why they are necessary?
Actually, they are not necessary. I will remove ___CP32 definition in 
the v5 patch.
Sorry, I was blindly influenced by the definition of __CP32. :( But, 
there it was needed.
Would you also review 8/11, 9/11, 10/11 and 10/11 before I re-spin a new 
series ? They have been reviewed by Michal (with some minor comments 
which I can address in v5).
- Ayan

> Cheers,