[PATCH v6 0/9] x86/cpu: KVM: Clean up PAT and VMX macros

[PATCH v6 0/9] x86/cpu: KVM: Clean up PAT and VMX macros

[Sean Christopherson]

This is v6 of Xin's series to clean up a variety of KVM's VMX macros, but
this version obviously goes beyond cleaning up the VMX macros.

While reviewiing the VMX changes, we realized that (a) KVM was defining
VMX specific macros for the architectural memtypes, (b) the PAT and MTRR
code define similar, yet different macros, and (c) that the PAT code not
only has macros for the types (well, enums), it also has macros for
encoding the entire PAT MSR.

The first half of the series moves as much of the memtype stuff to
common code as is reasonably possible.

The second half is Xin's cleanup (though split into more patches than
what were posted in v5).

Based on:

    https://github.com/kvm-x86/linux next

v6:
 - Add all the PAT/memtype patches.
 - Split the VMX changes into more appropriately sized chunks.
 - Multiple minor modifications to make the macro mess more maintainable
   (and yes, I edited that sentence to use "modifications" specifically
   for alliteration purposes).

v5:
* https://lore.kernel.org/all/20240206182032.1596-1-xin3.li@intel.com
* Do not split VMX_BASIC bit definitions across multiple files (Kai
  Huang).
* Put some words to the changelog to justify changes around memory
  type macros (Kai Huang).
* Remove a leftover ';' (Kai Huang).

v4:
* Remove vmx_basic_vmcs_basic_cap() (Kai Huang).
* Add 2 macros VMX_BASIC_VMCS12_SIZE and VMX_BASIC_MEM_TYPE_WB to
  avoid keeping 2 their bit shift macros (Kai Huang).

v3:
* Simply save the full/raw value of MSR_IA32_VMX_BASIC in the global
  vmcs_config, and then use the helpers to extract info from it as
  needed (Sean Christopherson).
* Move all VMX_MISC related changes to the second patch (Kai Huang).
* Commonize memory type definitions used in the VMX files, as memory
  types are architectural.

v2:
* Don't add field shift macros unless it's really needed, extra layer
  of indirect makes it harder to read (Sean Christopherson).
* Add a static_assert() to ensure that VMX_BASIC_FEATURES_MASK doesn't
  overlap with VMX_BASIC_RESERVED_BITS (Sean Christopherson).
* read MSR_IA32_VMX_BASIC into an u64 rather than 2 u32 (Sean
  Christopherson).
* Add 2 new functions for extracting fields from VMX basic (Sean
  Christopherson).
* Drop the tools header update (Sean Christopherson).
* Move VMX basic field macros to arch/x86/include/asm/vmx.h.

Sean Christopherson (5):
  x86/cpu: KVM: Add common defines for architectural memory types (PAT,
    MTRRs, etc.)
  x86/cpu: KVM: Move macro to encode PAT value to common header
  KVM: x86: Stuff vCPU's PAT with default value at RESET, not creation
  KVM: VMX: Track CPU's MSR_IA32_VMX_BASIC as a single 64-bit value
  KVM VMX: Move MSR_IA32_VMX_MISC bit defines to asm/vmx.h

Xin Li (4):
  KVM: VMX: Move MSR_IA32_VMX_BASIC bit defines to asm/vmx.h
  KVM: nVMX: Use macros and #defines in vmx_restore_vmx_basic()
  KVM: VMX: Open code VMX preemption timer rate mask in its accessor
  KVM: nVMX: Use macros and #defines in vmx_restore_vmx_misc()

 arch/x86/include/asm/msr-index.h | 34 +++++++++++--------
 arch/x86/include/asm/vmx.h       | 37 +++++++++++++++------
 arch/x86/kernel/cpu/mtrr/mtrr.c  |  6 ++++
 arch/x86/kvm/vmx/capabilities.h  | 10 +++---
 arch/x86/kvm/vmx/nested.c        | 56 +++++++++++++++++++++++---------
 arch/x86/kvm/vmx/nested.h        |  2 +-
 arch/x86/kvm/vmx/vmx.c           | 30 ++++++++---------
 arch/x86/kvm/x86.c               |  4 +--
 arch/x86/kvm/x86.h               |  3 +-
 arch/x86/mm/pat/memtype.c        | 35 ++++++--------------
 10 files changed, 127 insertions(+), 90 deletions(-)


base-commit: 964d0c614c7f71917305a5afdca9178fe8231434
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 1/9] x86/cpu: KVM: Add common defines for architectural memory types (PAT, MTRRs, etc.)

[Sean Christopherson]

Add defines for the architectural memory types that can be shoved into
various MSRs and registers, e.g. MTRRs, PAT, VMX capabilities MSRs, EPTPs,
etc.  While most MSRs/registers support only a subset of all memory types,
the values themselves are architectural and identical across all users.

Leave the goofy MTRR_TYPE_* definitions as-is since they are in a uapi
header, but add compile-time assertions to connect the dots (and sanity
check that the msr-index.h values didn't get fat-fingered).

Keep the VMX_EPTP_MT_* defines so that it's slightly more obvious that the
EPTP holds a single memory type in 3 of its 64 bits; those bits just
happen to be 2:0, i.e. don't need to be shifted.

Opportunistically use X86_MEMTYPE_WB instead of an open coded '6' in
setup_vmcs_config().

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/msr-index.h | 15 ++++++++++++++-
 arch/x86/include/asm/vmx.h       |  5 +++--
 arch/x86/kernel/cpu/mtrr/mtrr.c  |  6 ++++++
 arch/x86/kvm/vmx/nested.c        |  2 +-
 arch/x86/kvm/vmx/vmx.c           |  2 +-
 arch/x86/mm/pat/memtype.c        | 33 ++++++++++++--------------------
 6 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index f1bd7b91b3c6..29f0ea78e41c 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -36,6 +36,20 @@
 #define EFER_FFXSR		(1<<_EFER_FFXSR)
 #define EFER_AUTOIBRS		(1<<_EFER_AUTOIBRS)
 
+/*
+ * Architectural memory types that are common to MTRRs, PAT, VMX MSRs, etc.
+ * Most MSRs support/allow only a subset of memory types, but the values
+ * themselves are common across all relevant MSRs.
+ */
+#define X86_MEMTYPE_UC		0ull	/* Uncacheable, a.k.a. Strong Uncacheable */
+#define X86_MEMTYPE_WC		1ull	/* Write Combining */
+/* RESERVED			2 */
+/* RESERVED			3 */
+#define X86_MEMTYPE_WT		4ull	/* Write Through */
+#define X86_MEMTYPE_WP		5ull	/* Write Protected */
+#define X86_MEMTYPE_WB		6ull	/* Write Back */
+#define X86_MEMTYPE_UC_MINUS	7ull	/* Weak Uncacheabled (PAT only) */
+
 /* Intel MSRs. Some also available on other CPUs */
 
 #define MSR_TEST_CTRL				0x00000033
@@ -1108,7 +1122,6 @@
 #define VMX_BASIC_64		0x0001000000000000LLU
 #define VMX_BASIC_MEM_TYPE_SHIFT	50
 #define VMX_BASIC_MEM_TYPE_MASK	0x003c000000000000LLU
-#define VMX_BASIC_MEM_TYPE_WB	6LLU
 #define VMX_BASIC_INOUT		0x0040000000000000LLU
 
 /* Resctrl MSRs: */
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 0e73616b82f3..4fdc76263066 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -504,9 +504,10 @@ enum vmcs_field {
 #define VMX_EPTP_PWL_4				0x18ull
 #define VMX_EPTP_PWL_5				0x20ull
 #define VMX_EPTP_AD_ENABLE_BIT			(1ull << 6)
+/* The EPTP memtype is encoded in bits 2:0, i.e. doesn't need to be shifted. */
 #define VMX_EPTP_MT_MASK			0x7ull
-#define VMX_EPTP_MT_WB				0x6ull
-#define VMX_EPTP_MT_UC				0x0ull
+#define VMX_EPTP_MT_WB				X86_MEMTYPE_WB
+#define VMX_EPTP_MT_UC				X86_MEMTYPE_UC
 #define VMX_EPT_READABLE_MASK			0x1ull
 #define VMX_EPT_WRITABLE_MASK			0x2ull
 #define VMX_EPT_EXECUTABLE_MASK			0x4ull
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 767bf1c71aad..125e36010b82 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -55,6 +55,12 @@
 
 #include "mtrr.h"
 
+static_assert(X86_MEMTYPE_UC == MTRR_TYPE_UNCACHABLE);
+static_assert(X86_MEMTYPE_WC == MTRR_TYPE_WRCOMB);
+static_assert(X86_MEMTYPE_WT == MTRR_TYPE_WRTHROUGH);
+static_assert(X86_MEMTYPE_WP == MTRR_TYPE_WRPROT);
+static_assert(X86_MEMTYPE_WB == MTRR_TYPE_WRBACK);
+
 /* arch_phys_wc_add returns an MTRR register index plus this offset. */
 #define MTRR_TO_PHYS_WC_OFFSET 1000
 
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index d05ddf751491..82a35aba7d2b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -7006,7 +7006,7 @@ static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
 		VMCS12_REVISION |
 		VMX_BASIC_TRUE_CTLS |
 		((u64)VMCS12_SIZE << VMX_BASIC_VMCS_SIZE_SHIFT) |
-		(VMX_BASIC_MEM_TYPE_WB << VMX_BASIC_MEM_TYPE_SHIFT);
+		(X86_MEMTYPE_WB << VMX_BASIC_MEM_TYPE_SHIFT);
 
 	if (cpu_has_vmx_basic_inout())
 		msrs->basic |= VMX_BASIC_INOUT;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 7a74388f9ecf..71cc6e3b3221 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2692,7 +2692,7 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 #endif
 
 	/* Require Write-Back (WB) memory type for VMCS accesses. */
-	if (((vmx_msr_high >> 18) & 15) != 6)
+	if (((vmx_msr_high >> 18) & 15) != X86_MEMTYPE_WB)
 		return -EIO;
 
 	rdmsrl(MSR_IA32_VMX_MISC, misc_msr);
diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index 0904d7e8e126..3e0ba044925f 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -175,15 +175,6 @@ static inline void set_page_memtype(struct page *pg,
 }
 #endif
 
-enum {
-	PAT_UC = 0,		/* uncached */
-	PAT_WC = 1,		/* Write combining */
-	PAT_WT = 4,		/* Write Through */
-	PAT_WP = 5,		/* Write Protected */
-	PAT_WB = 6,		/* Write Back (default) */
-	PAT_UC_MINUS = 7,	/* UC, but can be overridden by MTRR */
-};
-
 #define CM(c) (_PAGE_CACHE_MODE_ ## c)
 
 static enum page_cache_mode __init pat_get_cache_mode(unsigned int pat_val,
@@ -193,13 +184,13 @@ static enum page_cache_mode __init pat_get_cache_mode(unsigned int pat_val,
 	char *cache_mode;
 
 	switch (pat_val) {
-	case PAT_UC:       cache = CM(UC);       cache_mode = "UC  "; break;
-	case PAT_WC:       cache = CM(WC);       cache_mode = "WC  "; break;
-	case PAT_WT:       cache = CM(WT);       cache_mode = "WT  "; break;
-	case PAT_WP:       cache = CM(WP);       cache_mode = "WP  "; break;
-	case PAT_WB:       cache = CM(WB);       cache_mode = "WB  "; break;
-	case PAT_UC_MINUS: cache = CM(UC_MINUS); cache_mode = "UC- "; break;
-	default:           cache = CM(WB);       cache_mode = "WB  "; break;
+	case X86_MEMTYPE_UC:       cache = CM(UC);       cache_mode = "UC  "; break;
+	case X86_MEMTYPE_WC:       cache = CM(WC);       cache_mode = "WC  "; break;
+	case X86_MEMTYPE_WT:       cache = CM(WT);       cache_mode = "WT  "; break;
+	case X86_MEMTYPE_WP:       cache = CM(WP);       cache_mode = "WP  "; break;
+	case X86_MEMTYPE_WB:       cache = CM(WB);       cache_mode = "WB  "; break;
+	case X86_MEMTYPE_UC_MINUS: cache = CM(UC_MINUS); cache_mode = "UC- "; break;
+	default:                   cache = CM(WB);       cache_mode = "WB  "; break;
 	}
 
 	memcpy(msg, cache_mode, 4);
@@ -254,11 +245,11 @@ void pat_cpu_init(void)
 void __init pat_bp_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
-#define PAT(p0, p1, p2, p3, p4, p5, p6, p7)			\
-	(((u64)PAT_ ## p0) | ((u64)PAT_ ## p1 << 8) |		\
-	((u64)PAT_ ## p2 << 16) | ((u64)PAT_ ## p3 << 24) |	\
-	((u64)PAT_ ## p4 << 32) | ((u64)PAT_ ## p5 << 40) |	\
-	((u64)PAT_ ## p6 << 48) | ((u64)PAT_ ## p7 << 56))
+#define PAT(p0, p1, p2, p3, p4, p5, p6, p7)				\
+	((X86_MEMTYPE_ ## p0)      | (X86_MEMTYPE_ ## p1 << 8)  |	\
+	(X86_MEMTYPE_ ## p2 << 16) | (X86_MEMTYPE_ ## p3 << 24) |	\
+	(X86_MEMTYPE_ ## p4 << 32) | (X86_MEMTYPE_ ## p5 << 40) |	\
+	(X86_MEMTYPE_ ## p6 << 48) | (X86_MEMTYPE_ ## p7 << 56))
 
 
 	if (!IS_ENABLED(CONFIG_X86_PAT))
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 2/9] x86/cpu: KVM: Move macro to encode PAT value to common header

[Sean Christopherson]

Move pat/memtype.c's PAT() macro to msr-index.h as PAT_VALUE(), and use it
in KVM to define the default (Power-On / RESET) PAT value instead of open
coding an inscrutable magic number.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/msr-index.h |  6 ++++++
 arch/x86/kvm/x86.h               |  3 ++-
 arch/x86/mm/pat/memtype.c        | 12 +++---------
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 29f0ea78e41c..af71f8bb76ae 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -352,6 +352,12 @@
 
 #define MSR_IA32_CR_PAT			0x00000277
 
+#define PAT_VALUE(p0, p1, p2, p3, p4, p5, p6, p7)			\
+	((X86_MEMTYPE_ ## p0)      | (X86_MEMTYPE_ ## p1 << 8)  |	\
+	(X86_MEMTYPE_ ## p2 << 16) | (X86_MEMTYPE_ ## p3 << 24) |	\
+	(X86_MEMTYPE_ ## p4 << 32) | (X86_MEMTYPE_ ## p5 << 40) |	\
+	(X86_MEMTYPE_ ## p6 << 48) | (X86_MEMTYPE_ ## p7 << 56))
+
 #define MSR_IA32_DEBUGCTLMSR		0x000001d9
 #define MSR_IA32_LASTBRANCHFROMIP	0x000001db
 #define MSR_IA32_LASTBRANCHTOIP		0x000001dc
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index a8b71803777b..753403639e72 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -87,7 +87,8 @@ static inline unsigned int __shrink_ple_window(unsigned int val,
 	return max(val, min);
 }
 
-#define MSR_IA32_CR_PAT_DEFAULT  0x0007040600070406ULL
+#define MSR_IA32_CR_PAT_DEFAULT	\
+	PAT_VALUE(WB, WT, UC_MINUS, UC, WB, WT, UC_MINUS, UC)
 
 void kvm_service_local_tlb_flush_requests(struct kvm_vcpu *vcpu);
 int kvm_check_nested_events(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index 3e0ba044925f..f2dedddfbaf2 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -245,12 +245,6 @@ void pat_cpu_init(void)
 void __init pat_bp_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
-#define PAT(p0, p1, p2, p3, p4, p5, p6, p7)				\
-	((X86_MEMTYPE_ ## p0)      | (X86_MEMTYPE_ ## p1 << 8)  |	\
-	(X86_MEMTYPE_ ## p2 << 16) | (X86_MEMTYPE_ ## p3 << 24) |	\
-	(X86_MEMTYPE_ ## p4 << 32) | (X86_MEMTYPE_ ## p5 << 40) |	\
-	(X86_MEMTYPE_ ## p6 << 48) | (X86_MEMTYPE_ ## p7 << 56))
-
 
 	if (!IS_ENABLED(CONFIG_X86_PAT))
 		pr_info_once("x86/PAT: PAT support disabled because CONFIG_X86_PAT is disabled in the kernel.\n");
@@ -281,7 +275,7 @@ void __init pat_bp_init(void)
 		 * NOTE: When WC or WP is used, it is redirected to UC- per
 		 * the default setup in __cachemode2pte_tbl[].
 		 */
-		pat_msr_val = PAT(WB, WT, UC_MINUS, UC, WB, WT, UC_MINUS, UC);
+		pat_msr_val = PAT_VALUE(WB, WT, UC_MINUS, UC, WB, WT, UC_MINUS, UC);
 	}
 
 	/*
@@ -321,7 +315,7 @@ void __init pat_bp_init(void)
 		 * NOTE: When WT or WP is used, it is redirected to UC- per
 		 * the default setup in __cachemode2pte_tbl[].
 		 */
-		pat_msr_val = PAT(WB, WC, UC_MINUS, UC, WB, WC, UC_MINUS, UC);
+		pat_msr_val = PAT_VALUE(WB, WC, UC_MINUS, UC, WB, WC, UC_MINUS, UC);
 	} else {
 		/*
 		 * Full PAT support.  We put WT in slot 7 to improve
@@ -349,7 +343,7 @@ void __init pat_bp_init(void)
 		 * The reserved slots are unused, but mapped to their
 		 * corresponding types in the presence of PAT errata.
 		 */
-		pat_msr_val = PAT(WB, WC, UC_MINUS, UC, WB, WP, UC_MINUS, WT);
+		pat_msr_val = PAT_VALUE(WB, WC, UC_MINUS, UC, WB, WP, UC_MINUS, WT);
 	}
 
 	memory_caching_control |= CACHE_PAT;
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 3/9] KVM: x86: Stuff vCPU's PAT with default value at RESET, not creation

[Sean Christopherson]

Move the stuffing of the vCPU's PAT to the architectural "default" value
from kvm_arch_vcpu_create() to kvm_vcpu_reset(), guarded by !init_event,
to better capture that the default value is the value "Following Power-up
or Reset".  E.g. setting PAT only during creation would break if KVM were
to expose a RESET ioctl() to userspace (which is unlikely, but that's not
a good reason to have unintuitive code).

No functional change.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 66c4381460dc..eac97b1b8379 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12134,8 +12134,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
 	vcpu->arch.reserved_gpa_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu);
 
-	vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT;
-
 	kvm_async_pf_hash_reset(vcpu);
 
 	vcpu->arch.perf_capabilities = kvm_caps.supported_perf_cap;
@@ -12302,6 +12300,8 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	if (!init_event) {
 		vcpu->arch.smbase = 0x30000;
 
+		vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT;
+
 		vcpu->arch.msr_misc_features_enables = 0;
 		vcpu->arch.ia32_misc_enable_msr = MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL |
 						  MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 4/9] KVM: VMX: Move MSR_IA32_VMX_BASIC bit defines to asm/vmx.h

[Sean Christopherson]

From: Xin Li <xin3.li@intel.com>

Move the bit defines for MSR_IA32_VMX_BASIC from msr-index.h to vmx.h so
that they are colocated with other VMX MSR bit defines, and with the
helpers that extract specific information from an MSR_IA32_VMX_BASIC value.

Opportunistically use BIT_ULL() instead of open coding hex values.

Opportunistically rename VMX_BASIC_64 to VMX_BASIC_32BIT_PHYS_ADDR_ONLY,
as "VMX_BASIC_64" is widly misleading.  The flag enumerates that addresses
are limited to 32 bits, not that 64-bit addresses are allowed.

Cc: Shan Kang <shan.kang@intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
[sean: split to separate patch, write changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/msr-index.h | 8 --------
 arch/x86/include/asm/vmx.h       | 7 +++++++
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index af71f8bb76ae..5ca81ad509b5 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1122,14 +1122,6 @@
 #define MSR_IA32_VMX_VMFUNC             0x00000491
 #define MSR_IA32_VMX_PROCBASED_CTLS3	0x00000492
 
-/* VMX_BASIC bits and bitmasks */
-#define VMX_BASIC_VMCS_SIZE_SHIFT	32
-#define VMX_BASIC_TRUE_CTLS		(1ULL << 55)
-#define VMX_BASIC_64		0x0001000000000000LLU
-#define VMX_BASIC_MEM_TYPE_SHIFT	50
-#define VMX_BASIC_MEM_TYPE_MASK	0x003c000000000000LLU
-#define VMX_BASIC_INOUT		0x0040000000000000LLU
-
 /* Resctrl MSRs: */
 /* - Intel: */
 #define MSR_IA32_L3_QOS_CFG		0xc81
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 4fdc76263066..c3a97dca4a33 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -133,6 +133,13 @@
 #define VMX_VMFUNC_EPTP_SWITCHING               VMFUNC_CONTROL_BIT(EPTP_SWITCHING)
 #define VMFUNC_EPTP_ENTRIES  512
 
+#define VMX_BASIC_VMCS_SIZE_SHIFT		32
+#define VMX_BASIC_32BIT_PHYS_ADDR_ONLY		BIT_ULL(48)
+#define VMX_BASIC_DUAL_MONITOR_TREATMENT	BIT_ULL(49)
+#define VMX_BASIC_MEM_TYPE_SHIFT		50
+#define VMX_BASIC_INOUT				BIT_ULL(54)
+#define VMX_BASIC_TRUE_CTLS			BIT_ULL(55)
+
 static inline u32 vmx_basic_vmcs_revision_id(u64 vmx_basic)
 {
 	return vmx_basic & GENMASK_ULL(30, 0);
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 5/9] KVM: VMX: Track CPU's MSR_IA32_VMX_BASIC as a single 64-bit value

[Sean Christopherson]

Track the "basic" capabilities VMX MSR as a single u64 in vmcs_config
instead of splitting it across three fields, that obviously don't combine
into a single 64-bit value, so that KVM can use the macros that define MSR
bits using their absolute position.  Replace all open coded shifts and
masks, many of which are relative to the "high" half, with the appropriate
macro.

Opportunistically use VMX_BASIC_32BIT_PHYS_ADDR_ONLY instead of an open
coded equivalent, and clean up the related comment to not reference a
specific SDM section (to the surprise of no one, the comment is stale).

No functional change intended (though obviously the code generation will
be quite different).

Cc: Shan Kang <shan.kang@intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
[sean: split to separate patch, write changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/vmx.h      |  5 +++++
 arch/x86/kvm/vmx/capabilities.h |  6 ++----
 arch/x86/kvm/vmx/vmx.c          | 28 ++++++++++++++--------------
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index c3a97dca4a33..ce6d166fc3c5 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -150,6 +150,11 @@ static inline u32 vmx_basic_vmcs_size(u64 vmx_basic)
 	return (vmx_basic & GENMASK_ULL(44, 32)) >> 32;
 }
 
+static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
+{
+	return (vmx_basic & GENMASK_ULL(53, 50)) >> 50;
+}
+
 static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
 {
 	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index 41a4533f9989..86ce8bb96bed 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -54,9 +54,7 @@ struct nested_vmx_msrs {
 };
 
 struct vmcs_config {
-	int size;
-	u32 basic_cap;
-	u32 revision_id;
+	u64 basic;
 	u32 pin_based_exec_ctrl;
 	u32 cpu_based_exec_ctrl;
 	u32 cpu_based_2nd_exec_ctrl;
@@ -76,7 +74,7 @@ extern struct vmx_capability vmx_capability __ro_after_init;
 
 static inline bool cpu_has_vmx_basic_inout(void)
 {
-	return	(((u64)vmcs_config.basic_cap << 32) & VMX_BASIC_INOUT);
+	return	vmcs_config.basic & VMX_BASIC_INOUT;
 }
 
 static inline bool cpu_has_virtual_nmis(void)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 71cc6e3b3221..e312c48f542f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2554,13 +2554,13 @@ static u64 adjust_vmx_controls64(u64 ctl_opt, u32 msr)
 static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 			     struct vmx_capability *vmx_cap)
 {
-	u32 vmx_msr_low, vmx_msr_high;
 	u32 _pin_based_exec_control = 0;
 	u32 _cpu_based_exec_control = 0;
 	u32 _cpu_based_2nd_exec_control = 0;
 	u64 _cpu_based_3rd_exec_control = 0;
 	u32 _vmexit_control = 0;
 	u32 _vmentry_control = 0;
+	u64 basic_msr;
 	u64 misc_msr;
 	int i;
 
@@ -2679,29 +2679,29 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 		_vmexit_control &= ~x_ctrl;
 	}
 
-	rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
+	rdmsrl(MSR_IA32_VMX_BASIC, basic_msr);
 
 	/* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
-	if ((vmx_msr_high & 0x1fff) > PAGE_SIZE)
+	if (vmx_basic_vmcs_size(basic_msr) > PAGE_SIZE)
 		return -EIO;
 
 #ifdef CONFIG_X86_64
-	/* IA-32 SDM Vol 3B: 64-bit CPUs always have VMX_BASIC_MSR[48]==0. */
-	if (vmx_msr_high & (1u<<16))
+	/*
+	 * KVM expects to be able to shove all legal physical addresses into
+	 * VMCS fields for 64-bit kernels, and per the SDM, "This bit is always
+	 * 0 for processors that support Intel 64 architecture".
+	 */
+	if (basic_msr & VMX_BASIC_32BIT_PHYS_ADDR_ONLY)
 		return -EIO;
 #endif
 
 	/* Require Write-Back (WB) memory type for VMCS accesses. */
-	if (((vmx_msr_high >> 18) & 15) != X86_MEMTYPE_WB)
+	if (vmx_basic_vmcs_mem_type(basic_msr) != X86_MEMTYPE_WB)
 		return -EIO;
 
 	rdmsrl(MSR_IA32_VMX_MISC, misc_msr);
 
-	vmcs_conf->size = vmx_msr_high & 0x1fff;
-	vmcs_conf->basic_cap = vmx_msr_high & ~0x1fff;
-
-	vmcs_conf->revision_id = vmx_msr_low;
-
+	vmcs_conf->basic = basic_msr;
 	vmcs_conf->pin_based_exec_ctrl = _pin_based_exec_control;
 	vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control;
 	vmcs_conf->cpu_based_2nd_exec_ctrl = _cpu_based_2nd_exec_control;
@@ -2851,13 +2851,13 @@ struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags)
 	if (!pages)
 		return NULL;
 	vmcs = page_address(pages);
-	memset(vmcs, 0, vmcs_config.size);
+	memset(vmcs, 0, vmx_basic_vmcs_size(vmcs_config.basic));
 
 	/* KVM supports Enlightened VMCS v1 only */
 	if (kvm_is_using_evmcs())
 		vmcs->hdr.revision_id = KVM_EVMCS_VERSION;
 	else
-		vmcs->hdr.revision_id = vmcs_config.revision_id;
+		vmcs->hdr.revision_id = vmx_basic_vmcs_revision_id(vmcs_config.basic);
 
 	if (shadow)
 		vmcs->hdr.shadow_vmcs = 1;
@@ -2950,7 +2950,7 @@ static __init int alloc_kvm_area(void)
 		 * physical CPU.
 		 */
 		if (kvm_is_using_evmcs())
-			vmcs->hdr.revision_id = vmcs_config.revision_id;
+			vmcs->hdr.revision_id = vmx_basic_vmcs_revision_id(vmcs_config.basic);
 
 		per_cpu(vmxarea, cpu) = vmcs;
 	}
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 6/9] KVM: nVMX: Use macros and #defines in vmx_restore_vmx_basic()

[Sean Christopherson]

From: Xin Li <xin3.li@intel.com>

Use macros in vmx_restore_vmx_basic() instead of open coding everything
using BIT_ULL() and GENMASK_ULL().  Opportunistically split feature bits
and reserved bits into separate variables, and add a comment explaining
the subset logic (it's not immediately obvious that the set of feature
bits is NOT the set of _supported_ feature bits).

Cc: Shan Kang <shan.kang@intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
[sean: split to separate patch, write changelog, drop #defines]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/nested.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 82a35aba7d2b..4ad8696c25af 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1228,21 +1228,32 @@ static bool is_bitwise_subset(u64 superset, u64 subset, u64 mask)
 
 static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx, u64 data)
 {
-	const u64 feature_and_reserved =
-		/* feature (except bit 48; see below) */
-		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
-		/* reserved */
-		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
+	const u64 feature_bits = VMX_BASIC_DUAL_MONITOR_TREATMENT |
+				 VMX_BASIC_INOUT |
+				 VMX_BASIC_TRUE_CTLS;
+
+	const u64 reserved_bits = GENMASK_ULL(63, 56) |
+				  GENMASK_ULL(47, 45) |
+				  BIT_ULL(31);
+
 	u64 vmx_basic = vmcs_config.nested.basic;
 
-	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
+	BUILD_BUG_ON(feature_bits & reserved_bits);
+
+	/*
+	 * Except for 32BIT_PHYS_ADDR_ONLY, which is an anti-feature bit (has
+	 * inverted polarity), the incoming value must not set feature bits or
+	 * reserved bits that aren't allowed/supported by KVM.  Fields, i.e.
+	 * multi-bit values, are explicitly checked below.
+	 */
+	if (!is_bitwise_subset(vmx_basic, data, feature_bits | reserved_bits))
 		return -EINVAL;
 
 	/*
 	 * KVM does not emulate a version of VMX that constrains physical
 	 * addresses of VMX structures (e.g. VMCS) to 32-bits.
 	 */
-	if (data & BIT_ULL(48))
+	if (data & VMX_BASIC_32BIT_PHYS_ADDR_ONLY)
 		return -EINVAL;
 
 	if (vmx_basic_vmcs_revision_id(vmx_basic) !=
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 7/9] KVM VMX: Move MSR_IA32_VMX_MISC bit defines to asm/vmx.h

[Sean Christopherson]

Move the handful of MSR_IA32_VMX_MISC bit defines that are currently in
msr-indx.h to vmx.h so that all of the VMX_MISC defines and wrappers can
be found in a single location.

Opportunistically use BIT_ULL() instead of open coding hex values, add
defines for feature bits that are architectural defined, and move the
defines down in the file so that they are colocated with the helpers for
getting fields from VMX_MISC.

No functional change intended.

Cc: Shan Kang <shan.kang@intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
[sean: split to separate patch, write changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/msr-index.h |  5 -----
 arch/x86/include/asm/vmx.h       | 19 ++++++++++++-------
 arch/x86/kvm/vmx/capabilities.h  |  4 ++--
 arch/x86/kvm/vmx/nested.c        |  2 +-
 arch/x86/kvm/vmx/nested.h        |  2 +-
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 5ca81ad509b5..3531856def3d 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1138,11 +1138,6 @@
 #define MSR_IA32_SMBA_BW_BASE		0xc0000280
 #define MSR_IA32_EVT_CFG_BASE		0xc0000400
 
-/* MSR_IA32_VMX_MISC bits */
-#define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)
-#define MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS (1ULL << 29)
-#define MSR_IA32_VMX_MISC_PREEMPTION_TIMER_SCALE   0x1F
-
 /* AMD-V MSRs */
 #define MSR_VM_CR                       0xc0010114
 #define MSR_VM_IGNNE                    0xc0010115
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index ce6d166fc3c5..6ff179b11235 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -120,13 +120,6 @@
 
 #define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR	0x000011ff
 
-#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	0x0000001f
-#define VMX_MISC_SAVE_EFER_LMA			0x00000020
-#define VMX_MISC_ACTIVITY_HLT			0x00000040
-#define VMX_MISC_ACTIVITY_WAIT_SIPI		0x00000100
-#define VMX_MISC_ZERO_LEN_INS			0x40000000
-#define VMX_MISC_MSR_LIST_MULTIPLIER		512
-
 /* VMFUNC functions */
 #define VMFUNC_CONTROL_BIT(x)	BIT((VMX_FEATURE_##x & 0x1f) - 28)
 
@@ -155,6 +148,18 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
 	return (vmx_basic & GENMASK_ULL(53, 50)) >> 50;
 }
 
+#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
+#define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
+#define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
+#define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
+#define VMX_MISC_ACTIVITY_WAIT_SIPI		BIT_ULL(8)
+#define VMX_MISC_INTEL_PT			BIT_ULL(14)
+#define VMX_MISC_RDMSR_IN_SMM			BIT_ULL(15)
+#define VMX_MISC_VMXOFF_BLOCK_SMI		BIT_ULL(28)
+#define VMX_MISC_VMWRITE_SHADOW_RO_FIELDS	BIT_ULL(29)
+#define VMX_MISC_ZERO_LEN_INS			BIT_ULL(30)
+#define VMX_MISC_MSR_LIST_MULTIPLIER		512
+
 static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
 {
 	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index 86ce8bb96bed..cb6588238f46 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -223,7 +223,7 @@ static inline bool cpu_has_vmx_vmfunc(void)
 static inline bool cpu_has_vmx_shadow_vmcs(void)
 {
 	/* check if the cpu supports writing r/o exit information fields */
-	if (!(vmcs_config.misc & MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS))
+	if (!(vmcs_config.misc & VMX_MISC_VMWRITE_SHADOW_RO_FIELDS))
 		return false;
 
 	return vmcs_config.cpu_based_2nd_exec_ctrl &
@@ -365,7 +365,7 @@ static inline bool cpu_has_vmx_invvpid_global(void)
 
 static inline bool cpu_has_vmx_intel_pt(void)
 {
-	return (vmcs_config.misc & MSR_IA32_VMX_MISC_INTEL_PT) &&
+	return (vmcs_config.misc & VMX_MISC_INTEL_PT) &&
 		(vmcs_config.cpu_based_2nd_exec_ctrl & SECONDARY_EXEC_PT_USE_GPA) &&
 		(vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_RTIT_CTL);
 }
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 4ad8696c25af..06512ee7a5c4 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6998,7 +6998,7 @@ static void nested_vmx_setup_misc_data(struct vmcs_config *vmcs_conf,
 {
 	msrs->misc_low = (u32)vmcs_conf->misc & VMX_MISC_SAVE_EFER_LMA;
 	msrs->misc_low |=
-		MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS |
+		VMX_MISC_VMWRITE_SHADOW_RO_FIELDS |
 		VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE |
 		VMX_MISC_ACTIVITY_HLT |
 		VMX_MISC_ACTIVITY_WAIT_SIPI;
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index cce4e2aa30fb..0782fe599757 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -109,7 +109,7 @@ static inline unsigned nested_cpu_vmx_misc_cr3_count(struct kvm_vcpu *vcpu)
 static inline bool nested_cpu_has_vmwrite_any_field(struct kvm_vcpu *vcpu)
 {
 	return to_vmx(vcpu)->nested.msrs.misc_low &
-		MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS;
+		VMX_MISC_VMWRITE_SHADOW_RO_FIELDS;
 }
 
 static inline bool nested_cpu_has_zero_length_injection(struct kvm_vcpu *vcpu)
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Sean Christopherson]

From: Xin Li <xin3.li@intel.com>

Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
that the function looks like all the helpers that grab values from
VMX_BASIC and VMX_MISC MSR values.

No functional change intended.

Cc: Shan Kang <shan.kang@intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
[sean: split to separate patch, write changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/vmx.h | 3 +--
 arch/x86/kvm/vmx/vmx.c     | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 6ff179b11235..90ed559076d7 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -148,7 +148,6 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
 	return (vmx_basic & GENMASK_ULL(53, 50)) >> 50;
 }
 
-#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
 #define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
 #define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
 #define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
@@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
 
 static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
 {
-	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
+	return vmx_misc & GENMASK_ULL(4, 0);
 }
 
 static inline int vmx_misc_cr3_count(u64 vmx_misc)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e312c48f542f..4fcfa1a213fa 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8613,7 +8613,7 @@ static __init int hardware_setup(void)
 		u64 use_timer_freq = 5000ULL * 1000 * 1000;
 
 		cpu_preemption_timer_multi =
-			vmcs_config.misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
+			vmx_misc_preemption_timer_rate(vmcs_config.misc);
 
 		if (tsc_khz)
 			use_timer_freq = (u64)tsc_khz * 1000;
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v6 9/9] KVM: nVMX: Use macros and #defines in vmx_restore_vmx_misc()

[Sean Christopherson]

From: Xin Li <xin3.li@intel.com>

Use macros in vmx_restore_vmx_misc() instead of open coding everything
using BIT_ULL() and GENMASK_ULL().  Opportunistically split feature bits
and reserved bits into separate variables, and add a comment explaining
the subset logic (it's not immediately obvious that the set of feature
bits is NOT the set of _supported_ feature bits).

Cc: Shan Kang <shan.kang@intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
[sean: split to separate patch, write changelog, drop #defines]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/nested.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 06512ee7a5c4..6610d258c680 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1322,16 +1322,29 @@ vmx_restore_control_msr(struct vcpu_vmx *vmx, u32 msr_index, u64 data)
 
 static int vmx_restore_vmx_misc(struct vcpu_vmx *vmx, u64 data)
 {
-	const u64 feature_and_reserved_bits =
-		/* feature */
-		BIT_ULL(5) | GENMASK_ULL(8, 6) | BIT_ULL(14) | BIT_ULL(15) |
-		BIT_ULL(28) | BIT_ULL(29) | BIT_ULL(30) |
-		/* reserved */
-		GENMASK_ULL(13, 9) | BIT_ULL(31);
+	const u64 feature_bits = VMX_MISC_SAVE_EFER_LMA |
+				 VMX_MISC_ACTIVITY_HLT |
+				 VMX_MISC_ACTIVITY_SHUTDOWN |
+				 VMX_MISC_ACTIVITY_WAIT_SIPI |
+				 VMX_MISC_INTEL_PT |
+				 VMX_MISC_RDMSR_IN_SMM |
+				 VMX_MISC_VMWRITE_SHADOW_RO_FIELDS |
+				 VMX_MISC_VMXOFF_BLOCK_SMI |
+				 VMX_MISC_ZERO_LEN_INS;
+
+	const u64 reserved_bits = BIT_ULL(31) | GENMASK_ULL(13, 9);
+
 	u64 vmx_misc = vmx_control_msr(vmcs_config.nested.misc_low,
 				       vmcs_config.nested.misc_high);
 
-	if (!is_bitwise_subset(vmx_misc, data, feature_and_reserved_bits))
+	BUILD_BUG_ON(feature_bits & reserved_bits);
+
+	/*
+	 * The incoming value must not set feature bits or reserved bits that
+	 * aren't allowed/supported by KVM.  Fields, i.e. multi-bit values, are
+	 * explicitly checked below.
+	 */
+	if (!is_bitwise_subset(vmx_misc, data, feature_bits | reserved_bits))
 		return -EINVAL;
 
 	if ((vmx->nested.msrs.pinbased_ctls_high &
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH v6 4/9] KVM: VMX: Move MSR_IA32_VMX_BASIC bit defines to asm/vmx.h

[Zhao Liu]

On Fri, Mar 08, 2024 at 05:27:20PM -0800, Sean Christopherson wrote:
> Date: Fri,  8 Mar 2024 17:27:20 -0800
> From: Sean Christopherson <seanjc@google.com>
> Subject: [PATCH v6 4/9] KVM: VMX: Move MSR_IA32_VMX_BASIC bit defines to
>  asm/vmx.h
> X-Mailer: git-send-email 2.44.0.278.ge034bb2e1d-goog
> 
> From: Xin Li <xin3.li@intel.com>
> 
> Move the bit defines for MSR_IA32_VMX_BASIC from msr-index.h to vmx.h so
> that they are colocated with other VMX MSR bit defines, and with the
> helpers that extract specific information from an MSR_IA32_VMX_BASIC value.
> 
> Opportunistically use BIT_ULL() instead of open coding hex values.
> 
> Opportunistically rename VMX_BASIC_64 to VMX_BASIC_32BIT_PHYS_ADDR_ONLY,
> as "VMX_BASIC_64" is widly misleading.  The flag enumerates that addresses
> are limited to 32 bits, not that 64-bit addresses are allowed.
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/msr-index.h | 8 --------
>  arch/x86/include/asm/vmx.h       | 7 +++++++
>  2 files changed, 7 insertions(+), 8 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH v6 5/9] KVM: VMX: Track CPU's MSR_IA32_VMX_BASIC as a single 64-bit value

[Zhao Liu]

Hi Sean,

On Fri, Mar 08, 2024 at 05:27:21PM -0800, Sean Christopherson wrote:
> Date: Fri,  8 Mar 2024 17:27:21 -0800
> From: Sean Christopherson <seanjc@google.com>
> Subject: [PATCH v6 5/9] KVM: VMX: Track CPU's MSR_IA32_VMX_BASIC as a
>  single 64-bit value
> X-Mailer: git-send-email 2.44.0.278.ge034bb2e1d-goog
> 
> Track the "basic" capabilities VMX MSR as a single u64 in vmcs_config
> instead of splitting it across three fields, that obviously don't combine
> into a single 64-bit value, so that KVM can use the macros that define MSR
> bits using their absolute position.  Replace all open coded shifts and
> masks, many of which are relative to the "high" half, with the appropriate
> macro.
> 
> Opportunistically use VMX_BASIC_32BIT_PHYS_ADDR_ONLY instead of an open
> coded equivalent, and clean up the related comment to not reference a
> specific SDM section (to the surprise of no one, the comment is stale).
> 
> No functional change intended (though obviously the code generation will
> be quite different).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/vmx.h      |  5 +++++
>  arch/x86/kvm/vmx/capabilities.h |  6 ++----
>  arch/x86/kvm/vmx/vmx.c          | 28 ++++++++++++++--------------
>  3 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index c3a97dca4a33..ce6d166fc3c5 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -150,6 +150,11 @@ static inline u32 vmx_basic_vmcs_size(u64 vmx_basic)
>  	return (vmx_basic & GENMASK_ULL(44, 32)) >> 32;

Maybe we could use VMX_BASIC_VMCS_SIZE_SHIFT to replace "32"?

>  }
>  
> +static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> +{
> +	return (vmx_basic & GENMASK_ULL(53, 50)) >> 50;
> +}
> +

Also here, we can use VMX_BASIC_MEM_TYPE_SHIFT to replace "50".

And what about defining VMX_BASIC_MEM_TYPE_MASK to replace
GENMASK_ULL(53, 50), and VMX_BASIC_VMCS_SIZE_MSAK to replace
GENMASK_ULL(44, 32) above?

Thanks,
Zhao

Re: [PATCH v6 6/9] KVM: nVMX: Use macros and #defines in vmx_restore_vmx_basic()

[Zhao Liu]

On Fri, Mar 08, 2024 at 05:27:22PM -0800, Sean Christopherson wrote:
> Date: Fri,  8 Mar 2024 17:27:22 -0800
> From: Sean Christopherson <seanjc@google.com>
> Subject: [PATCH v6 6/9] KVM: nVMX: Use macros and #defines in
>  vmx_restore_vmx_basic()
> X-Mailer: git-send-email 2.44.0.278.ge034bb2e1d-goog
> 
> From: Xin Li <xin3.li@intel.com>
> 
> Use macros in vmx_restore_vmx_basic() instead of open coding everything
> using BIT_ULL() and GENMASK_ULL().  Opportunistically split feature bits
> and reserved bits into separate variables, and add a comment explaining
> the subset logic (it's not immediately obvious that the set of feature
> bits is NOT the set of _supported_ feature bits).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog, drop #defines]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH v6 7/9] KVM VMX: Move MSR_IA32_VMX_MISC bit defines to asm/vmx.h

[Zhao Liu]

On Fri, Mar 08, 2024 at 05:27:23PM -0800, Sean Christopherson wrote:
> Date: Fri,  8 Mar 2024 17:27:23 -0800
> From: Sean Christopherson <seanjc@google.com>
> Subject: [PATCH v6 7/9] KVM VMX: Move MSR_IA32_VMX_MISC bit defines to
>  asm/vmx.h
> X-Mailer: git-send-email 2.44.0.278.ge034bb2e1d-goog
> 
> Move the handful of MSR_IA32_VMX_MISC bit defines that are currently in
> msr-indx.h to vmx.h so that all of the VMX_MISC defines and wrappers can
> be found in a single location.
> 
> Opportunistically use BIT_ULL() instead of open coding hex values, add
> defines for feature bits that are architectural defined, and move the
> defines down in the file so that they are colocated with the helpers for
> getting fields from VMX_MISC.
> 
> No functional change intended.
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/msr-index.h |  5 -----
>  arch/x86/include/asm/vmx.h       | 19 ++++++++++++-------
>  arch/x86/kvm/vmx/capabilities.h  |  4 ++--
>  arch/x86/kvm/vmx/nested.c        |  2 +-
>  arch/x86/kvm/vmx/nested.h        |  2 +-
>  5 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 5ca81ad509b5..3531856def3d 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1138,11 +1138,6 @@
>  #define MSR_IA32_SMBA_BW_BASE		0xc0000280
>  #define MSR_IA32_EVT_CFG_BASE		0xc0000400
>  
> -/* MSR_IA32_VMX_MISC bits */
> -#define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)
> -#define MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS (1ULL << 29)
> -#define MSR_IA32_VMX_MISC_PREEMPTION_TIMER_SCALE   0x1F
> -
>  /* AMD-V MSRs */
>  #define MSR_VM_CR                       0xc0010114
>  #define MSR_VM_IGNNE                    0xc0010115
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index ce6d166fc3c5..6ff179b11235 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -120,13 +120,6 @@
>  
>  #define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR	0x000011ff
>  
> -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	0x0000001f
> -#define VMX_MISC_SAVE_EFER_LMA			0x00000020
> -#define VMX_MISC_ACTIVITY_HLT			0x00000040
> -#define VMX_MISC_ACTIVITY_WAIT_SIPI		0x00000100
> -#define VMX_MISC_ZERO_LEN_INS			0x40000000
> -#define VMX_MISC_MSR_LIST_MULTIPLIER		512
> -
>  /* VMFUNC functions */
>  #define VMFUNC_CONTROL_BIT(x)	BIT((VMX_FEATURE_##x & 0x1f) - 28)
>  
> @@ -155,6 +148,18 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
>  	return (vmx_basic & GENMASK_ULL(53, 50)) >> 50;
>  }
>  
> +#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
> +#define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
> +#define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
> +#define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
> +#define VMX_MISC_ACTIVITY_WAIT_SIPI		BIT_ULL(8)
> +#define VMX_MISC_INTEL_PT			BIT_ULL(14)
> +#define VMX_MISC_RDMSR_IN_SMM			BIT_ULL(15)
> +#define VMX_MISC_VMXOFF_BLOCK_SMI		BIT_ULL(28)
> +#define VMX_MISC_VMWRITE_SHADOW_RO_FIELDS	BIT_ULL(29)
> +#define VMX_MISC_ZERO_LEN_INS			BIT_ULL(30)
> +#define VMX_MISC_MSR_LIST_MULTIPLIER		512
> +

Maybe it's better to mention this patch also define some new bits:

* VMX_MISC_ACTIVITY_SHUTDOWN
* VMX_MISC_RDMSR_IN_SMM
* VMX_MISC_VMXOFF_BLOCK_SMI

Otherwise,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Zhao Liu]

On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote:
> Date: Fri,  8 Mar 2024 17:27:24 -0800
> From: Sean Christopherson <seanjc@google.com>
> Subject: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask
>  in its accessor
> X-Mailer: git-send-email 2.44.0.278.ge034bb2e1d-goog
> 
> From: Xin Li <xin3.li@intel.com>
> 
> Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
> and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
> that the function looks like all the helpers that grab values from
> VMX_BASIC and VMX_MISC MSR values.
> 
> No functional change intended.
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/vmx.h | 3 +--
>  arch/x86/kvm/vmx/vmx.c     | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 6ff179b11235..90ed559076d7 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -148,7 +148,6 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
>  	return (vmx_basic & GENMASK_ULL(53, 50)) >> 50;
>  }
>  
> -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
>  #define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
>  #define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
>  #define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
> @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
>  
>  static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
>  {
> -	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
> +	return vmx_misc & GENMASK_ULL(4, 0);
>  }

I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than
GENMASK_ULL(4, 0), and the former improves code readability.

May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK?

Thanks,
Zhao

Re: [PATCH v6 9/9] KVM: nVMX: Use macros and #defines in vmx_restore_vmx_misc()

[Zhao Liu]

On Fri, Mar 08, 2024 at 05:27:25PM -0800, Sean Christopherson wrote:
> Date: Fri,  8 Mar 2024 17:27:25 -0800
> From: Sean Christopherson <seanjc@google.com>
> Subject: [PATCH v6 9/9] KVM: nVMX: Use macros and #defines in
>  vmx_restore_vmx_misc()
> X-Mailer: git-send-email 2.44.0.278.ge034bb2e1d-goog
> 
> From: Xin Li <xin3.li@intel.com>
> 
> Use macros in vmx_restore_vmx_misc() instead of open coding everything
> using BIT_ULL() and GENMASK_ULL().  Opportunistically split feature bits
> and reserved bits into separate variables, and add a comment explaining
> the subset logic (it's not immediately obvious that the set of feature
> bits is NOT the set of _supported_ feature bits).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog, drop #defines]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Sean Christopherson]

On Fri, Mar 15, 2024, Zhao Liu wrote:
> On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote:
> > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
> > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
> > that the function looks like all the helpers that grab values from
> > VMX_BASIC and VMX_MISC MSR values.

...

> > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
> >  #define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
> >  #define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
> >  #define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
> > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> >  
> >  static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
> >  {
> > -	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
> > +	return vmx_misc & GENMASK_ULL(4, 0);
> >  }
> 
> I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than
> GENMASK_ULL(4, 0), and the former improves code readability.
> 
> May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK?

I don't necessarily disagree, but in this case I value consistency over one
individual case.  As called out in the changelog, the motivation is to make
vmx_misc_preemption_timer_rate() look like all the surrounding helpers.

_If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(),
vmx_misc_max_msr(), etc.

I don't have a super strong preference, though I think my vote would be to not
add the masks and go with this patch.  These helpers are intended to be the _only_
way to access the fields, i.e. they effectively _are_ the mask macros, just in
function form.

Re: [PATCH v6 4/9] KVM: VMX: Move MSR_IA32_VMX_BASIC bit defines to asm/vmx.h

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> From: Xin Li <xin3.li@intel.com>
> 
> Move the bit defines for MSR_IA32_VMX_BASIC from msr-index.h to vmx.h so
> that they are colocated with other VMX MSR bit defines, and with the
> helpers that extract specific information from an MSR_IA32_VMX_BASIC value.
> 
> Opportunistically use BIT_ULL() instead of open coding hex values.
> 
> Opportunistically rename VMX_BASIC_64 to VMX_BASIC_32BIT_PHYS_ADDR_ONLY,
> as "VMX_BASIC_64" is widly misleading.  The flag enumerates that addresses
> are limited to 32 bits, not that 64-bit addresses are allowed.
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v6 5/9] KVM: VMX: Track CPU's MSR_IA32_VMX_BASIC as a single 64-bit value

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> Track the "basic" capabilities VMX MSR as a single u64 in vmcs_config
> instead of splitting it across three fields, that obviously don't combine
> into a single 64-bit value, so that KVM can use the macros that define MSR
> bits using their absolute position.  Replace all open coded shifts and
> masks, many of which are relative to the "high" half, with the appropriate
> macro.
> 
> Opportunistically use VMX_BASIC_32BIT_PHYS_ADDR_ONLY instead of an open
> coded equivalent, and clean up the related comment to not reference a
> specific SDM section (to the surprise of no one, the comment is stale).
> 
> No functional change intended (though obviously the code generation will
> be quite different).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Thanks for doing this:

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v6 6/9] KVM: nVMX: Use macros and #defines in vmx_restore_vmx_basic()

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> From: Xin Li <xin3.li@intel.com>
> 
> Use macros in vmx_restore_vmx_basic() instead of open coding everything
> using BIT_ULL() and GENMASK_ULL().  Opportunistically split feature bits
> and reserved bits into separate variables, and add a comment explaining
> the subset logic (it's not immediately obvious that the set of feature
> bits is NOT the set of _supported_ feature bits).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog, drop #defines]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v6 7/9] KVM VMX: Move MSR_IA32_VMX_MISC bit defines to asm/vmx.h

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> Move the handful of MSR_IA32_VMX_MISC bit defines that are currently in
> msr-indx.h to vmx.h so that all of the VMX_MISC defines and wrappers can
> be found in a single location.
> 
> Opportunistically use BIT_ULL() instead of open coding hex values, add
> defines for feature bits that are architectural defined, and move the
> defines down in the file so that they are colocated with the helpers for
> getting fields from VMX_MISC.
> 
> No functional change intended.
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> From: Xin Li <xin3.li@intel.com>
> 
> Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
> and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
> that the function looks like all the helpers that grab values from

Is "all other helpers" better?

> VMX_BASIC and VMX_MISC MSR values.
> 
> No functional change intended.
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v6 9/9] KVM: nVMX: Use macros and #defines in vmx_restore_vmx_misc()

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> From: Xin Li <xin3.li@intel.com>
> 
> Use macros in vmx_restore_vmx_misc() instead of open coding everything
> using BIT_ULL() and GENMASK_ULL().  Opportunistically split feature bits
> and reserved bits into separate variables, and add a comment explaining
> the subset logic (it's not immediately obvious that the set of feature
> bits is NOT the set of _supported_ feature bits).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog, drop #defines]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v6 1/9] x86/cpu: KVM: Add common defines for architectural memory types (PAT, MTRRs, etc.)

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> Add defines for the architectural memory types that can be shoved into
> various MSRs and registers, e.g. MTRRs, PAT, VMX capabilities MSRs, EPTPs,
> etc.  While most MSRs/registers support only a subset of all memory types,
> the values themselves are architectural and identical across all users.
> 
> Leave the goofy MTRR_TYPE_* definitions as-is since they are in a uapi
> header, but add compile-time assertions to connect the dots (and sanity
> check that the msr-index.h values didn't get fat-fingered).
> 
> Keep the VMX_EPTP_MT_* defines so that it's slightly more obvious that the
> EPTP holds a single memory type in 3 of its 64 bits; those bits just
> happen to be 2:0, i.e. don't need to be shifted.
> 
> Opportunistically use X86_MEMTYPE_WB instead of an open coded '6' in
> setup_vmcs_config().
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

[...]

>  
>  #include "mtrr.h"
>  
> +static_assert(X86_MEMTYPE_UC == MTRR_TYPE_UNCACHABLE);
> +static_assert(X86_MEMTYPE_WC == MTRR_TYPE_WRCOMB);
> +static_assert(X86_MEMTYPE_WT == MTRR_TYPE_WRTHROUGH);
> +static_assert(X86_MEMTYPE_WP == MTRR_TYPE_WRPROT);
> +static_assert(X86_MEMTYPE_WB == MTRR_TYPE_WRBACK);
> +
> 

Hi Sean,

IIUC, the purpose of this patch is for the kernel to use X86_MEMTYPE_xx, which
are architectural values, where applicable?

Yeah we need to keep MTRR_TYPE_xx in the uapi header, but in the kernel, should
we change all places that use MTRR_TYPE_xx to X86_MEMTYPE_xx?  The
static_assert()s above have guaranteed the two are the same, so there's nothing
wrong for the kernel to use X86_MEMTYPE_xx instead.

Both PAT_xx and VMX_BASIC_MEM_TYPE_xx to X86_MEMTYPE_xx, it seems a little bit
odd if we don't switch for MTRR_TYPE_xx.

However by simple search MEM_TYPE_xx are intensively used in many files, so...

Re: [PATCH v6 2/9] x86/cpu: KVM: Move macro to encode PAT value to common header

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> Move pat/memtype.c's PAT() macro to msr-index.h as PAT_VALUE(), and use it
> in KVM to define the default (Power-On / RESET) PAT value instead of open
> coding an inscrutable magic number.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v6 3/9] KVM: x86: Stuff vCPU's PAT with default value at RESET, not creation

[Huang, Kai]

On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> Move the stuffing of the vCPU's PAT to the architectural "default" value
> from kvm_arch_vcpu_create() to kvm_vcpu_reset(), guarded by !init_event,
> to better capture that the default value is the value "Following Power-up
> or Reset".  E.g. setting PAT only during creation would break if KVM were
> to expose a RESET ioctl() to userspace (which is unlikely, but that's not
> a good reason to have unintuitive code).
> 
> No functional change.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v6 2/9] x86/cpu: KVM: Move macro to encode PAT value to common header

[Xiaoyao Li]

On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> Move pat/memtype.c's PAT() macro to msr-index.h as PAT_VALUE(), and use it
> in KVM to define the default (Power-On / RESET) PAT value instead of open
> coding an inscrutable magic number.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

Re: [PATCH v6 3/9] KVM: x86: Stuff vCPU's PAT with default value at RESET, not creation

[Xiaoyao Li]

On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> Move the stuffing of the vCPU's PAT to the architectural "default" value
> from kvm_arch_vcpu_create() to kvm_vcpu_reset(), guarded by !init_event,
> to better capture that the default value is the value "Following Power-up
> or Reset".  E.g. setting PAT only during creation would break if KVM were
> to expose a RESET ioctl() to userspace (which is unlikely, but that's not
> a good reason to have unintuitive code).
> 
> No functional change.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   arch/x86/kvm/x86.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 66c4381460dc..eac97b1b8379 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12134,8 +12134,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>   	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
>   	vcpu->arch.reserved_gpa_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu);
>   
> -	vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT;
> -
>   	kvm_async_pf_hash_reset(vcpu);
>   
>   	vcpu->arch.perf_capabilities = kvm_caps.supported_perf_cap;
> @@ -12302,6 +12300,8 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>   	if (!init_event) {
>   		vcpu->arch.smbase = 0x30000;
>   
> +		vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT;
> +
>   		vcpu->arch.msr_misc_features_enables = 0;
>   		vcpu->arch.ia32_misc_enable_msr = MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL |
>   						  MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;

Re: [PATCH v6 4/9] KVM: VMX: Move MSR_IA32_VMX_BASIC bit defines to asm/vmx.h

[Xiaoyao Li]

On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> From: Xin Li <xin3.li@intel.com>
> 
> Move the bit defines for MSR_IA32_VMX_BASIC from msr-index.h to vmx.h so
> that they are colocated with other VMX MSR bit defines, and with the
> helpers that extract specific information from an MSR_IA32_VMX_BASIC value.

My understanding of msr-index.h is, it contains the index of various 
MSRs and the bit definitions of each MSRs.

Put the definition of each bit or bits below the definition of MSR index 
instead of dispersed in different headers looks more intact for me.

> Opportunistically use BIT_ULL() instead of open coding hex values.
> 
> Opportunistically rename VMX_BASIC_64 to VMX_BASIC_32BIT_PHYS_ADDR_ONLY,
> as "VMX_BASIC_64" is widly misleading.  The flag enumerates that addresses
> are limited to 32 bits, not that 64-bit addresses are allowed.
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/include/asm/msr-index.h | 8 --------
>   arch/x86/include/asm/vmx.h       | 7 +++++++
>   2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index af71f8bb76ae..5ca81ad509b5 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1122,14 +1122,6 @@
>   #define MSR_IA32_VMX_VMFUNC             0x00000491
>   #define MSR_IA32_VMX_PROCBASED_CTLS3	0x00000492
>   
> -/* VMX_BASIC bits and bitmasks */
> -#define VMX_BASIC_VMCS_SIZE_SHIFT	32
> -#define VMX_BASIC_TRUE_CTLS		(1ULL << 55)
> -#define VMX_BASIC_64		0x0001000000000000LLU
> -#define VMX_BASIC_MEM_TYPE_SHIFT	50
> -#define VMX_BASIC_MEM_TYPE_MASK	0x003c000000000000LLU
> -#define VMX_BASIC_INOUT		0x0040000000000000LLU
> -
>   /* Resctrl MSRs: */
>   /* - Intel: */
>   #define MSR_IA32_L3_QOS_CFG		0xc81
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 4fdc76263066..c3a97dca4a33 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -133,6 +133,13 @@
>   #define VMX_VMFUNC_EPTP_SWITCHING               VMFUNC_CONTROL_BIT(EPTP_SWITCHING)
>   #define VMFUNC_EPTP_ENTRIES  512
>   
> +#define VMX_BASIC_VMCS_SIZE_SHIFT		32
> +#define VMX_BASIC_32BIT_PHYS_ADDR_ONLY		BIT_ULL(48)
> +#define VMX_BASIC_DUAL_MONITOR_TREATMENT	BIT_ULL(49)
> +#define VMX_BASIC_MEM_TYPE_SHIFT		50
> +#define VMX_BASIC_INOUT				BIT_ULL(54)
> +#define VMX_BASIC_TRUE_CTLS			BIT_ULL(55)
> +
>   static inline u32 vmx_basic_vmcs_revision_id(u64 vmx_basic)
>   {
>   	return vmx_basic & GENMASK_ULL(30, 0);

Re: [PATCH v6 5/9] KVM: VMX: Track CPU's MSR_IA32_VMX_BASIC as a single 64-bit value

[Xiaoyao Li]

On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> Track the "basic" capabilities VMX MSR as a single u64 in vmcs_config
> instead of splitting it across three fields, that obviously don't combine
> into a single 64-bit value, so that KVM can use the macros that define MSR
> bits using their absolute position.  Replace all open coded shifts and
> masks, many of which are relative to the "high" half, with the appropriate
> macro.
> 
> Opportunistically use VMX_BASIC_32BIT_PHYS_ADDR_ONLY instead of an open
> coded equivalent, and clean up the related comment to not reference a
> specific SDM section (to the surprise of no one, the comment is stale).
> 
> No functional change intended (though obviously the code generation will
> be quite different).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog]

The patch author doesn't match with the signed-off

> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/include/asm/vmx.h      |  5 +++++
>   arch/x86/kvm/vmx/capabilities.h |  6 ++----
>   arch/x86/kvm/vmx/vmx.c          | 28 ++++++++++++++--------------
>   3 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index c3a97dca4a33..ce6d166fc3c5 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -150,6 +150,11 @@ static inline u32 vmx_basic_vmcs_size(u64 vmx_basic)
>   	return (vmx_basic & GENMASK_ULL(44, 32)) >> 32;
>   }
>   
> +static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> +{
> +	return (vmx_basic & GENMASK_ULL(53, 50)) >> 50;

#define VMX_BASIC_MEM_TYPE_SHIFT		50

We have the shift defined in previous patch, we need to use it I think,
Any maybe, we can define the MASK as well.

Otherwise, this cleanup is good.

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

Re: [PATCH v6 6/9] KVM: nVMX: Use macros and #defines in vmx_restore_vmx_basic()

[Xiaoyao Li]

On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> From: Xin Li <xin3.li@intel.com>
> 
> Use macros in vmx_restore_vmx_basic() instead of open coding everything
> using BIT_ULL() and GENMASK_ULL().  Opportunistically split feature bits
> and reserved bits into separate variables, and add a comment explaining
> the subset logic (it's not immediately obvious that the set of feature
> bits is NOT the set of _supported_ feature bits).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog, drop #defines]
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   arch/x86/kvm/vmx/nested.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 82a35aba7d2b..4ad8696c25af 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1228,21 +1228,32 @@ static bool is_bitwise_subset(u64 superset, u64 subset, u64 mask)
>   
>   static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx, u64 data)
>   {
> -	const u64 feature_and_reserved =
> -		/* feature (except bit 48; see below) */
> -		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
> -		/* reserved */
> -		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
> +	const u64 feature_bits = VMX_BASIC_DUAL_MONITOR_TREATMENT |
> +				 VMX_BASIC_INOUT |
> +				 VMX_BASIC_TRUE_CTLS;
> +
> +	const u64 reserved_bits = GENMASK_ULL(63, 56) |
> +				  GENMASK_ULL(47, 45) |
> +				  BIT_ULL(31);
> +
>   	u64 vmx_basic = vmcs_config.nested.basic;
>   
> -	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
> +	BUILD_BUG_ON(feature_bits & reserved_bits);
> +
> +	/*
> +	 * Except for 32BIT_PHYS_ADDR_ONLY, which is an anti-feature bit (has
> +	 * inverted polarity), the incoming value must not set feature bits or
> +	 * reserved bits that aren't allowed/supported by KVM.  Fields, i.e.
> +	 * multi-bit values, are explicitly checked below.
> +	 */
> +	if (!is_bitwise_subset(vmx_basic, data, feature_bits | reserved_bits))
>   		return -EINVAL;
>   
>   	/*
>   	 * KVM does not emulate a version of VMX that constrains physical
>   	 * addresses of VMX structures (e.g. VMCS) to 32-bits.
>   	 */
> -	if (data & BIT_ULL(48))
> +	if (data & VMX_BASIC_32BIT_PHYS_ADDR_ONLY)
>   		return -EINVAL;
>   
>   	if (vmx_basic_vmcs_revision_id(vmx_basic) !=

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Xiaoyao Li]

On 3/16/2024 1:54 AM, Sean Christopherson wrote:
> On Fri, Mar 15, 2024, Zhao Liu wrote:
>> On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote:
>>> Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
>>> and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
>>> that the function looks like all the helpers that grab values from
>>> VMX_BASIC and VMX_MISC MSR values.
> 
> ...
> 
>>> -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
>>>   #define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
>>>   #define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
>>>   #define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
>>> @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
>>>   
>>>   static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
>>>   {
>>> -	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
>>> +	return vmx_misc & GENMASK_ULL(4, 0);
>>>   }
>>
>> I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than
>> GENMASK_ULL(4, 0), and the former improves code readability.
>>
>> May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK?
> 
> I don't necessarily disagree, but in this case I value consistency over one
> individual case.  As called out in the changelog, the motivation is to make
> vmx_misc_preemption_timer_rate() look like all the surrounding helpers.
> 
> _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(),
> vmx_misc_max_msr(), etc.
> 
> I don't have a super strong preference, though I think my vote would be to not
> add the masks and go with this patch.  These helpers are intended to be the _only_
> way to access the fields, i.e. they effectively _are_ the mask macros, just in
> function form.
> 

+1.

However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, 
that I just recommended to define the MASK.

Because we already have

	#define VMX_BASIC_MEM_TYPE_SHIFT	50

and it has been used in vmx/nested.c,

static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
{
	return (vmx_basic & GENMASK_ULL(53, 50)) >>
		VMX_BASIC_MEM_TYPE_SHIFT;
}

looks not intuitive than original patch.

Re: [PATCH v6 9/9] KVM: nVMX: Use macros and #defines in vmx_restore_vmx_misc()

[Xiaoyao Li]

On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> From: Xin Li <xin3.li@intel.com>
> 
> Use macros in vmx_restore_vmx_misc() instead of open coding everything
> using BIT_ULL() and GENMASK_ULL().  Opportunistically split feature bits
> and reserved bits into separate variables, and add a comment explaining
> the subset logic (it's not immediately obvious that the set of feature
> bits is NOT the set of _supported_ feature bits).
> 
> Cc: Shan Kang <shan.kang@intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> [sean: split to separate patch, write changelog, drop #defines]
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   arch/x86/kvm/vmx/nested.c | 27 ++++++++++++++++++++-------
>   1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 06512ee7a5c4..6610d258c680 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1322,16 +1322,29 @@ vmx_restore_control_msr(struct vcpu_vmx *vmx, u32 msr_index, u64 data)
>   
>   static int vmx_restore_vmx_misc(struct vcpu_vmx *vmx, u64 data)
>   {
> -	const u64 feature_and_reserved_bits =
> -		/* feature */
> -		BIT_ULL(5) | GENMASK_ULL(8, 6) | BIT_ULL(14) | BIT_ULL(15) |
> -		BIT_ULL(28) | BIT_ULL(29) | BIT_ULL(30) |
> -		/* reserved */
> -		GENMASK_ULL(13, 9) | BIT_ULL(31);
> +	const u64 feature_bits = VMX_MISC_SAVE_EFER_LMA |
> +				 VMX_MISC_ACTIVITY_HLT |
> +				 VMX_MISC_ACTIVITY_SHUTDOWN |
> +				 VMX_MISC_ACTIVITY_WAIT_SIPI |
> +				 VMX_MISC_INTEL_PT |
> +				 VMX_MISC_RDMSR_IN_SMM |
> +				 VMX_MISC_VMWRITE_SHADOW_RO_FIELDS |
> +				 VMX_MISC_VMXOFF_BLOCK_SMI |
> +				 VMX_MISC_ZERO_LEN_INS;
> +
> +	const u64 reserved_bits = BIT_ULL(31) | GENMASK_ULL(13, 9);
> +
>   	u64 vmx_misc = vmx_control_msr(vmcs_config.nested.misc_low,
>   				       vmcs_config.nested.misc_high);
>   
> -	if (!is_bitwise_subset(vmx_misc, data, feature_and_reserved_bits))
> +	BUILD_BUG_ON(feature_bits & reserved_bits);
> +
> +	/*
> +	 * The incoming value must not set feature bits or reserved bits that
> +	 * aren't allowed/supported by KVM.  Fields, i.e. multi-bit values, are
> +	 * explicitly checked below.
> +	 */
> +	if (!is_bitwise_subset(vmx_misc, data, feature_bits | reserved_bits))
>   		return -EINVAL;
>   
>   	if ((vmx->nested.msrs.pinbased_ctls_high &

RE: [PATCH v6 4/9] KVM: VMX: Move MSR_IA32_VMX_BASIC bit defines to asm/vmx.h

[Li, Xin3]

> On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> > From: Xin Li <xin3.li@intel.com>
> >
> > Move the bit defines for MSR_IA32_VMX_BASIC from msr-index.h to vmx.h
> > so that they are colocated with other VMX MSR bit defines, and with
> > the helpers that extract specific information from an MSR_IA32_VMX_BASIC
> value.
> 
> My understanding of msr-index.h is, it contains the index of various MSRs and the
> bit definitions of each MSRs.

"index" in the name kind of tell what it wants to focus.

> Put the definition of each bit or bits below the definition of MSR index instead of
> dispersed in different headers looks more intact for me.

You're right when there is no other proper header for a MSR field definition.

While the Linux code is maintained in the manner of "divide and conquer",
thus I would say the VMX fields definitions belong to the KVM community,
and fortunately, there is such a vmx header.

BTW, It looks to me that some perf MSRs and fields are not in msr-index.h,
which avoids bothering the tip maintainers all the time.

Thanks!
    Xin

Re: [PATCH v6 4/9] KVM: VMX: Move MSR_IA32_VMX_BASIC bit defines to asm/vmx.h

[Sean Christopherson]

On Tue, Apr 02, 2024, Xin3 Li wrote:
> > On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> > > From: Xin Li <xin3.li@intel.com>
> > >
> > > Move the bit defines for MSR_IA32_VMX_BASIC from msr-index.h to vmx.h
> > > so that they are colocated with other VMX MSR bit defines, and with
> > > the helpers that extract specific information from an MSR_IA32_VMX_BASIC
> > value.
> > 
> > My understanding of msr-index.h is, it contains the index of various MSRs and the
> > bit definitions of each MSRs.
> 
> "index" in the name kind of tell what it wants to focus.

Heh, there are a lot of files with names that don't necessarily reflect the
entirety of what they contain, I wouldn't put too much stock in the name :-)

> > Put the definition of each bit or bits below the definition of MSR index instead of
> > dispersed in different headers looks more intact for me.
> 
> You're right when there is no other proper header for a MSR field definition.
> 
> While the Linux code is maintained in the manner of "divide and conquer",
> thus I would say the VMX fields definitions belong to the KVM community,
> and fortunately, there is such a vmx header.
> 
> BTW, It looks to me that some perf MSRs and fields are not in msr-index.h,
> which avoids bothering the tip maintainers all the time.

Ya, there is no hard rule that MSR indices and bits/masks _must_ go in msr-index.h.
Like many things, it's a judgment call, in this case to balance between keeping
a single file maintainble, usable, and readable, and making information easy and
intuitive to find.

There are many more examples, usually for things that are extremely "platform"
specific, e.g. the perf MSRs (especially for uncore stuff), synthetic MSRs defined
by hypervisors, etc.

In this particular case, I agree with Xin that putting the bit and mask definitions
in vmx.h makes the most sense.  Nothing outside of virtualization code is likely
to ever care about the bits of the VMX feature MSRs, and putting all of the info
and helpers in msr-index.h would add a fair bit of noise, and would make it more
annoying to tweak and add masks for KVM's usage.

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Sean Christopherson]

On Mon, Apr 01, 2024, Xiaoyao Li wrote:
> On 3/16/2024 1:54 AM, Sean Christopherson wrote:
> > On Fri, Mar 15, 2024, Zhao Liu wrote:
> > > On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote:
> > > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
> > > > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
> > > > that the function looks like all the helpers that grab values from
> > > > VMX_BASIC and VMX_MISC MSR values.
> > 
> > ...
> > 
> > > > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
> > > >   #define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
> > > >   #define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
> > > >   #define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
> > > > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> > > >   static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
> > > >   {
> > > > -	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
> > > > +	return vmx_misc & GENMASK_ULL(4, 0);
> > > >   }
> > > 
> > > I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than
> > > GENMASK_ULL(4, 0), and the former improves code readability.
> > > 
> > > May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK?
> > 
> > I don't necessarily disagree, but in this case I value consistency over one
> > individual case.  As called out in the changelog, the motivation is to make
> > vmx_misc_preemption_timer_rate() look like all the surrounding helpers.
> > 
> > _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(),
> > vmx_misc_max_msr(), etc.
> > 
> > I don't have a super strong preference, though I think my vote would be to not
> > add the masks and go with this patch.  These helpers are intended to be the _only_
> > way to access the fields, i.e. they effectively _are_ the mask macros, just in
> > function form.
> > 
> 
> +1.
> 
> However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I
> just recommended to define the MASK.
> 
> Because we already have
> 
> 	#define VMX_BASIC_MEM_TYPE_SHIFT	50
> 
> and it has been used in vmx/nested.c,
> 
> static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> {
> 	return (vmx_basic & GENMASK_ULL(53, 50)) >>
> 		VMX_BASIC_MEM_TYPE_SHIFT;
> }
> 
> looks not intuitive than original patch.

Yeah, agreed, that's taking the worst of both worlds.  I'll update patch 5 to drop
VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type().

Thanks!

Re: [PATCH v6 1/9] x86/cpu: KVM: Add common defines for architectural memory types (PAT, MTRRs, etc.)

[Sean Christopherson]

On Wed, Mar 27, 2024, Kai Huang wrote:
> On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> > Add defines for the architectural memory types that can be shoved into
> > various MSRs and registers, e.g. MTRRs, PAT, VMX capabilities MSRs, EPTPs,
> > etc.  While most MSRs/registers support only a subset of all memory types,
> > the values themselves are architectural and identical across all users.
> > 
> > Leave the goofy MTRR_TYPE_* definitions as-is since they are in a uapi
> > header, but add compile-time assertions to connect the dots (and sanity
> > check that the msr-index.h values didn't get fat-fingered).
> > 
> > Keep the VMX_EPTP_MT_* defines so that it's slightly more obvious that the
> > EPTP holds a single memory type in 3 of its 64 bits; those bits just
> > happen to be 2:0, i.e. don't need to be shifted.
> > 
> > Opportunistically use X86_MEMTYPE_WB instead of an open coded '6' in
> > setup_vmcs_config().
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > 
> 
> [...]
> 
> >  
> >  #include "mtrr.h"
> >  
> > +static_assert(X86_MEMTYPE_UC == MTRR_TYPE_UNCACHABLE);
> > +static_assert(X86_MEMTYPE_WC == MTRR_TYPE_WRCOMB);
> > +static_assert(X86_MEMTYPE_WT == MTRR_TYPE_WRTHROUGH);
> > +static_assert(X86_MEMTYPE_WP == MTRR_TYPE_WRPROT);
> > +static_assert(X86_MEMTYPE_WB == MTRR_TYPE_WRBACK);
> > +
> > 
> 
> Hi Sean,
> 
> IIUC, the purpose of this patch is for the kernel to use X86_MEMTYPE_xx, which
> are architectural values, where applicable?

Maybe?  Probably?

> Yeah we need to keep MTRR_TYPE_xx in the uapi header, but in the kernel, should
> we change all places that use MTRR_TYPE_xx to X86_MEMTYPE_xx?  The
> static_assert()s above have guaranteed the two are the same, so there's nothing
> wrong for the kernel to use X86_MEMTYPE_xx instead.
> 
> Both PAT_xx and VMX_BASIC_MEM_TYPE_xx to X86_MEMTYPE_xx, it seems a little bit
> odd if we don't switch for MTRR_TYPE_xx.
> 
> However by simple search MEM_TYPE_xx are intensively used in many files, so...

Yeah, I definitely don't want to do it in this series due to the amount of churn
that would be required.

  $ git grep MTRR_TYPE_ | wc -l
  100

I'm not even entirely convinced that it would be a net positive.  Much of the KVM
usage that's being cleaned up is flat out wrong, e.g. using "MTRR" enums in places
that having nothing to do with MTRRs.  But the majority of the remaining usage is
in MTRR code, i.e. isn't wrong, and is arguably better off using the MTRR specific
#defines.

Re: [PATCH v6 2/9] x86/cpu: KVM: Move macro to encode PAT value to common header

[Huang, Kai]

>   void __init pat_bp_init(void)
>   {
>   	struct cpuinfo_x86 *c = &boot_cpu_data;
> -#define PAT(p0, p1, p2, p3, p4, p5, p6, p7)				\
> -	((X86_MEMTYPE_ ## p0)      | (X86_MEMTYPE_ ## p1 << 8)  |	\
> -	(X86_MEMTYPE_ ## p2 << 16) | (X86_MEMTYPE_ ## p3 << 24) |	\
> -	(X86_MEMTYPE_ ## p4 << 32) | (X86_MEMTYPE_ ## p5 << 40) |	\
> -	(X86_MEMTYPE_ ## p6 << 48) | (X86_MEMTYPE_ ## p7 << 56))
> -
>   
>   	if (!IS_ENABLED(CONFIG_X86_PAT))
>   		pr_info_once("x86/PAT: PAT support disabled because CONFIG_X86_PAT is disabled in the kernel.\n");
> @@ -281,7 +275,7 @@ void __init pat_bp_init(void)
>   		 * NOTE: When WC or WP is used, it is redirected to UC- per
>   		 * the default setup in __cachemode2pte_tbl[].
>   		 */
> -		pat_msr_val = PAT(WB, WT, UC_MINUS, UC, WB, WT, UC_MINUS, UC);
> +		pat_msr_val = PAT_VALUE(WB, WT, UC_MINUS, UC, WB, WT, UC_MINUS, UC);
>   	}
>   
>   	/*
> @@ -321,7 +315,7 @@ void __init pat_bp_init(void)
>   		 * NOTE: When WT or WP is used, it is redirected to UC- per
>   		 * the default setup in __cachemode2pte_tbl[].
>   		 */
> -		pat_msr_val = PAT(WB, WC, UC_MINUS, UC, WB, WC, UC_MINUS, UC);
> +		pat_msr_val = PAT_VALUE(WB, WC, UC_MINUS, UC, WB, WC, UC_MINUS, UC);
>   	} else {
>   		/*
>   		 * Full PAT support.  We put WT in slot 7 to improve
> @@ -349,7 +343,7 @@ void __init pat_bp_init(void)
>   		 * The reserved slots are unused, but mapped to their
>   		 * corresponding types in the presence of PAT errata.
>   		 */
> -		pat_msr_val = PAT(WB, WC, UC_MINUS, UC, WB, WP, UC_MINUS, WT);
> +		pat_msr_val = PAT_VALUE(WB, WC, UC_MINUS, UC, WB, WP, UC_MINUS, WT);
>   	}
>   
>   	memory_caching_control |= CACHE_PAT;

I found there's one "#undef PAT" at the end of pat_bp_init() and it 
should be removed:

void __init pat_bp_init(void)
{
         struct cpuinfo_x86 *c = &boot_cpu_data;
#define PAT(p0, p1, p2, p3, p4, p5, p6, p7)                     \
         (((u64)PAT_ ## p0) | ((u64)PAT_ ## p1 << 8) |           \
         ((u64)PAT_ ## p2 << 16) | ((u64)PAT_ ## p3 << 24) |     \
         ((u64)PAT_ ## p4 << 32) | ((u64)PAT_ ## p5 << 40) |     \
         ((u64)PAT_ ## p6 << 48) | ((u64)PAT_ ## p7 << 56))

	...

         memory_caching_control |= CACHE_PAT;

         init_cache_modes(pat_msr_val);
#undef PAT
}

Re: [PATCH v6 1/9] x86/cpu: KVM: Add common defines for architectural memory types (PAT, MTRRs, etc.)

[Huang, Kai]

On 4/04/2024 7:57 am, Sean Christopherson wrote:
> On Wed, Mar 27, 2024, Kai Huang wrote:
>> On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
>>> Add defines for the architectural memory types that can be shoved into
>>> various MSRs and registers, e.g. MTRRs, PAT, VMX capabilities MSRs, EPTPs,
>>> etc.  While most MSRs/registers support only a subset of all memory types,
>>> the values themselves are architectural and identical across all users.
>>>
>>> Leave the goofy MTRR_TYPE_* definitions as-is since they are in a uapi
>>> header, but add compile-time assertions to connect the dots (and sanity
>>> check that the msr-index.h values didn't get fat-fingered).
>>>
>>> Keep the VMX_EPTP_MT_* defines so that it's slightly more obvious that the
>>> EPTP holds a single memory type in 3 of its 64 bits; those bits just
>>> happen to be 2:0, i.e. don't need to be shifted.
>>>
>>> Opportunistically use X86_MEMTYPE_WB instead of an open coded '6' in
>>> setup_vmcs_config().
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>>
>>
>> [...]
>>
>>>   
>>>   #include "mtrr.h"
>>>   
>>> +static_assert(X86_MEMTYPE_UC == MTRR_TYPE_UNCACHABLE);
>>> +static_assert(X86_MEMTYPE_WC == MTRR_TYPE_WRCOMB);
>>> +static_assert(X86_MEMTYPE_WT == MTRR_TYPE_WRTHROUGH);
>>> +static_assert(X86_MEMTYPE_WP == MTRR_TYPE_WRPROT);
>>> +static_assert(X86_MEMTYPE_WB == MTRR_TYPE_WRBACK);
>>> +
>>>
>>
>> Hi Sean,
>>
>> IIUC, the purpose of this patch is for the kernel to use X86_MEMTYPE_xx, which
>> are architectural values, where applicable?
> 
> Maybe?  Probably?
> 
>> Yeah we need to keep MTRR_TYPE_xx in the uapi header, but in the kernel, should
>> we change all places that use MTRR_TYPE_xx to X86_MEMTYPE_xx?  The
>> static_assert()s above have guaranteed the two are the same, so there's nothing
>> wrong for the kernel to use X86_MEMTYPE_xx instead.
>>
>> Both PAT_xx and VMX_BASIC_MEM_TYPE_xx to X86_MEMTYPE_xx, it seems a little bit
>> odd if we don't switch for MTRR_TYPE_xx.
>>
>> However by simple search MEM_TYPE_xx are intensively used in many files, so...
> 
> Yeah, I definitely don't want to do it in this series due to the amount of churn
> that would be required.
> 
>    $ git grep MTRR_TYPE_ | wc -l
>    100
> 
> I'm not even entirely convinced that it would be a net positive.  Much of the KVM
> usage that's being cleaned up is flat out wrong, e.g. using "MTRR" enums in places
> that having nothing to do with MTRRs.  But the majority of the remaining usage is
> in MTRR code, i.e. isn't wrong, and is arguably better off using the MTRR specific
> #defines.

Yeah understood.

But the patch title says we also "add common defines for ... MTRRs", so 
to me looks we should get rid of MTRR_TPYE_xx and use the common ones 
instead.  And it also looks a little bit inconsistent if we remove the 
PAT_xx but keep the MTRR_TYPE_xx.

Perhaps we can keep PAT_xx but add macros?

   #define PAT_UC	X86_MEMTYPE_UC
   ...

But looks not nice either because the only purpose is to keep the PAT_xx..

So up to you :-)

Re: [PATCH v6 1/9] x86/cpu: KVM: Add common defines for architectural memory types (PAT, MTRRs, etc.)

[Sean Christopherson]

On Thu, Apr 04, 2024, Kai Huang wrote:
> On 4/04/2024 7:57 am, Sean Christopherson wrote:
> > On Wed, Mar 27, 2024, Kai Huang wrote:
> > > IIUC, the purpose of this patch is for the kernel to use X86_MEMTYPE_xx, which
> > > are architectural values, where applicable?
> > 
> > Maybe?  Probably?
> > 
> > > Yeah we need to keep MTRR_TYPE_xx in the uapi header, but in the kernel, should
> > > we change all places that use MTRR_TYPE_xx to X86_MEMTYPE_xx?  The
> > > static_assert()s above have guaranteed the two are the same, so there's nothing
> > > wrong for the kernel to use X86_MEMTYPE_xx instead.
> > > 
> > > Both PAT_xx and VMX_BASIC_MEM_TYPE_xx to X86_MEMTYPE_xx, it seems a little bit
> > > odd if we don't switch for MTRR_TYPE_xx.
> > > 
> > > However by simple search MEM_TYPE_xx are intensively used in many files, so...
> > 
> > Yeah, I definitely don't want to do it in this series due to the amount of churn
> > that would be required.
> > 
> >    $ git grep MTRR_TYPE_ | wc -l
> >    100
> > 
> > I'm not even entirely convinced that it would be a net positive.  Much of the KVM
> > usage that's being cleaned up is flat out wrong, e.g. using "MTRR" enums in places
> > that having nothing to do with MTRRs.  But the majority of the remaining usage is
> > in MTRR code, i.e. isn't wrong, and is arguably better off using the MTRR specific
> > #defines.
> 
> Yeah understood.
> 
> But the patch title says we also "add common defines for ... MTRRs", so to
> me looks we should get rid of MTRR_TPYE_xx and use the common ones instead.
> And it also looks a little bit inconsistent if we remove the PAT_xx but keep
> the MTRR_TYPE_xx.
> 
> Perhaps we can keep PAT_xx but add macros?
> 
>   #define PAT_UC	X86_MEMTYPE_UC
>   ...
> 
> But looks not nice either because the only purpose is to keep the PAT_xx..

Ya, keeping PAT_* is the only option I strongly object to.  I have no problem
replacing MTRR_TPYE_* usage, I would simply prefer not to do it in this series.
And I obviously have no problem leaving the MTRR stuff as-is.

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Sean Christopherson]

On Tue, Apr 02, 2024, Sean Christopherson wrote:
> On Mon, Apr 01, 2024, Xiaoyao Li wrote:
> > On 3/16/2024 1:54 AM, Sean Christopherson wrote:
> > > On Fri, Mar 15, 2024, Zhao Liu wrote:
> > > > On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote:
> > > > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
> > > > > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
> > > > > that the function looks like all the helpers that grab values from
> > > > > VMX_BASIC and VMX_MISC MSR values.
> > > 
> > > ...
> > > 
> > > > > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
> > > > >   #define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
> > > > >   #define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
> > > > >   #define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
> > > > > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> > > > >   static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
> > > > >   {
> > > > > -	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
> > > > > +	return vmx_misc & GENMASK_ULL(4, 0);
> > > > >   }
> > > > 
> > > > I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than
> > > > GENMASK_ULL(4, 0), and the former improves code readability.
> > > > 
> > > > May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK?
> > > 
> > > I don't necessarily disagree, but in this case I value consistency over one
> > > individual case.  As called out in the changelog, the motivation is to make
> > > vmx_misc_preemption_timer_rate() look like all the surrounding helpers.
> > > 
> > > _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(),
> > > vmx_misc_max_msr(), etc.
> > > 
> > > I don't have a super strong preference, though I think my vote would be to not
> > > add the masks and go with this patch.  These helpers are intended to be the _only_
> > > way to access the fields, i.e. they effectively _are_ the mask macros, just in
> > > function form.
> > > 
> > 
> > +1.
> > 
> > However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I
> > just recommended to define the MASK.
> > 
> > Because we already have
> > 
> > 	#define VMX_BASIC_MEM_TYPE_SHIFT	50
> > 
> > and it has been used in vmx/nested.c,
> > 
> > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> > {
> > 	return (vmx_basic & GENMASK_ULL(53, 50)) >>
> > 		VMX_BASIC_MEM_TYPE_SHIFT;
> > }
> > 
> > looks not intuitive than original patch.
> 
> Yeah, agreed, that's taking the worst of both worlds.  I'll update patch 5 to drop
> VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type().

Drat.  Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't
work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT,
which is presumably why past me kept them around.

I'm leaning towards keeping things as proposed in this series.  I don't see us
gaining a third copy, or even a third user, i.e. I don't think we are creating a
future problem by open coding the shift in vmx_basic_vmcs_mem_type().  And IMO
code like this

	return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >>
	       VMX_BASIC_MEM_TYPE_SHIFT;

is an unnecessary obfuscation when there is literally one user (the accessor).

Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT,
and either open code the values or use local const variables, but that also seems
like a net negative, e.g. splits the effective definitions over too many locations.

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Huang, Kai]

On Wed, 2024-04-24 at 13:06 -0700, Sean Christopherson wrote:
> On Tue, Apr 02, 2024, Sean Christopherson wrote:
> > On Mon, Apr 01, 2024, Xiaoyao Li wrote:
> > > On 3/16/2024 1:54 AM, Sean Christopherson wrote:
> > > > On Fri, Mar 15, 2024, Zhao Liu wrote:
> > > > > On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote:
> > > > > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
> > > > > > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
> > > > > > that the function looks like all the helpers that grab values from
> > > > > > VMX_BASIC and VMX_MISC MSR values.
> > > > 
> > > > ...
> > > > 
> > > > > > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
> > > > > >   #define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
> > > > > >   #define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
> > > > > >   #define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
> > > > > > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> > > > > >   static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
> > > > > >   {
> > > > > > -	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
> > > > > > +	return vmx_misc & GENMASK_ULL(4, 0);
> > > > > >   }
> > > > > 
> > > > > I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than
> > > > > GENMASK_ULL(4, 0), and the former improves code readability.
> > > > > 
> > > > > May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK?
> > > > 
> > > > I don't necessarily disagree, but in this case I value consistency over one
> > > > individual case.  As called out in the changelog, the motivation is to make
> > > > vmx_misc_preemption_timer_rate() look like all the surrounding helpers.
> > > > 
> > > > _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(),
> > > > vmx_misc_max_msr(), etc.
> > > > 
> > > > I don't have a super strong preference, though I think my vote would be to not
> > > > add the masks and go with this patch.  These helpers are intended to be the _only_
> > > > way to access the fields, i.e. they effectively _are_ the mask macros, just in
> > > > function form.
> > > > 
> > > 
> > > +1.
> > > 
> > > However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I
> > > just recommended to define the MASK.
> > > 
> > > Because we already have
> > > 
> > > 	#define VMX_BASIC_MEM_TYPE_SHIFT	50
> > > 
> > > and it has been used in vmx/nested.c,
> > > 
> > > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> > > {
> > > 	return (vmx_basic & GENMASK_ULL(53, 50)) >>
> > > 		VMX_BASIC_MEM_TYPE_SHIFT;
> > > }
> > > 
> > > looks not intuitive than original patch.
> > 
> > Yeah, agreed, that's taking the worst of both worlds.  I'll update patch 5 to drop
> > VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type().
> 
> Drat.  Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't
> work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT,
> which is presumably why past me kept them around.
> 
> I'm leaning towards keeping things as proposed in this series.  I don't see us
> gaining a third copy, or even a third user, i.e. I don't think we are creating a
> future problem by open coding the shift in vmx_basic_vmcs_mem_type().  And IMO
> code like this
> 
> 	return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >>
> 	       VMX_BASIC_MEM_TYPE_SHIFT;
> 
> is an unnecessary obfuscation when there is literally one user (the accessor).
> 
> Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT,
> and either open code the values or use local const variables, but that also seems
> like a net negative, e.g. splits the effective definitions over too many locations.

Alternatively, we can add macros like below to <asm/vmx.h> close to
vmx_basic_vmcs_size() etc, so it's straightforward to see.

+#define VMX_BSAIC_VMCS12_SIZE	((u64)VMCS12_SIZE << 32)
+#define VMX_BASIC_MEM_TYPE_WB	(MEM_TYPE_WB << 50)

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Xiaoyao Li]

On 4/25/2024 4:06 AM, Sean Christopherson wrote:
> On Tue, Apr 02, 2024, Sean Christopherson wrote:
>> On Mon, Apr 01, 2024, Xiaoyao Li wrote:
>>> On 3/16/2024 1:54 AM, Sean Christopherson wrote:
>>>> On Fri, Mar 15, 2024, Zhao Liu wrote:
>>>>> On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote:
>>>>>> Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(),
>>>>>> and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so
>>>>>> that the function looks like all the helpers that grab values from
>>>>>> VMX_BASIC and VMX_MISC MSR values.
>>>>
>>>> ...
>>>>
>>>>>> -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	GENMASK_ULL(4, 0)
>>>>>>    #define VMX_MISC_SAVE_EFER_LMA			BIT_ULL(5)
>>>>>>    #define VMX_MISC_ACTIVITY_HLT			BIT_ULL(6)
>>>>>>    #define VMX_MISC_ACTIVITY_SHUTDOWN		BIT_ULL(7)
>>>>>> @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
>>>>>>    static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc)
>>>>>>    {
>>>>>> -	return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK;
>>>>>> +	return vmx_misc & GENMASK_ULL(4, 0);
>>>>>>    }
>>>>>
>>>>> I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than
>>>>> GENMASK_ULL(4, 0), and the former improves code readability.
>>>>>
>>>>> May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK?
>>>>
>>>> I don't necessarily disagree, but in this case I value consistency over one
>>>> individual case.  As called out in the changelog, the motivation is to make
>>>> vmx_misc_preemption_timer_rate() look like all the surrounding helpers.
>>>>
>>>> _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(),
>>>> vmx_misc_max_msr(), etc.
>>>>
>>>> I don't have a super strong preference, though I think my vote would be to not
>>>> add the masks and go with this patch.  These helpers are intended to be the _only_
>>>> way to access the fields, i.e. they effectively _are_ the mask macros, just in
>>>> function form.
>>>>
>>>
>>> +1.
>>>
>>> However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I
>>> just recommended to define the MASK.
>>>
>>> Because we already have
>>>
>>> 	#define VMX_BASIC_MEM_TYPE_SHIFT	50
>>>
>>> and it has been used in vmx/nested.c,
>>>
>>> static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
>>> {
>>> 	return (vmx_basic & GENMASK_ULL(53, 50)) >>
>>> 		VMX_BASIC_MEM_TYPE_SHIFT;
>>> }
>>>
>>> looks not intuitive than original patch.
>>
>> Yeah, agreed, that's taking the worst of both worlds.  I'll update patch 5 to drop
>> VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type().
> 
> Drat.  Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't
> work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT,
> which is presumably why past me kept them around.
> 
> I'm leaning towards keeping things as proposed in this series.  

If it goes this way, I beg for a comment above the code to explain. 
Otherwise, people might send patch to use defined MARCO in the future.

> I don't see us
> gaining a third copy, or even a third user, i.e. I don't think we are creating a
> future problem by open coding the shift in vmx_basic_vmcs_mem_type().  And IMO
> code like this
> 
> 	return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >>
> 	       VMX_BASIC_MEM_TYPE_SHIFT;
> 
> is an unnecessary obfuscation when there is literally one user (the accessor).
> 
> Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT,
> and either open code the values or use local const variables, but that also seems
> like a net negative, e.g. splits the effective definitions over too many locations.

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Sean Christopherson]

On Thu, Apr 25, 2024, Kai Huang wrote:
> On Wed, 2024-04-24 at 13:06 -0700, Sean Christopherson wrote:
> > > > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> > > > {
> > > > 	return (vmx_basic & GENMASK_ULL(53, 50)) >>
> > > > 		VMX_BASIC_MEM_TYPE_SHIFT;
> > > > }
> > > > 
> > > > looks not intuitive than original patch.
> > > 
> > > Yeah, agreed, that's taking the worst of both worlds.  I'll update patch 5 to drop
> > > VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type().
> > 
> > Drat.  Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't
> > work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT,
> > which is presumably why past me kept them around.
> > 
> > I'm leaning towards keeping things as proposed in this series.  I don't see us
> > gaining a third copy, or even a third user, i.e. I don't think we are creating a
> > future problem by open coding the shift in vmx_basic_vmcs_mem_type().  And IMO
> > code like this
> > 
> > 	return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >>
> > 	       VMX_BASIC_MEM_TYPE_SHIFT;
> > 
> > is an unnecessary obfuscation when there is literally one user (the accessor).
> > 
> > Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT,
> > and either open code the values or use local const variables, but that also seems
> > like a net negative, e.g. splits the effective definitions over too many locations.
> 
> Alternatively, we can add macros like below to <asm/vmx.h> close to
> vmx_basic_vmcs_size() etc, so it's straightforward to see.
> 
> +#define VMX_BSAIC_VMCS12_SIZE	((u64)VMCS12_SIZE << 32)
> +#define VMX_BASIC_MEM_TYPE_WB	(MEM_TYPE_WB << 50)

Hmm, it's a bit hard to see it's specifically VMCS12 size, and given that prior
to this series, VMX_BASIC_MEM_TYPE_WB = 6, I'm hesitant to re-introduce/redefine
that macro with a different value.

What if we add a helper in vmx.h to encode the VMCS info?  Then the #defines for
the shifts can go away because the open coded shifts are colocated and more
obviously related.  E.g.

  static inline u64 vmx_basic_encode_vmcs_info(u32 revision, u16 size, u8 memtype)
  {
	return revision | ((u64)size << 32) | ((u64)memtype << 50);
  }


and

  static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
  {
	/*
	 * This MSR reports some information about VMX support. We
	 * should return information about the VMX we emulate for the
	 * guest, and the VMCS structure we give it - not about the
	 * VMX support of the underlying hardware.
	 */
	msrs->basic = vmx_basic_encode_vmcs_info(VMCS12_REVISION, VMCS12_SIZE,
						 X86_MEMTYPE_WB);

	msrs->basic |= VMX_BASIC_TRUE_CTLS
	if (cpu_has_vmx_basic_inout())
		msrs->basic |= VMX_BASIC_INOUT;
  }

Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor

[Huang, Kai]

On Thu, 2024-04-25 at 07:42 -0700, Sean Christopherson wrote:
> On Thu, Apr 25, 2024, Kai Huang wrote:
> > On Wed, 2024-04-24 at 13:06 -0700, Sean Christopherson wrote:
> > > > > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> > > > > {
> > > > > 	return (vmx_basic & GENMASK_ULL(53, 50)) >>
> > > > > 		VMX_BASIC_MEM_TYPE_SHIFT;
> > > > > }
> > > > > 
> > > > > looks not intuitive than original patch.
> > > > 
> > > > Yeah, agreed, that's taking the worst of both worlds.  I'll update patch 5 to drop
> > > > VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type().
> > > 
> > > Drat.  Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't
> > > work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT,
> > > which is presumably why past me kept them around.
> > > 
> > > I'm leaning towards keeping things as proposed in this series.  I don't see us
> > > gaining a third copy, or even a third user, i.e. I don't think we are creating a
> > > future problem by open coding the shift in vmx_basic_vmcs_mem_type().  And IMO
> > > code like this
> > > 
> > > 	return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >>
> > > 	       VMX_BASIC_MEM_TYPE_SHIFT;
> > > 
> > > is an unnecessary obfuscation when there is literally one user (the accessor).
> > > 
> > > Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT,
> > > and either open code the values or use local const variables, but that also seems
> > > like a net negative, e.g. splits the effective definitions over too many locations.
> > 
> > Alternatively, we can add macros like below to <asm/vmx.h> close to
> > vmx_basic_vmcs_size() etc, so it's straightforward to see.
> > 
> > +#define VMX_BSAIC_VMCS12_SIZE	((u64)VMCS12_SIZE << 32)
> > +#define VMX_BASIC_MEM_TYPE_WB	(MEM_TYPE_WB << 50)
> 
> Hmm, it's a bit hard to see it's specifically VMCS12 size, and given that prior
> to this series, VMX_BASIC_MEM_TYPE_WB = 6, I'm hesitant to re-introduce/redefine
> that macro with a different value.
> 
> What if we add a helper in vmx.h to encode the VMCS info?  Then the #defines for
> the shifts can go away because the open coded shifts are colocated and more
> obviously related.  E.g.
> 
>   static inline u64 vmx_basic_encode_vmcs_info(u32 revision, u16 size, u8 memtype)
>   {
> 	return revision | ((u64)size << 32) | ((u64)memtype << 50);
>   }
> 
> 
> and
> 
>   static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
>   {
> 	/*
> 	 * This MSR reports some information about VMX support. We
> 	 * should return information about the VMX we emulate for the
> 	 * guest, and the VMCS structure we give it - not about the
> 	 * VMX support of the underlying hardware.
> 	 */
> 	msrs->basic = vmx_basic_encode_vmcs_info(VMCS12_REVISION, VMCS12_SIZE,
> 						 X86_MEMTYPE_WB);
> 
> 	msrs->basic |= VMX_BASIC_TRUE_CTLS
> 	if (cpu_has_vmx_basic_inout())
> 		msrs->basic |= VMX_BASIC_INOUT;
>   }

Yeah this is better.  Thanks.