RE: [PATCH] xen/arm: Hide PMU feature

Hi,

Thank you for the comments.

> > During this probe, Linux accesses PMMIR_EL1, which causes unhandled
> > register traps and crashes the domain. Merely adding emulation code
> > for PMMIR_EL1 in Xen is insufficient to fix the issue, as the guest
> > PMU driver subsequently stalls during its initialization sequence.
> >
> > Fix this by explicitly masking PMU capability fields in
> > create_domain_cpuinfo(). Additionally, preemptively mask other
> > capability fields to prevent similar potential issues.
> 
> While I agree Xen doesn't support PMU capability for every guest, I
> believe we are still allowing to expose the PMU in some cases (see
> commit dbb948110a "xen: Expose the PMU to the guests"). So we can't
> simply mask the features. So I think ...

I will modify the sysreg trap handler for ID_AA64DFR0_EL1 to dynamically
mask the basic PMU capabilities based on the domain's vPMU
configuration.

> > Fixes: 3669a1cb9598 "xen/arm: create a cpuinfo structure for guest"
> > Signed-off-by: Hirokazu Takahashi <[email protected]>
> > ---
> >   xen/arch/arm/cpufeature.c             | 16 ++++++++++++++++
> >   xen/arch/arm/include/asm/cpufeature.h | 10 ++++++----
> >   2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> > index 94d14fb6a9..71d745d3cb 100644
> > --- a/xen/arch/arm/cpufeature.c
> > +++ b/xen/arch/arm/cpufeature.c
> > @@ -219,8 +219,24 @@ static int __init create_domain_cpuinfo(void)
> >       domain_cpuinfo.isa64.api = 0;
> >       domain_cpuinfo.isa64.gpa = 0;
> >       domain_cpuinfo.isa64.gpi = 0;
> > +
> > +    /* Hide PMUv3 support as Xen does not support it */
> > +    domain_cpuinfo.dbg64.pmu_ver = 0;
> > +    domain_cpuinfo.dbg64.mtpmu = 0;
> > +    domain_cpuinfo.dbg64.pmss = 0;
> 
> ... this section needs to be conditional.

Okay.
 
> > +
> > +    /* 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;
> 
> This section should be fine to unconditionally mask.

Okay.

> >   #endif
> >
> > +    /* Hide PMUv1,v2 support as Xen does not support it */
> > +    domain_cpuinfo.dbg32.perfmon = 0;
> > +
> >       /* Hide AMU support */
> >   #ifdef CONFIG_ARM_64
> >       domain_cpuinfo.pfr64.amu = 0;
> > diff --git a/xen/arch/arm/include/asm/cpufeature.h
> b/xen/arch/arm/include/asm/cpufeature.h
> > index bf902a3970..c92b2651c7 100644
> > --- a/xen/arch/arm/include/asm/cpufeature.h
> > +++ b/xen/arch/arm/include/asm/cpufeature.h
> > @@ -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;
> >               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;

Thak you,
Hirokazu Takahashi.