Re: [PATCH 3/5] x86/perf: expose LBR format in PERF_CAPABILITIES

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 20, 2022 at 02:10:31PM +0000, Andrew Cooper wrote:
> On 20/05/2022 14:37, Roger Pau Monne wrote:
> > Allow exposing the PDCM bit in CPUID for HVM guests if present on the
> > platform, which in turn allows exposing PERF_CAPABILITIES.  Limit the
> > information exposed in PERF_CAPABILITIES to the LBR format only.
> >
> > This is helpful as hardware without model-specific LBRs set format to
> > 0x3f in order to notify the feature is not present.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Seeing as we have never exposed PDCM in CPUID I wonder whether there's
> > something that I'm missing that makes exposing PERF_CAPABILITIES LBR
> > format not as trivial as it looks.
> > ---
> >  xen/arch/x86/msr.c                          | 9 +++++++++
> >  xen/include/public/arch-x86/cpufeatureset.h | 2 +-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> > index 01a15857b7..423a795d1d 100644
> > --- a/xen/arch/x86/msr.c
> > +++ b/xen/arch/x86/msr.c
> > @@ -316,6 +316,15 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
> > *val)
> >          *val = 0;
> >          break;
> >  
> > +    case MSR_IA32_PERF_CAPABILITIES:
> > +        if ( !cp->basic.pdcm )
> > +            goto gp_fault;
> > +
> > +        /* Only report LBR format. */
> > +        rdmsrl(MSR_IA32_PERF_CAPABILITIES, *val);
> > +        *val &= MSR_IA32_PERF_CAP_LBR_FORMAT;
> 
> Urgh.  We should have this info cached from boot.  Except caching on
> boot is broken on hybrid cpus.  The P and E cores of an AlderLake report
> a different format iirc (they differ between linear, and effective addr).
> 
> Given the other pain points with hybrid cpus, I think we can ignore it
> in the short term.
> 
> > +        break;
> > +
> >      case MSR_X2APIC_FIRST ... MSR_X2APIC_LAST:
> >          if ( !is_hvm_domain(d) || v != curr )
> >              goto gp_fault;
> > diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
> > b/xen/include/public/arch-x86/cpufeatureset.h
> > index cd6409f9f3..5fdaec43c5 100644
> > --- a/xen/include/public/arch-x86/cpufeatureset.h
> > +++ b/xen/include/public/arch-x86/cpufeatureset.h
> > @@ -135,7 +135,7 @@ XEN_CPUFEATURE(SSSE3,         1*32+ 9) /*A  
> > Supplemental Streaming SIMD Extensio
> >  XEN_CPUFEATURE(FMA,           1*32+12) /*A  Fused Multiply Add */
> >  XEN_CPUFEATURE(CX16,          1*32+13) /*A  CMPXCHG16B */
> >  XEN_CPUFEATURE(XTPR,          1*32+14) /*   Send Task Priority Messages */
> > -XEN_CPUFEATURE(PDCM,          1*32+15) /*   Perf/Debug Capability MSR */
> > +XEN_CPUFEATURE(PDCM,          1*32+15) /*S  Perf/Debug Capability MSR */
> 
> This is the bit which requires more toolstack logic to safely enable. 
> Using 's' for off-by-default is fine if we want to get the series in now.
> 
> But before we expose the MSR generally, we need to:
> 
> 1) Put the configuration in msr_policy so the toolstack can reason about it
> 2) Reject migration attempts to destinations where the LBR format changes
> 3) Actually put the lBR registers in the migration stream

So far we have allowed guests to enable LBRs (DEBUGCTLMSR.LBR) freely
without any restrictions, and migration of guests using LBRs certainly
won't work currently, hence I wonder why exposing the LBR format makes
it worse as to require all this extra handling.

I'm not saying it's not worth having, but IMO we should better spend
the time in getting architectural LBRs available to guests and
migration safe, and for architectural LBRs we don't really care about
PERF_CAPABILITIES.LBR_FORMAT other than hardcoding it to 0x3f.

Thanks, Roger.