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

On 10-Jul-26 11:31, 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.
> 
> On a GICv3 or newer host, a vGICv2 domain can observe a nonzero GIC
> field even though Xen disables EL1 access to the GIC CPU system register
> interface for that domain. On a GICv4.1 host, a vGICv3 domain can
> observe encoding 0b0011, which advertises GICv4.1 CPU interface support
> that Xen's vGICv3 model does not expose.
> 
> Derive both fields from d->arch.vgic.version in every trap path. 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.
> 
> This is consistent with KVM, which derives both ID fields from the
> configured virtual GIC model.
> 
> 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 <[email protected]>
> ---
>  xen/arch/arm/arm64/vsysreg.c | 35 ++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/vcpreg.c        | 25 ++++++++++++++++++++++++-
>  2 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index d14258290f..6b898f13f3 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>
>  #include <asm/regs.h>
>  #include <asm/traps.h>
>  #include <asm/vreg.h>
> @@ -79,6 +80,24 @@ TVM_REG(CONTEXTIDR_EL1)
>                                    1, domain_cpuinfo.field.bits[offset]); \
>      }
>  
> +#define ID_REG_GIC_WIDTH 4
> +
> +static unsigned int vgic_id_gic_field(const struct domain *d)
> +{
> +    ASSERT(d->arch.vgic.version == GIC_V2 ||
NIT: Put brackets around individual expressions

> +           d->arch.vgic.version == GIC_V3);
> +
> +    return d->arch.vgic.version == GIC_V3;
> +}
> +
> +static 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);
> +}
vgic_id_gic_field() and ID_REG_GIC_WIDTH are copied 1:1 into vcpreg.c,
and vcpreg.c open-codes the mask/insert that id_reg_set_gic_field() already
provides. Please factor the shared logic into one place that both files include.

~Michal