| Message ID | 20220520133746.66142-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/lbr: handle lack of model-specific LBRs | expand |
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
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
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.
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
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.
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
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 --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 */
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(-)