diff mbox series

[v3,3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD

Message ID 20220331092717.9023-4-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series amd/msr: implement MSR_VIRT_SPEC_CTRL for HVM guests | expand

Commit Message

Roger Pau Monné March 31, 2022, 9:27 a.m. UTC
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 <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Fix codding style issues.
 - Use AMD_ZEN1_MAX_SOCKETS to define the max number of possible
   sockets in Zen1 systems.

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/spec_ctrl.c       |   4 +-
 5 files changed, 127 insertions(+), 19 deletions(-)

Comments

Jan Beulich April 21, 2022, 9:50 a.m. UTC | #1
On 31.03.2022 11:27, Roger Pau Monne wrote:
> 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.

So where exactly is the boundary? If I'm not mistaken Zen2 is also
Fam17 (and only Zen3 is Fam19), yet here and elsewhere you look to
take Zen1 == Fam17.

Just one further nit on the code:

> +static struct ssbd_core {
> +    spinlock_t lock;
> +    unsigned int count;
> +} *ssbd_core;
> +static unsigned int __ro_after_init ssbd_max_cores;
> +#define AMD_ZEN1_MAX_SOCKETS 2

This #define looks to render ...

> +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 >= 0x80000008) {
> +		ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x80000008), 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 * AMD_ZEN1_MAX_SOCKETS);

... largely redundant the comment here; if anywhere I think the comment
would want to go next to the #define.

Jan
Roger Pau Monné April 21, 2022, 3:21 p.m. UTC | #2
On Thu, Apr 21, 2022 at 11:50:16AM +0200, Jan Beulich wrote:
> On 31.03.2022 11:27, Roger Pau Monne wrote:
> > 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.
> 
> So where exactly is the boundary? If I'm not mistaken Zen2 is also
> Fam17 (and only Zen3 is Fam19), yet here and elsewhere you look to
> take Zen1 == Fam17.

Right, but Zen2 already has AMD_SSBD (ie: SPEC_CTRL), so it's not
using this logic.

The AMD whitepaper is more clear about this: any Fam17h processor that
is using the non-architectural MSRs to set SSBD and has more than 1
logical processor for each logical core must synchronize the setting
of SSBD.

I think just dropping the mention of Zen 1 in the commit message
should remove your concerns?

> Just one further nit on the code:
> 
> > +static struct ssbd_core {
> > +    spinlock_t lock;
> > +    unsigned int count;
> > +} *ssbd_core;
> > +static unsigned int __ro_after_init ssbd_max_cores;
> > +#define AMD_ZEN1_MAX_SOCKETS 2
> 
> This #define looks to render ...
> 
> > +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 >= 0x80000008) {
> > +		ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x80000008), 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 * AMD_ZEN1_MAX_SOCKETS);
> 
> ... largely redundant the comment here; if anywhere I think the comment
> would want to go next to the #define.

I guess I should also rename the define to FAM17H instead of ZEN1, as
all Fam17h is limited to two sockets, then the comment can be removed
as I think the define is self-explanatory.

Thanks, Roger.
Jan Beulich April 21, 2022, 3:27 p.m. UTC | #3
On 21.04.2022 17:21, Roger Pau Monné wrote:
> On Thu, Apr 21, 2022 at 11:50:16AM +0200, Jan Beulich wrote:
>> On 31.03.2022 11:27, Roger Pau Monne wrote:
>>> 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.
>>
>> So where exactly is the boundary? If I'm not mistaken Zen2 is also
>> Fam17 (and only Zen3 is Fam19), yet here and elsewhere you look to
>> take Zen1 == Fam17.
> 
> Right, but Zen2 already has AMD_SSBD (ie: SPEC_CTRL), so it's not
> using this logic.
> 
> The AMD whitepaper is more clear about this: any Fam17h processor that
> is using the non-architectural MSRs to set SSBD and has more than 1
> logical processor for each logical core must synchronize the setting
> of SSBD.
> 
> I think just dropping the mention of Zen 1 in the commit message
> should remove your concerns?

Or keep it, but qualify it by saying that Zen2 isn't expected to take
this path because of having SSBD. But iirc SSBD was introduced to
Zen2 only by a ucode update, so such a description should not be wrong
wrt such not-up-to-date systems.

>> Just one further nit on the code:
>>
>>> +static struct ssbd_core {
>>> +    spinlock_t lock;
>>> +    unsigned int count;
>>> +} *ssbd_core;
>>> +static unsigned int __ro_after_init ssbd_max_cores;
>>> +#define AMD_ZEN1_MAX_SOCKETS 2
>>
>> This #define looks to render ...
>>
>>> +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 >= 0x80000008) {
>>> +		ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x80000008), 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 * AMD_ZEN1_MAX_SOCKETS);
>>
>> ... largely redundant the comment here; if anywhere I think the comment
>> would want to go next to the #define.
> 
> I guess I should also rename the define to FAM17H instead of ZEN1, as
> all Fam17h is limited to two sockets, then the comment can be removed
> as I think the define is self-explanatory.

Yes, this rename would help both eliminate my confusion as well as the
need for the comment.

Jan
Roger Pau Monné April 22, 2022, 9:55 a.m. UTC | #4
On Thu, Apr 21, 2022 at 05:27:18PM +0200, Jan Beulich wrote:
> On 21.04.2022 17:21, Roger Pau Monné wrote:
> > On Thu, Apr 21, 2022 at 11:50:16AM +0200, Jan Beulich wrote:
> >> On 31.03.2022 11:27, Roger Pau Monne wrote:
> >>> 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.
> >>
> >> So where exactly is the boundary? If I'm not mistaken Zen2 is also
> >> Fam17 (and only Zen3 is Fam19), yet here and elsewhere you look to
> >> take Zen1 == Fam17.
> > 
> > Right, but Zen2 already has AMD_SSBD (ie: SPEC_CTRL), so it's not
> > using this logic.
> > 
> > The AMD whitepaper is more clear about this: any Fam17h processor that
> > is using the non-architectural MSRs to set SSBD and has more than 1
> > logical processor for each logical core must synchronize the setting
> > of SSBD.
> > 
> > I think just dropping the mention of Zen 1 in the commit message
> > should remove your concerns?
> 
> Or keep it, but qualify it by saying that Zen2 isn't expected to take
> this path because of having SSBD. But iirc SSBD was introduced to
> Zen2 only by a ucode update, so such a description should not be wrong
> wrt such not-up-to-date systems.

FTAOD I've worded this as:

"Note that on AMD Family 17h and Hygon Family 18h processors 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."

Which I think is correct in all cases.  Iff Zen2 was to resort to
using the non-architectural way of setting SSBD (if that's even
possible) it should synchronize it between threads according to my
read of the AMD whitepaper.

I've also added handling for Hygon Fam18h, seeing as those also make
use of the non-architectural way of setting SSBD.

Thanks, Roger.
Jan Beulich April 22, 2022, 10:01 a.m. UTC | #5
On 22.04.2022 11:55, Roger Pau Monné wrote:
> On Thu, Apr 21, 2022 at 05:27:18PM +0200, Jan Beulich wrote:
>> On 21.04.2022 17:21, Roger Pau Monné wrote:
>>> On Thu, Apr 21, 2022 at 11:50:16AM +0200, Jan Beulich wrote:
>>>> On 31.03.2022 11:27, Roger Pau Monne wrote:
>>>>> 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.
>>>>
>>>> So where exactly is the boundary? If I'm not mistaken Zen2 is also
>>>> Fam17 (and only Zen3 is Fam19), yet here and elsewhere you look to
>>>> take Zen1 == Fam17.
>>>
>>> Right, but Zen2 already has AMD_SSBD (ie: SPEC_CTRL), so it's not
>>> using this logic.
>>>
>>> The AMD whitepaper is more clear about this: any Fam17h processor that
>>> is using the non-architectural MSRs to set SSBD and has more than 1
>>> logical processor for each logical core must synchronize the setting
>>> of SSBD.
>>>
>>> I think just dropping the mention of Zen 1 in the commit message
>>> should remove your concerns?
>>
>> Or keep it, but qualify it by saying that Zen2 isn't expected to take
>> this path because of having SSBD. But iirc SSBD was introduced to
>> Zen2 only by a ucode update, so such a description should not be wrong
>> wrt such not-up-to-date systems.
> 
> FTAOD I've worded this as:
> 
> "Note that on AMD Family 17h and Hygon Family 18h processors 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."

Thanks.

> Which I think is correct in all cases.  Iff Zen2 was to resort to
> using the non-architectural way of setting SSBD (if that's even
> possible) it should synchronize it between threads according to my
> read of the AMD whitepaper.
> 
> I've also added handling for Hygon Fam18h, seeing as those also make
> use of the non-architectural way of setting SSBD.

Right, better be on the safe side.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 4999f8be2b..a256a9d882 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 == &boot_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;
+#define AMD_ZEN1_MAX_SOCKETS 2
+
+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 >= 0x80000008) {
+		ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x80000008), 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 * AMD_ZEN1_MAX_SOCKETS);
+	if (!ssbd_core)
+		return false;
+
+	for (i = 0; i < ssbd_max_cores * AMD_ZEN1_MAX_SOCKETS; i++) {
+		spin_lock_init(&ssbd_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 = &current_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 >= AMD_ZEN1_MAX_SOCKETS);
+	BUG_ON(c->cpu_core_id >= ssbd_max_cores);
+	core = &ssbd_core[c->phys_proc_id * ssbd_max_cores + c->cpu_core_id];
+	/*
+	 * Use irqsave variant to make check_lock() happy. When called from
+	 * vm{exit,entry}_virt_spec_ctrl GIF=0, but the state of IF could be 1,
+	 * thus fooling the spinlock check.
+	 */
+	spin_lock_irqsave(&core->lock, flags);
+	core->count += enable ? 1 : -1;
+	ASSERT(core->count <= c->x86_num_siblings);
+	if (enable ? core->count == 1 : !core->count)
+		BUG_ON(!set_legacy_ssbd(c, enable));
+	spin_unlock_irqrestore(&core->lock, flags);
 }
 
 void __init detect_zen2_null_seg_behaviour(void)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 91e53284fc..e278fee689 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -537,6 +537,16 @@  static void __init calculate_hvm_max_policy(void)
     if ( !boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
         /* Clear VIRT_SSBD if VIRT_SPEC_CTRL is not exposed to guests. */
         __clear_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
+    else
+        /*
+         * Expose VIRT_SSBD if VIRT_SPEC_CTRL is supported, as that implies the
+         * underlying hardware is capable of setting SSBD using
+         * non-architectural way or VIRT_SSBD is available.
+         *
+         * Note that if the hardware supports VIRT_SSBD natively this setting
+         * will just override an already set bit.
+         */
+        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
 
     /*
      * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 40ff28ecf1..9b8f8d21bd 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -3133,7 +3133,12 @@  void vmexit_virt_spec_ctrl(void)
         if ( val != lo )
             wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
         current->arch.msrs->virt_spec_ctrl.raw = lo;
+
+        return;
     }
+
+    if ( val != current->arch.msrs->virt_spec_ctrl.raw )
+        amd_set_legacy_ssbd(val & SPEC_CTRL_SSBD);
 }
 
 /* Called with GIF=0. */
@@ -3142,7 +3147,12 @@  void vmentry_virt_spec_ctrl(void)
     unsigned int val = current->arch.msrs->virt_spec_ctrl.raw;
 
     if ( val != (opt_ssbd ? SPEC_CTRL_SSBD : 0) )
-        wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
+    {
+        if ( cpu_has_virt_ssbd )
+            wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
+        else
+            amd_set_legacy_ssbd(val & SPEC_CTRL_SSBD);
+    }
 }
 
 /*
diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h
index a82382e6bf..6a42f68542 100644
--- a/xen/arch/x86/include/asm/amd.h
+++ b/xen/arch/x86/include/asm/amd.h
@@ -151,4 +151,8 @@  void check_enable_amd_mmconf_dmi(void);
 extern bool amd_acpi_c1e_quirk;
 void amd_check_disable_c1e(unsigned int port, u8 value);
 
+extern bool amd_legacy_ssbd;
+bool amd_setup_legacy_ssbd(void);
+void amd_set_legacy_ssbd(bool enable);
+
 #endif /* __AMD_H__ */
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 0d5ec877d1..495e6f9405 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -22,6 +22,7 @@ 
 #include <xen/param.h>
 #include <xen/warning.h>
 
+#include <asm/amd.h>
 #include <asm/hvm/svm/svm.h>
 #include <asm/microcode.h>
 #include <asm/msr.h>
@@ -1073,7 +1074,8 @@  void __init init_speculation_mitigations(void)
     }
 
     /* Support VIRT_SPEC_CTRL.SSBD if AMD_SSBD is not available. */
-    if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd && cpu_has_virt_ssbd )
+    if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd &&
+         (cpu_has_virt_ssbd || (amd_legacy_ssbd && amd_setup_legacy_ssbd())) )
         setup_force_cpu_cap(X86_FEATURE_VIRT_SC_MSR_HVM);
 
     /* If we have IBRS available, see whether we should use it. */