From: Xin Li <[email protected]>
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 <[email protected]>
Cc: Kai Huang <[email protected]>
Signed-off-by: Xin Li <[email protected]>
[sean: split to separate patch, write changelog]
Signed-off-by: Sean Christopherson <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
>
> 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 <[email protected]>
> Cc: Kai Huang <[email protected]>
> Signed-off-by: Xin Li <[email protected]>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> 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 <[email protected]>
On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
> From: Xin Li <[email protected]>
>
> 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 <[email protected]>
> Cc: Kai Huang <[email protected]>
> Signed-off-by: Xin Li <[email protected]>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <[email protected]>
>
Reviewed-by: Kai Huang <[email protected]>
On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> From: Xin Li <[email protected]>
>
> 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 <[email protected]>
> Cc: Kai Huang <[email protected]>
> Signed-off-by: Xin Li <[email protected]>
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> 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);
> On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> > From: Xin Li <[email protected]>
> >
> > 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
On Tue, Apr 02, 2024, Xin3 Li wrote:
> > On 3/9/2024 9:27 AM, Sean Christopherson wrote:
> > > From: Xin Li <[email protected]>
> > >
> > > 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.