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

Previous thread: none

Next thread: Re: [PATCH 0 of 3] Low memory corruption detection and workaround by Hugh Dickins on Monday, September 8, 2008 - 3:52 am. (1 message)
From: Sheng Yang
Date: Monday, September 8, 2008 - 3:42 am

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

From: Sheng Yang
Date: Monday, September 8, 2008 - 3:42 am

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       ...
From: Avi Kivity
Date: Tuesday, September 9, 2008 - 6:47 am

Might as well move these two bitmask definitions.


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

--

From: Yang, Sheng
Date: Wednesday, September 10, 2008 - 3:44 am

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

From: Avi Kivity
Date: Wednesday, September 10, 2008 - 7:30 am

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

--

From: Yang, Sheng
Date: Wednesday, September 10, 2008 - 6:56 pm

I agree now, sorry for miss that... 

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

-- 
regards
Yang, Sheng
--

From: Sheng Yang
Date: Monday, September 8, 2008 - 3:42 am

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 ...
From: Andi Kleen
Date: Monday, September 8, 2008 - 7:04 am

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

From: Ingo Molnar
Date: Monday, September 8, 2008 - 7:09 am

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

From: H. Peter Anvin
Date: Monday, September 8, 2008 - 8:30 am

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

From: H. Peter Anvin
Date: Monday, September 8, 2008 - 10:17 am

Just to clarify:

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

	-hpa
--

From: Yang, Sheng
Date: Monday, September 8, 2008 - 8:32 pm

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

-- 
regards


--

Previous thread: none

Next thread: Re: [PATCH 0 of 3] Low memory corruption detection and workaround by Hugh Dickins on Monday, September 8, 2008 - 3:52 am. (1 message)