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

[Julien Grall <julien@xxxxxxx>]

On 28/10/2022 15:22, Ayan Kumar Halder wrote:
> 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.
That's my mistake. We should use...

> However, READ_SYSREG is defined as READ_CP32(), so should we use 
> READ_CP32() or READ_SYSREG() ?
READ_CP32() instead of READ_SYSREG() for arm32 specific code. The latter 
is only provided to avoid #ifdef in the common code.
Cheers,

--
Julien Grall