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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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

~Andrew