Re: [PATCH v4] xen/arm: Hide PMU registers from the guest, when the vPMU feature is disabled.
"Orzel, Michal" <[email protected]>
| Newsgroups | gmane.comp.emulators.xen.devel |
|---|---|
| Message-ID | <[email protected]> |
On 26-Aug-26 12:55, Hirokazu Takahashi wrote:
> On ARMv8.4-A and newer platforms, booting Dom0 Linux with ACPI enabled
> causes the domain to probe advanced PMU feature based on system ID
> register ID_AA64DFR0_EL1. During this probe, Linux accesses PMMIR_EL1,
> which causes unhandled register traps and crashes the domain.
>
> To address this issue, implement the following:
>
> - Add emulation for ID_AA64DFR{0,1}_EL1, ID_DFR{0,1}_EL1 and
> ID_DFR{0,1} register accesses for guest domains.
> - Hide PMU registers from a guest domain when its vPMU feature is
> disabled.
> - Proactively make SPE, TRBE, BRBE, Trace Extensions, SPMU, ITE and
> EBEP inaccessible to arm64 guest domains and hide TraceFilt,
> MMapTrc and CopTrc to arm32 guest domains, as they could
> potentially cause similar issues.
>
> Fixes: dbb948110a0e ("xen: Expose the PMU to the guests")
> Fixes: 07b9acea116e ("xen/arm: Add handler for ID registers on arm64")
> Fixes: 3669a1cb9598 ("xen/arm: create a cpuinfo structure for guest")
> Signed-off-by: Hirokazu Takahashi <[email protected]>
> ---
> Changes in v4:
> * Hide FEAT_HPMN0 from guests when the vPMU feature is disabled.
> * Unconditionally hide FEAT_SPMU and FEAT_EBEP from guest domains.
> * Fix arm32 build failure in vcpreg.c.
> * Perform code cleanups and address coding style feedback.
>
> xen/arch/arm/arm64/vsysreg.c | 62 +++++++++++++++++++++++++--
> xen/arch/arm/cpufeature.c | 17 ++++++++
> xen/arch/arm/domain.c | 2 +-
> xen/arch/arm/include/asm/arm64/hsr.h | 1 +
> xen/arch/arm/include/asm/cpregs.h | 1 +
> xen/arch/arm/include/asm/cpufeature.h | 27 +++++++++---
> xen/arch/arm/vcpreg.c | 29 ++++++++++++-
> xen/include/xen/sched.h | 5 +++
> 8 files changed, 130 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index d9e3619dfb..bf612a133d 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -227,6 +227,7 @@ void do_sysreg(struct cpu_user_regs *regs,
> */
> case HSR_SYSREG_PMINTENSET_EL1:
> case HSR_SYSREG_PMINTENCLR_EL1:
> + case HSR_SYSREG_PMMIR_EL1:
> return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> case HSR_SYSREG_PMUSERENR_EL0:
> /* RO at EL0. RAZ/WI at EL1 */
> @@ -300,8 +301,32 @@ void do_sysreg(struct cpu_user_regs *regs,
> GENERATE_TID3_INFO(ID_PFR0_EL1, pfr32, 0)
> GENERATE_TID3_INFO(ID_PFR1_EL1, pfr32, 1)
> GENERATE_TID3_INFO(ID_PFR2_EL1, pfr32, 2)
> - GENERATE_TID3_INFO(ID_DFR0_EL1, dbg32, 0)
> - GENERATE_TID3_INFO(ID_DFR1_EL1, dbg32, 1)
> +
> + case HSR_SYSREG_ID_DFR0_EL1:
> + {
> + union cpuinfo_dbg32 info_dbg32 = domain_cpuinfo.dbg32;
> +
> + if ( !is_vpmu_domain(v->domain) )
> + info_dbg32.perfmon = 0;
> +
> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> + info_dbg32.bits[0]);
> + }
> +
> + case HSR_SYSREG_ID_DFR1_EL1:
> + {
> + union cpuinfo_dbg32 info_dbg32 = domain_cpuinfo.dbg32;
> +
> + if ( !is_vpmu_domain(v->domain) )
> + {
> + info_dbg32.mtpmu = 0;
> + info_dbg32.hpmn0 = 0;
> + }
> +
> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> + info_dbg32.bits[1]);
> + }
> +
> GENERATE_TID3_INFO(ID_AFR0_EL1, aux32, 0)
> GENERATE_TID3_INFO(ID_MMFR0_EL1, mm32, 0)
> GENERATE_TID3_INFO(ID_MMFR1_EL1, mm32, 1)
> @@ -342,8 +367,37 @@ void do_sysreg(struct cpu_user_regs *regs,
> }
>
> GENERATE_TID3_INFO(ID_AA64PFR1_EL1, pfr64, 1)
> - GENERATE_TID3_INFO(ID_AA64DFR0_EL1, dbg64, 0)
> - GENERATE_TID3_INFO(ID_AA64DFR1_EL1, dbg64, 1)
> +
> + case HSR_SYSREG_ID_AA64DFR0_EL1:
> + {
> + union cpuinfo_dbg64 info_dbg64 = domain_cpuinfo.dbg64;
> +
> + if ( !is_vpmu_domain(v->domain) )
> + {
> + info_dbg64.pmu_ver = 0;
> + info_dbg64.pmss = 0;
> + info_dbg64.mtpmu = 0;
> + info_dbg64.hpmn0 = 0;
> + }
> +
> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> + info_dbg64.bits[0]);
> + }
> +
> + case HSR_SYSREG_ID_AA64DFR1_EL1:
> + {
> + union cpuinfo_dbg64 info_dbg64 = domain_cpuinfo.dbg64;
> +
> + if ( !is_vpmu_domain(v->domain) )
> + {
> + info_dbg64.pmicntr = 0;
> + info_dbg64.dpfzs = 0;
FEAT_SPE_DPFZS depends on SPE, and you already clear pms_ver. Move it next to
clearing pms_ver.
> + }
> +
> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> + info_dbg64.bits[1]);
> + }
> +
> GENERATE_TID3_INFO(ID_AA64ISAR0_EL1, isa64, 0)
> GENERATE_TID3_INFO(ID_AA64ISAR1_EL1, isa64, 1)
> GENERATE_TID3_INFO(ID_AA64MMFR0_EL1, mm64, 0)
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index 94d14fb6a9..88478fea46 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -219,8 +219,25 @@ static int __init create_domain_cpuinfo(void)
> domain_cpuinfo.isa64.api = 0;
> domain_cpuinfo.isa64.gpa = 0;
> domain_cpuinfo.isa64.gpi = 0;
> +
> + /* Hide SPE, TRBE, BRBE, Trace Extensions, SPMU, ITE and EBEP */
> + domain_cpuinfo.dbg64.trace_ver = 0;
> + domain_cpuinfo.dbg64.pms_ver = 0;
> + domain_cpuinfo.dbg64.trace_filt = 0;
> + domain_cpuinfo.dbg64.trace_buffer = 0;
> + domain_cpuinfo.dbg64.ext_trc_buff = 0;
> + domain_cpuinfo.dbg64.brbe = 0;
> + domain_cpuinfo.dbg64.syspmuid = 0;
> + domain_cpuinfo.dbg64.spmu = 0;
> + domain_cpuinfo.dbg64.ite = 0;
> + domain_cpuinfo.dbg64.ebep = 0;
> #endif
>
> + /* Hide Trace Extensions for AArch32 domain */
> + domain_cpuinfo.dbg32.coptrc = 0;
> + domain_cpuinfo.dbg32.mmaptrc = 0;
> + domain_cpuinfo.dbg32.tracefilt = 0;
> +
> /* Hide AMU support */
> #ifdef CONFIG_ARM_64
> domain_cpuinfo.pfr64.amu = 0;
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index baa3a5d708..a739dd157e 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -501,7 +501,7 @@ int arch_vcpu_create(struct vcpu *v)
> v->arch.hcr_el2 = get_default_hcr_flags();
>
> v->arch.mdcr_el2 = HDCR_TDRA | HDCR_TDOSA | HDCR_TDA;
> - if ( !(v->domain->options & XEN_DOMCTL_CDF_vpmu) )
> + if ( !is_vpmu_domain(v->domain) )
> v->arch.mdcr_el2 |= HDCR_TPM | HDCR_TPMCR;
>
> if ( (rc = vcpu_vgic_init(v)) != 0 )
> diff --git a/xen/arch/arm/include/asm/arm64/hsr.h b/xen/arch/arm/include/asm/arm64/hsr.h
> index 1495ccddea..ed18184cc7 100644
> --- a/xen/arch/arm/include/asm/arm64/hsr.h
> +++ b/xen/arch/arm/include/asm/arm64/hsr.h
> @@ -84,6 +84,7 @@
> #define HSR_SYSREG_FAR_EL1 HSR_SYSREG(3,0,c6, c0,0)
> #define HSR_SYSREG_PMINTENSET_EL1 HSR_SYSREG(3,0,c9,c14,1)
> #define HSR_SYSREG_PMINTENCLR_EL1 HSR_SYSREG(3,0,c9,c14,2)
> +#define HSR_SYSREG_PMMIR_EL1 HSR_SYSREG(3,0,c9,c14,6)
> #define HSR_SYSREG_MAIR_EL1 HSR_SYSREG(3,0,c10,c2,0)
> #define HSR_SYSREG_AMAIR_EL1 HSR_SYSREG(3,0,c10,c3,0)
> #define HSR_SYSREG_ICC_SGI1R_EL1 HSR_SYSREG(3,0,c12,c11,5)
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index a7503a190f..3f62b38dc0 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -246,6 +246,7 @@
> #define PMINTENSET p15,0,c9,c14,1 /* Perf. Mon. Interrupt Enable Set Register */
> #define PMINTENCLR p15,0,c9,c14,2 /* Perf. Mon. Interrupt Enable Clear Register */
> #define PMOVSSET p15,0,c9,c14,3 /* Perf. Mon. Overflow Flag Status Set register */
> +#define PMMIR p15,0,c9,c14,6 /* Perf. Mon. Machine Identification Register */
>
> /* CP15 CR10: */
> #define MAIR0 p15,0,c10,c2,0 /* Memory Attribute Indirection Register 0 AKA PRRR */
> diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
> index bf902a3970..c554686415 100644
> --- a/xen/arch/arm/include/asm/cpufeature.h
> +++ b/xen/arch/arm/include/asm/cpufeature.h
> @@ -208,7 +208,7 @@ struct cpuinfo_arm {
> };
> } pfr64;
>
> - union {
> + union cpuinfo_dbg64 {
> register_t bits[2];
> struct {
> /* DFR0 */
> @@ -216,19 +216,31 @@ struct cpuinfo_arm {
> unsigned long trace_ver:4;
> unsigned long pmu_ver:4;
> unsigned long brps:4;
> - unsigned long __res0:4;
> + unsigned long pmss:4;
> unsigned long wrps:4;
> unsigned long __res1:4;
> unsigned long ctx_cmps:4;
> unsigned long pms_ver:4;
> unsigned long double_lock:4;
> unsigned long trace_filt:4;
> - unsigned long __res2:4;
> + unsigned long trace_buffer:4;
> unsigned long mtpmu:4;
> - unsigned long __res3:12;
> + unsigned long brbe:4;
> + unsigned long ext_trc_buff:4;
> + unsigned long hpmn0:4;
>
> /* DFR1 */
> - unsigned long __res4:64;
> + unsigned long syspmuid:8;
> + unsigned long brps1:8;
> + unsigned long wrps1:8;
> + unsigned long ctx_cmps1:8;
> + unsigned long spmu:4;
> + unsigned long pmicntr:4;
> + unsigned long able:4;
> + unsigned long ite:4;
> + unsigned long ebep:4;
> + unsigned long dpfzs:4;
> + unsigned long abl_cmps:8;
> };
> } dbg64;
>
> @@ -408,7 +420,7 @@ struct cpuinfo_arm {
> };
> } pfr32;
>
> - union {
> + union cpuinfo_dbg32 {
> register_t bits[2];
> struct {
> /* DFR0 */
> @@ -426,7 +438,8 @@ struct cpuinfo_arm {
>
> /* DFR1 */
> unsigned long mtpmu:4;
> - unsigned long __res1:28;
> + unsigned long hpmn0:4;
> + unsigned long __res1:24;
> #ifdef CONFIG_ARM_64
> unsigned long __res2:32;
> #endif
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index 3205c7df46..db8909d96e 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -292,6 +292,7 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
> return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
> case HSR_CPREG32(PMINTENSET):
> case HSR_CPREG32(PMINTENCLR):
> + case HSR_CPREG32(PMMIR):
There's a comment above this block that we don't trap ID_DFR0 which is not true.
It should be dropped as part of this patch.
I'll do the fixes on commit. Thanks for the patch.
Reviewed-by: Michal Orzel <[email protected]>
~Michal