Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Borislav Petkov]

On Thu, Sep 23, 2021 at 08:15:28PM -0500, Babu Moger wrote:
> Linux kernel does not have the interface to enable/disable PSFD yet. Plan
> here is to expose the PSFD technology to KVM so that the guest kernel can
> make use of it if they wish to.

Why should the guest kernel expose it if we said that for now we want to
disable it with the SSBD control?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Paolo Bonzini]

On 27/09/21 12:59, Borislav Petkov wrote:
> On Thu, Sep 23, 2021 at 08:15:28PM -0500, Babu Moger wrote:
>> Linux kernel does not have the interface to enable/disable PSFD yet. Plan
>> here is to expose the PSFD technology to KVM so that the guest kernel can
>> make use of it if they wish to.
> 
> Why should the guest kernel expose it if we said that for now we want to
> disable it with the SSBD control?

Because the guest kernel needs to know which MSRs to write when you 
touch the SSBD prctl, so that PSFD is properly disabled *inside the guest*.

Paolo

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Borislav Petkov]

On Mon, Sep 27, 2021 at 01:13:26PM +0200, Paolo Bonzini wrote:
> Because the guest kernel needs to know which MSRs to write when you touch
> the SSBD prctl, so that PSFD is properly disabled *inside the guest*.

It already knows which - the same one which disables SSB. PSF is
disabled *together* with SSB, for now...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Paolo Bonzini]

On 27/09/21 14:06, Borislav Petkov wrote:
>> Because the guest kernel needs to know which MSRs to write when you touch
>> the SSBD prctl, so that PSFD is properly disabled*inside the guest*.
> It already knows which - the same one which disables SSB. PSF is
> disabled*together*  with SSB, for now...

Right, not which MSR to write but which value to write.  It doesn't know 
that the PSF disable bit is valid unless the corresponding CPUID bit is set.

Paolo

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Borislav Petkov]

On Mon, Sep 27, 2021 at 02:14:52PM +0200, Paolo Bonzini wrote:
> Right, not which MSR to write but which value to write.  It doesn't know
> that the PSF disable bit is valid unless the corresponding CPUID bit is set.

There's no need for the separate PSF CPUID bit yet. We have decided for
now to not control PSF separately but disable it through SSB. Please
follow this thread:

https://lore.kernel.org/all/20210904172334.lfjyqi4qfzvbxef7@treble/T/#u

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Paolo Bonzini]

On 27/09/21 14:28, Borislav Petkov wrote:
> On Mon, Sep 27, 2021 at 02:14:52PM +0200, Paolo Bonzini wrote:
>> Right, not which MSR to write but which value to write.  It doesn't know
>> that the PSF disable bit is valid unless the corresponding CPUID bit is set.
> 
> There's no need for the separate PSF CPUID bit yet. We have decided for
> now to not control PSF separately but disable it through SSB. Please
> follow this thread:

There are other guests than Linux.  This patch is just telling userspace 
that KVM knows what the PSFD bit is.  It is also possible to expose the 
bit in KVM without having any #define in cpufeatures.h or without the 
kernel using it.  For example KVM had been exposing FSGSBASE long before 
Linux supported it.

That said, the patch is incomplete because it should also add the new 
CPUID bit to guest_has_spec_ctrl_msr (what KVM *really* cares about is 
not the individual bits, only whether SPEC_CTRL exists at all).

Paolo

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Borislav Petkov]

On Mon, Sep 27, 2021 at 02:54:08PM +0200, Paolo Bonzini wrote:
> There are other guests than Linux.  This patch is just telling userspace
> that KVM knows what the PSFD bit is.  It is also possible to expose the bit
> in KVM without having any #define in cpufeatures.h

Ok, then there's no need for the cpufeatures.h hunk.

> or without the kernel using it. For example KVM had been exposing
> FSGSBASE long before Linux supported it.

Ok, please do that for now then, if you want to expose it to other
guests. I'm sceptical they will have a use case for it either but I'm
always open to suggestions.

For the same reason as for baremetal, though, I wouldn't do that and do
that solely through the SSBD control but that's your call.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Babu Moger]

On 9/27/21 7:54 AM, Paolo Bonzini wrote:
> On 27/09/21 14:28, Borislav Petkov wrote:
>> On Mon, Sep 27, 2021 at 02:14:52PM +0200, Paolo Bonzini wrote:
>>> Right, not which MSR to write but which value to write.  It doesn't know
>>> that the PSF disable bit is valid unless the corresponding CPUID bit is
>>> set.
>>
>> There's no need for the separate PSF CPUID bit yet. We have decided for
>> now to not control PSF separately but disable it through SSB. Please
>> follow this thread:
> 
> There are other guests than Linux.  This patch is just telling userspace

Yes, That is the reason for this patch.

> that KVM knows what the PSFD bit is.  It is also possible to expose the
> bit in KVM without having any #define in cpufeatures.h or without the
> kernel using it.  For example KVM had been exposing FSGSBASE long before
> Linux supported it.
> 
> That said, the patch is incomplete because it should also add the new
> CPUID bit to guest_has_spec_ctrl_msr (what KVM *really* cares about is not
> the individual bits, only whether SPEC_CTRL exists at all).

Yea, I missed that. Will add it in next revision,
Thanks
Babu

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Borislav Petkov]

On Mon, Sep 27, 2021 at 10:47:01AM -0500, Babu Moger wrote:
> > There are other guests than Linux.  This patch is just telling userspace
> 
> Yes, That is the reason for this patch.

And can you share, per chance, what their use case is?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Paolo Bonzini]

On 24/09/21 03:15, Babu Moger wrote:
>   arch/x86/include/asm/cpufeatures.h |    1 +
>   arch/x86/kvm/cpuid.c               |    2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)

Queued, with a private #define instead of the one in cpufeatures.h:

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index fe03bd978761..343a01a05058 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -53,9 +53,16 @@ static u32 xstate_required_size(u64 xstate_bv, bool compacted)
  	return ret;
  }
  
+/*
+ * This one is tied to SSB in the user API, and not
+ * visible in /proc/cpuinfo.
+ */
+#define KVM_X86_FEATURE_PSFD		(13*32+28) /* Predictive Store Forwarding Disable */
+
  #define F feature_bit
  #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0)
  
+
  static inline struct kvm_cpuid_entry2 *cpuid_entry2_find(
  	struct kvm_cpuid_entry2 *entries, int nent, u32 function, u32 index)
  {
@@ -500,7 +507,8 @@ void kvm_set_cpu_caps(void)
  	kvm_cpu_cap_mask(CPUID_8000_0008_EBX,
  		F(CLZERO) | F(XSAVEERPTR) |
  		F(WBNOINVD) | F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) | F(VIRT_SSBD) |
-		F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON)
+		F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON) |
+		__feature_bit(KVM_X86_FEATURE_PSFD)
  	);
  
  	/*

Paolo

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Babu Moger]

On 9/28/21 11:04 AM, Paolo Bonzini wrote:
> On 24/09/21 03:15, Babu Moger wrote:
>>   arch/x86/include/asm/cpufeatures.h |    1 +
>>   arch/x86/kvm/cpuid.c               |    2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> Queued, with a private #define instead of the one in cpufeatures.h:

Thanks Paolo. Don't we need change in guest_has_spec_ctrl_msr?

> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index fe03bd978761..343a01a05058 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -53,9 +53,16 @@ static u32 xstate_required_size(u64 xstate_bv, bool
> compacted)
>      return ret;
>  }
>  
> +/*
> + * This one is tied to SSB in the user API, and not
> + * visible in /proc/cpuinfo.
> + */
> +#define KVM_X86_FEATURE_PSFD        (13*32+28) /* Predictive Store
> Forwarding Disable */
> +
>  #define F feature_bit
>  #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0)
>  
> +
>  static inline struct kvm_cpuid_entry2 *cpuid_entry2_find(
>      struct kvm_cpuid_entry2 *entries, int nent, u32 function, u32 index)
>  {
> @@ -500,7 +507,8 @@ void kvm_set_cpu_caps(void)
>      kvm_cpu_cap_mask(CPUID_8000_0008_EBX,
>          F(CLZERO) | F(XSAVEERPTR) |
>          F(WBNOINVD) | F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) |
> F(VIRT_SSBD) |
> -        F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON)
> +        F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON) |
> +        __feature_bit(KVM_X86_FEATURE_PSFD)
>      );
>  
>      /*
> 
> Paolo
> 

-- 
Thanks
Babu Moger

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Paolo Bonzini]

On 29/09/21 22:27, Babu Moger wrote:
> 
> On 9/28/21 11:04 AM, Paolo Bonzini wrote:
>> On 24/09/21 03:15, Babu Moger wrote:
>>>    arch/x86/include/asm/cpufeatures.h |    1 +
>>>    arch/x86/kvm/cpuid.c               |    2 +-
>>>    2 files changed, 2 insertions(+), 1 deletion(-)
>> Queued, with a private #define instead of the one in cpufeatures.h:
> Thanks Paolo. Don't we need change in guest_has_spec_ctrl_msr?

Not strictly necessary unless you expect processors to have PSFD and not 
SSBD; but yes it's cleaner.

Paolo

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Babu Moger]

On 9/30/21 8:41 AM, Paolo Bonzini wrote:
> On 29/09/21 22:27, Babu Moger wrote:
>>
>> On 9/28/21 11:04 AM, Paolo Bonzini wrote:
>>> On 24/09/21 03:15, Babu Moger wrote:
>>>>    arch/x86/include/asm/cpufeatures.h |    1 +
>>>>    arch/x86/kvm/cpuid.c               |    2 +-
>>>>    2 files changed, 2 insertions(+), 1 deletion(-)
>>> Queued, with a private #define instead of the one in cpufeatures.h:
>> Thanks Paolo. Don't we need change in guest_has_spec_ctrl_msr?
> 
> Not strictly necessary unless you expect processors to have PSFD and not
> SSBD; but yes it's cleaner.

It is always both SSBD and PSFD together. We are good.Thanks

Tested-By: Babu Moger <babu.moger@amd.com>

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Jim Mattson]

On Thu, Sep 23, 2021 at 6:15 PM Babu Moger <babu.moger@amd.com> wrote:
>
> From: Babu Moger <Babu.Moger@amd.com>
>
> Predictive Store Forwarding: AMD Zen3 processors feature a new
> technology called Predictive Store Forwarding (PSF).
>
> PSF is a hardware-based micro-architectural optimization designed
> to improve the performance of code execution by predicting address
> dependencies between loads and stores.
>
> How PSF works:
>
> It is very common for a CPU to execute a load instruction to an address
> that was recently written by a store. Modern CPUs implement a technique
> known as Store-To-Load-Forwarding (STLF) to improve performance in such
> cases. With STLF, data from the store is forwarded directly to the load
> without having to wait for it to be written to memory. In a typical CPU,
> STLF occurs after the address of both the load and store are calculated
> and determined to match.
>
> PSF expands on this by speculating on the relationship between loads and
> stores without waiting for the address calculation to complete. With PSF,
> the CPU learns over time the relationship between loads and stores. If
> STLF typically occurs between a particular store and load, the CPU will
> remember this.
>
> In typical code, PSF provides a performance benefit by speculating on
> the load result and allowing later instructions to begin execution
> sooner than they otherwise would be able to.
>
> The details of security analysis of AMD predictive store forwarding is
> documented here.
> https://www.amd.com/system/files/documents/security-analysis-predictive-store-forwarding.pdf
>
> Predictive Store Forwarding controls:
> There are two hardware control bits which influence the PSF feature:
> - MSR 48h bit 2 – Speculative Store Bypass (SSBD)
> - MSR 48h bit 7 – Predictive Store Forwarding Disable (PSFD)
>
> The PSF feature is disabled if either of these bits are set.  These bits
> are controllable on a per-thread basis in an SMT system. By default, both
> SSBD and PSFD are 0 meaning that the speculation features are enabled.
>
> While the SSBD bit disables PSF and speculative store bypass, PSFD only
> disables PSF.
>
> PSFD may be desirable for software which is concerned with the
> speculative behavior of PSF but desires a smaller performance impact than
> setting SSBD.
>
> Support for PSFD is indicated in CPUID Fn8000_0008 EBX[28].
> All processors that support PSF will also support PSFD.
>
> Linux kernel does not have the interface to enable/disable PSFD yet. Plan
> here is to expose the PSFD technology to KVM so that the guest kernel can
> make use of it if they wish to.
>
> Signed-off-by: Babu Moger <Babu.Moger@amd.com>
> ---
> NOTE: Per earlier discussions, Linux kernel interface for PSF mitigation
> is not included in this series. This series only exposes the PSFD technology
> to KVM guests. Here is the link for earlier discussion.
> https://lore.kernel.org/lkml/20210517220059.6452-1-rsaripal@amd.com/
> https://lore.kernel.org/lkml/20210505190923.276051-1-rsaripal@amd.com/
> https://lore.kernel.org/lkml/20210430131733.192414-1-rsaripal@amd.com/
> https://lore.kernel.org/lkml/20210428160349.158774-1-rsaripal@amd.com/
> https://lore.kernel.org/lkml/20210422171013.50207-1-rsaripal@amd.com/
> https://lore.kernel.org/lkml/20210421090117.22315-1-rsaripal@amd.com/
>
>  arch/x86/include/asm/cpufeatures.h |    1 +
>  arch/x86/kvm/cpuid.c               |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index d0ce5cfd3ac1..7d6268ede35a 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -313,6 +313,7 @@
>  #define X86_FEATURE_AMD_SSBD           (13*32+24) /* "" Speculative Store Bypass Disable */
>  #define X86_FEATURE_VIRT_SSBD          (13*32+25) /* Virtualized Speculative Store Bypass Disable */
>  #define X86_FEATURE_AMD_SSB_NO         (13*32+26) /* "" Speculative Store Bypass is fixed in hardware. */
> +#define X86_FEATURE_PSFD               (13*32+28) /* Predictive Store Forwarding Disable */
>
>  /* Thermal and Power Management Leaf, CPUID level 0x00000006 (EAX), word 14 */
>  #define X86_FEATURE_DTHERM             (14*32+ 0) /* Digital Thermal Sensor */
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index fe03bd978761..ba773919f21d 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -500,7 +500,7 @@ void kvm_set_cpu_caps(void)
>         kvm_cpu_cap_mask(CPUID_8000_0008_EBX,
>                 F(CLZERO) | F(XSAVEERPTR) |
>                 F(WBNOINVD) | F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) | F(VIRT_SSBD) |
> -               F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON)
> +               F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON) | F(PSFD)
>         );
>
>         /*
>
>

For consistency, should this feature be renamed AMD_PSFD, now that
Intel is enumerating PSFD with CPUID.(EAX=7,ECX=2):EDX.PSFD[bit 0]?
See https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html.

And, Paolo, why are we carrying X86_FEATURE_PSFD as a private #define
in KVM rather than putting it where it belongs in cpufeatures.h?

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Paolo Bonzini]

On 8/23/22 23:26, Jim Mattson wrote:
> For consistency, should this feature be renamed AMD_PSFD, now that
> Intel is enumerating PSFD with CPUID.(EAX=7,ECX=2):EDX.PSFD[bit 0]?
> Seehttps://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html.
> 
> And, Paolo, why are we carrying X86_FEATURE_PSFD as a private #define
> in KVM rather than putting it where it belongs in cpufeatures.h?
> 

Borislav asked to not show psfd in /proc/cpuinfo, because Linux had
decided not to control PSF separately; instead it just piggybacked
on SSBD which should disable PSF as well.  Honestly I disagree but
it's not my area of maintenance.

Paolo

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Jim Mattson]

On Wed, Aug 24, 2022 at 10:13 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 8/23/22 23:26, Jim Mattson wrote:
> > For consistency, should this feature be renamed AMD_PSFD, now that
> > Intel is enumerating PSFD with CPUID.(EAX=7,ECX=2):EDX.PSFD[bit 0]?
> > Seehttps://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html.
> >
> > And, Paolo, why are we carrying X86_FEATURE_PSFD as a private #define
> > in KVM rather than putting it where it belongs in cpufeatures.h?
> >
>
> Borislav asked to not show psfd in /proc/cpuinfo, because Linux had
> decided not to control PSF separately; instead it just piggybacked
> on SSBD which should disable PSF as well.  Honestly I disagree but
> it's not my area of maintenance.

Do we expose PSFD in KVM for the use of another popular guest OS?

Re: [PATCH] KVM: x86: Expose Predictive Store Forwarding Disable

[Paolo Bonzini]

On 8/24/22 19:16, Jim Mattson wrote:
>> Borislav asked to not show psfd in /proc/cpuinfo, because Linux had
>> decided not to control PSF separately; instead it just piggybacked
>> on SSBD which should disable PSF as well.  Honestly I disagree but
>> it's not my area of maintenance.
> 
> Do we expose PSFD in KVM for the use of another popular guest OS?

Yes, that was the purpose of this patch and we expose it via 
MSR_IA32_SPEC_CTRL (the only validation that KVM does is in 
kvm_spec_ctrl_test_value(), so it does not need to know more about the 
specific bits).

Paolo