[v4,1/2] KVM: VMX: Cleanup VMX basic information defines and usages

Message ID 20240112093449.88583-1-xin3.li@intel.com
State New
Headers
Series [v4,1/2] KVM: VMX: Cleanup VMX basic information defines and usages |

Commit Message

Li, Xin3 Jan. 12, 2024, 9:34 a.m. UTC
  Define VMX basic information fields with BIT_ULL()/GENMASK_ULL(), and
replace hardcoded VMX basic numbers with these field macros.

Per Sean's ask, save the full/raw value of MSR_IA32_VMX_BASIC in the
global vmcs_config as type u64 to get rid of the hi/lo crud, and then
use VMX_BASIC helpers to extract info as needed.

Tested-by: Shan Kang <shan.kang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
---

Changes since v3:
* 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).

Changes since v2:
* 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.

Changes since v1:
* 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.
---
 arch/x86/include/asm/msr-index.h |  9 ---------
 arch/x86/include/asm/vmx.h       | 15 ++++++++++++--
 arch/x86/kvm/vmx/capabilities.h  |  6 ++----
 arch/x86/kvm/vmx/nested.c        | 34 +++++++++++++++++++++-----------
 arch/x86/kvm/vmx/vmx.c           | 24 ++++++++++------------
 5 files changed, 48 insertions(+), 40 deletions(-)


base-commit: 1c6d984f523f67ecfad1083bb04c55d91977bb15
  

Comments

Kai Huang Jan. 15, 2024, 11:46 a.m. UTC | #1
On Fri, 2024-01-12 at 01:34 -0800, Li, Xin3 wrote:
> Define VMX basic information fields with BIT_ULL()/GENMASK_ULL(), and
> replace hardcoded VMX basic numbers with these field macros.
> 
> Per Sean's ask, save the full/raw value of MSR_IA32_VMX_BASIC in the
> global vmcs_config as type u64 to get rid of the hi/lo crud, and then
> use VMX_BASIC helpers to extract info as needed.

For the sake of wanting a single 'u64 vmx_basic', feel free to add:

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

Although I still don't like splitting  ...

[...]


> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -120,6 +120,14 @@
>  
...

> +/* VMX_BASIC bits and bitmasks */
> +#define VMX_BASIC_32BIT_PHYS_ADDR_ONLY		BIT_ULL(48)
> +#define VMX_BASIC_INOUT				BIT_ULL(54)
> +
> 
...

> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1226,23 +1226,32 @@ static bool is_bitwise_subset(u64 superset, u64 subset, u64 mask)
>  	return (superset | subset) == superset;
>  }
>  
> +#define VMX_BASIC_DUAL_MONITOR_TREATMENT	BIT_ULL(49)
> +#define VMX_BASIC_TRUE_CTLS			BIT_ULL(55)

... these VMX_BASIC bit definitions across multiple files.

[...]


> +#define VMX_BASIC_MEM_TYPE_WB	(MEM_TYPE_WB << 50);

Also, please fix this one.
  
Li, Xin3 Jan. 16, 2024, 4:13 a.m. UTC | #2
> > Define VMX basic information fields with BIT_ULL()/GENMASK_ULL(), and
> > replace hardcoded VMX basic numbers with these field macros.
> >
> > Per Sean's ask, save the full/raw value of MSR_IA32_VMX_BASIC in the
> > global vmcs_config as type u64 to get rid of the hi/lo crud, and then
> > use VMX_BASIC helpers to extract info as needed.
> 
> For the sake of wanting a single 'u64 vmx_basic', feel free to add:
> 
> Acked-by: Kai Huang <kai.huang@intel.com>
> 
> Although I still don't like splitting  ...
> 
> [...]
> 
> 
> > --- a/arch/x86/include/asm/vmx.h
> > +++ b/arch/x86/include/asm/vmx.h
> > @@ -120,6 +120,14 @@
> >
> ...
> 
> > +/* VMX_BASIC bits and bitmasks */
> > +#define VMX_BASIC_32BIT_PHYS_ADDR_ONLY		BIT_ULL(48)
> > +#define VMX_BASIC_INOUT				BIT_ULL(54)
> > +
> >
> ...
> 
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -1226,23 +1226,32 @@ static bool is_bitwise_subset(u64 superset, u64
> subset, u64 mask)
> >  	return (superset | subset) == superset;  }
> >
> > +#define VMX_BASIC_DUAL_MONITOR_TREATMENT	BIT_ULL(49)
> > +#define VMX_BASIC_TRUE_CTLS			BIT_ULL(55)
> 
> ... these VMX_BASIC bit definitions across multiple files.

IIUC, Sean prefers to define a macro just above the function in which
it is only used:
https://lore.kernel.org/kvm/ZTBJO75Zu1JBsqvw@google.com/

> 
> [...]
> 
> 
> > +#define VMX_BASIC_MEM_TYPE_WB	(MEM_TYPE_WB << 50);
> 
> Also, please fix this one.

Bah!
  
Chao Gao Jan. 16, 2024, 7:46 a.m. UTC | #3
On Fri, Jan 12, 2024 at 01:34:48AM -0800, Xin Li wrote:
> 
>+#define VMX_BASIC_DUAL_MONITOR_TREATMENT	BIT_ULL(49)
>+#define VMX_BASIC_TRUE_CTLS			BIT_ULL(55)
>+
>+#define VMX_BASIC_FEATURES_MASK			\
>+	(VMX_BASIC_DUAL_MONITOR_TREATMENT |	\
>+	 VMX_BASIC_INOUT |			\
>+	 VMX_BASIC_TRUE_CTLS)
>+
>+#define VMX_BASIC_RESERVED_BITS			\
>+	(GENMASK_ULL(63, 56) | GENMASK_ULL(47, 45) | BIT_ULL(31))

When we add a new feature (e.g., in CET series, bit 56 is added), the above
two macros need to be modified.

Would it be better to use a macro for bits exempt from the bitwise check below
e.g.,

#define VMX_BASIC_MULTI_BITS_FEATURES_MASK

	(GENMASK_ULL(53, 50) | GENMASK_ULL(44, 32) | GENMASK_ULL(30, 0))

and do
	if (!is_bitwise_subset(vmx_basic, data,
			       ~VMX_BASIC_MULTI_BITS_FEATURES_MASK)

then we don't need to change the macro when adding new features.

>+
> 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);
> 	u64 vmx_basic = vmcs_config.nested.basic;
> 
>-	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
>+	static_assert(!(VMX_BASIC_FEATURES_MASK & VMX_BASIC_RESERVED_BITS));
>+
>+	if (!is_bitwise_subset(vmx_basic, data,
>+			       VMX_BASIC_FEATURES_MASK | VMX_BASIC_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;

Side topic:

Actually, there is no need to handle bit 48 as a special case. If we add bit 48
to VMX_BASIC_FEATURES_MASK, the bitwise check will fail if bit 48 of @data is 1.
  
Li, Xin3 Jan. 17, 2024, 4:34 p.m. UTC | #4
> >+#define VMX_BASIC_FEATURES_MASK			\
> >+	(VMX_BASIC_DUAL_MONITOR_TREATMENT |	\
> >+	 VMX_BASIC_INOUT |			\
> >+	 VMX_BASIC_TRUE_CTLS)
> >+
> >+#define VMX_BASIC_RESERVED_BITS			\
> >+	(GENMASK_ULL(63, 56) | GENMASK_ULL(47, 45) | BIT_ULL(31))
> 
> When we add a new feature (e.g., in CET series, bit 56 is added), the above
> two macros need to be modified.
> 
> Would it be better to use a macro for bits exempt from the bitwise check below
> e.g.,
> 
> #define VMX_BASIC_MULTI_BITS_FEATURES_MASK
> 
> 	(GENMASK_ULL(53, 50) | GENMASK_ULL(44, 32) | GENMASK_ULL(30, 0))
> 
> and do
> 	if (!is_bitwise_subset(vmx_basic, data,
> 			       ~VMX_BASIC_MULTI_BITS_FEATURES_MASK)
> 
> then we don't need to change the macro when adding new features.

Sounds a good idea to me, and just need to add comments about why.

> 
> >+
> > 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);
> > 	u64 vmx_basic = vmcs_config.nested.basic;
> >
> >-	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
> >+	static_assert(!(VMX_BASIC_FEATURES_MASK &
> VMX_BASIC_RESERVED_BITS));
> >+
> >+	if (!is_bitwise_subset(vmx_basic, data,
> >+			       VMX_BASIC_FEATURES_MASK |
> VMX_BASIC_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;
> 
> Side topic:
> 
> Actually, there is no need to handle bit 48 as a special case. If we add bit 48
> to VMX_BASIC_FEATURES_MASK, the bitwise check will fail if bit 48 of @data is 1.

Good point!  This is also what you suggested above.
  
Kai Huang Jan. 18, 2024, 3:08 a.m. UTC | #5
On Wed, 2024-01-17 at 16:34 +0000, Li, Xin3 wrote:
> > > +#define VMX_BASIC_FEATURES_MASK			\
> > > +	(VMX_BASIC_DUAL_MONITOR_TREATMENT |	\
> > > +	 VMX_BASIC_INOUT |			\
> > > +	 VMX_BASIC_TRUE_CTLS)
> > > +
> > > +#define VMX_BASIC_RESERVED_BITS			\
> > > +	(GENMASK_ULL(63, 56) | GENMASK_ULL(47, 45) | BIT_ULL(31))
> > 
> > When we add a new feature (e.g., in CET series, bit 56 is added), the above
> > two macros need to be modified.
> > 
> > Would it be better to use a macro for bits exempt from the bitwise check below
> > e.g.,
> > 
> > #define VMX_BASIC_MULTI_BITS_FEATURES_MASK
> > 
> > 	(GENMASK_ULL(53, 50) | GENMASK_ULL(44, 32) | GENMASK_ULL(30, 0))
> > 
> > and do
> > 	if (!is_bitwise_subset(vmx_basic, data,
> > 			       ~VMX_BASIC_MULTI_BITS_FEATURES_MASK)
> > 
> > then we don't need to change the macro when adding new features.
> 
> Sounds a good idea to me, and just need to add comments about why.
> 
> > 
> > > +
> > > 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);
> > > 	u64 vmx_basic = vmcs_config.nested.basic;
> > > 
> > > -	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
> > > +	static_assert(!(VMX_BASIC_FEATURES_MASK &
> > VMX_BASIC_RESERVED_BITS));
> > > +
> > > +	if (!is_bitwise_subset(vmx_basic, data,
> > > +			       VMX_BASIC_FEATURES_MASK |
> > VMX_BASIC_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;
> > 
> > Side topic:
> > 
> > Actually, there is no need to handle bit 48 as a special case. If we add bit 48
> > to VMX_BASIC_FEATURES_MASK, the bitwise check will fail if bit 48 of @data is 1.
> 
> Good point!  This is also what you suggested above.
> 

Please try to avoid mixing things together in one patch.  If you want to do
above, could you please do it in a separate patch so that can be reviewed
separately?

E.g., people who have reviewed or acked this patch may not be interested in the
new (logically separate) things.
  
Kai Huang Jan. 18, 2024, 3:13 a.m. UTC | #6
On Fri, 2024-01-12 at 01:34 -0800, Xin Li wrote:
> Define VMX basic information fields with BIT_ULL()/GENMASK_ULL(), and
> replace hardcoded VMX basic numbers with these field macros.
> 
> Per Sean's ask, save the full/raw value of MSR_IA32_VMX_BASIC in the
> global vmcs_config as type u64 to get rid of the hi/lo crud, and then
> use VMX_BASIC helpers to extract info as needed.
> 
> Tested-by: Shan Kang <shan.kang@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> ---
> 

[...]

> +/* x86 memory types, explicitly used in VMX only */
> +#define MEM_TYPE_WB				0x6ULL
> +#define MEM_TYPE_UC				0x0ULL
> +

[...]

> -#define VMX_EPTP_MT_WB				0x6ull
> -#define VMX_EPTP_MT_UC				0x0ull

Could you also put some words to the changelog to justify your change around
memory type macros?
  
Li, Xin3 Jan. 18, 2024, 5:15 a.m. UTC | #7
> > > > 	 * 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;
> > >
> > > Side topic:
> > >
> > > Actually, there is no need to handle bit 48 as a special case. If we
> > > add bit 48 to VMX_BASIC_FEATURES_MASK, the bitwise check will fail if bit 48
> of @data is 1.
> >
> > Good point!  This is also what you suggested above.
> >
> 
> Please try to avoid mixing things together in one patch.  If you want to do above,
> could you please do it in a separate patch so that can be reviewed separately?
> 
> E.g., people who have reviewed or acked this patch may not be interested in the
> new (logically separate) things.

I got to echo what Chao has suggested.

Yeah, if we are going to make this change in this patch set, it has to
be a new patch.  It can also be a separate patch.
  
Li, Xin3 Jan. 18, 2024, 5:16 a.m. UTC | #8
> On Fri, 2024-01-12 at 01:34 -0800, Xin Li wrote:
> > Define VMX basic information fields with BIT_ULL()/GENMASK_ULL(), and
> > replace hardcoded VMX basic numbers with these field macros.
> >
> > Per Sean's ask, save the full/raw value of MSR_IA32_VMX_BASIC in the
> > global vmcs_config as type u64 to get rid of the hi/lo crud, and then
> > use VMX_BASIC helpers to extract info as needed.
> >
> > Tested-by: Shan Kang <shan.kang@intel.com>
> > Signed-off-by: Xin Li <xin3.li@intel.com>
> > ---
> >
> 
> [...]
> 
> > +/* x86 memory types, explicitly used in VMX only */
> > +#define MEM_TYPE_WB				0x6ULL
> > +#define MEM_TYPE_UC				0x0ULL
> > +
> 
> [...]
> 
> > -#define VMX_EPTP_MT_WB				0x6ull
> > -#define VMX_EPTP_MT_UC				0x0ull
> 
> Could you also put some words to the changelog to justify your change around
> memory type macros?

NP.
  

Patch

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 1d51e1850ed0..9068cdd6f063 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1094,15 +1094,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_MEM_TYPE_WB	6LLU
-#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 0e73616b82f3..b8a745ff8c06 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -120,6 +120,14 @@ 
 
 #define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR	0x000011ff
 
+/* x86 memory types, explicitly used in VMX only */
+#define MEM_TYPE_WB				0x6ULL
+#define MEM_TYPE_UC				0x0ULL
+
+/* VMX_BASIC bits and bitmasks */
+#define VMX_BASIC_32BIT_PHYS_ADDR_ONLY		BIT_ULL(48)
+#define VMX_BASIC_INOUT				BIT_ULL(54)
+
 #define VMX_MISC_PREEMPTION_TIMER_RATE_MASK	0x0000001f
 #define VMX_MISC_SAVE_EFER_LMA			0x00000020
 #define VMX_MISC_ACTIVITY_HLT			0x00000040
@@ -143,6 +151,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;
@@ -505,8 +518,6 @@  enum vmcs_field {
 #define VMX_EPTP_PWL_5				0x20ull
 #define VMX_EPTP_AD_ENABLE_BIT			(1ull << 6)
 #define VMX_EPTP_MT_MASK			0x7ull
-#define VMX_EPTP_MT_WB				0x6ull
-#define VMX_EPTP_MT_UC				0x0ull
 #define VMX_EPT_READABLE_MASK			0x1ull
 #define VMX_EPT_WRITABLE_MASK			0x2ull
 #define VMX_EPT_EXECUTABLE_MASK			0x4ull
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/nested.c b/arch/x86/kvm/vmx/nested.c
index db0ad1e6ec4b..735f729a22f4 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1226,23 +1226,32 @@  static bool is_bitwise_subset(u64 superset, u64 subset, u64 mask)
 	return (superset | subset) == superset;
 }
 
+#define VMX_BASIC_DUAL_MONITOR_TREATMENT	BIT_ULL(49)
+#define VMX_BASIC_TRUE_CTLS			BIT_ULL(55)
+
+#define VMX_BASIC_FEATURES_MASK			\
+	(VMX_BASIC_DUAL_MONITOR_TREATMENT |	\
+	 VMX_BASIC_INOUT |			\
+	 VMX_BASIC_TRUE_CTLS)
+
+#define VMX_BASIC_RESERVED_BITS			\
+	(GENMASK_ULL(63, 56) | GENMASK_ULL(47, 45) | BIT_ULL(31))
+
 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);
 	u64 vmx_basic = vmcs_config.nested.basic;
 
-	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
+	static_assert(!(VMX_BASIC_FEATURES_MASK & VMX_BASIC_RESERVED_BITS));
+
+	if (!is_bitwise_subset(vmx_basic, data,
+			       VMX_BASIC_FEATURES_MASK | VMX_BASIC_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) !=
@@ -2726,11 +2735,11 @@  static bool nested_vmx_check_eptp(struct kvm_vcpu *vcpu, u64 new_eptp)
 
 	/* Check for memory type validity */
 	switch (new_eptp & VMX_EPTP_MT_MASK) {
-	case VMX_EPTP_MT_UC:
+	case MEM_TYPE_UC:
 		if (CC(!(vmx->nested.msrs.ept_caps & VMX_EPTP_UC_BIT)))
 			return false;
 		break;
-	case VMX_EPTP_MT_WB:
+	case MEM_TYPE_WB:
 		if (CC(!(vmx->nested.msrs.ept_caps & VMX_EPTP_WB_BIT)))
 			return false;
 		break;
@@ -6994,6 +7003,9 @@  static void nested_vmx_setup_misc_data(struct vmcs_config *vmcs_conf,
 	msrs->misc_high = 0;
 }
 
+#define VMX_BSAIC_VMCS12_SIZE	((u64)VMCS12_SIZE << 32)
+#define VMX_BASIC_MEM_TYPE_WB	(MEM_TYPE_WB << 50);
+
 static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
 {
 	/*
@@ -7005,8 +7017,8 @@  static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
 	msrs->basic =
 		VMCS12_REVISION |
 		VMX_BASIC_TRUE_CTLS |
-		((u64)VMCS12_SIZE << VMX_BASIC_VMCS_SIZE_SHIFT) |
-		(VMX_BASIC_MEM_TYPE_WB << VMX_BASIC_MEM_TYPE_SHIFT);
+		VMX_BSAIC_VMCS12_SIZE |
+		VMX_BASIC_MEM_TYPE_WB;
 
 	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 d21f55f323ea..25d261b7ed74 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2563,13 +2563,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;
 
@@ -2688,29 +2688,25 @@  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))
+	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) != 6)
+	if (vmx_basic_vmcs_mem_type(basic_msr) != MEM_TYPE_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;
@@ -2860,13 +2856,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;
@@ -2959,7 +2955,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;
 	}
@@ -3361,7 +3357,7 @@  static int vmx_get_max_ept_level(void)
 
 u64 construct_eptp(struct kvm_vcpu *vcpu, hpa_t root_hpa, int root_level)
 {
-	u64 eptp = VMX_EPTP_MT_WB;
+	u64 eptp = MEM_TYPE_WB;
 
 	eptp |= (root_level == 5) ? VMX_EPTP_PWL_5 : VMX_EPTP_PWL_4;