[PATCH] Proper use of VMX execution controls MSR.

[PATCH] Proper use of VMX execution controls MSR.

[Li, Xin B]

[-- Attachment #1: Type: text/plain, Size: 88 bytes --]

Better use of VMX execution controls MSR.

Signed-off-by: Xin Li<xin.b.li@intel.com>

[-- Attachment #2: vmx_ctrls_adjust.patch --]
[-- Type: application/octet-stream, Size: 695 bytes --]

diff -r 7e6ef2b914aa xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c	Wed Mar 28 12:38:13 2007 +0100
+++ b/xen/arch/x86/hvm/vmx/vmcs.c	Wed Mar 28 22:29:17 2007 +0800
@@ -86,10 +86,13 @@ static u32 adjust_vmx_controls(u32 ctrls
 
     rdmsr(msr, vmx_msr_low, vmx_msr_high);
 
-    /* Bit == 0 means must be zero. */
-    BUG_ON(ctrls & ~vmx_msr_high);
-
-    /* Bit == 1 means must be one. */
+    /* bits setting we should respect */
+    ctrls &= vmx_msr_low ^ vmx_msr_high;
+
+    /* Bit == 0 in high bits of the MSR means must be zero. */
+    ctrls &= vmx_msr_high;
+
+    /* Bit == 1 in low bits of the MSR means must be one. */
     ctrls |= vmx_msr_low;
 
     return ctrls;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Proper use of VMX execution controls MSR.

[Keir Fraser]

On 28/3/07 16:51, "Li, Xin B" <xin.b.li@intel.com> wrote:

> Better use of VMX execution controls MSR.
> 
> Signed-off-by: Xin Li<xin.b.li@intel.com>

Is this actually to fix a problem with a future processor?

This whole bit-forcing thing seems extremely odd to me. We set the controls
that Xen currently needs to do its job as a VMM properly -- we can't just
clear some of those controls because the processor says to do so. So I think
our current treatment of the MSR high bits is appropriate (if it tells us to
zero one of the control bits that we make use of, we are in trouble -- we
have a processor that isn't backwards compatible!).

I also feel uneasy about setting extra bits (as specified by the MSR low
bits), but I reason that if we are told to set bits of flags which are
currently architecturally-undefined then it is reasonable to let the
processor tell us what to do with them. Which is why I do respect the MSR
low bits.

Why did Intel ever choose this insane scheme? Would it have been so hard to
have defined bitmaps with set-to-enable semantics, and always require zero
for reserved bits? Actually I suppose you do have set-to-enable semantics
(otherwise my current asymmetric treatment of MSR high and low words would
not make sense). But all this messing with setting vs. clearing reserved
bits seems pretty stupid.

 -- Keir

RE: [PATCH] Proper use of VMX execution controls MSR.

[Cui, Dexuan]

The patch conforms to the spec strictly, but I also think the spec is really odd...
Seems Keir hasn't checked it in.

 -- Dexuan

>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
>Sent: 2007年3月29日 2:02
>To: Li, Xin B; xen-devel@lists.xensource.com
>Subject: Re: [Xen-devel] [PATCH] Proper use of VMX execution controls MSR.
>
>On 28/3/07 16:51, "Li, Xin B" <xin.b.li@intel.com> wrote:
>
>> Better use of VMX execution controls MSR.
>>
>> Signed-off-by: Xin Li<xin.b.li@intel.com>
>
>Is this actually to fix a problem with a future processor?
>
>This whole bit-forcing thing seems extremely odd to me. We set the controls
>that Xen currently needs to do its job as a VMM properly -- we can't just
>clear some of those controls because the processor says to do so. So I think
>our current treatment of the MSR high bits is appropriate (if it tells us to
>zero one of the control bits that we make use of, we are in trouble -- we
>have a processor that isn't backwards compatible!).
>
>I also feel uneasy about setting extra bits (as specified by the MSR low
>bits), but I reason that if we are told to set bits of flags which are
>currently architecturally-undefined then it is reasonable to let the
>processor tell us what to do with them. Which is why I do respect the MSR
>low bits.
>
>Why did Intel ever choose this insane scheme? Would it have been so hard to
>have defined bitmaps with set-to-enable semantics, and always require zero
>for reserved bits? Actually I suppose you do have set-to-enable semantics
>(otherwise my current asymmetric treatment of MSR high and low words would
>not make sense). But all this messing with setting vs. clearing reserved
>bits seems pretty stupid.
>
> -- Keir
>
>
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel

RE: [PATCH] Proper use of VMX execution controls MSR.

[Nakajima, Jun]

Keir Fraser wrote:
> On 28/3/07 16:51, "Li, Xin B" <xin.b.li@intel.com> wrote:
> 
>> Better use of VMX execution controls MSR.
>> 
>> Signed-off-by: Xin Li<xin.b.li@intel.com>
> 
> Is this actually to fix a problem with a future processor?
> 
> This whole bit-forcing thing seems extremely odd to me. We set the
> controls that Xen currently needs to do its job as a VMM properly --
> we can't just clear some of those controls because the processor says
> to do so. So I think our current treatment of the MSR high bits is
> appropriate (if it tells us to zero one of the control bits that we
> make use of, we are in trouble -- we have a processor that isn't
> backwards compatible!). 
> 

Right. BUG_ON() is correct because the processor does not meet the
programmer's assumption. 

> I also feel uneasy about setting extra bits (as specified by the MSR
> low bits), but I reason that if we are told to set bits of flags
> which are currently architecturally-undefined then it is reasonable
> to let the processor tell us what to do with them. Which is why I do
> respect the MSR low bits.
> 

This is okay because newer processors simply provide more settings, i.e
1 => 0 or 1. The code usually is written with the setting = 1. Some VMM
may use the setting 0 for new processors if it can benefit from that.

> 
>  -- Keir

Jun
---
Intel Open Source Technology Center

RE: [PATCH] Proper use of VMX execution controls MSR.

[Li, Xin B]

[-- Attachment #1: Type: text/plain, Size: 2700 bytes --]

>On 28/3/07 16:51, "Li, Xin B" <xin.b.li@intel.com> wrote:
>
>> Better use of VMX execution controls MSR.
>> 
>> Signed-off-by: Xin Li<xin.b.li@intel.com>
>
>Is this actually to fix a problem with a future processor?

No, this is just for better coding for hardware features that may not be
available
on all processors.

>This whole bit-forcing thing seems extremely odd to me. We set the
controls
>that Xen currently needs to do its job as a VMM properly -- we can't
just
>clear some of those controls because the processor says to do so. So I
think
>our current treatment of the MSR high bits is appropriate (if it tells
us to
>zero one of the control bits that we make use of, we are in trouble --
we
>have a processor that isn't backwards compatible!).

I also think its appropriate.

>I also feel uneasy about setting extra bits (as specified by the MSR
low bits),
>but I reason that if we are told to set bits of flags which are
currently 
>architecturally-undefined then it is reasonable to let the processor
tell us what
>to do with them. Which is why I do respect the MSR low bits.

Yes, we must repect MSR low bits.

>
>Why did Intel ever choose this insane scheme? Would it have been so
hard to
>have defined bitmaps with set-to-enable semantics, and always require
zero
>for reserved bits? Actually I suppose you do have set-to-enable
semantics
>(otherwise my current asymmetric treatment of MSR high and low words
would
>not make sense). But all this messing with setting vs. clearing
reserved
>bits seems pretty stupid.

Yeah, I think the basic logic is still set-to-enable semantics here.

I made this patch when we add VMX MSR bitmap support, which is used to
avoid 
FS_BASE and GS_BASE MSR accesses VMExits in 64 bit guests. But some
early
precessors may not support this feature, so the MSR reports 0 in high
bits, meaning we
must set the bit to zero in VMCS.  So a straightforward way is to check
if the feature
is availale and then set to VMCS accordingly. But we have lots of
hardware features,
with new features being added to hareware, and I really don't like to
add the almost
same code just for another new feature detection.  The idea here is to
use the control MSR
to adjust the input controls we _hardcoded_ in Xen, for the features
we've supported in Xen,
we always set to 1 in the input control, like in
MONITOR_CPU_BASED_EXEC_CONTROLS,
then on processors supporting this feature, We get 1, while on
processors not supporting this
feature, we get 0.  This adjusted output control value can be also used
to choose code
path on different processors, and the attached patch can be an example.
-Xin

[-- Attachment #2: vmx_msr_bitmap.patch --]
[-- Type: application/octet-stream, Size: 8207 bytes --]

diff -r 78af882eb6fc linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/msr.h
--- a/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/msr.h	Wed Mar 28 20:19:11 2007 +0100
+++ b/linux-2.6-xen-sparse/include/asm-x86_64/mach-xen/asm/msr.h	Thu Mar 29 13:15:54 2007 +0800
@@ -158,8 +158,8 @@ static inline unsigned int cpuid_edx(uns
 #define MSR_LSTAR 0xc0000082 		/* long mode SYSCALL target */
 #define MSR_CSTAR 0xc0000083		/* compatibility mode SYSCALL target */
 #define MSR_SYSCALL_MASK 0xc0000084	/* EFLAGS mask for syscall */
-#define MSR_FS_BASE 0xc0000100		/* 64bit GS base */
-#define MSR_GS_BASE 0xc0000101		/* 64bit FS base */
+#define MSR_FS_BASE 0xc0000100		/* 64bit FS base */
+#define MSR_GS_BASE 0xc0000101		/* 64bit GS base */
 #define MSR_KERNEL_GS_BASE  0xc0000102	/* SwapGS GS shadow (or USER_GS from kernel) */ 
 /* EFER bits: */ 
 #define _EFER_SCE 0  /* SYSCALL/SYSRET */
diff -r 78af882eb6fc xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Wed Mar 28 20:19:11 2007 +0100
+++ b/xen/arch/x86/hvm/hvm.c	Thu Mar 29 13:15:54 2007 +0800
@@ -59,6 +59,9 @@ struct hvm_function_table hvm_funcs __re
 /* I/O permission bitmap is globally shared by all HVM guests. */
 char __attribute__ ((__section__ (".bss.page_aligned")))
     hvm_io_bitmap[3*PAGE_SIZE];
+/* msr bitmap */
+char __attribute__ ((__section__ (".bss.page_aligned")))
+    hvm_msr_bitmap[PAGE_SIZE];
 
 void hvm_enable(struct hvm_function_table *fns)
 {
@@ -71,6 +74,9 @@ void hvm_enable(struct hvm_function_tabl
      */
     memset(hvm_io_bitmap, ~0, sizeof(hvm_io_bitmap));
     clear_bit(0x80, hvm_io_bitmap);
+
+    /* initialize hvm_msr_bitmap, all MSR accesses cause VMExits still */
+    memset(hvm_msr_bitmap, ~0, sizeof(hvm_msr_bitmap));
 
     hvm_funcs   = *fns;
     hvm_enabled = 1;
diff -r 78af882eb6fc xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c	Wed Mar 28 20:19:11 2007 +0100
+++ b/xen/arch/x86/hvm/vmx/vmcs.c	Thu Mar 29 13:15:54 2007 +0800
@@ -57,6 +57,7 @@
       CPU_BASED_MWAIT_EXITING |                         \
       CPU_BASED_MOV_DR_EXITING |                        \
       CPU_BASED_ACTIVATE_IO_BITMAP |                    \
+      CPU_BASED_ACTIVATE_MSR_BITMAP |                   \
       CPU_BASED_USE_TSC_OFFSETING )
 
 /* Basic flags for VM-Exit controls. */
@@ -72,6 +73,43 @@
 /* Basic flags for VM-Entry controls. */
 #define MONITOR_VM_ENTRY_CONTROLS                       0x00000000
 
+/* MSR bitmap macros */
+#define MSR_BITMAP_READ_LOW_OFFSET      0
+#define MSR_BITMAP_READ_HIGH_OFFSET     1024
+#define MSR_BITMAP_WRITE_LOW_OFFSET     2048
+#define MSR_BITMAP_WRITE_HIGH_OFFSET    3072
+
+#define MSR_BITMAP_LOW_START_ADDR   0x00000000
+#define MSR_BITMAP_LOW_END_ADDR     0x00002000
+#define MSR_BITMAP_HIGH_START_ADDR  0xC0000000
+#define MSR_BITMAP_HIGH_END_ADDR    0xC0002000
+
+/* set MSR bitmap to avoid vm exit when access MSR */
+#define MSR_BITMAP_NO_VMEXIT_ON(msr)                        \
+    {                                                       \
+        u32 msr_offset = msr;                               \
+        if ( (msr) >= MSR_BITMAP_LOW_START_ADDR &&          \
+             (msr) < MSR_BITMAP_LOW_END_ADDR )              \
+        {                                                   \
+            msr_offset -= MSR_BITMAP_LOW_START_ADDR;        \
+            clear_bit(msr_offset, hvm_msr_bitmap +          \
+                      MSR_BITMAP_READ_LOW_OFFSET);          \
+            clear_bit(msr_offset, hvm_msr_bitmap +          \
+                      MSR_BITMAP_WRITE_LOW_OFFSET);         \
+        }                                                   \
+        else if ( (msr) >= MSR_BITMAP_HIGH_START_ADDR &&    \
+             (msr) < MSR_BITMAP_HIGH_END_ADDR )             \
+        {                                                   \
+            msr_offset -= MSR_BITMAP_HIGH_START_ADDR;       \
+            clear_bit(msr_offset, hvm_msr_bitmap +          \
+                      MSR_BITMAP_READ_HIGH_OFFSET);         \
+            clear_bit(msr_offset, hvm_msr_bitmap +          \
+                      MSR_BITMAP_WRITE_HIGH_OFFSET);        \
+        }                                                   \
+        else                                                \
+            BUG();                                          \
+    }
+
 /* Dynamic (run-time adjusted) execution control flags. */
 static u32 vmx_pin_based_exec_control;
 static u32 vmx_cpu_based_exec_control;
@@ -86,10 +124,13 @@ static u32 adjust_vmx_controls(u32 ctrls
 
     rdmsr(msr, vmx_msr_low, vmx_msr_high);
 
-    /* Bit == 0 means must be zero. */
-    BUG_ON(ctrls & ~vmx_msr_high);
-
-    /* Bit == 1 means must be one. */
+    /* bits setting we should respect */
+    ctrls &= vmx_msr_low ^ vmx_msr_high;
+
+    /* Bit == 0 in high bits of the MSR means must be zero. */
+    ctrls &= vmx_msr_high;
+
+    /* Bit == 1 in low bits of the MSR means must be one. */
     ctrls |= vmx_msr_low;
 
     return ctrls;
@@ -133,6 +174,12 @@ void vmx_init_vmcs_config(void)
         BUG_ON(vmx_cpu_based_exec_control != _vmx_cpu_based_exec_control);
         BUG_ON(vmx_vmexit_control != _vmx_vmexit_control);
         BUG_ON(vmx_vmentry_control != _vmx_vmentry_control);
+    }
+
+    if ( vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP )
+    {
+        MSR_BITMAP_NO_VMEXIT_ON(MSR_FS_BASE);
+        MSR_BITMAP_NO_VMEXIT_ON(MSR_GS_BASE);
     }
 
     /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
@@ -307,6 +354,9 @@ static void construct_vmcs(struct vcpu *
     __vmwrite(CPU_BASED_VM_EXEC_CONTROL, vmx_cpu_based_exec_control);
     v->arch.hvm_vcpu.u.vmx.exec_control = vmx_cpu_based_exec_control;
 
+    if ( vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP )
+        __vmwrite(MSR_BITMAP, virt_to_maddr(hvm_msr_bitmap));
+
     /* I/O access bitmap. */
     __vmwrite(IO_BITMAP_A, virt_to_maddr(hvm_io_bitmap));
     __vmwrite(IO_BITMAP_B, virt_to_maddr(hvm_io_bitmap + PAGE_SIZE));
diff -r 78af882eb6fc xen/include/asm-x86/hvm/support.h
--- a/xen/include/asm-x86/hvm/support.h	Wed Mar 28 20:19:11 2007 +0100
+++ b/xen/include/asm-x86/hvm/support.h	Thu Mar 29 13:15:54 2007 +0800
@@ -215,6 +215,7 @@ int hvm_load(struct domain *d, hvm_domai
 /* End of save/restore */
 
 extern char hvm_io_bitmap[];
+extern char hvm_msr_bitmap[];
 extern int hvm_enabled;
 
 void hvm_enable(struct hvm_function_table *);
diff -r 78af882eb6fc xen/include/asm-x86/hvm/vmx/vmcs.h
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h	Wed Mar 28 20:19:11 2007 +0100
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h	Thu Mar 29 13:15:54 2007 +0800
@@ -109,6 +109,7 @@ extern int vmcs_version;
 #define CPU_BASED_MOV_DR_EXITING        0x00800000
 #define CPU_BASED_UNCOND_IO_EXITING     0x01000000
 #define CPU_BASED_ACTIVATE_IO_BITMAP    0x02000000
+#define CPU_BASED_ACTIVATE_MSR_BITMAP   0x10000000
 #define CPU_BASED_MONITOR_EXITING       0x20000000
 #define CPU_BASED_PAUSE_EXITING         0x40000000
 
@@ -143,6 +144,8 @@ enum vmcs_field {
     IO_BITMAP_A_HIGH                = 0x00002001,
     IO_BITMAP_B                     = 0x00002002,
     IO_BITMAP_B_HIGH                = 0x00002003,
+    MSR_BITMAP                      = 0x00002004,
+    MSR_BITMAP_HIGH                 = 0x00002005,
     VM_EXIT_MSR_STORE_ADDR          = 0x00002006,
     VM_EXIT_MSR_STORE_ADDR_HIGH     = 0x00002007,
     VM_EXIT_MSR_LOAD_ADDR           = 0x00002008,
diff -r 78af882eb6fc xen/include/asm-x86/msr.h
--- a/xen/include/asm-x86/msr.h	Wed Mar 28 20:19:11 2007 +0100
+++ b/xen/include/asm-x86/msr.h	Thu Mar 29 13:15:54 2007 +0800
@@ -126,8 +126,8 @@ static inline void wrmsrl(unsigned int m
 #define MSR_LSTAR 0xc0000082 		/* long mode SYSCALL target */
 #define MSR_CSTAR 0xc0000083		/* compatibility mode SYSCALL target */
 #define MSR_SYSCALL_MASK 0xc0000084	/* EFLAGS mask for syscall */
-#define MSR_FS_BASE 0xc0000100		/* 64bit GS base */
-#define MSR_GS_BASE 0xc0000101		/* 64bit FS base */
+#define MSR_FS_BASE 0xc0000100		/* 64bit FS base */
+#define MSR_GS_BASE 0xc0000101		/* 64bit GS base */
 #define MSR_SHADOW_GS_BASE  0xc0000102	/* SwapGS GS shadow */ 
 /* EFER bits: */ 
 #define _EFER_SCE 0  /* SYSCALL/SYSRET */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Proper use of VMX execution controls MSR.

[Keir Fraser]

Okay, I see. Well, that makes sense but I think we should have two masks for
each control word -- the bits we would like to set, and the bits we must be
able to set. Also your previous patch added an unnecessary AND with 'msr_hi
XOR msr_lo'.

This is pretty trivial to fix up. I'll do a patch.

 -- Keir

On 29/3/07 07:22, "Li, Xin B" <xin.b.li@intel.com> wrote:

> But we have lots of
> hardware features,
> with new features being added to hareware, and I really don't like to
> add the almost
> same code just for another new feature detection.  The idea here is to
> use the control MSR
> to adjust the input controls we _hardcoded_ in Xen, for the features
> we've supported in Xen,
> we always set to 1 in the input control, like in
> MONITOR_CPU_BASED_EXEC_CONTROLS,
> then on processors supporting this feature, We get 1, while on
> processors not supporting this
> feature, we get 0.  This adjusted output control value can be also used
> to choose code
> path on different processors, and the attached patch can be an example.

RE: [PATCH] Proper use of VMX execution controls MSR.

[Li, Xin B]

>Okay, I see. Well, that makes sense but I think we should have two
masks for
>each control word -- the bits we would like to set, and the bits we
must be
>able to set. Also your previous patch added an unnecessary AND 
>with 'msr_hi XOR msr_lo'.

No, it's necessary. According to Intel Spec:
If the bit in msr_lo is 1, then the control bit written to VMCS must be
1
If the bit in msr_hi is 0, then the control bit written to VMCS must be
0 

The msr_lo and msr_hi combinations are:
1) L = 0 and H = 0: the adjusted control bit is 0, no matter what's the
input control value, that's why we need AND msr_hi.
2) L = 0 and H = 1: the adjusted control bit can be 0 or 1, depends on
if we support it.
3) L = 1 and H = 0: INVALID combination!!!
4) L = 1 and H = 1: the adjusted control bit is 1, no matter what's the
input control value, that's why we need OR msr_lo.

Specially for case 2, 
if we have the feature support in Xen, we should set the input control
bit, and expect the adjusted value to be 1;
if we don't have the feature support in Xen, we should clear the input
control bit, and expect adjusted value to be 0;
That's meaning the adjusted output is determined by the input control.
So the expression should be "AND (msr_hi XOR msr_lo)".

-Xin

Re: [PATCH] Proper use of VMX execution controls MSR.

[Keir Fraser]

On 29/3/07 09:37, "Li, Xin B" <xin.b.li@intel.com> wrote:

> if we have the feature support in Xen, we should set the input control
> bit, and expect the adjusted value to be 1;
> if we don't have the feature support in Xen, we should clear the input
> control bit, and expect adjusted value to be 0;
> That's meaning the adjusted output is determined by the input control.
> So the expression should be "AND (msr_hi XOR msr_lo)".

The subsequent AND with msr_hi and the OR with msr_lo has exactly the same
effect. So the AND (msr_hi XOR msr_lo) is not incorrect, but it is
redundant.

 -- Keir

RE: [PATCH] Proper use of VMX execution controls MSR.

[Li, Xin B]

>The subsequent AND with msr_hi and the OR with msr_lo has exactly the
same
>effect. So the AND (msr_hi XOR msr_lo) is not incorrect, but it is
redundant.
>

Seems correct :-).
-Xin