Re: [PATCH v2 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD
On 28.03.2022 17:24, Roger Pau Monné wrote:
> On Mon, Mar 28, 2022 at 04:21:02PM +0200, Jan Beulich wrote:
>> On 15.03.2022 15:18, Roger Pau Monne wrote:
>>> @@ -677,14 +680,17 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr,
>>> uint64_t val)
>>> if ( !cp->extd.virt_ssbd )
>>> goto gp_fault;
>>>
>>> -/*
>>> - * Only supports SSBD bit, the rest are ignored. Only modify the
>>> SSBD
>>> - * bit in case other bits are set.
>>> - */
>>> -if ( val & SPEC_CTRL_SSBD )
>>> -msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
>>> +/* Only supports SSBD bit, the rest are ignored. */
>>> +if ( cpu_has_amd_ssbd )
>>> +{
>>> +/* Only modify the SSBD bit in case other bits are set. */
>>
>> While more a comment on the earlier patch introducing this wording, it
>> occurred to me only here that this is ambiguous: It can also be read as
>> "Only modify the SSBD bit as long as other bits are set."
>
> Hm, no, that's not what I meant. I meant to note that here we are
> careful to only modify the SSBD bit of spec_ctrl, because other bits
> might be used for other purposes.
Right, I understand that's what you mean, and because I understand
the ambiguity also slipped my attention in the earlier patch.
> We can't do:
>
> msrs->spec_ctrl.raw = SPEC_CTRL_SSBD;
>
> But maybe this doesn't require a comment, as it seems to raise more
> questions than answer?
I wouldn't mind if (in the earlier patch) you simply dropped the 2nd
sentence. Or alternatively how about "Also only record the SSBD bit
to return for future reads" or something along these lines?
Jan
Re: [PATCH v2 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD
On Mon, Mar 28, 2022 at 04:21:02PM +0200, Jan Beulich wrote:
> On 15.03.2022 15:18, Roger Pau Monne wrote:
> > +void amd_init_ssbd(const struct cpuinfo_x86 *c)
> > +{
> > + if (cpu_has_ssb_no)
> > + return;
> > +
> > + if (cpu_has_amd_ssbd) {
> > + /* Handled by common MSR_SPEC_CTRL logic */
> > + return;
> > + }
> > +
> > + if (cpu_has_virt_ssbd) {
> > + wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> > + return;
> > + }
> > +
> > + if (!set_legacy_ssbd(c, opt_ssbd))
> > + {
>
> Nit: In this file the brace belongs on the earlier line and ...
>
> > printk_once(XENLOG_ERR "No SSBD controls available\n");
> > + if (amd_legacy_ssbd)
> > + panic("CPU feature mismatch: no legacy SSBD\n");
> > + }
> > + else if ( c == _cpu_data )
>
> ... you want to omit the blanks immediately inside the parentheses here.
Ouch, yes.
> > + amd_legacy_ssbd = true;
> > +}
> > +
> > +static struct ssbd_core {
> > +spinlock_t lock;
> > +unsigned int count;
> > +} *ssbd_core;
> > +static unsigned int __ro_after_init ssbd_max_cores;
> > +
> > +bool __init amd_setup_legacy_ssbd(void)
> > +{
> > + unsigned int i;
> > +
> > + if (boot_cpu_data.x86 != 0x17 || boot_cpu_data.x86_num_siblings <= 1)
> > + return true;
> > +
> > + /*
> > +* One could be forgiven for thinking that c->x86_max_cores is the
> > +* correct value to use here.
> > +*
> > +* However, that value is derived from the current configuration, and
> > +* c->cpu_core_id is sparse on all but the top end CPUs. Derive
> > +* max_cpus from ApicIdCoreIdSize which will cover any sparseness.
> > +*/
> > + if (boot_cpu_data.extended_cpuid_level >= 0x8008) {
> > + ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x8008), 0xf000);
> > + ssbd_max_cores /= boot_cpu_data.x86_num_siblings;
> > + }
> > + if (!ssbd_max_cores)
> > + return false;
> > +
> > + /* Max is two sockets for Fam17h hardware. */
> > + ssbd_core = xzalloc_array(struct ssbd_core, ssbd_max_cores * 2);
>
> If I'm not mistaken this literal 2, ...
>
> > + if (!ssbd_core)
> > + return false;
> > +
> > + for (i = 0; i < ssbd_max_cores * 2; i++) {
>
> ... this one, and ...
>
> > + spin_lock_init(_core[i].lock);
> > + /* Record initial state, also applies to any hotplug CPU. */
> > + if (opt_ssbd)
> > + ssbd_core[i].count = boot_cpu_data.x86_num_siblings;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +void amd_set_legacy_ssbd(bool enable)
> > +{
> > + const struct cpuinfo_x86 *c = _cpu_data;
> > + struct ssbd_core *core;
> > + unsigned long flags;
> > +
> > + if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> > + BUG_ON(!set_legacy_ssbd(c, enable));
> > + return;
> > + }
> > +
> > + BUG_ON(c->phys_proc_id >= 2);
>
> .. this one are all referring to the same thing. Please use a #define to
> make the connection obvious.
Indeed. That's the maximum number of sockets possible with that CPU
family (2).
> > @@ -677,14 +680,17 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr,
> > uint64_t val)
> > if ( !cp->extd.virt_ssbd )
> > goto gp_fault;
> >
> > -/*
> > - * Only supports SSBD bit, the rest are ignored. Only modify the
> > SSBD
> > - * bit in case other bits are set.
> > - */
> > -if ( val & SPEC_CTRL_SSBD )
> > -msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
> > +/* Only supports SSBD bit, the rest are ignored. */
> > +if ( cpu_has_amd_ssbd )
> > +{
> > +/* Only modify the SSBD bit in case other bits are set. */
>
> While more a comment on the earlier patch introducing this wording, it
> occurred to me only here that this is ambiguous: It can also be read as
> "Only modify the SSBD bit as long as other bits are set."
Hm, no, that's not what I meant. I meant to note that here we are
careful to only modify the SSBD bit of spec_ctrl, because other bits
might be used for other purposes. We can't do:
msrs->spec_ctrl.raw = SPEC_CTRL_SSBD;
But maybe this doesn't require a comment, as it seems to raise more
questions than answer?
> > +if ( val & SPEC_CTRL_SSBD )
> > +msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
> > +else
> > +msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> > +}
> > else
> > -msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> > +msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD;
>
> I also think the comment applies equally to the "else" logic, so perhaps
> the comment would best remain as is (and merely be re-worded in the
> earlier patch)?
Sure, let's see if we can get consensus on a proper wording.
Thanks, Roger.
Re: [PATCH v2 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD
On 15.03.2022 15:18, Roger Pau Monne wrote:
> +void amd_init_ssbd(const struct cpuinfo_x86 *c)
> +{
> + if (cpu_has_ssb_no)
> + return;
> +
> + if (cpu_has_amd_ssbd) {
> + /* Handled by common MSR_SPEC_CTRL logic */
> + return;
> + }
> +
> + if (cpu_has_virt_ssbd) {
> + wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> + return;
> + }
> +
> + if (!set_legacy_ssbd(c, opt_ssbd))
> + {
Nit: In this file the brace belongs on the earlier line and ...
> printk_once(XENLOG_ERR "No SSBD controls available\n");
> + if (amd_legacy_ssbd)
> + panic("CPU feature mismatch: no legacy SSBD\n");
> + }
> + else if ( c == _cpu_data )
... you want to omit the blanks immediately inside the parentheses here.
> + amd_legacy_ssbd = true;
> +}
> +
> +static struct ssbd_core {
> +spinlock_t lock;
> +unsigned int count;
> +} *ssbd_core;
> +static unsigned int __ro_after_init ssbd_max_cores;
> +
> +bool __init amd_setup_legacy_ssbd(void)
> +{
> + unsigned int i;
> +
> + if (boot_cpu_data.x86 != 0x17 || boot_cpu_data.x86_num_siblings <= 1)
> + return true;
> +
> + /*
> + * One could be forgiven for thinking that c->x86_max_cores is the
> + * correct value to use here.
> + *
> + * However, that value is derived from the current configuration, and
> + * c->cpu_core_id is sparse on all but the top end CPUs. Derive
> + * max_cpus from ApicIdCoreIdSize which will cover any sparseness.
> + */
> + if (boot_cpu_data.extended_cpuid_level >= 0x8008) {
> + ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x8008), 0xf000);
> + ssbd_max_cores /= boot_cpu_data.x86_num_siblings;
> + }
> + if (!ssbd_max_cores)
> + return false;
> +
> + /* Max is two sockets for Fam17h hardware. */
> + ssbd_core = xzalloc_array(struct ssbd_core, ssbd_max_cores * 2);
If I'm not mistaken this literal 2, ...
> + if (!ssbd_core)
> + return false;
> +
> + for (i = 0; i < ssbd_max_cores * 2; i++) {
... this one, and ...
> + spin_lock_init(_core[i].lock);
> + /* Record initial state, also applies to any hotplug CPU. */
> + if (opt_ssbd)
> + ssbd_core[i].count = boot_cpu_data.x86_num_siblings;
> + }
> +
> + return true;
> +}
> +
> +void amd_set_legacy_ssbd(bool enable)
> +{
> + const struct cpuinfo_x86 *c = _cpu_data;
> + struct ssbd_core *core;
> + unsigned long flags;
> +
> + if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> + BUG_ON(!set_legacy_ssbd(c, enable));
> + return;
> + }
> +
> + BUG_ON(c->phys_proc_id >= 2);
.. this one are all referring to the same thing. Please use a #define to
make the connection obvious.
> @@ -677,14 +680,17 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t
> val)
> if ( !cp->extd.virt_ssbd )
> goto gp_fault;
>
> -/*
> - * Only supports SSBD bit, the rest are ignored. Only modify the SSBD
> - * bit in case other bits are set.
> - */
> -if ( val & SPEC_CTRL_SSBD )
> -msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
> +/* Only supports SSBD bit, the rest are ignored. */
> +if ( cpu_has_amd_ssbd )
> +{
> +/* Only modify the SSBD bit in case other bits are set. */
While more a comment on the earlier patch introducing this wording, it
occurred to me only here that this is ambiguous: It can also be read as
"Only modify the SSBD bit as long as other bits are set."
> +if ( val & SPEC_CTRL_SSBD )
> +msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
> +else
> +msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> +}
> else
> -msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> +msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD;
I also think the comment applies equally to the "else" logic, so perhaps
the comment would best remain as is (and merely be re-worded in the
earlier patch)?
Jan
[PATCH v2 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD
Expose VIRT_SSBD to guests if the hardware supports setting SSBD in
the LS_CFG MSR (a.k.a. non-architectural way). Different AMD CPU
families use different bits in LS_CFG, so exposing VIRT_SPEC_CTRL.SSBD
allows for an unified way of exposing SSBD support to guests on AMD
hardware that's compatible migration wise, regardless of what
underlying mechanism is used to set SSBD.
Note that on AMD Family 17h (Zen 1) the value of SSBD in LS_CFG is
shared between threads on the same core, so there's extra logic in
order to synchronize the value and have SSBD set as long as one of the
threads in the core requires it to be set. Such logic also requires
extra storage for each thread state, which is allocated at
initialization time.
Do the context switching of the SSBD selection in LS_CFG between
hypervisor and guest in the same handler that's already used to switch
the value of VIRT_SPEC_CTRL in the hardware when supported.
Suggested-by: Andrew Cooper
Signed-off-by: Roger Pau Monné
---
Changes since v1:
- Report legacy SSBD support using a global variable.
- Use ro_after_init for ssbd_max_cores.
- Handle boot_cpu_data.x86_num_siblings < 1.
- Add comment regarding _irqsave usage in amd_set_legacy_ssbd.
---
xen/arch/x86/cpu/amd.c | 116 -
xen/arch/x86/cpuid.c | 10 +++
xen/arch/x86/hvm/svm/svm.c | 12 +++-
xen/arch/x86/include/asm/amd.h | 4 ++
xen/arch/x86/msr.c | 22 ---
xen/arch/x86/spec_ctrl.c | 4 +-
6 files changed, 141 insertions(+), 27 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 4999f8be2b..63d466b1df 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -48,6 +48,7 @@ boolean_param("allow_unsafe", opt_allow_unsafe);
/* Signal whether the ACPI C1E quirk is required. */
bool __read_mostly amd_acpi_c1e_quirk;
+bool __ro_after_init amd_legacy_ssbd;
static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
unsigned int *hi)
@@ -685,23 +686,10 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
* Refer to the AMD Speculative Store Bypass whitepaper:
*
https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
*/
-void amd_init_ssbd(const struct cpuinfo_x86 *c)
+static bool set_legacy_ssbd(const struct cpuinfo_x86 *c, bool enable)
{
int bit = -1;
- if (cpu_has_ssb_no)
- return;
-
- if (cpu_has_amd_ssbd) {
- /* Handled by common MSR_SPEC_CTRL logic */
- return;
- }
-
- if (cpu_has_virt_ssbd) {
- wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
- return;
- }
-
switch (c->x86) {
case 0x15: bit = 54; break;
case 0x16: bit = 33; break;
@@ -715,20 +703,114 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c)
if (rdmsr_safe(MSR_AMD64_LS_CFG, val) ||
({
val &= ~mask;
- if (opt_ssbd)
+ if (enable)
val |= mask;
false;
}) ||
wrmsr_safe(MSR_AMD64_LS_CFG, val) ||
({
rdmsrl(MSR_AMD64_LS_CFG, val);
- (val & mask) != (opt_ssbd * mask);
+ (val & mask) != (enable * mask);
}))
bit = -1;
}
- if (bit < 0)
+ return bit >= 0;
+}
+
+void amd_init_ssbd(const struct cpuinfo_x86 *c)
+{
+ if (cpu_has_ssb_no)
+ return;
+
+ if (cpu_has_amd_ssbd) {
+ /* Handled by common MSR_SPEC_CTRL logic */
+ return;
+ }
+
+ if (cpu_has_virt_ssbd) {
+ wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
+ return;
+ }
+
+ if (!set_legacy_ssbd(c, opt_ssbd))
+ {
printk_once(XENLOG_ERR "No SSBD controls available\n");
+ if (amd_legacy_ssbd)
+ panic("CPU feature mismatch: no legacy SSBD\n");
+ }
+ else if ( c == _cpu_data )
+ amd_legacy_ssbd = true;
+}
+
+static struct ssbd_core {
+spinlock_t lock;
+unsigned int count;
+} *ssbd_core;
+static unsigned int __ro_after_init ssbd_max_cores;
+
+bool __init amd_setup_legacy_ssbd(void)
+{
+ unsigned int i;
+
+ if (boot_cpu_data.x86 != 0x17 || boot_cpu_data.x86_num_siblings <= 1)
+ return true;
+
+ /*
+* One could be forgiven for thinking that c->x86_max_cores is the
+* correct value to use here.
+*
+* However, that value is derived from the current configuration, and
+* c->cpu_core_id is sparse on all but the top end CPUs. Derive
+* max_cpus from ApicIdCoreIdSize which will cover
