diff mbox series

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

Message ID 20220520133746.66142-4-roger.pau@citrix.com (mailing list archive)
State New
Headers show
Series x86/lbr: handle lack of model-specific LBRs | expand

Commit Message

Roger Pau Monné May 20, 2022, 1:37 p.m. UTC
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@citrix.com>
---
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(-)

Comments

Andrew Cooper May 20, 2022, 2:10 p.m. UTC | #1
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@citrix.com>
> ---
> 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
Jan Beulich May 20, 2022, 2:19 p.m. UTC | #2
On 20.05.2022 16:10, Andrew Cooper wrote:
> On 20/05/2022 14:37, Roger Pau Monne wrote:
>> --- 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

Since this could be quite restrictive, and since people needing to know
they need to hide this feature for migration to work, I guess this would
further want qualifying by "did the guest actually use LBRs so far"?

Jan

> 3) Actually put the lBR registers in the migration stream
> 
> ~Andrew
Roger Pau Monné May 20, 2022, 2:52 p.m. UTC | #3
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@citrix.com>
> > ---
> > 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.
Andrew Cooper May 20, 2022, 2:58 p.m. UTC | #4
On 20/05/2022 15:19, Jan Beulich wrote:
> On 20.05.2022 16:10, Andrew Cooper wrote:
>> On 20/05/2022 14:37, Roger Pau Monne wrote:
>>> --- 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
> Since this could be quite restrictive, and since people needing to know
> they need to hide this feature for migration to work, I guess this would
> further want qualifying by "did the guest actually use LBRs so far"?

In practice, it's every major generation ("tock" on Intel's old model),
so isn't actually limiting the kinds of heterogeneous setups used in
production.  (Migration gets steadily less stable the further apart the
two CPUs are.)

As to dynamic, no - that would be a security bug in a cloud scenario,
because there must not be anything the guest can do to interfere with
the manageability.

Use of LBR is rare, as demonstrated by the fact that noone has
complained about the fact that migrating such a VM will malfunction.

As we now have a way of reporting "no model-specific LBR", I'm tempted
to suggest that VMs get no LBR by default, and someone wanting LBR has
to opt in, which is also an explicit agreement to the migration limitation.

~Andrew
Roger Pau Monné May 23, 2022, 8:04 a.m. UTC | #5
On Fri, May 20, 2022 at 02:58:01PM +0000, Andrew Cooper wrote:
> On 20/05/2022 15:19, Jan Beulich wrote:
> > On 20.05.2022 16:10, Andrew Cooper wrote:
> >> On 20/05/2022 14:37, Roger Pau Monne wrote:
> >>> --- 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
> > Since this could be quite restrictive, and since people needing to know
> > they need to hide this feature for migration to work, I guess this would
> > further want qualifying by "did the guest actually use LBRs so far"?
> 
> In practice, it's every major generation ("tock" on Intel's old model),
> so isn't actually limiting the kinds of heterogeneous setups used in
> production.  (Migration gets steadily less stable the further apart the
> two CPUs are.)
> 
> As to dynamic, no - that would be a security bug in a cloud scenario,
> because there must not be anything the guest can do to interfere with
> the manageability.
> 
> Use of LBR is rare, as demonstrated by the fact that noone has
> complained about the fact that migrating such a VM will malfunction.
> 
> As we now have a way of reporting "no model-specific LBR", I'm tempted
> to suggest that VMs get no LBR by default, and someone wanting LBR has
> to opt in, which is also an explicit agreement to the migration limitation.

I did also consider exposing "no model-specific LBR" in
PERF_CAPABILITIES unconditionally, but I was worried whether that
would break existing setups.

If we think that providing an option to expose the native LBR format
in PERF_CAPABILITIES is fine that could be a sensible solution IMO.

Thanks, Roger.
Jan Beulich May 23, 2022, 8:12 a.m. UTC | #6
On 20.05.2022 16:58, Andrew Cooper wrote:
> On 20/05/2022 15:19, Jan Beulich wrote:
>> On 20.05.2022 16:10, Andrew Cooper wrote:
>>> On 20/05/2022 14:37, Roger Pau Monne wrote:
>>>> --- 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
>> Since this could be quite restrictive, and since people needing to know
>> they need to hide this feature for migration to work, I guess this would
>> further want qualifying by "did the guest actually use LBRs so far"?
> 
> In practice, it's every major generation ("tock" on Intel's old model),
> so isn't actually limiting the kinds of heterogeneous setups used in
> production.  (Migration gets steadily less stable the further apart the
> two CPUs are.)
> 
> As to dynamic, no - that would be a security bug in a cloud scenario,
> because there must not be anything the guest can do to interfere with
> the manageability.
> 
> Use of LBR is rare, as demonstrated by the fact that noone has
> complained about the fact that migrating such a VM will malfunction.
> 
> As we now have a way of reporting "no model-specific LBR",

Which only rather new guest kernels will know to look for. Hence ...

> I'm tempted
> to suggest that VMs get no LBR by default, and someone wanting LBR has
> to opt in, which is also an explicit agreement to the migration limitation.

... while in principle I agree with this, I see a practical issue.

Jan
Roger Pau Monné May 23, 2022, 9:53 a.m. UTC | #7
On Mon, May 23, 2022 at 10:12:55AM +0200, Jan Beulich wrote:
> On 20.05.2022 16:58, Andrew Cooper wrote:
> > On 20/05/2022 15:19, Jan Beulich wrote:
> >> On 20.05.2022 16:10, Andrew Cooper wrote:
> >>> On 20/05/2022 14:37, Roger Pau Monne wrote:
> >>>> --- 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
> >> Since this could be quite restrictive, and since people needing to know
> >> they need to hide this feature for migration to work, I guess this would
> >> further want qualifying by "did the guest actually use LBRs so far"?
> > 
> > In practice, it's every major generation ("tock" on Intel's old model),
> > so isn't actually limiting the kinds of heterogeneous setups used in
> > production.  (Migration gets steadily less stable the further apart the
> > two CPUs are.)
> > 
> > As to dynamic, no - that would be a security bug in a cloud scenario,
> > because there must not be anything the guest can do to interfere with
> > the manageability.
> > 
> > Use of LBR is rare, as demonstrated by the fact that noone has
> > complained about the fact that migrating such a VM will malfunction.
> > 
> > As we now have a way of reporting "no model-specific LBR",
> 
> Which only rather new guest kernels will know to look for. Hence ...
> 
> > I'm tempted
> > to suggest that VMs get no LBR by default, and someone wanting LBR has
> > to opt in, which is also an explicit agreement to the migration limitation.
> 
> ... while in principle I agree with this, I see a practical issue.

I think it should be fine to expose no model-specific LBR support in
PERF_CAPABILITIES, but we shouldn't change the behavior of
DEBUGCTLMSR.LBR exposed to guests if the underlying platform has
model-specific LBRs and those are known to Xen.

That way old guest kernels that ignore PERF_CAPABILITIES.LBR_FORMAT
will continue to work, while newish kernels that check the format will
avoid using LBRs.

In case we introduce a guest config option to enable LBR, should we
then expose the native LBR format in PERF_CAPABILITIES?  Or would it
be better to just keep the current model and not expose
PERF_CAPABILITIES at all (and don't report PDCM in CPUID in that
case).

Thanks, Roger.
diff mbox series

Patch

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;
+        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 */
 XEN_CPUFEATURE(PCID,          1*32+17) /*H  Process Context ID */
 XEN_CPUFEATURE(DCA,           1*32+18) /*   Direct Cache Access */
 XEN_CPUFEATURE(SSE4_1,        1*32+19) /*A  Streaming SIMD Extensions 4.1 */