| Message ID | 20200305013437.8578-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID emulation and tracing fixes | expand |
On 3/5/2020 9:34 AM, Sean Christopherson wrote: > Add helpers to provide CPUID-based guest vendor checks, i.e. to do the > ugly register comparisons. Use the new helpers to check for an AMD > guest vendor in guest_cpuid_is_amd() as well as in the existing emulator > flows. > > Using the new helpers fixes a _very_ theoretical bug where > guest_cpuid_is_amd() would get a false positive on a non-AMD virtual CPU > with a vendor string beginning with "Auth" due to the previous logic > only checking EBX. It also fixes a marginally less theoretically bug > where guest_cpuid_is_amd() would incorrectly return false for a guest > CPU with "AMDisbetter!" as its vendor string. > > Fixes: a0c0feb57992c ("KVM: x86: reserve bit 8 of non-leaf PDPEs and PML4Es in 64-bit mode on AMD") > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/include/asm/kvm_emulate.h | 24 ++++++++++++++++++++ > arch/x86/kvm/cpuid.h | 2 +- > arch/x86/kvm/emulate.c | 36 +++++++----------------------- > 3 files changed, 33 insertions(+), 29 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h > index bf5f5e476f65..2754972c36e6 100644 > --- a/arch/x86/include/asm/kvm_emulate.h > +++ b/arch/x86/include/asm/kvm_emulate.h > @@ -393,6 +393,30 @@ struct x86_emulate_ctxt { > #define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e > #define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69 > > +static inline bool is_guest_vendor_intel(u32 ebx, u32 ecx, u32 edx) > +{ > + return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx && > + ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx && > + edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx; > +} > + > +static inline bool is_guest_vendor_amd(u32 ebx, u32 ecx, u32 edx) > +{ > + return (ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx && > + ecx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx && > + edx == X86EMUL_CPUID_VENDOR_AuthenticAMD_edx) || > + (ebx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx && > + ecx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx && > + edx == X86EMUL_CPUID_VENDOR_AMDisbetterI_edx); > +} > + > +static inline bool is_guest_vendor_hygon(u32 ebx, u32 ecx, u32 edx) > +{ > + return ebx == X86EMUL_CPUID_VENDOR_HygonGenuine_ebx && > + ecx == X86EMUL_CPUID_VENDOR_HygonGenuine_ecx && > + edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx; > +} > + Why not define those in cpuid.h ? And also move X86EMUL_CPUID_VENDOR_* to cpuid.h and remove the "EMUL" prefix. > enum x86_intercept_stage { > X86_ICTP_NONE = 0, /* Allow zero-init to not match anything */ > X86_ICPT_PRE_EXCEPT, > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > index 7366c618aa04..13eb3e92c6a9 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -145,7 +145,7 @@ static inline bool guest_cpuid_is_amd(struct kvm_vcpu *vcpu) > struct kvm_cpuid_entry2 *best; > > best = kvm_find_cpuid_entry(vcpu, 0, 0); > - return best && best->ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx; > + return best && is_guest_vendor_amd(best->ebx, best->ecx, best->edx); > } > > static inline int guest_cpuid_family(struct kvm_vcpu *vcpu) > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index dd19fb3539e0..9cf303984fe5 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -2712,9 +2712,7 @@ static bool vendor_intel(struct x86_emulate_ctxt *ctxt) > > eax = ecx = 0; > ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false); > - return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx > - && ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx > - && edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx; > + return is_guest_vendor_intel(ebx, ecx, edx); > } > > static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt) > @@ -2733,34 +2731,16 @@ static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt) > ecx = 0x00000000; > ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false); > /* > - * Intel ("GenuineIntel") > - * remark: Intel CPUs only support "syscall" in 64bit > - * longmode. Also an 64bit guest with a > - * 32bit compat-app running will #UD !! While this > - * behaviour can be fixed (by emulating) into AMD > - * response - CPUs of AMD can't behave like Intel. > + * remark: Intel CPUs only support "syscall" in 64bit longmode. Also a > + * 64bit guest with a 32bit compat-app running will #UD !! While this > + * behaviour can be fixed (by emulating) into AMD response - CPUs of > + * AMD can't behave like Intel. > */ > - if (ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx && > - ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx && > - edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx) > + if (is_guest_vendor_intel(ebx, ecx, edx)) > return false; > > - /* AMD ("AuthenticAMD") */ > - if (ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx && > - ecx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx && > - edx == X86EMUL_CPUID_VENDOR_AuthenticAMD_edx) > - return true; > - > - /* AMD ("AMDisbetter!") */ > - if (ebx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx && > - ecx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx && > - edx == X86EMUL_CPUID_VENDOR_AMDisbetterI_edx) > - return true; > - > - /* Hygon ("HygonGenuine") */ > - if (ebx == X86EMUL_CPUID_VENDOR_HygonGenuine_ebx && > - ecx == X86EMUL_CPUID_VENDOR_HygonGenuine_ecx && > - edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx) > + if (is_guest_vendor_amd(ebx, ecx, edx) || > + is_guest_vendor_hygon(ebx, ecx, edx)) > return true; > > /* >
On Wed, Mar 4, 2020 at 5:34 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > Add helpers to provide CPUID-based guest vendor checks, i.e. to do the > ugly register comparisons. Use the new helpers to check for an AMD > guest vendor in guest_cpuid_is_amd() as well as in the existing emulator > flows. > > Using the new helpers fixes a _very_ theoretical bug where > guest_cpuid_is_amd() would get a false positive on a non-AMD virtual CPU > with a vendor string beginning with "Auth" due to the previous logic > only checking EBX. It also fixes a marginally less theoretically bug > where guest_cpuid_is_amd() would incorrectly return false for a guest > CPU with "AMDisbetter!" as its vendor string. > > Fixes: a0c0feb57992c ("KVM: x86: reserve bit 8 of non-leaf PDPEs and PML4Es in 64-bit mode on AMD") > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Reviewed-by: Jim Mattson <jmattson@google.com>
On Thu, Mar 05, 2020 at 11:48:20AM +0800, Xiaoyao Li wrote: > On 3/5/2020 9:34 AM, Sean Christopherson wrote: > >Add helpers to provide CPUID-based guest vendor checks, i.e. to do the > >ugly register comparisons. Use the new helpers to check for an AMD > >guest vendor in guest_cpuid_is_amd() as well as in the existing emulator > >flows. > > > >Using the new helpers fixes a _very_ theoretical bug where > >guest_cpuid_is_amd() would get a false positive on a non-AMD virtual CPU > >with a vendor string beginning with "Auth" due to the previous logic > >only checking EBX. It also fixes a marginally less theoretically bug > >where guest_cpuid_is_amd() would incorrectly return false for a guest > >CPU with "AMDisbetter!" as its vendor string. > > > >Fixes: a0c0feb57992c ("KVM: x86: reserve bit 8 of non-leaf PDPEs and PML4Es in 64-bit mode on AMD") > >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > >--- > > arch/x86/include/asm/kvm_emulate.h | 24 ++++++++++++++++++++ > > arch/x86/kvm/cpuid.h | 2 +- > > arch/x86/kvm/emulate.c | 36 +++++++----------------------- > > 3 files changed, 33 insertions(+), 29 deletions(-) > > > >diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h > >index bf5f5e476f65..2754972c36e6 100644 > >--- a/arch/x86/include/asm/kvm_emulate.h > >+++ b/arch/x86/include/asm/kvm_emulate.h > >@@ -393,6 +393,30 @@ struct x86_emulate_ctxt { > > #define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e > > #define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69 > >+static inline bool is_guest_vendor_intel(u32 ebx, u32 ecx, u32 edx) > >+{ > >+ return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx && > >+ ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx && > >+ edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx; > >+} > >+ > >+static inline bool is_guest_vendor_amd(u32 ebx, u32 ecx, u32 edx) > >+{ > >+ return (ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx && > >+ ecx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx && > >+ edx == X86EMUL_CPUID_VENDOR_AuthenticAMD_edx) || > >+ (ebx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx && > >+ ecx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx && > >+ edx == X86EMUL_CPUID_VENDOR_AMDisbetterI_edx); > >+} > >+ > >+static inline bool is_guest_vendor_hygon(u32 ebx, u32 ecx, u32 edx) > >+{ > >+ return ebx == X86EMUL_CPUID_VENDOR_HygonGenuine_ebx && > >+ ecx == X86EMUL_CPUID_VENDOR_HygonGenuine_ecx && > >+ edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx; > >+} > >+ > > Why not define those in cpuid.h ? > And also move X86EMUL_CPUID_VENDOR_* to cpuid.h and remove the "EMUL" > prefix. To avoid pulling cpuid.h into the emulator. Ideally, the emulator would only use KVM APIs defined in kvm_emulate.h. Obviously that's a bit of a pipe dream at the moment :-) #include <linux/kvm_host.h> #include "kvm_cache_regs.h" #include <asm/kvm_emulate.h> #include <linux/stringify.h> #include <asm/fpu/api.h> #include <asm/debugreg.h> #include <asm/nospec-branch.h> #include "x86.h" #include "tss.h" #include "mmu.h" #include "pmu.h" > > enum x86_intercept_stage { > > X86_ICTP_NONE = 0, /* Allow zero-init to not match anything */ > > X86_ICPT_PRE_EXCEPT, > >diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > >index 7366c618aa04..13eb3e92c6a9 100644 > >--- a/arch/x86/kvm/cpuid.h > >+++ b/arch/x86/kvm/cpuid.h > >@@ -145,7 +145,7 @@ static inline bool guest_cpuid_is_amd(struct kvm_vcpu *vcpu) > > struct kvm_cpuid_entry2 *best; > > best = kvm_find_cpuid_entry(vcpu, 0, 0); > >- return best && best->ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx; > >+ return best && is_guest_vendor_amd(best->ebx, best->ecx, best->edx); > > } > > static inline int guest_cpuid_family(struct kvm_vcpu *vcpu) > >diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > >index dd19fb3539e0..9cf303984fe5 100644 > >--- a/arch/x86/kvm/emulate.c > >+++ b/arch/x86/kvm/emulate.c > >@@ -2712,9 +2712,7 @@ static bool vendor_intel(struct x86_emulate_ctxt *ctxt) > > eax = ecx = 0; > > ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false); > >- return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx > >- && ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx > >- && edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx; > >+ return is_guest_vendor_intel(ebx, ecx, edx); > > } > > static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt) > >@@ -2733,34 +2731,16 @@ static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt) > > ecx = 0x00000000; > > ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false); > > /* > >- * Intel ("GenuineIntel") > >- * remark: Intel CPUs only support "syscall" in 64bit > >- * longmode. Also an 64bit guest with a > >- * 32bit compat-app running will #UD !! While this > >- * behaviour can be fixed (by emulating) into AMD > >- * response - CPUs of AMD can't behave like Intel. > >+ * remark: Intel CPUs only support "syscall" in 64bit longmode. Also a > >+ * 64bit guest with a 32bit compat-app running will #UD !! While this > >+ * behaviour can be fixed (by emulating) into AMD response - CPUs of > >+ * AMD can't behave like Intel. > > */ > >- if (ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx && > >- ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx && > >- edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx) > >+ if (is_guest_vendor_intel(ebx, ecx, edx)) > > return false; > >- /* AMD ("AuthenticAMD") */ > >- if (ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx && > >- ecx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx && > >- edx == X86EMUL_CPUID_VENDOR_AuthenticAMD_edx) > >- return true; > >- > >- /* AMD ("AMDisbetter!") */ > >- if (ebx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx && > >- ecx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx && > >- edx == X86EMUL_CPUID_VENDOR_AMDisbetterI_edx) > >- return true; > >- > >- /* Hygon ("HygonGenuine") */ > >- if (ebx == X86EMUL_CPUID_VENDOR_HygonGenuine_ebx && > >- ecx == X86EMUL_CPUID_VENDOR_HygonGenuine_ecx && > >- edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx) > >+ if (is_guest_vendor_amd(ebx, ecx, edx) || > >+ is_guest_vendor_hygon(ebx, ecx, edx)) > > return true; > > /* > > >
diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h index bf5f5e476f65..2754972c36e6 100644 --- a/arch/x86/include/asm/kvm_emulate.h +++ b/arch/x86/include/asm/kvm_emulate.h @@ -393,6 +393,30 @@ struct x86_emulate_ctxt { #define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e #define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69 +static inline bool is_guest_vendor_intel(u32 ebx, u32 ecx, u32 edx) +{ + return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx && + ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx && + edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx; +} + +static inline bool is_guest_vendor_amd(u32 ebx, u32 ecx, u32 edx) +{ + return (ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx && + ecx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx && + edx == X86EMUL_CPUID_VENDOR_AuthenticAMD_edx) || + (ebx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx && + ecx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx && + edx == X86EMUL_CPUID_VENDOR_AMDisbetterI_edx); +} + +static inline bool is_guest_vendor_hygon(u32 ebx, u32 ecx, u32 edx) +{ + return ebx == X86EMUL_CPUID_VENDOR_HygonGenuine_ebx && + ecx == X86EMUL_CPUID_VENDOR_HygonGenuine_ecx && + edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx; +} + enum x86_intercept_stage { X86_ICTP_NONE = 0, /* Allow zero-init to not match anything */ X86_ICPT_PRE_EXCEPT, diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h index 7366c618aa04..13eb3e92c6a9 100644 --- a/arch/x86/kvm/cpuid.h +++ b/arch/x86/kvm/cpuid.h @@ -145,7 +145,7 @@ static inline bool guest_cpuid_is_amd(struct kvm_vcpu *vcpu) struct kvm_cpuid_entry2 *best; best = kvm_find_cpuid_entry(vcpu, 0, 0); - return best && best->ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx; + return best && is_guest_vendor_amd(best->ebx, best->ecx, best->edx); } static inline int guest_cpuid_family(struct kvm_vcpu *vcpu) diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index dd19fb3539e0..9cf303984fe5 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -2712,9 +2712,7 @@ static bool vendor_intel(struct x86_emulate_ctxt *ctxt) eax = ecx = 0; ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false); - return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx - && ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx - && edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx; + return is_guest_vendor_intel(ebx, ecx, edx); } static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt) @@ -2733,34 +2731,16 @@ static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt) ecx = 0x00000000; ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false); /* - * Intel ("GenuineIntel") - * remark: Intel CPUs only support "syscall" in 64bit - * longmode. Also an 64bit guest with a - * 32bit compat-app running will #UD !! While this - * behaviour can be fixed (by emulating) into AMD - * response - CPUs of AMD can't behave like Intel. + * remark: Intel CPUs only support "syscall" in 64bit longmode. Also a + * 64bit guest with a 32bit compat-app running will #UD !! While this + * behaviour can be fixed (by emulating) into AMD response - CPUs of + * AMD can't behave like Intel. */ - if (ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx && - ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx && - edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx) + if (is_guest_vendor_intel(ebx, ecx, edx)) return false; - /* AMD ("AuthenticAMD") */ - if (ebx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx && - ecx == X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx && - edx == X86EMUL_CPUID_VENDOR_AuthenticAMD_edx) - return true; - - /* AMD ("AMDisbetter!") */ - if (ebx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx && - ecx == X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx && - edx == X86EMUL_CPUID_VENDOR_AMDisbetterI_edx) - return true; - - /* Hygon ("HygonGenuine") */ - if (ebx == X86EMUL_CPUID_VENDOR_HygonGenuine_ebx && - ecx == X86EMUL_CPUID_VENDOR_HygonGenuine_ecx && - edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx) + if (is_guest_vendor_amd(ebx, ecx, edx) || + is_guest_vendor_hygon(ebx, ecx, edx)) return true; /*
Add helpers to provide CPUID-based guest vendor checks, i.e. to do the ugly register comparisons. Use the new helpers to check for an AMD guest vendor in guest_cpuid_is_amd() as well as in the existing emulator flows. Using the new helpers fixes a _very_ theoretical bug where guest_cpuid_is_amd() would get a false positive on a non-AMD virtual CPU with a vendor string beginning with "Auth" due to the previous logic only checking EBX. It also fixes a marginally less theoretically bug where guest_cpuid_is_amd() would incorrectly return false for a guest CPU with "AMDisbetter!" as its vendor string. Fixes: a0c0feb57992c ("KVM: x86: reserve bit 8 of non-leaf PDPEs and PML4Es in 64-bit mode on AMD") Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/asm/kvm_emulate.h | 24 ++++++++++++++++++++ arch/x86/kvm/cpuid.h | 2 +- arch/x86/kvm/emulate.c | 36 +++++++----------------------- 3 files changed, 33 insertions(+), 29 deletions(-)