diff mbox series

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

Message ID 20240309012725.1409949-6-seanjc@google.com (mailing list archive)
State New, archived
Headers show
Series x86/cpu: KVM: Clean up PAT and VMX macros | expand

Commit Message

Sean Christopherson March 9, 2024, 1:27 a.m. UTC 
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(-)

Comments

Zhao Liu March 15, 2024, 3:26 p.m. UTC | #1 
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
Huang, Kai March 27, 2024, 10:38 a.m. UTC | #2 
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>
Xiaoyao Li April 1, 2024, 6:27 a.m. UTC | #3 
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>
diff mbox series

Patch

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;
 	}