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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 10-Aug-26 23:12, 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 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    | 18 +++++++++++++++++-
>  xen/arch/arm/include/asm/vreg.h | 20 ++++++++++++++++++++
>  xen/arch/arm/vcpreg.c           | 15 ++++++++++++++-
>  3 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index d14258290f..a02ad951f9 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -21,6 +21,7 @@
>  #include <asm/arm64/cpufeature.h>
>  #include <asm/arm64/sve.h>
>  #include <asm/current.h>
> +#include <asm/gic.h>
Stale include? Nothing references gic.h here anymore.

>  #include <asm/regs.h>
>  #include <asm/traps.h>
>  #include <asm/vreg.h>
> @@ -304,7 +305,18 @@ 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];
> +
> +        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)
>      GENERATE_TID3_INFO(ID_DFR0_EL1, dbg32, 0)
>      GENERATE_TID3_INFO(ID_DFR1_EL1, dbg32, 1)
> @@ -343,6 +355,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/vreg.h b/xen/arch/arm/include/asm/vreg.h
> index 387ce76e7e..24735aaea1 100644
> --- a/xen/arch/arm/include/asm/vreg.h
> +++ b/xen/arch/arm/include/asm/vreg.h
> @@ -9,6 +9,26 @@ typedef bool (*vreg_reg64_fn_t)(struct cpu_user_regs *regs, 
> uint64_t *r,
>  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;
> +}
> +
> +static inline register_t id_reg_set_gic_field(register_t val,
> +                                               unsigned int shift,
> +                                               const struct domain *d)
These two are incorrectly indented (off by one to the right).

> +{
> +    register_t mask = GENMASK(shift + ID_REG_GIC_WIDTH - 1, shift);
> +
> +    return (val & ~mask) |
> +           ((register_t)vgic_id_gic_field(d) << shift);
Incorrect indentation: continuation line should be indented +1

> +}
vreg.h does not include any header, so please add appropriate headers for
objects you are adding.

> +
>  static inline bool vreg_emulate_cp32(struct cpu_user_regs *regs, union hsr 
> hsr,
>                                       vreg_reg_fn_t fn)
>  {
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index e7c484f2c1..d6f9326b71 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -12,6 +12,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/cpregs.h>
>  #include <asm/current.h>
> +#include <asm/gic.h>
>  #include <asm/regs.h>
>  #include <asm/traps.h>
>  #include <asm/vreg.h>
> @@ -173,6 +174,8 @@ TVM_REG32(CONTEXTIDR, CONTEXTIDR_EL1)
>                                    domain_cpuinfo.field.bits[offset]);\
>      }
>  
> +#define ID_PFR1_GIC_SHIFT 28
Move this to cpregs.h and remove one from arm64/sysregs.h to avoid duplicate
entries.

~Michal

> +
>  /* helper to define cases for all registers for one CRm value */
>  #define HSR_CPREG32_TID3_CASES(REG)     case HSR_CPREG32(p15,0,c0,REG,0): \
>                                          case HSR_CPREG32(p15,0,c0,REG,1): \
> @@ -321,7 +324,17 @@ void do_cp15_32(struct cpu_user_regs *regs, const union 
> hsr hsr)
>       * to identify the processor features
>       */
>      GENERATE_TID3_INFO(ID_PFR0, pfr32, 0)
> -    GENERATE_TID3_INFO(ID_PFR1, pfr32, 1)
> +    case HSR_CPREG32(ID_PFR1):
> +    {
> +        register_t guest_reg_value = domain_cpuinfo.pfr32.bits[1];
> +
> +        guest_reg_value = id_reg_set_gic_field(guest_reg_value,
> +                                               ID_PFR1_GIC_SHIFT,
> +                                               v->domain);
> +
> +        return handle_ro_read_val(regs, regidx, cp32.read, hsr, 1,
> +                                  guest_reg_value);
> +    }
>      GENERATE_TID3_INFO(ID_PFR2, pfr32, 2)
>      GENERATE_TID3_INFO(ID_DFR0, dbg32, 0)
>      GENERATE_TID3_INFO(ID_DFR1, dbg32, 1)