Re: [PATCH] x86: Add CPUID KVM support for new instruction WBNOINVD

["Huang, Kai" <kai.huang@intel.com>]

On Mon, 2019-09-30 at 12:23 -0700, Jim Mattson wrote:
> On Mon, Sep 30, 2019 at 10:54 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > CCing qemu-devel.
> > 
> > On Tue, Sep 24, 2019 at 01:30:04PM -0700, Jim Mattson wrote:
> > > On Wed, Dec 19, 2018 at 1:02 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > On 19/12/18 18:39, Jim Mattson wrote:
> > > > > Is this an instruction that kvm has to be able to emulate before it
> > > > > can enumerate its existence?
> > > > 
> > > > It doesn't have any operands, so no.
> > > > 
> > > > Paolo
> > > > 
> > > > > On Wed, Dec 19, 2018 at 5:51 AM Robert Hoo <robert.hu@linux.intel.com>
> > > > > wrote:
> > > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.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 28c4a50..932b19f 100644
> > > > > > --- a/arch/x86/include/asm/cpufeatures.h
> > > > > > +++ b/arch/x86/include/asm/cpufeatures.h
> > > > > > @@ -280,6 +280,7 @@
> > > > > >  /* AMD-defined CPU features, CPUID level 0x80000008 (EBX), word 13
> > > > > > */
> > > > > >  #define X86_FEATURE_CLZERO             (13*32+ 0) /* CLZERO
> > > > > > instruction */
> > > > > >  #define X86_FEATURE_IRPERF             (13*32+ 1) /* Instructions
> > > > > > Retired Count */
> > > > > > +#define X86_FEATURE_WBNOINVD           (13*32+ 9) /* Writeback and
> > > > > > Don't invalid cache */
> > > > > >  #define X86_FEATURE_XSAVEERPTR         (13*32+ 2) /* Always
> > > > > > save/restore FP error pointers */
> > > > > >  #define X86_FEATURE_AMD_IBPB           (13*32+12) /* "" Indirect
> > > > > > Branch Prediction Barrier */
> > > > > >  #define X86_FEATURE_AMD_IBRS           (13*32+14) /* "" Indirect
> > > > > > Branch Restricted Speculation */
> > > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > > > > index cc6dd65..763e115 100644
> > > > > > --- a/arch/x86/kvm/cpuid.c
> > > > > > +++ b/arch/x86/kvm/cpuid.c
> > > > > > @@ -380,7 +380,7 @@ static inline int __do_cpuid_ent(struct
> > > > > > kvm_cpuid_entry2 *entry, u32 function,
> > > > > > 
> > > > > >         /* cpuid 0x80000008.ebx */
> > > > > >         const u32 kvm_cpuid_8000_0008_ebx_x86_features =
> > > > > > -               F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) |
> > > > > > F(VIRT_SSBD) |
> > > > > > +               F(WBNOINVD) | F(AMD_IBPB) | F(AMD_IBRS) |
> > > > > > F(AMD_SSBD) | F(VIRT_SSBD) |
> > > > > >                 F(AMD_SSB_NO) | F(AMD_STIBP);
> > > > > > 
> > > > > >         /* cpuid 0xC0000001.edx */
> > > > > > --
> > > > > > 1.8.3.1
> > > > > > 
> > > 
> > > What is the point of enumerating support for WBNOINVD if kvm is going
> > > to implement it as WBINVD?
> > 
> > I expect GET_SUPPORTED_CPUID to return WBNOINVD, because it
> > indicates to userspace what is supported by KVM.  Are there any
> > expectations that GET_SUPPORTED_CPUID will also dictate what is
> > enabled by default in some cases?
> > 
> > In either case, your question applies to QEMU: why do we want
> > WBNOINVD to be enabled by "-cpu host" by default and be part of
> > QEMU's Icelake-* CPU model definitions?
> 
> I had only looked at the SVM implementation of WBNOINVD, which is
> exactly the same as the SVM implementation of WBINVD. So, the question
> is, "why enumerate WBNOINVD if its implementation is exactly the same
> as WBINVD?"
> 
> WBNOINVD appears to be only partially documented in Intel document
> 319433-037, "Intel® Architecture Instruction Set Extensions and Future
> Features Programming Reference." In particular, there is no
> documentation regarding the instruction's behavior in VMX non-root
> mode. Does WBNOINVD cause a VM-exit when the VM-execution control,
> "WBINVD exiting," is set? If so, does it have the same VM-exit reason
> as WBINVD (54), or a different one? If it does have the same VM-exit
> reason (a la SVM), how does one distinguish a WBINVD VM-exit from a
> WBNOINVD VM-exit? If one can't distinguish (a la SVM), then it would
> seem that the VMX implementation also implements WBNOINVD as WBINVD.
> If that's the case, the question for VMX is the same as for SVM.

Unfortunately WBNOINVD interaction with VMX has not been made to public yet. I
am reaching out internally to see when it can be done. I agree it may not be
necessary to expose WBNOINVD if its implementation is exactly the same as
WBINVD, but it also doesn't have any harm, right?

Thanks,
-Kai