Re: [PATCH v3] xen/arm: derive GIC CPU interface ID fields from the vGIC
"Orzel, Michal" <[email protected]>
| Newsgroups | gmane.comp.emulators.xen.devel |
|---|---|
| Message-ID | <[email protected]> |
On 18-Aug-26 11:19, 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 <[email protected]>
> ---
> 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@epam.com/
> ---
> xen/arch/arm/arm64/cpufeature.c | 1 +
> xen/arch/arm/arm64/vsysreg.c | 18 +++++++++++++++-
> xen/arch/arm/include/asm/arm64/sysregs.h | 1 -
> xen/arch/arm/include/asm/cpregs.h | 1 +
> xen/arch/arm/include/asm/vreg.h | 26 ++++++++++++++++++++++++
> xen/arch/arm/vcpreg.c | 12 ++++++++++-
> 6 files changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
> index 6fb8974ade..8bae8915e8 100644
> --- a/xen/arch/arm/arm64/cpufeature.c
> +++ b/xen/arch/arm/arm64/cpufeature.c
> @@ -72,6 +72,7 @@
> #include <xen/bug.h>
> #include <xen/types.h>
> #include <xen/kernel.h>
> +#include <asm/cpregs.h>
> #include <asm/sysregs.h>
> #include <asm/cpufeature.h>
> #include <asm/arm64/cpufeature.h>
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index d9e3619dfb..4fb50b6972 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -20,6 +20,7 @@
>
> #include <asm/arm64/cpufeature.h>
> #include <asm/arm64/sve.h>
> +#include <asm/cpregs.h>
> #include <asm/current.h>
> #include <asm/regs.h>
> #include <asm/traps.h>
> @@ -298,7 +299,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) )
This check deserves a comment.
> + 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)
> @@ -337,6 +349,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..c0a6827b08 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -438,7 +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
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index a7503a190f..79203324fa 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -114,6 +114,7 @@
> #define MPIDR p15,0,c0,c0,5 /* Multiprocessor Affinity Register */
> #define ID_PFR0 p15,0,c0,c1,0 /* Processor Feature Register 0 */
> #define ID_PFR1 p15,0,c0,c1,1 /* Processor Feature Register 1 */
> +#define ID_PFR1_GIC_SHIFT 28
It does not look nice to split CP15 encoding list with this macro. Furthermore,
it looks a bit odd to move just GIC and not the other fields. Also, how about
moving the block to asm/sysregs.h? You would not need to include cpregs.h then.
> #define ID_PFR2 p15,0,c0,c3,4 /* Processor Feature Register 2 */
> #define ID_DFR0 p15,0,c0,c1,2 /* Debug Feature Register 0 */
> #define ID_DFR1 p15,0,c0,c3,5 /* Debug Feature Register 1 */
> diff --git a/xen/arch/arm/include/asm/vreg.h b/xen/arch/arm/include/asm/vreg.h
> index 387ce76e7e..bb6c904f2e 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;
Wouldn't this be a violation of MISRA C R10.3?
~Michal