[RFC][PATCH 0/2] x86: Add "virt flags"

[RFC][PATCH 0/2] x86: Add "virt flags"

[Sheng Yang]

Hi, Ingo

(sorry for former noises, I mistake the address... Report to lkml)
I've sent this patchset before, but got no comments from upstream at that
time.  So I'd like to resend this.

The virt flags is used for the important hardware virtualization features,
like EPT of incoming Nehalem. Because the feature availability are read from
MSRs, and I think virtualization features should not at the same level as
"vmx", so I added a new flags catagory here.

But I still have concern, for this may broke some not that reliable userspace
programs. So Avi suggested that we can add more fields to flags rather than a
new catagory.  What's your opinion? We indeed need a generic user visible way
to tell the HW virtualization features.

Thanks!
--
regards
Yang, Sheng

[PATCH 1/2] x86: Move VMX MSRs to msr-index.h

[Sheng Yang]

They are hardware specific MSRs, and we would use them in virtualization
feature detection later.

Signed-off-by: Sheng Yang <sheng.yang@intel.com>
---
 arch/x86/kvm/vmx.h          |   15 ---------------
 include/asm-x86/msr-index.h |   16 ++++++++++++++++
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/vmx.h b/arch/x86/kvm/vmx.h
index 425a134..b32d4e5 100644
--- a/arch/x86/kvm/vmx.h
+++ b/arch/x86/kvm/vmx.h
@@ -331,21 +331,6 @@ enum vmcs_field {
 
 #define AR_RESERVD_MASK 0xfffe0f00
 
-#define MSR_IA32_VMX_BASIC                      0x480
-#define MSR_IA32_VMX_PINBASED_CTLS              0x481
-#define MSR_IA32_VMX_PROCBASED_CTLS             0x482
-#define MSR_IA32_VMX_EXIT_CTLS                  0x483
-#define MSR_IA32_VMX_ENTRY_CTLS                 0x484
-#define MSR_IA32_VMX_MISC                       0x485
-#define MSR_IA32_VMX_CR0_FIXED0                 0x486
-#define MSR_IA32_VMX_CR0_FIXED1                 0x487
-#define MSR_IA32_VMX_CR4_FIXED0                 0x488
-#define MSR_IA32_VMX_CR4_FIXED1                 0x489
-#define MSR_IA32_VMX_VMCS_ENUM                  0x48a
-#define MSR_IA32_VMX_PROCBASED_CTLS2            0x48b
-#define MSR_IA32_VMX_EPT_VPID_CAP               0x48c
-
-#define MSR_IA32_FEATURE_CONTROL                0x3a
 #define MSR_IA32_FEATURE_CONTROL_LOCKED         0x1
 #define MSR_IA32_FEATURE_CONTROL_VMXON_ENABLED  0x4
 
diff --git a/include/asm-x86/msr-index.h b/include/asm-x86/msr-index.h
index 3052f05..0bb4330 100644
--- a/include/asm-x86/msr-index.h
+++ b/include/asm-x86/msr-index.h
@@ -176,6 +176,7 @@
 #define MSR_IA32_TSC			0x00000010
 #define MSR_IA32_PLATFORM_ID		0x00000017
 #define MSR_IA32_EBL_CR_POWERON		0x0000002a
+#define MSR_IA32_FEATURE_CONTROL        0x0000003a
 
 #define MSR_IA32_APICBASE		0x0000001b
 #define MSR_IA32_APICBASE_BSP		(1<<8)
@@ -310,4 +311,19 @@
 /* Geode defined MSRs */
 #define MSR_GEODE_BUSCONT_CONF0		0x00001900
 
+/* Intel VT MSRs */
+#define MSR_IA32_VMX_BASIC              0x00000480
+#define MSR_IA32_VMX_PINBASED_CTLS      0x00000481
+#define MSR_IA32_VMX_PROCBASED_CTLS     0x00000482
+#define MSR_IA32_VMX_EXIT_CTLS          0x00000483
+#define MSR_IA32_VMX_ENTRY_CTLS         0x00000484
+#define MSR_IA32_VMX_MISC               0x00000485
+#define MSR_IA32_VMX_CR0_FIXED0         0x00000486
+#define MSR_IA32_VMX_CR0_FIXED1         0x00000487
+#define MSR_IA32_VMX_CR4_FIXED0         0x00000488
+#define MSR_IA32_VMX_CR4_FIXED1         0x00000489
+#define MSR_IA32_VMX_VMCS_ENUM          0x0000048a
+#define MSR_IA32_VMX_PROCBASED_CTLS2    0x0000048b
+#define MSR_IA32_VMX_EPT_VPID_CAP       0x0000048c
+
 #endif /* ASM_X86__MSR_INDEX_H */
-- 
1.5.4.5

Re: [PATCH 1/2] x86: Move VMX MSRs to msr-index.h

[Avi Kivity]

Sheng Yang wrote:
> They are hardware specific MSRs, and we would use them in virtualization
> feature detection later.
>
>   


> -
> -#define MSR_IA32_FEATURE_CONTROL                0x3a
>  #define MSR_IA32_FEATURE_CONTROL_LOCKED         0x1
>  #define MSR_IA32_FEATURE_CONTROL_VMXON_ENABLED  0x

Might as well move these two bitmask definitions.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] x86: Move VMX MSRs to msr-index.h

[Yang, Sheng]

On Tuesday 09 September 2008 21:47:03 Avi Kivity wrote:
> Sheng Yang wrote:
> > They are hardware specific MSRs, and we would use them in virtualization
> > feature detection later.
> >
> >
> >
> >
> > -
> > -#define MSR_IA32_FEATURE_CONTROL                0x3a
> >  #define MSR_IA32_FEATURE_CONTROL_LOCKED         0x1
> >  #define MSR_IA32_FEATURE_CONTROL_VMXON_ENABLED  0x
>
> Might as well move these two bitmask definitions.

Um... I think it's better to leave them here, for the MSR_IA32_FEATURE_CONTROL 
is a MSR which can be put into msr-index.h, but the others are only bits of 
MSR...

-- 
regards
Yang, Sheng

Re: [PATCH 1/2] x86: Move VMX MSRs to msr-index.h

[Avi Kivity]

Yang, Sheng wrote:
> On Tuesday 09 September 2008 21:47:03 Avi Kivity wrote:
>   
>> Sheng Yang wrote:
>>     
>>> They are hardware specific MSRs, and we would use them in virtualization
>>> feature detection later.
>>>
>>>
>>>
>>>
>>> -
>>> -#define MSR_IA32_FEATURE_CONTROL                0x3a
>>>  #define MSR_IA32_FEATURE_CONTROL_LOCKED         0x1
>>>  #define MSR_IA32_FEATURE_CONTROL_VMXON_ENABLED  0x
>>>       
>> Might as well move these two bitmask definitions.
>>     
>
> Um... I think it's better to leave them here, for the MSR_IA32_FEATURE_CONTROL 
> is a MSR which can be put into msr-index.h, but the others are only bits of 
> MSR...
>   

The EFER bits are in msr-index.h, and I think the msr index in one file 
and the bits in another detract from clarity.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] x86: Move VMX MSRs to msr-index.h

[Yang, Sheng]

On Wednesday 10 September 2008 22:30:15 Avi Kivity wrote:
> Yang, Sheng wrote:
> > On Tuesday 09 September 2008 21:47:03 Avi Kivity wrote:
> >> Sheng Yang wrote:
> >>> They are hardware specific MSRs, and we would use them in
> >>> virtualization feature detection later.
> >>>
> >>>
> >>>
> >>>
> >>> -
> >>> -#define MSR_IA32_FEATURE_CONTROL                0x3a
> >>>  #define MSR_IA32_FEATURE_CONTROL_LOCKED         0x1
> >>>  #define MSR_IA32_FEATURE_CONTROL_VMXON_ENABLED  0x
> >>
> >> Might as well move these two bitmask definitions.
> >
> > Um... I think it's better to leave them here, for the
> > MSR_IA32_FEATURE_CONTROL is a MSR which can be put into msr-index.h, but
> > the others are only bits of MSR...
>
> The EFER bits are in msr-index.h, and I think the msr index in one file
> and the bits in another detract from clarity.

I agree now, sorry for miss that... 

Would post another patch(es) to do this clean up...

-- 
regards
Yang, Sheng

[PATCH 2/2] x86: Add "virt flags" in /proc/cpuinfo

[Sheng Yang]

The hardware virtualization technology evolves very fast. But currently
it's hard to tell if your CPU support a certain kind of HW technology
without digging into the source code.

The patch add a new item under /proc/cpuinfo, named "virt flags". The "virt
flags" got the similar purpose as "flags". It is used to indicate what
HW virtulization features does this CPU supported, and it don't cover all
features but only the important ones.

Current implementation just cover Intel VMX side.

Signed-off-by: Sheng Yang <sheng.yang@intel.com>
---
 arch/x86/kernel/cpu/proc.c   |   28 ++++++++++++++++++++++++++++
 include/asm-x86/cpufeature.h |    8 ++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index a26c480..737a1bd 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -77,6 +77,31 @@ static void show_cpuinfo_misc(struct seq_file *m, struct cpuinfo_x86 *c)
 }
 #endif
 
+static void show_cpuinfo_vmx_virtflags(struct seq_file *m)
+{
+	u32 vmx_msr_low, vmx_msr_high, msr_ctl, msr_ctl2;
+
+	seq_printf(m, "\nvirt flags\t:");
+	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS, vmx_msr_low, vmx_msr_high);
+	msr_ctl = vmx_msr_high | vmx_msr_low;
+	if (msr_ctl & X86_VMX_FEATURE_PROC_CTLS_TPR_SHADOW)
+		seq_printf(m, " tpr_shadow");
+	if (msr_ctl & X86_VMX_FEATURE_PROC_CTLS_VNMI)
+		seq_printf(m, " vnmi");
+	if (msr_ctl & X86_VMX_FEATURE_PROC_CTLS_2ND_CTLS) {
+		rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2,
+		      vmx_msr_low, vmx_msr_high);
+		msr_ctl2 = vmx_msr_high | vmx_msr_low;
+		if ((msr_ctl2 & X86_VMX_FEATURE_PROC_CTLS2_VIRT_APIC) &&
+		    (msr_ctl & X86_VMX_FEATURE_PROC_CTLS_TPR_SHADOW))
+			seq_printf(m, " flexpriority");
+		if (msr_ctl2 & X86_VMX_FEATURE_PROC_CTLS2_EPT)
+			seq_printf(m, " ept");
+		if (msr_ctl2 & X86_VMX_FEATURE_PROC_CTLS2_VPID)
+			seq_printf(m, " vpid");
+	}
+}
+
 static int show_cpuinfo(struct seq_file *m, void *v)
 {
 	struct cpuinfo_x86 *c = v;
@@ -123,6 +148,9 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 		if (cpu_has(c, i) && x86_cap_flags[i] != NULL)
 			seq_printf(m, " %s", x86_cap_flags[i]);
 
+	if (cpu_has(c, X86_FEATURE_VMX))
+		show_cpuinfo_vmx_virtflags(m);
+
 	seq_printf(m, "\nbogomips\t: %lu.%02lu\n",
 		   c->loops_per_jiffy/(500000/HZ),
 		   (c->loops_per_jiffy/(5000/HZ)) % 100);
diff --git a/include/asm-x86/cpufeature.h b/include/asm-x86/cpufeature.h
index 7ac4d93..b5e4aef 100644
--- a/include/asm-x86/cpufeature.h
+++ b/include/asm-x86/cpufeature.h
@@ -150,6 +150,14 @@
  */
 #define X86_FEATURE_IDA		(7*32+ 0) /* Intel Dynamic Acceleration */
 
+/* Intel VMX MSR indicated features */
+#define X86_VMX_FEATURE_PROC_CTLS_TPR_SHADOW	0x00200000
+#define X86_VMX_FEATURE_PROC_CTLS_VNMI		0x00400000
+#define X86_VMX_FEATURE_PROC_CTLS_2ND_CTLS	0x80000000
+#define X86_VMX_FEATURE_PROC_CTLS2_VIRT_APIC	0x00000001
+#define X86_VMX_FEATURE_PROC_CTLS2_EPT		0x00000002
+#define X86_VMX_FEATURE_PROC_CTLS2_VPID		0x00000020
+
 #if defined(__KERNEL__) && !defined(__ASSEMBLY__)
 
 #include <linux/bitops.h>
-- 
1.5.4.5

Re: [RFC][PATCH 0/2] x86: Add "virt flags"

[Andi Kleen]

Sheng Yang <sheng.yang@intel.com> writes:
>
> But I still have concern, for this may broke some not that reliable userspace
> programs. So Avi suggested that we can add more fields to flags rather than a
> new catagory.


FWIW when I added the "power management:" flags back then for the
64bit kernel I didn't get any reports of user programs breaking. Also
some other fields got added like cpu cores or initial apicid etc. and
that also didn't lead to breakage. So adding new lines to cpuinfo
should be ok.

-Andi
-- 
ak@linux.intel.com

Re: [RFC][PATCH 0/2] x86: Add "virt flags"

[Ingo Molnar]

* Sheng Yang <sheng.yang@intel.com> wrote:

> Hi, Ingo
> 
> (sorry for former noises, I mistake the address... Report to lkml) 
> I've sent this patchset before, but got no comments from upstream at 
> that time.  So I'd like to resend this.
> 
> The virt flags is used for the important hardware virtualization 
> features, like EPT of incoming Nehalem. Because the feature 
> availability are read from MSRs, and I think virtualization features 
> should not at the same level as "vmx", so I added a new flags catagory 
> here.
> 
> But I still have concern, for this may broke some not that reliable 
> userspace programs. So Avi suggested that we can add more fields to 
> flags rather than a new catagory.  What's your opinion? We indeed need 
> a generic user visible way to tell the HW virtualization features.

hm, i think extending the already existing flags category sounds like a 
better solution than the separate virtual CPU flags line in 
/proc/cpuinfo. We already have self-invented flag entries (such as 
X86_FEATURE_NOPL), and adding more for virtualization would be quite 
natural to do, as long as it's reasonably close to the meaning of a 'CPU 
feature'.

Peter, what would be your preference?

	Ingo

Re: [RFC][PATCH 0/2] x86: Add "virt flags"

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> hm, i think extending the already existing flags category sounds like a 
> better solution than the separate virtual CPU flags line in 
> /proc/cpuinfo. We already have self-invented flag entries (such as 
> X86_FEATURE_NOPL), and adding more for virtualization would be quite 
> natural to do, as long as it's reasonably close to the meaning of a 'CPU 
> feature'.
> 
> Peter, what would be your preference?
> 

It probably makes sense to separate these out as a separate word, 
especially if they come from the hardware in any reasonable way.  But 
yes, adding them to the feature array makes sense.

	-hpa

Re: [RFC][PATCH 0/2] x86: Add "virt flags"

[H. Peter Anvin]

H. Peter Anvin wrote:
> Ingo Molnar wrote:
>>
>> hm, i think extending the already existing flags category sounds like 
>> a better solution than the separate virtual CPU flags line in 
>> /proc/cpuinfo. We already have self-invented flag entries (such as 
>> X86_FEATURE_NOPL), and adding more for virtualization would be quite 
>> natural to do, as long as it's reasonably close to the meaning of a 
>> 'CPU feature'.
>>
>> Peter, what would be your preference?
>>
> 
> It probably makes sense to separate these out as a separate word, 
> especially if they come from the hardware in any reasonable way.  But 
> yes, adding them to the feature array makes sense.
> 

Just to clarify:

I'm suggesting adding these to the existing feature flags array, in a 
separate binary word.

	-hpa

Re: [RFC][PATCH 0/2] x86: Add "virt flags"

[Yang, Sheng]

On Tuesday 09 September 2008 01:17:02 H. Peter Anvin wrote:
> H. Peter Anvin wrote:
> > Ingo Molnar wrote:
> >> hm, i think extending the already existing flags category sounds like
> >> a better solution than the separate virtual CPU flags line in
> >> /proc/cpuinfo. We already have self-invented flag entries (such as
> >> X86_FEATURE_NOPL), and adding more for virtualization would be quite
> >> natural to do, as long as it's reasonably close to the meaning of a
> >> 'CPU feature'.
> >>
> >> Peter, what would be your preference?
> >
> > It probably makes sense to separate these out as a separate word,
> > especially if they come from the hardware in any reasonable way.  But
> > yes, adding them to the feature array makes sense.
>
> Just to clarify:
>
> I'm suggesting adding these to the existing feature flags array, in a
> separate binary word.
>

Thanks! I will update the patch to add another category, and merge the array.  

-- 
regards
Yang, Sheng
> 	-hpa