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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Michal,

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

Okay.

> >          return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> >      case HSR_SYSREG_PMUSERENR_EL0:
> >          /* RO at EL0. RAZ/WI at EL1 */


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

Okay.

> > +        union cpuinfo_dbg32 info_dbg32 = domain_cpuinfo.dbg32;
> > +
> > +        if ( !is_vpmu_domain(d) )
> > +        {
> Please omit the braces for a single line `if` block.

Okay.

> > +            info_dbg32.perfmon = 0;
> > +        }
> > +
> > +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> > +                                  info_dbg32.bits[0]);
> > +    }

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

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

Understood.

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

Okay.
 
> > +            info_dbg64.dpfzs = 0;
> > +        }
> > +
> > +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
> > +                                  info_dbg64.bits[1]);
> > +    }

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

Okay

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

Okay

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

I will fix it.

> > +                                  info_dbg32.bits[0]);
> > +    }
> > +

Thank you,
Hirokazu Takahashi.