Re: [PATCH v4] xen/arm: derive GIC CPU interface ID fields from the vGIC

[Mykola Kvach <mykola_kvach@xxxxxxxx>]

Hi Michal,

Thanks again for reviewing the new version.

On Tue, Sep 22, 2026 at 12:40:45PM +0200, Orzel, Michal wrote:
> 
> 
> On 22-Sep-26 09:21, Mykola Kvach wrote:
> > Xen exposes ID_AA64PFR0_EL1.GIC and ID_PFR1.GIC from domain_cpuinfo,
> > which is initialized from the sanitized host CPU feature state. This
> > does not necessarily match the virtual interrupt controller configured
> > for a domain.
> > 
> > A vGICv2 domain can therefore observe a nonzero GIC field when the host
> > supports the GIC system register interface, even though Xen disables that
> > interface for the domain. On a GICv4.1-capable host, a vGICv3 domain can
> > observe encoding 0b0011, although Xen exposes only its vGICv3 model.
> > 
> > Derive the fields from the domain's vGIC version instead. Expose 0b0000
> > for vGICv2 and 0b0001 for vGICv3. This covers ID_AA64PFR0_EL1 and the
> > ID_PFR1_EL1 alias in AArch64 state, as well as ID_PFR1 accessed through
> > CP15 in AArch32 state. Leave the alias unchanged when AArch32 is
> > unavailable.
> > 
> > Fixes: 07b9acea116e ("xen/arm: Add handler for ID registers on arm64")
> > Fixes: 8f81064a07c6 ("xen/arm: Add handler for cp15 ID registers")
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in v4:
> > - Explain why ID_PFR1_EL1 is left unchanged without AArch32 EL0 support.
> > - Move all ID_PFR1_*_SHIFT definitions to the common asm/sysregs.h and
> >   remove the unused cpregs.h includes from the ARM64 files.
> > - Return explicit unsigned GIC field values to avoid MISRA C Rule 10.3.
> > 
> > Changes in v3:
> > - Add direct dependencies for the shared ID register helpers.
> > - Move ID_PFR1_GIC_SHIFT to the common CP15 register header.
> > - Apply cosmetic fixes from review.
> > 
> > Changes in v2:
> > - Share the GIC ID field helpers between the AArch64 and AArch32 paths.
> > - Parenthesize the individual ASSERT conditions.
> > - Preserve ID_PFR1_EL1.GIC when AArch32 is unavailable.
> > - Target master instead of the 4.22 release.
> > 
> > v1: 
> > https://patchew.org/Xen/ba4f779d68c54efc80c4a566dca38ac2e6f9a073.1783675708.git.mykola._5Fkvach@xxxxxxxx/
> > ---
> >  xen/arch/arm/arm64/vsysreg.c             | 21 ++++++++++++++++++-
> >  xen/arch/arm/include/asm/arm64/sysregs.h |  9 --------
> >  xen/arch/arm/include/asm/sysregs.h       |  9 ++++++++
> >  xen/arch/arm/include/asm/vreg.h          | 26 ++++++++++++++++++++++++
> >  xen/arch/arm/vcpreg.c                    | 12 ++++++++++-
> >  5 files changed, 66 insertions(+), 11 deletions(-)
> > 
> > diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> > index 66f4f23bb3..2912445301 100644
> > --- a/xen/arch/arm/arm64/vsysreg.c
> > +++ b/xen/arch/arm/arm64/vsysreg.c
> > @@ -299,7 +299,22 @@ void do_sysreg(struct cpu_user_regs *regs,
> >       * to identify the processor features
> >       */
> >      GENERATE_TID3_INFO(ID_PFR0_EL1, pfr32, 0)
> > -    GENERATE_TID3_INFO(ID_PFR1_EL1, pfr32, 1)
> > +    case HSR_SYSREG_ID_PFR1_EL1:
> > +    {
> > +        register_t guest_reg_value = domain_cpuinfo.pfr32.bits[1];
> > +
> > +        /*
> > +         * Preserve the sanitized ID_PFR1_EL1 value when AArch32 EL0
> > +         * is not supported, as for the other AArch32 ID registers.
> > +         */
> > +        if ( cpu_feature64_has_el0_32(&domain_cpuinfo) )
> > +            guest_reg_value = id_reg_set_gic_field(guest_reg_value,
> > +                                                   ID_PFR1_GIC_SHIFT,
> > +                                                   v->domain);
> > +
> > +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> > +                                  guest_reg_value);
> > +    }
> >      GENERATE_TID3_INFO(ID_PFR2_EL1, pfr32, 2)
> >  
> >      case HSR_SYSREG_ID_DFR0_EL1:
> > @@ -362,6 +377,10 @@ void do_sysreg(struct cpu_user_regs *regs,
> >              guest_reg_value |= (sysval << ID_AA64PFR0_SVE_SHIFT) & mask;
> >          }
> >  
> > +        guest_reg_value = id_reg_set_gic_field(guest_reg_value,
> > +                                               ID_AA64PFR0_GIC_SHIFT,
> > +                                               v->domain);
> > +
> >          return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> >                                    guest_reg_value);
> >      }
> > diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h 
> > b/xen/arch/arm/include/asm/arm64/sysregs.h
> > index f3c11d871e..f6ece8f972 100644
> > --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> > @@ -438,15 +438,6 @@
> >  #define MVFR1_FPDNAN_SHIFT           4
> >  #define MVFR1_FPFTZ_SHIFT            0
> >  
> > -#define ID_PFR1_GIC_SHIFT            28
> > -#define ID_PFR1_VIRT_FRAC_SHIFT      24
> > -#define ID_PFR1_SEC_FRAC_SHIFT       20
> > -#define ID_PFR1_GENTIMER_SHIFT       16
> > -#define ID_PFR1_VIRTUALIZATION_SHIFT 12
> > -#define ID_PFR1_MPROGMOD_SHIFT       8
> > -#define ID_PFR1_SECURITY_SHIFT       4
> > -#define ID_PFR1_PROGMOD_SHIFT        0
> > -
> >  #define MVFR2_FPMISC_SHIFT           4
> >  #define MVFR2_SIMDMISC_SHIFT         0
> >  
> > diff --git a/xen/arch/arm/include/asm/sysregs.h 
> > b/xen/arch/arm/include/asm/sysregs.h
> > index f6af987ef5..8dcf82a694 100644
> > --- a/xen/arch/arm/include/asm/sysregs.h
> > +++ b/xen/arch/arm/include/asm/sysregs.h
> > @@ -9,6 +9,15 @@
> >  # error "unknown ARM variant"
> >  #endif
> >  
> > +#define ID_PFR1_GIC_SHIFT            28
> > +#define ID_PFR1_VIRT_FRAC_SHIFT      24
> > +#define ID_PFR1_SEC_FRAC_SHIFT       20
> > +#define ID_PFR1_GENTIMER_SHIFT       16
> > +#define ID_PFR1_VIRTUALIZATION_SHIFT 12
> > +#define ID_PFR1_MPROGMOD_SHIFT       8
> > +#define ID_PFR1_SECURITY_SHIFT       4
> > +#define ID_PFR1_PROGMOD_SHIFT        0
> > +
> >  #ifndef __ASSEMBLER__
> >  
> >  #include <asm/alternative.h>
> > diff --git a/xen/arch/arm/include/asm/vreg.h 
> > b/xen/arch/arm/include/asm/vreg.h
> > index 387ce76e7e..96de1c4916 100644
> > --- a/xen/arch/arm/include/asm/vreg.h
> > +++ b/xen/arch/arm/include/asm/vreg.h
> > @@ -4,11 +4,37 @@
> >  #ifndef __ASM_ARM_VREG__
> >  #define __ASM_ARM_VREG__
> >  
> > +#include <xen/bitops.h>
> > +#include <xen/bug.h>
> > +#include <xen/sched.h>
> > +
> > +#include <asm/gic.h>
> > +
> >  typedef bool (*vreg_reg64_fn_t)(struct cpu_user_regs *regs, uint64_t *r,
> >                                     bool read);
> >  typedef bool (*vreg_reg_fn_t)(struct cpu_user_regs *regs, register_t *r,
> >                                     bool read);
> >  
> > +#define ID_REG_GIC_WIDTH 4
> > +
> > +static inline unsigned int vgic_id_gic_field(const struct domain *d)
> > +{
> > +    ASSERT((d->arch.vgic.version == GIC_V2) ||
> > +           (d->arch.vgic.version == GIC_V3));
> > +
> > +    return d->arch.vgic.version == GIC_V3 ? 1U : 0U;
> > +}
> > +
> > +static inline register_t id_reg_set_gic_field(register_t val,
> > +                                              unsigned int shift,
> > +                                              const struct domain *d)
> > +{
> > +    register_t mask = GENMASK(shift + ID_REG_GIC_WIDTH - 1, shift);
> > +
> > +    return (val & ~mask) |
> > +            ((register_t)vgic_id_gic_field(d) << shift);
> No need for split. It would fit 80 chars.

Ack.

> 
> > +}
> This is a header included by a few source files, so the less we expose the
> better. Please combine vgic_id_gic_field() into id_reg_set_gic_field() given 
> its
> simplicity and the fact that it is only used in the latter. Also, your new
> helpers use a vgic.h namespace (this header uses vreg_ prefix). I think it 
> makes
> sense to s/id_reg_set_gic_field/vreg_id_reg_set_gic_field/ and
> s/ID_REG_GIC_WIDTH/VREG_ID_REG_GIC_WIDTH/.

Thanks, will be addressed in v5.

> 
> With that changed:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

Best regards,
Mykola