Re: [RFC PATCH v1 07/12] Arm: GICv3: Emulate ICH_LR<n>_EL2 on AArch32

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 22/10/2022 12:03, Julien Grall wrote:
> Hi Ayan,
Hi Julien,

I need a clarification.

> Title: Xen doesn't emulate ICH_LR* (we don't expose them to the 
> guest). Instead Xen will use the registers and your patch provides 
> wrappers to use access the registers on 32-bit host.
> On 21/10/2022 16:31, Ayan Kumar Halder wrote:
> > diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h 
> > b/xen/arch/arm/include/asm/arm32/sysregs.h
> > index 6841d5de43..f3b4dfbca8 100644
> > --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> > +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> > @@ -62,9 +62,61 @@
> >   #define READ_SYSREG(R...)       READ_SYSREG32(R)
> >   #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
> >   +#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) \
> > + (READ_SYSREG(ICH_LRC_REG(INDEX)))) << 32) | \
> > + (READ_SYSREG(ICH_LR_REG(INDEX))))
> This is a bit dense to read. Also, we should use READ_CP64() when 
> dealing with arm32 only code. So how about (formatting will need to be 
> done):
> #define READ_SYSREG_LR(INDEX) ({   \
>     uint32_t lrc_ = READ_CP64(ICH_LRC_REG(INDEX)); \
>     uint32_t lr_ = READ_CP64(ICH_LR_REG(INDEX));   \
>                                                    \
I think this looks incorrect. These are read using 'mrc' so they should 
be READ_CP32(). They are 32 bit registers.
However, READ_SYSREG is defined as READ_CP32(), so should we use 
READ_CP32() or READ_SYSREG() ?
- Ayan

> (uint64_t)(lrc_ << 32) | lr_;
> })
>
> > +
> > +#define WRITE_SYSREG_LR(INDEX, V) WRITE_SYSREG \
> > +                                  (V&0xFFFFFFFF, ICH_LR_REG(INDEX)); \
> > +                                  WRITE_SYSREG(V>>32, 
> > ICH_LRC_REG(INDEX));
> This code is fragile. If V is a function call, then you will call it 
> twice. You want something like:
> do {
>   uint64_t v_ = (V);
>
>   WRITE_SYSREG(v_ & 0xFFFFFFFF, ICH_LR_REG(INDEX));
>   WRITE_SYSREG(v_ >> 32, ICH_LRC_REG(INDEX));
> } while(0);
>
> And maybe replacing the opencoding Fs with GENMASK.
>
> > +
> >   /* MVFR2 is not defined on ARMv7 */
> >   #define MVFR2_MAYBE_UNDEFINED
> >   +#define ___CP32(a,b,c,d,e)   a,b,c,d,e
> I am not entirely sure why you need to define __CP32() here. However, 
> co-processors registers should be defined in asm/cpregs.h rather than 
> arm32/sysregs.h.
> > +#define __LR0_EL2(x) ___CP32(p15,4,c12,c12,x)
> > +#define __LR8_EL2(x)              ___CP32(p15,4,c12,c13,x)
> > +
> > +#define __LRC0_EL2(x)             ___CP32(p15,4,c12,c14,x)
> > +#define __LRC8_EL2(x)             ___CP32(p15,4,c12,c15,x)
> > +
> > +#define ICH_LR0_EL2               __LR0_EL2(0)
> > +#define ICH_LR1_EL2               __LR0_EL2(1)
> > +#define ICH_LR2_EL2               __LR0_EL2(2)
> > +#define ICH_LR3_EL2               __LR0_EL2(3)
> > +#define ICH_LR4_EL2               __LR0_EL2(4)
> > +#define ICH_LR5_EL2               __LR0_EL2(5)
> > +#define ICH_LR6_EL2               __LR0_EL2(6)
> > +#define ICH_LR7_EL2               __LR0_EL2(7)
> > +#define ICH_LR8_EL2               __LR8_EL2(0)
> > +#define ICH_LR9_EL2               __LR8_EL2(1)
> > +#define ICH_LR10_EL2              __LR8_EL2(2)
> > +#define ICH_LR11_EL2              __LR8_EL2(3)
> > +#define ICH_LR12_EL2              __LR8_EL2(4)
> > +#define ICH_LR13_EL2              __LR8_EL2(5)
> > +#define ICH_LR14_EL2              __LR8_EL2(6)
> > +#define ICH_LR15_EL2              __LR8_EL2(7)
> > +
> > +#define ICH_LRC0_EL2               __LRC0_EL2(0)
> > +#define ICH_LRC1_EL2               __LRC0_EL2(1)
> > +#define ICH_LRC2_EL2               __LRC0_EL2(2)
> > +#define ICH_LRC3_EL2               __LRC0_EL2(3)
> > +#define ICH_LRC4_EL2               __LRC0_EL2(4)
> > +#define ICH_LRC5_EL2               __LRC0_EL2(5)
> > +#define ICH_LRC6_EL2               __LRC0_EL2(6)
> > +#define ICH_LRC7_EL2               __LRC0_EL2(7)
> > +#define ICH_LRC8_EL2               __LRC8_EL2(0)
> > +#define ICH_LRC9_EL2               __LRC8_EL2(1)
> > +#define ICH_LRC10_EL2              __LRC8_EL2(2)
> > +#define ICH_LRC11_EL2              __LRC8_EL2(3)
> > +#define ICH_LRC12_EL2              __LRC8_EL2(4)
> > +#define ICH_LRC13_EL2              __LRC8_EL2(5)
> > +#define ICH_LRC14_EL2              __LRC8_EL2(6)
> > +#define ICH_LRC15_EL2              __LRC8_EL2(7)
> > +
> >   #endif /* __ASSEMBLY__ */
> >     #endif /* __ASM_ARM_ARM32_SYSREGS_H */
> > diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h 
> > b/xen/arch/arm/include/asm/arm64/sysregs.h
> > index 54670084c3..d45fe815f9 100644
> > --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> > @@ -469,8 +469,11 @@
> >       asm volatile("mrs  %0, "__stringify(name) : "=r" (_r));         \
> >       _r; })
> >   -#define READ_SYSREG(name)     READ_SYSREG64(name)
> > -#define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
> > +#define READ_SYSREG(name)          READ_SYSREG64(name)
> > +#define WRITE_SYSREG(v, name)      WRITE_SYSREG64(v, name)
> Please don't re-indent existing macro. This is only introducing 
> unnecessary extra churn.
> > +#define ICH_LR_REG(index)          ICH_LR ## index ## _EL2
> > +#define WRITE_SYSREG_LR(index, v)  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/gic_v3_defs.h 
> > b/xen/arch/arm/include/asm/gic_v3_defs.h
> > index 48a1bc401e..87115f8b25 100644
> > --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> > +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> > @@ -185,9 +185,9 @@
> >   #define ICH_LR_HW_SHIFT              61
> >   #define ICH_LR_GRP_MASK              0x1
> >   #define ICH_LR_GRP_SHIFT             60
> > -#define ICH_LR_MAINTENANCE_IRQ       (1UL<<41)
> > -#define ICH_LR_GRP1                  (1UL<<60)
> > -#define ICH_LR_HW                    (1UL<<61)
> > +#define ICH_LR_MAINTENANCE_IRQ       (1ULL<<41)
> > +#define ICH_LR_GRP1                  (1ULL<<60)
> > +#define ICH_LR_HW                    (1ULL<<61)
> >     #define ICH_VTR_NRLRGS               0x3f
> >   #define ICH_VTR_PRIBITS_MASK         0x7
> Cheers,