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

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

On 25-Sep-26 00:45, Volodymyr Babchuk wrote:
> Hi Mykola,
> 
> Mykola Kvach <mykola_kvach@xxxxxxxx> writes:
> 
>> 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>
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> Changes in v5:
>> - Fold the vGIC version mapping into the shared ID register helper.
>> - Use the vreg_ and VREG_ prefixes for the helper and field width.
>> - Cosmetic changes after review
>>
>> 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          | 20 ++++++++++++++++++++
>>  xen/arch/arm/vcpreg.c                    | 12 +++++++++++-
>>  5 files changed, 60 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>> index 66f4f23bb3..d8f380ccb7 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 = vreg_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 = vreg_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..a85fefcc3e 100644
>> --- a/xen/arch/arm/include/asm/vreg.h
>> +++ b/xen/arch/arm/include/asm/vreg.h
>> @@ -4,11 +4,31 @@
>>  #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 VREG_ID_REG_GIC_WIDTH 4
> 
> Should this go into sysregs.h?
> 
> Something like ID_AArchx_PFR_GIC_SHIFT? I am open to alternatives in naming.
I don't think that's necessary, given that we don't have any _WIDTH macros in
there and other places hard code widths (e.g. SVE). Especially that we are now
at v5 and we should avoid subjective NITs.

> 
>> +
>> +static inline register_t vreg_id_reg_set_gic_field(register_t val,
>> +                                                   unsigned int shift,
>> +                                                   const struct domain *d)
>> +{
>> +    register_t mask = GENMASK(shift + VREG_ID_REG_GIC_WIDTH - 1, shift);
>> +    enum gic_version vgic_ver = d->arch.vgic.version;
>> +
>> +    ASSERT((vgic_ver == GIC_V2) || (vgic_ver == GIC_V3));
>> +
>> +    return (val & ~mask) | ((vgic_ver == GIC_V3 ? 1U : 0U) << shift);
> 
> Probably better to use constants?
> 
> Something like ID_AArchx_PFR_GIC_V3 and ID_AArchx_PFR_GIC_NO_CPU_INTF.
This suggestion is valid. Please add in common sysregs.h:
/* GIC field encodings, common to ID_PFR1{,_EL1} and ID_AA64PFR0_EL1 */
#define ID_PFR_GIC_NI    0x0
#define ID_PFR_GIC_V3    0x1

You can retain my Rb.

~Michal