[PATCH v2 0/7] KVM: x86: CPUID emulation and tracing fixes

[PATCH v2 0/7] KVM: x86: CPUID emulation and tracing fixes

[Sean Christopherson]

Round two of trying to wrangle kvm_cpuid() into submission.  Two more bug
fixes, both related to querying for an "AMD" vendor, in addition to the
fixes in v1 (tracing and Hypervisor/Centaur range checks).

In theory, everything up to the refactoring is non-controversial, i.e. we
can bikeshed the refactoring without delaying the bug fixes.

v2:
  - Use Jan's patch to fix the trace bug. [Everyone]
  - Rework Hypervisor/Centaur handling so that only the Hypervisor
    sub-ranges get the restrictive 0xffffff00 mask, and so that Centaur's
    range only gets recognized when the guest vendor is Centaur. [Jim]
  - Add the aforementioned bug fixes.
  - Add a patch to do build time assertions on the vendor string, which
    are hand coded u32s in the emulator (for direct comparison against
    CPUID register output).
  - Drop the patch to add CPUID.maxphyaddr emulator helper. [Paolo]
  - Redo refactoring patches to land them after all the bug fixes
    and to do the refactoring without any semantic changes in the
    emulator.

Jan Kiszka (1):
  KVM: x86: Trace the original requested CPUID function in kvm_cpuid()

Sean Christopherson (6):
  KVM: x86: Add helpers to perform CPUID-based guest vendor check
  KVM x86: Extend AMD specific guest behavior to Hygon virtual CPUs
  KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes
  KVM: x86: Add build-time assertions on validity of vendor strings
  KVM: x86: Refactor out-of-range logic to contain the madness
  KVM: x86: Refactor kvm_cpuid() param that controls out-of-range logic

 arch/x86/include/asm/kvm_emulate.h |  37 +++++++++-
 arch/x86/kvm/cpuid.c               | 111 +++++++++++++++++++++--------
 arch/x86/kvm/cpuid.h               |   8 ++-
 arch/x86/kvm/emulate.c             |  64 ++++++++---------
 arch/x86/kvm/mmu/mmu.c             |   3 +-
 arch/x86/kvm/svm.c                 |   2 +-
 arch/x86/kvm/x86.c                 |   7 +-
 7 files changed, 162 insertions(+), 70 deletions(-)

-- 
2.24.1

[PATCH v2 1/7] KVM: x86: Trace the original requested CPUID function in kvm_cpuid()

[Sean Christopherson]

From: Jan Kiszka <jan.kiszka@siemens.com>

Trace the requested CPUID function instead of the effective function,
e.g. if the requested function is out-of-range and KVM is emulating an
Intel CPU, as the intent of the tracepoint is to show if the output came
from the actual leaf as opposed to the max basic leaf via redirection.

Similarly, leave "found" as is, i.e. report that an entry was found if
and only if the requested entry was found.

Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
[Sean: Drop "found" semantic change, reword changelong accordingly ]
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b1c469446b07..b4beb3707d1b 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1000,7 +1000,7 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
 bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 	       u32 *ecx, u32 *edx, bool check_limit)
 {
-	u32 function = *eax, index = *ecx;
+	u32 orig_function = *eax, function = *eax, index = *ecx;
 	struct kvm_cpuid_entry2 *entry;
 	struct kvm_cpuid_entry2 *max;
 	bool found;
@@ -1049,7 +1049,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 			}
 		}
 	}
-	trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, found);
+	trace_kvm_cpuid(orig_function, *eax, *ebx, *ecx, *edx, found);
 	return found;
 }
 EXPORT_SYMBOL_GPL(kvm_cpuid);
-- 
2.24.1

[PATCH v2 2/7] KVM: x86: Add helpers to perform CPUID-based guest vendor check

[Sean Christopherson]

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;
+}
+
 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;
 
 	/*
-- 
2.24.1

[PATCH v2 3/7] KVM x86: Extend AMD specific guest behavior to Hygon virtual CPUs

[Sean Christopherson]

Extend guest_cpuid_is_amd() to cover Hygon virtual CPUs and rename it
accordingly.  Hygon CPUs use an AMD-based core and so have the same
basic behavior as AMD CPUs.

Fixes: b8f4abb652146 ("x86/kvm: Add Hygon Dhyana support to KVM")
Cc: Pu Wen <puwen@hygon.cn>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c   | 2 +-
 arch/x86/kvm/cpuid.h   | 6 ++++--
 arch/x86/kvm/mmu/mmu.c | 3 ++-
 arch/x86/kvm/x86.c     | 2 +-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b4beb3707d1b..5a9891cb2bc6 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1013,7 +1013,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 	 * requested. AMD CPUID semantics returns all zeroes for any
 	 * undefined leaf, whether or not the leaf is in range.
 	 */
-	if (!entry && check_limit && !guest_cpuid_is_amd(vcpu) &&
+	if (!entry && check_limit && !guest_cpuid_is_amd_or_hygon(vcpu) &&
 	    !cpuid_function_in_range(vcpu, function)) {
 		max = kvm_find_cpuid_entry(vcpu, 0, 0);
 		if (max) {
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 13eb3e92c6a9..332068db0fc2 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -140,12 +140,14 @@ static __always_inline void guest_cpuid_clear(struct kvm_vcpu *vcpu, unsigned x8
 		*reg &= ~__feature_bit(x86_feature);
 }
 
-static inline bool guest_cpuid_is_amd(struct kvm_vcpu *vcpu)
+static inline bool guest_cpuid_is_amd_or_hygon(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
 
 	best = kvm_find_cpuid_entry(vcpu, 0, 0);
-	return best && is_guest_vendor_amd(best->ebx, best->ecx, best->edx);
+	return best &&
+	       (is_guest_vendor_amd(best->ebx, best->ecx, best->edx) ||
+		is_guest_vendor_hygon(best->ebx, best->ecx, best->edx));
 }
 
 static inline int guest_cpuid_family(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index a1f4e325420e..c7ef2745a4e0 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4507,7 +4507,8 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu,
 				cpuid_maxphyaddr(vcpu), context->root_level,
 				context->nx,
 				guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES),
-				is_pse(vcpu), guest_cpuid_is_amd(vcpu));
+				is_pse(vcpu),
+				guest_cpuid_is_amd_or_hygon(vcpu));
 }
 
 static void
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fa03f31ab33c..1a4836ed1230 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2524,7 +2524,7 @@ static void kvmclock_sync_fn(struct work_struct *work)
 static bool can_set_mci_status(struct kvm_vcpu *vcpu)
 {
 	/* McStatusWrEn enabled? */
-	if (guest_cpuid_is_amd(vcpu))
+	if (guest_cpuid_is_amd_or_hygon(vcpu))
 		return !!(vcpu->arch.msr_hwcr & BIT_ULL(18));
 
 	return false;
-- 
2.24.1

[PATCH v2 4/7] KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes

[Sean Christopherson]

Rework the masking in the out-of-range CPUID logic to handle the
Hypervisor sub-classes, as well as the Centaur class if the guest
virtual CPU vendor is Centaur.

Masking against 0x80000000 only handles basic and extended leafs, which
results in Hypervisor range checks being performed against the basic
CPUID class, and Centuar range checks being performed against the
Extended class.  E.g. if CPUID.0x40000000.EAX returns 0x4000000A and
there is no entry for CPUID.0x40000006, then function 0x40000006 would
be incorrectly reported as out of bounds.

While there is no official definition of what constitutes a class, the
convention established for Hypervisor classes effectively uses bits 31:8
as the mask by virtue of checking for different bases in increments of
0x100, e.g. KVM advertises its CPUID functions starting at 0x40000100
when HyperV features are advertised at the default base of 0x40000000.

The bad range check doesn't cause functional problems for any known VMM
because out-of-range semantics only come into play if the exact entry
isn't found, and VMMs either support a very limited Hypervisor range,
e.g. the official KVM range is 0x40000000-0x40000001 (effectively no
room for undefined leafs) or explicitly defines gaps to be zero, e.g.
Qemu explicitly creates zeroed entries up to the Cenatur and Hypervisor
limits (the latter comes into play when providing HyperV features).

The bad behavior can be visually confirmed by dumping CPUID output in
the guest when running Qemu with a stable TSC, as Qemu extends the limit
of range 0x40000000 to 0x40000010 to advertise VMware's cpuid_freq,
without defining zeroed entries for 0x40000002 - 0x4000000f.

Note, documentation of Centaur/VIA CPUs is hard to come by.  Designating
0xc0000000 - 0xcfffffff as the Centaur class is a best guess as to the
behavior of a real Centaur/VIA CPU.

Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
Cc: Jim Mattson <jmattson@google.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

I don't particularly like the "!basic" check in cpuid_function_in_range(),
but it's semantically correct, it's just weird when verbalizing the logic.
E.g. "all functions are in range if there are no ranges".  I thought about
renaming and inverting the function so that the logic would read "all
functions are out of range if there are no ranges", but that added churn
without tangible value.

I could have buried the check in the Centaur leaf handling, which is why
it exists in this patch, but I didn't like the resulting code and the
whole weirdness goes away once the code is refactored to put all the
logic into a single helper.

 arch/x86/include/asm/kvm_emulate.h | 11 ++++++
 arch/x86/kvm/cpuid.c               | 54 ++++++++++++++++++++++++++----
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 2754972c36e6..0fb41a2c972f 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -393,6 +393,10 @@ struct x86_emulate_ctxt {
 #define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e
 #define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69
 
+#define X86EMUL_CPUID_VENDOR_CentaurHauls_ebx 0x746e6543
+#define X86EMUL_CPUID_VENDOR_CentaurHauls_ecx 0x736c7561
+#define X86EMUL_CPUID_VENDOR_CentaurHauls_edx 0x48727561
+
 static inline bool is_guest_vendor_intel(u32 ebx, u32 ecx, u32 edx)
 {
 	return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx &&
@@ -417,6 +421,13 @@ static inline bool is_guest_vendor_hygon(u32 ebx, u32 ecx, u32 edx)
 	       edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx;
 }
 
+static inline bool is_guest_vendor_centaur(u32 ebx, u32 ecx, u32 edx)
+{
+	return ebx == X86EMUL_CPUID_VENDOR_CentaurHauls_ebx &&
+	       ecx == X86EMUL_CPUID_VENDOR_CentaurHauls_ecx &&
+	       edx == X86EMUL_CPUID_VENDOR_CentaurHauls_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.c b/arch/x86/kvm/cpuid.c
index 5a9891cb2bc6..dd3c91ffae11 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -985,16 +985,58 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);
 
 /*
- * If the basic or extended CPUID leaf requested is higher than the
- * maximum supported basic or extended leaf, respectively, then it is
- * out of range.
+ * Intel CPUID semantics treats any query for an out-of-range leaf as if the
+ * highest basic leaf (i.e. CPUID.0H:EAX) were requested.  AMD CPUID semantics
+ * returns all zeroes for any undefined leaf, whether or not the leaf is in
+ * range.  Centaur/VIA follows Intel semantics.
+ *
+ * A leaf is considered out-of-range if its function is higher than the maximum
+ * supported leaf of its associated class or if its associated class does not
+ * exist.
+ *
+ * There are three primary classes to be considered, with their respective
+ * ranges described as "<base> - <top>[,<base2> - <top2>] inclusive.  A primary
+ * class exists if a guest CPUID entry for its <base> leaf exists.  For a given
+ * class, CPUID.<base>.EAX contains the max supported leaf for the class.
+ *
+ *  - Basic:      0x00000000 - 0x3fffffff, 0x50000000 - 0x7fffffff
+ *  - Hypervisor: 0x40000000 - 0x4fffffff
+ *  - Extended:   0x80000000 - 0xffffffff
+ *
+ * The Hypervisor class is further subdivided into sub-classes that each act as
+ * their own indepdent class associated with a 0x100 byte range.  E.g. if Qemu
+ * is advertising support for both HyperV and KVM, the resulting Hypervisor
+ * CPUID sub-classes are:
+ *
+ *  - HyperV:     0x40000000 - 0x400000ff
+ *  - KVM:        0x40000100 - 0x400001ff
+ *
+ * If the guest vendor is Centaur, a fourth top-level class is defined and
+ * effectively steals its range from the Extended class.  I.e. the primary
+ * classes for a Centaur guest CPU become:
+ *
+ *  - Basic:      0x00000000 - 0x3fffffff, 0x50000000 - 0x7fffffff
+ *  - Hypervisor: 0x40000000 - 0x4fffffff
+ *  - Extended:   0x80000000 - 0xbfffffff, 0xd0000000 - 0xffffffff
+ *  - Centaur:    0xc0000000 - 0xcfffffff
  */
 static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
 {
-	struct kvm_cpuid_entry2 *max;
+	struct kvm_cpuid_entry2 *basic, *class;
 
-	max = kvm_find_cpuid_entry(vcpu, function & 0x80000000, 0);
-	return max && function <= max->eax;
+	basic = kvm_find_cpuid_entry(vcpu, 0, 0);
+	if (!basic)
+		return true;
+
+	if (function >= 0x40000000 && function <= 0x4fffffff)
+		class = kvm_find_cpuid_entry(vcpu, function & 0xffffff00, 0);
+	else if (function >= 0xc0000000 && function <= 0xcfffffff &&
+		 is_guest_vendor_centaur(basic->ebx, basic->ecx, basic->edx))
+		class = kvm_find_cpuid_entry(vcpu, function & 0xc0000000, 0);
+	else
+		class = kvm_find_cpuid_entry(vcpu, function & 0x80000000, 0);
+
+	return class && function <= class->eax;
 }
 
 bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
-- 
2.24.1

[PATCH v2 5/7] KVM: x86: Add build-time assertions on validity of vendor strings

[Sean Christopherson]

Add build-time assertions on the transcoded ASCII->u32 values for the
vendor strings.  The u32 values are inscrutable, and to make things
worse, the order of registers used to build the strings is B->D->C,
i.e. completely illogical.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/emulate.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 9cf303984fe5..7391e1471e53 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3952,6 +3952,26 @@ static int em_cpuid(struct x86_emulate_ctxt *ctxt)
 	u32 eax, ebx, ecx, edx;
 	u64 msr = 0;
 
+	BUILD_BUG_ON(X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx != *(u32 *)"Auth" ||
+		     X86EMUL_CPUID_VENDOR_AuthenticAMD_edx != *(u32 *)"enti" ||
+		     X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx != *(u32 *)"cAMD");
+
+	BUILD_BUG_ON(X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx != *(u32 *)"AMDi" ||
+		     X86EMUL_CPUID_VENDOR_AMDisbetterI_edx != *(u32 *)"sbet" ||
+		     X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx != *(u32 *)"ter!");
+
+	BUILD_BUG_ON(X86EMUL_CPUID_VENDOR_HygonGenuine_ebx != *(u32 *)"Hygo" ||
+		     X86EMUL_CPUID_VENDOR_HygonGenuine_edx != *(u32 *)"nGen" ||
+		     X86EMUL_CPUID_VENDOR_HygonGenuine_ecx != *(u32 *)"uine");
+
+	BUILD_BUG_ON(X86EMUL_CPUID_VENDOR_GenuineIntel_ebx != *(u32 *)"Genu" ||
+		     X86EMUL_CPUID_VENDOR_GenuineIntel_edx != *(u32 *)"ineI" ||
+		     X86EMUL_CPUID_VENDOR_GenuineIntel_ecx != *(u32 *)"ntel");
+
+	BUILD_BUG_ON(X86EMUL_CPUID_VENDOR_CentaurHauls_ebx != *(u32 *)"Cent" ||
+		     X86EMUL_CPUID_VENDOR_CentaurHauls_edx != *(u32 *)"aurH" ||
+		     X86EMUL_CPUID_VENDOR_CentaurHauls_ecx != *(u32 *)"auls");
+
 	ctxt->ops->get_msr(ctxt, MSR_MISC_FEATURES_ENABLES, &msr);
 	if (msr & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
 	    ctxt->ops->cpl(ctxt)) {
-- 
2.24.1

[PATCH v2 6/7] KVM: x86: Refactor out-of-range logic to contain the madness

[Sean Christopherson]

Move all of the out-of-range logic into a single helper,
get_out_of_range_cpuid_entry(), to avoid an extra lookup of CPUID.0.0
and to provide a single location for documenting the out-of-range
behavior.

No functional change intended.

Cc: Jim Mattson <jmattson@google.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c | 47 +++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index dd3c91ffae11..e4f104c7ace8 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1020,13 +1020,19 @@ EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);
  *  - Extended:   0x80000000 - 0xbfffffff, 0xd0000000 - 0xffffffff
  *  - Centaur:    0xc0000000 - 0xcfffffff
  */
-static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
+static struct kvm_cpuid_entry2 *
+get_out_of_range_cpuid_entry(struct kvm_vcpu *vcpu, u32 *fn_ptr, u32 index)
 {
 	struct kvm_cpuid_entry2 *basic, *class;
+	u32 function = *fn_ptr;
 
 	basic = kvm_find_cpuid_entry(vcpu, 0, 0);
 	if (!basic)
-		return true;
+		return NULL;
+
+	if (is_guest_vendor_amd(basic->ebx, basic->ecx, basic->edx) ||
+	    is_guest_vendor_hygon(basic->ebx, basic->ecx, basic->edx))
+		return NULL;
 
 	if (function >= 0x40000000 && function <= 0x4fffffff)
 		class = kvm_find_cpuid_entry(vcpu, function & 0xffffff00, 0);
@@ -1036,7 +1042,23 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
 	else
 		class = kvm_find_cpuid_entry(vcpu, function & 0x80000000, 0);
 
-	return class && function <= class->eax;
+	if (class && function <= class->eax)
+		return NULL;
+
+	/*
+	 * Leaf specific adjustments are also applied when redirecting to the
+	 * max basic entry, e.g. if the max basic leaf is 0xb but there is no
+	 * entry for CPUID.0xb.index (see below), then the output value for EDX
+	 * needs to be pulled from CPUID.0xb.1.
+	 */
+	*fn_ptr = basic->eax;
+
+	/*
+	 * The class does not exist or the requested function is out of range;
+	 * the effective CPUID entry is the max basic leaf.  Note, the index of
+	 * the original requested leaf is observed!
+	 */
+	return kvm_find_cpuid_entry(vcpu, basic->eax, index);
 }
 
 bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
@@ -1044,25 +1066,14 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 {
 	u32 orig_function = *eax, function = *eax, index = *ecx;
 	struct kvm_cpuid_entry2 *entry;
-	struct kvm_cpuid_entry2 *max;
 	bool found;
 
 	entry = kvm_find_cpuid_entry(vcpu, function, index);
 	found = entry;
-	/*
-	 * Intel CPUID semantics treats any query for an out-of-range
-	 * leaf as if the highest basic leaf (i.e. CPUID.0H:EAX) were
-	 * requested. AMD CPUID semantics returns all zeroes for any
-	 * undefined leaf, whether or not the leaf is in range.
-	 */
-	if (!entry && check_limit && !guest_cpuid_is_amd_or_hygon(vcpu) &&
-	    !cpuid_function_in_range(vcpu, function)) {
-		max = kvm_find_cpuid_entry(vcpu, 0, 0);
-		if (max) {
-			function = max->eax;
-			entry = kvm_find_cpuid_entry(vcpu, function, index);
-		}
-	}
+
+	if (!entry && check_limit)
+		entry = get_out_of_range_cpuid_entry(vcpu, &function, index);
+
 	if (entry) {
 		*eax = entry->eax;
 		*ebx = entry->ebx;
-- 
2.24.1

[PATCH v2 7/7] KVM: x86: Refactor kvm_cpuid() param that controls out-of-range logic

[Sean Christopherson]

Invert and rename the kvm_cpuid() param that controls out-of-range logic
to better reflect the semantics of the affected callers, i.e. callers
that bypass the out-of-range logic do so because they are looking up an
exact guest CPUID entry, e.g. to query the maxphyaddr.

Similarly, rename kvm_cpuid()'s internal "found" to "exact" to clarify
that it tracks whether or not the exact requested leaf was found, as
opposed to any usable leaf being found.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_emulate.h |  2 +-
 arch/x86/kvm/cpuid.c               | 14 +++++++-------
 arch/x86/kvm/cpuid.h               |  2 +-
 arch/x86/kvm/emulate.c             |  8 ++++----
 arch/x86/kvm/svm.c                 |  2 +-
 arch/x86/kvm/x86.c                 |  5 +++--
 6 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 0fb41a2c972f..e20d16bd81d3 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -221,7 +221,7 @@ struct x86_emulate_ops {
 			 enum x86_intercept_stage stage);
 
 	bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt, u32 *eax, u32 *ebx,
-			  u32 *ecx, u32 *edx, bool check_limit);
+			  u32 *ecx, u32 *edx, bool exact_only);
 	bool (*guest_has_long_mode)(struct x86_emulate_ctxt *ctxt);
 	bool (*guest_has_movbe)(struct x86_emulate_ctxt *ctxt);
 	bool (*guest_has_fxsr)(struct x86_emulate_ctxt *ctxt);
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index e4f104c7ace8..d8a190d5dac0 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1062,16 +1062,16 @@ get_out_of_range_cpuid_entry(struct kvm_vcpu *vcpu, u32 *fn_ptr, u32 index)
 }
 
 bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
-	       u32 *ecx, u32 *edx, bool check_limit)
+	       u32 *ecx, u32 *edx, bool exact_only)
 {
 	u32 orig_function = *eax, function = *eax, index = *ecx;
 	struct kvm_cpuid_entry2 *entry;
-	bool found;
+	bool exact;
 
 	entry = kvm_find_cpuid_entry(vcpu, function, index);
-	found = entry;
+	exact = !!entry;
 
-	if (!entry && check_limit)
+	if (!entry && !exact_only)
 		entry = get_out_of_range_cpuid_entry(vcpu, &function, index);
 
 	if (entry) {
@@ -1102,8 +1102,8 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 			}
 		}
 	}
-	trace_kvm_cpuid(orig_function, *eax, *ebx, *ecx, *edx, found);
-	return found;
+	trace_kvm_cpuid(orig_function, *eax, *ebx, *ecx, *edx, exact);
+	return exact;
 }
 EXPORT_SYMBOL_GPL(kvm_cpuid);
 
@@ -1116,7 +1116,7 @@ int kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
 
 	eax = kvm_rax_read(vcpu);
 	ecx = kvm_rcx_read(vcpu);
-	kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);
+	kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, false);
 	kvm_rax_write(vcpu, eax);
 	kvm_rbx_write(vcpu, ebx);
 	kvm_rcx_write(vcpu, ecx);
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 332068db0fc2..917ee014af55 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -23,7 +23,7 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
 			      struct kvm_cpuid2 *cpuid,
 			      struct kvm_cpuid_entry2 __user *entries);
 bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
-	       u32 *ecx, u32 *edx, bool check_limit);
+	       u32 *ecx, u32 *edx, bool exact_only);
 
 int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu);
 
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 7391e1471e53..d54e259fbbfd 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2711,7 +2711,7 @@ static bool vendor_intel(struct x86_emulate_ctxt *ctxt)
 	u32 eax, ebx, ecx, edx;
 
 	eax = ecx = 0;
-	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false);
+	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, true);
 	return is_guest_vendor_intel(ebx, ecx, edx);
 }
 
@@ -2729,7 +2729,7 @@ static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt)
 
 	eax = 0x00000000;
 	ecx = 0x00000000;
-	ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false);
+	ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, true);
 	/*
 	 * remark: Intel CPUs only support "syscall" in 64bit longmode. Also a
 	 * 64bit guest with a 32bit compat-app running will #UD !! While this
@@ -3980,7 +3980,7 @@ static int em_cpuid(struct x86_emulate_ctxt *ctxt)
 
 	eax = reg_read(ctxt, VCPU_REGS_RAX);
 	ecx = reg_read(ctxt, VCPU_REGS_RCX);
-	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, true);
+	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, false);
 	*reg_write(ctxt, VCPU_REGS_RAX) = eax;
 	*reg_write(ctxt, VCPU_REGS_RBX) = ebx;
 	*reg_write(ctxt, VCPU_REGS_RCX) = ecx;
@@ -4250,7 +4250,7 @@ static int check_cr_write(struct x86_emulate_ctxt *ctxt)
 			eax = 0x80000008;
 			ecx = 0;
 			if (ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx,
-						 &edx, false))
+						 &edx, true))
 				maxphyaddr = eax & 0xff;
 			else
 				maxphyaddr = 36;
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 48c9390011d0..fbe5252be3d9 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2171,7 +2171,7 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	}
 	init_vmcb(svm);
 
-	kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, true);
+	kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, false);
 	kvm_rdx_write(vcpu, eax);
 
 	if (kvm_vcpu_apicv_active(vcpu) && !init_event)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1a4836ed1230..3e44fac4fb09 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6205,9 +6205,10 @@ static int emulator_intercept(struct x86_emulate_ctxt *ctxt,
 }
 
 static bool emulator_get_cpuid(struct x86_emulate_ctxt *ctxt,
-			u32 *eax, u32 *ebx, u32 *ecx, u32 *edx, bool check_limit)
+			      u32 *eax, u32 *ebx, u32 *ecx, u32 *edx,
+			      bool exact_only)
 {
-	return kvm_cpuid(emul_to_vcpu(ctxt), eax, ebx, ecx, edx, check_limit);
+	return kvm_cpuid(emul_to_vcpu(ctxt), eax, ebx, ecx, edx, exact_only);
 }
 
 static bool emulator_guest_has_long_mode(struct x86_emulate_ctxt *ctxt)
-- 
2.24.1

Re: [PATCH v2 2/7] KVM: x86: Add helpers to perform CPUID-based guest vendor check

[Xiaoyao Li]

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;
>   
>   	/*
> 

Re: [PATCH v2 0/7] KVM: x86: CPUID emulation and tracing fixes

[Paolo Bonzini]

On 05/03/20 02:34, Sean Christopherson wrote:
> 
> In theory, everything up to the refactoring is non-controversial, i.e. we
> can bikeshed the refactoring without delaying the bug fixes.

Even the refactoring itself is much less controversial.  I queued
everything, there's always time to unqueue.

Paolo

> v2:
>   - Use Jan's patch to fix the trace bug. [Everyone]
>   - Rework Hypervisor/Centaur handling so that only the Hypervisor
>     sub-ranges get the restrictive 0xffffff00 mask, and so that Centaur's
>     range only gets recognized when the guest vendor is Centaur. [Jim]
>   - Add the aforementioned bug fixes.
>   - Add a patch to do build time assertions on the vendor string, which
>     are hand coded u32s in the emulator (for direct comparison against
>     CPUID register output).
>   - Drop the patch to add CPUID.maxphyaddr emulator helper. [Paolo]
>   - Redo refactoring patches to land them after all the bug fixes
>     and to do the refactoring without any semantic changes in the
>     emulator.

Re: [PATCH v2 0/7] KVM: x86: CPUID emulation and tracing fixes

[Sean Christopherson]

On Thu, Mar 05, 2020 at 05:42:59PM +0100, Paolo Bonzini wrote:
> On 05/03/20 02:34, Sean Christopherson wrote:
> > 
> > In theory, everything up to the refactoring is non-controversial, i.e. we
> > can bikeshed the refactoring without delaying the bug fixes.
> 
> Even the refactoring itself is much less controversial.  I queued
> everything, there's always time to unqueue.

Looks like the build-time assertions don't play nice with older versions of
gcc :-(

config: x86_64-randconfig-s2-20200305 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/preempt.h:10,
                    from include/linux/hardirq.h:5,
                    from include/linux/kvm_host.h:7,
                    from arch/x86/kvm/emulate.c:21:
   arch/x86/kvm/emulate.c: In function 'em_cpuid':
>> include/linux/compiler.h:350:38: error: call to '__compiletime_assert_3957' declared with attribute error: BUILD_BUG_ON failed: X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx != *(u32 *)"Auth" || X86EMUL_CPUID_VENDOR_AuthenticAMD_edx != *(u32 *)"enti" || X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx != *(u32 *)"cAMD"
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

Re: [PATCH v2 1/7] KVM: x86: Trace the original requested CPUID function in kvm_cpuid()

[Jim Mattson]

On Wed, Mar 4, 2020 at 5:34 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Trace the requested CPUID function instead of the effective function,
> e.g. if the requested function is out-of-range and KVM is emulating an
> Intel CPU, as the intent of the tracepoint is to show if the output came
> from the actual leaf as opposed to the max basic leaf via redirection.
>
> Similarly, leave "found" as is, i.e. report that an entry was found if
> and only if the requested entry was found.
>
> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> [Sean: Drop "found" semantic change, reword changelong accordingly ]
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH v2 2/7] KVM: x86: Add helpers to perform CPUID-based guest vendor check

[Jim Mattson]

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>

Re: [PATCH v2 4/7] KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes

[Jim Mattson]

On Wed, Mar 4, 2020 at 5:34 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> Rework the masking in the out-of-range CPUID logic to handle the
> Hypervisor sub-classes, as well as the Centaur class if the guest
> virtual CPU vendor is Centaur.
>
> Masking against 0x80000000 only handles basic and extended leafs, which
> results in Hypervisor range checks being performed against the basic
> CPUID class, and Centuar range checks being performed against the
> Extended class.  E.g. if CPUID.0x40000000.EAX returns 0x4000000A and
> there is no entry for CPUID.0x40000006, then function 0x40000006 would
> be incorrectly reported as out of bounds.
>
> While there is no official definition of what constitutes a class, the
> convention established for Hypervisor classes effectively uses bits 31:8
> as the mask by virtue of checking for different bases in increments of
> 0x100, e.g. KVM advertises its CPUID functions starting at 0x40000100
> when HyperV features are advertised at the default base of 0x40000000.
>
> The bad range check doesn't cause functional problems for any known VMM
> because out-of-range semantics only come into play if the exact entry
> isn't found, and VMMs either support a very limited Hypervisor range,
> e.g. the official KVM range is 0x40000000-0x40000001 (effectively no
> room for undefined leafs) or explicitly defines gaps to be zero, e.g.
> Qemu explicitly creates zeroed entries up to the Cenatur and Hypervisor
> limits (the latter comes into play when providing HyperV features).
>
> The bad behavior can be visually confirmed by dumping CPUID output in
> the guest when running Qemu with a stable TSC, as Qemu extends the limit
> of range 0x40000000 to 0x40000010 to advertise VMware's cpuid_freq,
> without defining zeroed entries for 0x40000002 - 0x4000000f.
>
> Note, documentation of Centaur/VIA CPUs is hard to come by.  Designating
> 0xc0000000 - 0xcfffffff as the Centaur class is a best guess as to the
> behavior of a real Centaur/VIA CPU.

Don't forget Transmeta's CPUID range at 0x80860000 through 0x8086FFFF!

Re: [PATCH v2 4/7] KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes

[Sean Christopherson]

On Thu, Mar 05, 2020 at 10:43:51AM -0800, Jim Mattson wrote:
> On Wed, Mar 4, 2020 at 5:34 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > Rework the masking in the out-of-range CPUID logic to handle the
> > Hypervisor sub-classes, as well as the Centaur class if the guest
> > virtual CPU vendor is Centaur.
> >
> > Masking against 0x80000000 only handles basic and extended leafs, which
> > results in Hypervisor range checks being performed against the basic
> > CPUID class, and Centuar range checks being performed against the
> > Extended class.  E.g. if CPUID.0x40000000.EAX returns 0x4000000A and
> > there is no entry for CPUID.0x40000006, then function 0x40000006 would
> > be incorrectly reported as out of bounds.
> >
> > While there is no official definition of what constitutes a class, the
> > convention established for Hypervisor classes effectively uses bits 31:8
> > as the mask by virtue of checking for different bases in increments of
> > 0x100, e.g. KVM advertises its CPUID functions starting at 0x40000100
> > when HyperV features are advertised at the default base of 0x40000000.
> >
> > The bad range check doesn't cause functional problems for any known VMM
> > because out-of-range semantics only come into play if the exact entry
> > isn't found, and VMMs either support a very limited Hypervisor range,
> > e.g. the official KVM range is 0x40000000-0x40000001 (effectively no
> > room for undefined leafs) or explicitly defines gaps to be zero, e.g.
> > Qemu explicitly creates zeroed entries up to the Cenatur and Hypervisor
> > limits (the latter comes into play when providing HyperV features).
> >
> > The bad behavior can be visually confirmed by dumping CPUID output in
> > the guest when running Qemu with a stable TSC, as Qemu extends the limit
> > of range 0x40000000 to 0x40000010 to advertise VMware's cpuid_freq,
> > without defining zeroed entries for 0x40000002 - 0x4000000f.
> >
> > Note, documentation of Centaur/VIA CPUs is hard to come by.  Designating
> > 0xc0000000 - 0xcfffffff as the Centaur class is a best guess as to the
> > behavior of a real Centaur/VIA CPU.
> 
> Don't forget Transmeta's CPUID range at 0x80860000 through 0x8086FFFF!

Hmm, is it actually needed here?  KVM doesn't advertise support for that
range in KVM_GET_SUPPORTED_CPUID.  That's also why I limited the Centaur
range to vendor==CENTAUR, as KVM_GET_SUPPORTED_CPUID enumerates the
Centaur range if and only if the host CPU is Centaur.

Re: [PATCH v2 2/7] KVM: x86: Add helpers to perform CPUID-based guest vendor check

[Sean Christopherson]

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;
> >  	/*
> >
> 

Re: [PATCH v2 4/7] KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes

[Jim Mattson]

On Thu, Mar 5, 2020 at 11:25 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Thu, Mar 05, 2020 at 10:43:51AM -0800, Jim Mattson wrote:
> > On Wed, Mar 4, 2020 at 5:34 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> > > Rework the masking in the out-of-range CPUID logic to handle the
> > > Hypervisor sub-classes, as well as the Centaur class if the guest
> > > virtual CPU vendor is Centaur.
> > >
> > > Masking against 0x80000000 only handles basic and extended leafs, which
> > > results in Hypervisor range checks being performed against the basic
> > > CPUID class, and Centuar range checks being performed against the
> > > Extended class.  E.g. if CPUID.0x40000000.EAX returns 0x4000000A and
> > > there is no entry for CPUID.0x40000006, then function 0x40000006 would
> > > be incorrectly reported as out of bounds.
> > >
> > > While there is no official definition of what constitutes a class, the
> > > convention established for Hypervisor classes effectively uses bits 31:8
> > > as the mask by virtue of checking for different bases in increments of
> > > 0x100, e.g. KVM advertises its CPUID functions starting at 0x40000100
> > > when HyperV features are advertised at the default base of 0x40000000.
> > >
> > > The bad range check doesn't cause functional problems for any known VMM
> > > because out-of-range semantics only come into play if the exact entry
> > > isn't found, and VMMs either support a very limited Hypervisor range,
> > > e.g. the official KVM range is 0x40000000-0x40000001 (effectively no
> > > room for undefined leafs) or explicitly defines gaps to be zero, e.g.
> > > Qemu explicitly creates zeroed entries up to the Cenatur and Hypervisor
> > > limits (the latter comes into play when providing HyperV features).
> > >
> > > The bad behavior can be visually confirmed by dumping CPUID output in
> > > the guest when running Qemu with a stable TSC, as Qemu extends the limit
> > > of range 0x40000000 to 0x40000010 to advertise VMware's cpuid_freq,
> > > without defining zeroed entries for 0x40000002 - 0x4000000f.
> > >
> > > Note, documentation of Centaur/VIA CPUs is hard to come by.  Designating
> > > 0xc0000000 - 0xcfffffff as the Centaur class is a best guess as to the
> > > behavior of a real Centaur/VIA CPU.
> >
> > Don't forget Transmeta's CPUID range at 0x80860000 through 0x8086FFFF!
>
> Hmm, is it actually needed here?  KVM doesn't advertise support for that
> range in KVM_GET_SUPPORTED_CPUID.  That's also why I limited the Centaur
> range to vendor==CENTAUR, as KVM_GET_SUPPORTED_CPUID enumerates the
> Centaur range if and only if the host CPU is Centaur.

Ah. So cross-vendor CPUID specifications are not supported?

Re: [PATCH v2 4/7] KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes

[Sean Christopherson]

On Thu, Mar 05, 2020 at 01:10:57PM -0800, Jim Mattson wrote:
> On Thu, Mar 5, 2020 at 11:25 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Thu, Mar 05, 2020 at 10:43:51AM -0800, Jim Mattson wrote:
> > > On Wed, Mar 4, 2020 at 5:34 PM Sean Christopherson
> > > <sean.j.christopherson@intel.com> wrote:
> > > >
> > > > Rework the masking in the out-of-range CPUID logic to handle the
> > > > Hypervisor sub-classes, as well as the Centaur class if the guest
> > > > virtual CPU vendor is Centaur.
> > > >
> > > > Masking against 0x80000000 only handles basic and extended leafs, which
> > > > results in Hypervisor range checks being performed against the basic
> > > > CPUID class, and Centuar range checks being performed against the
> > > > Extended class.  E.g. if CPUID.0x40000000.EAX returns 0x4000000A and
> > > > there is no entry for CPUID.0x40000006, then function 0x40000006 would
> > > > be incorrectly reported as out of bounds.
> > > >
> > > > While there is no official definition of what constitutes a class, the
> > > > convention established for Hypervisor classes effectively uses bits 31:8
> > > > as the mask by virtue of checking for different bases in increments of
> > > > 0x100, e.g. KVM advertises its CPUID functions starting at 0x40000100
> > > > when HyperV features are advertised at the default base of 0x40000000.
> > > >
> > > > The bad range check doesn't cause functional problems for any known VMM
> > > > because out-of-range semantics only come into play if the exact entry
> > > > isn't found, and VMMs either support a very limited Hypervisor range,
> > > > e.g. the official KVM range is 0x40000000-0x40000001 (effectively no
> > > > room for undefined leafs) or explicitly defines gaps to be zero, e.g.
> > > > Qemu explicitly creates zeroed entries up to the Cenatur and Hypervisor
> > > > limits (the latter comes into play when providing HyperV features).
> > > >
> > > > The bad behavior can be visually confirmed by dumping CPUID output in
> > > > the guest when running Qemu with a stable TSC, as Qemu extends the limit
> > > > of range 0x40000000 to 0x40000010 to advertise VMware's cpuid_freq,
> > > > without defining zeroed entries for 0x40000002 - 0x4000000f.
> > > >
> > > > Note, documentation of Centaur/VIA CPUs is hard to come by.  Designating
> > > > 0xc0000000 - 0xcfffffff as the Centaur class is a best guess as to the
> > > > behavior of a real Centaur/VIA CPU.
> > >
> > > Don't forget Transmeta's CPUID range at 0x80860000 through 0x8086FFFF!
> >
> > Hmm, is it actually needed here?  KVM doesn't advertise support for that
> > range in KVM_GET_SUPPORTED_CPUID.

> > That's also why I limited the Centaur
> > range to vendor==CENTAUR, as KVM_GET_SUPPORTED_CPUID enumerates the
> > Centaur range if and only if the host CPU is Centaur.

Actually, I take this part of that comment back.  I limited this to
vendor==CENTAUR so that it wouldn't conflict with an actual Intel CPU if
Intel adds a CPUID leaf in the 0xc0000000 range.

> Ah. So cross-vendor CPUID specifications are not supported?

Cross-vendor CPUID is sort of allowed?  E.g. this plays nice with creating
a Centaur CPU on an Intel platform.  My interpretation of GET_SUPPORTED...
is that KVM won't prevent enumerating what you want in CPUID, but it only
promises to correctly support select leafs.

Re: [PATCH v2 0/7] KVM: x86: CPUID emulation and tracing fixes

[Paolo Bonzini]

On 05/03/20 18:12, Sean Christopherson wrote:
>>> In theory, everything up to the refactoring is non-controversial, i.e. we
>>> can bikeshed the refactoring without delaying the bug fixes.
>> Even the refactoring itself is much less controversial.  I queued
>> everything, there's always time to unqueue.
> Looks like the build-time assertions don't play nice with older versions of
> gcc :-(

Yes, I was quite surprised that they worked.  I suppose you could write
a macro that checks against 'G', 'e', 'n', 'u', 'i', 'n', 'e', 'I', 'n',
't', 'e', 'l'...

Paolo

Re: [PATCH v2 4/7] KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes

[Paolo Bonzini]

On 05/03/20 22:51, Sean Christopherson wrote:
>> Ah. So cross-vendor CPUID specifications are not supported?
> Cross-vendor CPUID is sort of allowed?  E.g. this plays nice with creating
> a Centaur CPU on an Intel platform.  My interpretation of GET_SUPPORTED...
> is that KVM won't prevent enumerating what you want in CPUID, but it only
> promises to correctly support select leafs.

But in practice does this change anything?  If the vendor is not Centaur 
it's unlikely that there is a 0xc0000000 leaf.  The 0x80000000 bound is
certainly not going to be at 0xc0000000 or beyond, and likewise to 0xc0000000
bound is not going to be at 0xd0000000 or beyond.  So I'm not sure if
anything is lost from this simplification:

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ed5e0bda672c..f43a8875c126 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -963,8 +963,7 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
 
 	if (function >= 0x40000000 && function <= 0x4fffffff)
 		class = kvm_find_cpuid_entry(vcpu, function & 0xffffff00, 0);
-	else if (function >= 0xc0000000 && function <= 0xcfffffff &&
-		 is_guest_vendor_centaur(basic->ebx, basic->ecx, basic->edx))
+	else if (function >= 0xc0000000)
 		class = kvm_find_cpuid_entry(vcpu, 0xc0000000, 0);
 	else
 		class = kvm_find_cpuid_entry(vcpu, function & 0x80000000, 0);
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 12ddfa493bae..3cb50eda606d 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -424,13 +424,6 @@ static inline bool is_guest_vendor_hygon(u32 ebx, u32 ecx, u32 edx)
 	       edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx;
 }
 
-static inline bool is_guest_vendor_centaur(u32 ebx, u32 ecx, u32 edx)
-{
-	return ebx == X86EMUL_CPUID_VENDOR_CentaurHauls_ebx &&
-	       ecx == X86EMUL_CPUID_VENDOR_CentaurHauls_ecx &&
-	       edx == X86EMUL_CPUID_VENDOR_CentaurHauls_edx;
-}
-
 enum x86_intercept_stage {
 	X86_ICTP_NONE = 0,   /* Allow zero-init to not match anything */
 	X86_ICPT_PRE_EXCEPT,

Re: [PATCH v2 4/7] KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes

[Sean Christopherson]

On Fri, Mar 06, 2020 at 10:03:37AM +0100, Paolo Bonzini wrote:
> On 05/03/20 22:51, Sean Christopherson wrote:
> >> Ah. So cross-vendor CPUID specifications are not supported?
> > Cross-vendor CPUID is sort of allowed?  E.g. this plays nice with creating
> > a Centaur CPU on an Intel platform.  My interpretation of GET_SUPPORTED...
> > is that KVM won't prevent enumerating what you want in CPUID, but it only
> > promises to correctly support select leafs.
> 
> But in practice does this change anything?  If the vendor is not Centaur 
> it's unlikely that there is a 0xc0000000 leaf.  The 0x80000000 bound is
> certainly not going to be at 0xc0000000 or beyond, and likewise to 0xc0000000
> bound is not going to be at 0xd0000000 or beyond.  So I'm not sure if
> anything is lost from this simplification:

Probably not?  But in the unlikely scenario that Intel wants to add a CPUID
leaf above 0xc0000000, I don't want to have to explain that it might cause
problems for KVM guests because I added code to emulate (alleged) Centaur
behavior for virtual Intel CPUs.

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index ed5e0bda672c..f43a8875c126 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -963,8 +963,7 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
>  
>  	if (function >= 0x40000000 && function <= 0x4fffffff)
>  		class = kvm_find_cpuid_entry(vcpu, function & 0xffffff00, 0);
> -	else if (function >= 0xc0000000 && function <= 0xcfffffff &&
> -		 is_guest_vendor_centaur(basic->ebx, basic->ecx, basic->edx))
> +	else if (function >= 0xc0000000)
>  		class = kvm_find_cpuid_entry(vcpu, 0xc0000000, 0);
>  	else
>  		class = kvm_find_cpuid_entry(vcpu, function & 0x80000000, 0);
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index 12ddfa493bae..3cb50eda606d 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -424,13 +424,6 @@ static inline bool is_guest_vendor_hygon(u32 ebx, u32 ecx, u32 edx)
>  	       edx == X86EMUL_CPUID_VENDOR_HygonGenuine_edx;
>  }
>  
> -static inline bool is_guest_vendor_centaur(u32 ebx, u32 ecx, u32 edx)
> -{
> -	return ebx == X86EMUL_CPUID_VENDOR_CentaurHauls_ebx &&
> -	       ecx == X86EMUL_CPUID_VENDOR_CentaurHauls_ecx &&
> -	       edx == X86EMUL_CPUID_VENDOR_CentaurHauls_edx;
> -}
> -
>  enum x86_intercept_stage {
>  	X86_ICTP_NONE = 0,   /* Allow zero-init to not match anything */
>  	X86_ICPT_PRE_EXCEPT,
> 

Re: [PATCH v2 4/7] KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes

[Jim Mattson]

On Tue, Mar 10, 2020 at 10:10 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, Mar 06, 2020 at 10:03:37AM +0100, Paolo Bonzini wrote:
> > On 05/03/20 22:51, Sean Christopherson wrote:
> > >> Ah. So cross-vendor CPUID specifications are not supported?
> > > Cross-vendor CPUID is sort of allowed?  E.g. this plays nice with creating
> > > a Centaur CPU on an Intel platform.  My interpretation of GET_SUPPORTED...
> > > is that KVM won't prevent enumerating what you want in CPUID, but it only
> > > promises to correctly support select leafs.
> >
> > But in practice does this change anything?  If the vendor is not Centaur
> > it's unlikely that there is a 0xc0000000 leaf.  The 0x80000000 bound is
> > certainly not going to be at 0xc0000000 or beyond, and likewise to 0xc0000000
> > bound is not going to be at 0xd0000000 or beyond.  So I'm not sure if
> > anything is lost from this simplification:
>
> Probably not?  But in the unlikely scenario that Intel wants to add a CPUID
> leaf above 0xc0000000, I don't want to have to explain that it might cause
> problems for KVM guests because I added code to emulate (alleged) Centaur
> behavior for virtual Intel CPUs.

And there is some precedent for that, with the 0x20000000 leaves.