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

[Jim Mattson]

On Mon, Sep 30, 2019 at 10:54 AM Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden> wrote:
> > > >>
> > > >> Signed-off-by: Robert Hoo <address@hidden>
> > > >> ---
> > > >>  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.