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

On 13-Aug-26 05:29, 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, I implement the following:
Please use the imperative mood

> 
> - Hide PMU registers from a guest domain when its vPMU feature is
>   disabled.
> - Proactively make SPE, TRBE, BRBE, and Trace Extensions inaccessible
>   to guest domains, as they could potentially cause similar issues.
> - Add emulation for PMMIR_EL1 register accesses performed by a guest
>   domain when vPMU is enabled. However, similar to reads from other
When vPMU is enabled, there is no trap/emulation

>   PMU registers, the read value returns zero (note that this is a
>   temporary implementation).
> - Emulation for PMSS (PMU Snapshot) register accesses is not yet
>   implemented, because PMSS support is not available in
>   qemu-system-aarch64 and could not be verified.
> 
> Fixes: 07b9acea116e "xen/arm: Add handler for ID registers on arm64"
> Fixes: 3669a1cb9598 "xen/arm: create a cpuinfo structure for guest"
Fixes commit title needs to be in brackets ()

> Signed-off-by: Hirokazu Takahashi <[email protected]>
> ---
> Changes in v2:
>  * Instead of unconditionally hiding the PMU feature from guests,
>    we now determine whether to expose PMU to a guest domain based on
>    its configuration.
> 
>  xen/arch/arm/arm64/vsysreg.c          | 31 +++++++++++++++++++++++++--
>  xen/arch/arm/cpufeature.c             |  8 +++++++
>  xen/arch/arm/include/asm/arm64/hsr.h  |  1 +
>  xen/arch/arm/include/asm/cpufeature.h | 14 ++++++------
>  4 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index d14258290f..520faa02ca 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -229,6 +229,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>       */
>      case HSR_SYSREG_PMINTENSET_EL1:
>      case HSR_SYSREG_PMINTENCLR_EL1:
> +    case HSR_SYSREG_PMMIR_EL1:
What about AArch32 PMMIR?

>          /*
>           * Accessible from EL1 only, but if EL0 trap happens handle as
>           * undef.
> @@ -306,7 +307,6 @@ 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)
>      GENERATE_TID3_INFO(ID_AFR0_EL1, aux32, 0)
>      GENERATE_TID3_INFO(ID_MMFR0_EL1, mm32, 0)
> @@ -326,6 +326,18 @@ void do_sysreg(struct cpu_user_regs *regs,
>      GENERATE_TID3_INFO(MVFR1_EL1, mvfr, 1)
>      GENERATE_TID3_INFO(MVFR2_EL1, mvfr, 2)
>  
> +    case HSR_SYSREG_ID_DFR0_EL1:
You only cover AArch64. What about AArch32 DFR0?

> +    {
> +        struct domain *d = current->domain;
Use v->domain instead like the surrounding code

> +        union cpuinfo_dbg32 info_dbg32 = domain_cpuinfo.dbg32;
> +
> +        if ( !(d->options & XEN_DOMCTL_CDF_vpmu) )
> +            info_dbg32.perfmon = 0;
> +
> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> +                                  info_dbg32.bits[0]);
> +    }
> +
>      case HSR_SYSREG_ID_AA64PFR0_EL1:
>      {
>          register_t guest_reg_value = domain_cpuinfo.pfr64.bits[0];
> @@ -348,7 +360,6 @@ 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)
What about DFR1 fields like PMICNTR? They suffer from the same problem.

>      GENERATE_TID3_INFO(ID_AA64ISAR0_EL1, isa64, 0)
>      GENERATE_TID3_INFO(ID_AA64ISAR1_EL1, isa64, 1)
> @@ -358,6 +369,22 @@ void do_sysreg(struct cpu_user_regs *regs,
>      GENERATE_TID3_INFO(ID_AA64AFR0_EL1, aux64, 0)
>      GENERATE_TID3_INFO(ID_AA64AFR1_EL1, aux64, 1)
>  
> +    case HSR_SYSREG_ID_AA64DFR0_EL1:
Please adhere to the order in which the cases were originally placed
> +    {
> +        struct domain *d = current->domain;
> +        union cpuinfo_dbg64 info_dbg64 = domain_cpuinfo.dbg64;
> +
> +        if ( !(d->options & XEN_DOMCTL_CDF_vpmu) )
To avoid duplication, please introduce is_vpmu_domain
> +        {
> +            info_dbg64.pmu_ver = 0;
> +            info_dbg64.mtpmu = 0;
> +            info_dbg64.pmss = 0;
> +        }
> +
> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> +                                  info_dbg64.bits[0]);
> +    }
> +
>      case HSR_SYSREG_ID_AA64ZFR0_EL1:
>      {
>          /*
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index 94d14fb6a9..0e9bf15ca5 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -219,6 +219,14 @@ 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, and Trace Extensions */
> +    domain_cpuinfo.dbg64.pms_ver = 0;
> +    domain_cpuinfo.dbg64.trace_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;
>  #endif
>  
>      /* Hide AMU support */
> 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/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
> index bf902a3970..ce8b58458f 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,16 +216,18 @@ 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 sebep:4;
Where did you take this field from? I can't see it in the latest Arm ARM:
https://support.arm.com/documentation/ddi0487/mc/-Part-D-The-AArch64-System-Level-Architecture/-Chapter-D24-AArch64-System-Register-Descriptions/-D24-2-General-system-control-registers/-D24-2-79-ID-AA64DFR0-EL1--AArch64-Debug-Feature-Register-0?lang=en

>              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;
> @@ -408,7 +410,7 @@ struct cpuinfo_arm {
>          };
>      } pfr32;
>  
> -    union {
> +    union cpuinfo_dbg32 {
>          register_t bits[2];
>          struct {
>              /* DFR0 */

~Michal