Re: [PATCH v3] xen/arm: Hide PMU registers from the guest, when the vPMU feature is disabled.
"Orzel, Michal" <[email protected]>
| Newsgroups | org.xenproject.lists.xen-devel |
|---|---|
| Message-ID | <[email protected]> |
On 24-Aug-26 06:45, 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 and ITE
> 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 v3:
> * Mask PMU-related fields in ID_AA64DFR1_EL1 and ID_DFR1 when a guest
> domain lacks the vPMU feature.
> * Add emulation for AArch32 PMMIR register accesses performed by
> 32-bit guest domains without the vPMU feature.
> * Code cleanups.
>
> Note: The combination of AArch32 EL1 and PMUv3 could not be verified,
> as no real ARM CPU implements both while the Arm Architecture
> Reference Manual for A-profile architecture allows it.
>
>
> xen/arch/arm/arm64/vsysreg.c | 74 +++++++++++++++++++++++++--
> xen/arch/arm/cpufeature.c | 14 +++++
> 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 | 33 +++++++++++-
> xen/include/xen/sched.h | 5 ++
> 8 files changed, 143 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index d9e3619dfb..1d1f7b0631 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -227,6 +227,11 @@ void do_sysreg(struct cpu_user_regs *regs,
> */
> case HSR_SYSREG_PMINTENSET_EL1:
> case HSR_SYSREG_PMINTENCLR_EL1:
> + case HSR_SYSREG_PMMIR_EL1:
> + /*
> + * Accessible from EL1 only, but if EL0 trap happens handle as
> + * undef.
> + */
This comment was recently removed, so please do not re-introduce it.
> 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 +305,36 @@ 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:
> + {
> + struct domain *d = v->domain;
There is no need for a separate local variable if it's only used once. In this
series, please just use `v->domain` for `is_vpmu_domain()`.
> + union cpuinfo_dbg32 info_dbg32 = domain_cpuinfo.dbg32;
> +
> + if ( !is_vpmu_domain(d) )
> + {
Please omit the braces for a single line `if` block.
> + 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:
> + {
> + struct domain *d = v->domain;
> + union cpuinfo_dbg32 info_dbg32 = domain_cpuinfo.dbg32;
> +
> + if ( !is_vpmu_domain(d) )
> + {
> + 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 +375,41 @@ 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:
> + {
> + struct domain *d = v->domain;
> + union cpuinfo_dbg64 info_dbg64 = domain_cpuinfo.dbg64;
> +
> + if ( !is_vpmu_domain(d) )
> + {
> + info_dbg64.pmu_ver = 0;
> + info_dbg64.pmss = 0;
> + info_dbg64.mtpmu = 0;
Why don't you clear hpmn0?
> + }
> +
> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> + info_dbg64.bits[0]);
> + }
> +
> + case HSR_SYSREG_ID_AA64DFR1_EL1:
> + {
> + struct domain *d = v->domain;
> + union cpuinfo_dbg64 info_dbg64 = domain_cpuinfo.dbg64;
> +
> + if ( !is_vpmu_domain(d) )
> + {
> + info_dbg64.syspmuid = 0;
> + info_dbg64.spmu = 0;
System PMU accesses undef for a vPMU-enabled domain too. Clear them together
with SPE, TRBE, etc.
> + info_dbg64.pmicntr = 0;
> + info_dbg64.ebep = 0;
FEAT_EBEP is Armv9 just like SEBEP, so you should apply my comments to it as well.
> + info_dbg64.dpfzs = 0;
> + }
> +
> + 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..05f4d0f5df 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -219,8 +219,22 @@ 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 and ITE */
> + 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.ite = 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..e03218b51e 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. Performance Monitors Machine Identification Register */
Please drop "Performance Monitors". It's the same as "Perf. Mon".
>
> /* 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..a660cb0734 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -305,6 +305,7 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
> case HSR_CPREG32(PMXEVTYPER):
> case HSR_CPREG32(PMXEVCNTR):
> case HSR_CPREG32(PMOVSSET):
> + case HSR_CPREG32(PMMIR):
PMMIR is EL1 only, so it does not belong to this block and this comment. Place
it next to PMINTENCLR.
> /*
> * Accessible at EL0 only if PMUSERENR_EL0.EN is set. We
> * emulate that register as 0 above.
> @@ -320,8 +321,36 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
> GENERATE_TID3_INFO(ID_PFR0, pfr32, 0)
> GENERATE_TID3_INFO(ID_PFR1, pfr32, 1)
> GENERATE_TID3_INFO(ID_PFR2, pfr32, 2)
> - GENERATE_TID3_INFO(ID_DFR0, dbg32, 0)
> - GENERATE_TID3_INFO(ID_DFR1, dbg32, 1)
> +
> + case HSR_CPREG32(ID_DFR0):
> + {
> + struct domain *d = v->domain;
> + union cpuinfo_dbg32 info_dbg32 = domain_cpuinfo.dbg32;
> +
> + if ( !is_vpmu_domain(d) )
> + {
> + info_dbg32.perfmon = 0;
> + }
> +
> + return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
This breaks the arm32 build (you should always at least build test the patches):
s/hsr.sysreg.read/cp32.read/
> + info_dbg32.bits[0]);
> + }
> +
> + case HSR_CPREG32(ID_DFR1):
> + {
> + struct domain *d = v->domain;
> + union cpuinfo_dbg32 info_dbg32 = domain_cpuinfo.dbg32;
> +
> + if ( !is_vpmu_domain(d) )
> + {
> + 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, aux32, 0)
> GENERATE_TID3_INFO(ID_MMFR0, mm32, 0)
> GENERATE_TID3_INFO(ID_MMFR1, mm32, 1)
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index eef10c2ea2..e352e2b38e 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1269,6 +1269,11 @@ static always_inline bool is_iommu_enabled(const struct domain *d)
> return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu);
> }
>
> +static inline bool is_vpmu_domain(const struct domain *d)
> +{
> + return d->options & XEN_DOMCTL_CDF_vpmu;
> +}
> +
> #ifdef CONFIG_MEM_PAGING
> # define mem_paging_enabled(d) vm_event_check_ring((d)->vm_event_paging)
> #else
~Michal