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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hello,

Thanks for the comments.

> > 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

Okay.

> > - 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

Understood.

> >   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 ()

Okay.

> > --- 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?

Okay, I will add a trap handler entry for AArch32 PMMIR register accesses.

> > @@ -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?

Okay, I will also add a trap handler entry for it.
 
> > +    {
> > +        struct domain *d = current->domain;
> Use v->domain instead like the surrounding code

Okay.
 
> > +        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.

Okay, I will fix it.
 
> >      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

Okay.

> > +    {
> > +        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

Okay, I will.

> > +        {
> > +            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]);
> > +    }
> > +

> > @@ -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-Descri
> ptions/-D24-2-General-system-control-registers/-D24-2-79-ID-AA64DFR0-
> EL1--AArch64-Debug-Feature-Register-0?lang=en

In a slightly older Architecture Reference Manual, there was a SEBEP field
in ID_AA64DFR0_EL1. Has it been removed from the spec?
https://support.arm.com/documentation/111180/2025-09_ASL1/AArch64-Registers/ID-AA64DFR0-EL1--AArch64-Debug-Feature-Register-0?lang=en

Thank you,
Hirokazu Takahashi.