Re: [Qemu-devel] [PATCH 1/2] i386: define the AMD 'amd-ssbd' CPUID feature bit

[Daniel P . Berrangé]

On Wed, Jun 13, 2018 at 12:34:21PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 13, 2018 at 05:21:29PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jun 13, 2018 at 12:09:59PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Jun 13, 2018 at 11:19:49AM +0100, Daniel P. Berrangé wrote:
> > > > On Mon, Jun 04, 2018 at 04:22:05PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > > On Mon, Jun 04, 2018 at 05:07:01PM -0300, Eduardo Habkost wrote:
> > > > > > On Fri, Jun 01, 2018 at 11:38:08AM -0400, Konrad Rzeszutek Wilk 
> > > > > > wrote:
> > > > > > > AMD future CPUs expose _two_ ways to utilize the Intel equivalant
> > > > > > > of the Speculative Store Bypass Disable. The first is via
> > > > > > > the virtualized VIRT_SPEC CTRL MSR (0xC001_011f) and the second
> > > > > > > is via the SPEC_CTRL MSR (0x48). The document titled:
> > > > > > > 124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
> > > > > > > 
> > > > > > > gives priority of SPEC CTRL MSR over the VIRT SPEC CTRL MSR.
> > > > > > > 
> > > > > > > A copy of this document is available at
> > > > > > >       https://bugzilla.kernel.org/show_bug.cgi?id=199889
> > > > > > > 
> > > > > > > Anyhow, this means that on future AMD CPUs there will be  _two_ 
> > > > > > > ways to
> > > > > > > deal with SSBD.
> > > > > > 
> > > > > > Does anybody know if there are AMD CPUs where virt-ssbd won't
> > > > > > work and would require amd-ssbd to mitigate vulnerabilities?
> > > > > > 
> > > > > > Also, do we have kernel arch/x86/kvm/cpuid.c patches, already?
> > > > > 
> > > > > Not yet. They are being discussed right now. I figured I would send
> > > > > these patches out as a 'Hey, coming at you!', but failed to change
> > > > > the title to be 'RFC'.
> > > > > 
> > > > > > I prefer to add new CPUID flag names only after the flag name is
> > > > > > already agreed upon on the kernel side.
> > > > > 
> > > > > Of course. I will respin once that discussion has calmed down.
> > > > 
> > > > Looks like the kernel side has merged now, and we'll need to rename
> > > > the 2nd CPU bit from what I see.
> > > 
> > > What name did you have in mind?
> > 
> > IIUC from the kernel patches, it will be reported as 'amd-ssbd' and
> > 'amd-ssb-no' in /proc/cpuinfo, so only your second patch needs a simple
> > tweak to match that naming.
> 
> It will only report 'ssbd' but not 'amd-ssb-no' nor 'amd-ssbd'.
> 
> If the cpufeature.h has "" in the comment section then it is hidden. That is:
> 
> #define X86_FEATURE_MSR_SPEC_CTRL       ( 7*32+16) /* "" MSR SPEC_CTRL is 
> implemented */
> ..sniup..
> +#define X86_FEATURE_AMD_SSBD           (13*32+24) /* "" Speculative Store 
> Bypass Disable */
> +#define X86_FEATURE_AMD_SSB_NO         (13*32+26) /* "" Speculative Store 
> Bypass is fixed in hardware. */
> 
> are hidden ones, while:
> #define X86_FEATURE_SSBD                ( 7*32+17) /* Speculative Store 
> Bypass Disable */
> 
> is visible.

Ah, thanks for explaining that ! 


> The code that finds the AMD_SSBD and sets the 'ssbd' is:
> 
> +       if (cpu_has(c, X86_FEATURE_AMD_SSBD)) {
> +               set_cpu_cap(c, X86_FEATURE_SSBD);
> +               set_cpu_cap(c, X86_FEATURE_MSR_SPEC_CTRL);
> +               clear_cpu_cap(c, X86_FEATURE_VIRT_SSBD);
> +       }
> 
> Meaning the 'ssbd' will show up in /proc/cpuinfo

Given that, there's no exposed kernel naming we need to align with.

So personally I'd be fine with the current patches that exist, but
I'll defer to Eduardo for the final say, wrt amd-ssb-no vs amd-no-ssb.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|