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

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

On 18-Aug-26 10:50, Hirokazu Takahashi wrote:
> 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?
I can see it's been added since Armv9.3. I'd recommend leaving it as RES given
that you do not use it anyway in this patch. We usually do the update looking at
the latest Armv8-A spec.

~Michal