Re: [PATCH v3] xen/arm: Hide PMU registers from the guest, when the vPMU feature is disabled.

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

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 <taka@xxxxxxxxxxxxx>
> ---
> 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