[PATCH 0/5] Fix EFER.NX=0 with EPT

[PATCH 0/5] Fix EFER.NX=0 with EPT

[Avi Kivity]

Currently we run with EFER.NX=1 on the guest even if the guest value is 0.
This is fine with shadow, since we check bit 63 when instantiating a page
table, and fault if bit 63 is set while EFER.NX is clear.

This doesn't work with EPT, since we no longer get the change to check guest
ptes.  So we need to run with EFER.NX=0.

This is complicated by the fact that if we switch EFER.NX on the host, we'll
trap immediately, since some host pages are mapped with the NX bit set.  As
a result, we need to switch the MSR atomically during guest entry and exit.

This patchset implements the complications described above.

Avi Kivity (5):
  KVM: Let vcpu structure alignment be determined at runtime
  KVM: VMX: Add definition for msr autoload entry
  KVM: VMX: Add definitions for guest and host EFER autoswitch vmcs
    entries
  KVM: VMX: Add facility to atomically switch MSRs on guest entry/exit
  KVM: VMX: Atomically switch efer if EPT && !EFER.NX

 arch/ia64/kvm/vmm.c        |    2 +-
 arch/powerpc/kvm/44x.c     |    2 +-
 arch/powerpc/kvm/book3s.c  |    3 +-
 arch/powerpc/kvm/e500.c    |    2 +-
 arch/s390/kvm/kvm-s390.c   |    2 +-
 arch/x86/include/asm/vmx.h |   12 +++++++-
 arch/x86/kvm/svm.c         |    2 +-
 arch/x86/kvm/vmx.c         |   63 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/kvm_host.h   |    2 +-
 virt/kvm/kvm_main.c        |    7 +++--
 10 files changed, 85 insertions(+), 12 deletions(-)

[PATCH 1/5] KVM: Let vcpu structure alignment be determined at runtime

[Avi Kivity]

vmx and svm vcpus have different contents and therefore may have different
alignmment requirements.  Let each specify its required alignment.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/ia64/kvm/vmm.c       |    2 +-
 arch/powerpc/kvm/44x.c    |    2 +-
 arch/powerpc/kvm/book3s.c |    3 ++-
 arch/powerpc/kvm/e500.c   |    2 +-
 arch/s390/kvm/kvm-s390.c  |    2 +-
 arch/x86/kvm/svm.c        |    2 +-
 arch/x86/kvm/vmx.c        |    3 ++-
 include/linux/kvm_host.h  |    2 +-
 virt/kvm/kvm_main.c       |    7 ++++---
 9 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/arch/ia64/kvm/vmm.c b/arch/ia64/kvm/vmm.c
index 7a62f75..f0b9cac 100644
--- a/arch/ia64/kvm/vmm.c
+++ b/arch/ia64/kvm/vmm.c
@@ -51,7 +51,7 @@ static int __init  kvm_vmm_init(void)
 	vmm_fpswa_interface = fpswa_interface;
 
 	/*Register vmm data to kvm side*/
-	return kvm_init(&vmm_info, 1024, THIS_MODULE);
+	return kvm_init(&vmm_info, 1024, 0, THIS_MODULE);
 }
 
 static void __exit kvm_vmm_exit(void)
diff --git a/arch/powerpc/kvm/44x.c b/arch/powerpc/kvm/44x.c
index 689a57c..73c0a3f 100644
--- a/arch/powerpc/kvm/44x.c
+++ b/arch/powerpc/kvm/44x.c
@@ -147,7 +147,7 @@ static int __init kvmppc_44x_init(void)
 	if (r)
 		return r;
 
-	return kvm_init(NULL, sizeof(struct kvmppc_vcpu_44x), THIS_MODULE);
+	return kvm_init(NULL, sizeof(struct kvmppc_vcpu_44x), 0, THIS_MODULE);
 }
 
 static void __exit kvmppc_44x_exit(void)
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 9f97dbe..16e3f4b 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1384,7 +1384,8 @@ int __kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 
 static int kvmppc_book3s_init(void)
 {
-	return kvm_init(NULL, sizeof(struct kvmppc_vcpu_book3s), THIS_MODULE);
+	return kvm_init(NULL, sizeof(struct kvmppc_vcpu_book3s), 0,
+			THIS_MODULE);
 }
 
 static void kvmppc_book3s_exit(void)
diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
index 669a5c5..bc2b400 100644
--- a/arch/powerpc/kvm/e500.c
+++ b/arch/powerpc/kvm/e500.c
@@ -161,7 +161,7 @@ static int __init kvmppc_e500_init(void)
 	flush_icache_range(kvmppc_booke_handlers,
 			kvmppc_booke_handlers + max_ivor + kvmppc_handler_len);
 
-	return kvm_init(NULL, sizeof(struct kvmppc_vcpu_e500), THIS_MODULE);
+	return kvm_init(NULL, sizeof(struct kvmppc_vcpu_e500), 0, THIS_MODULE);
 }
 
 static void __init kvmppc_e500_exit(void)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ee7c713..8093e6f 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -752,7 +752,7 @@ gfn_t unalias_gfn(struct kvm *kvm, gfn_t gfn)
 static int __init kvm_s390_init(void)
 {
 	int ret;
-	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), THIS_MODULE);
+	ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
 	if (ret)
 		return ret;
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 889f660..2511664 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3319,7 +3319,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 static int __init svm_init(void)
 {
 	return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
-			      THIS_MODULE);
+			__alignof__(struct vcpu_svm), THIS_MODULE);
 }
 
 static void __exit svm_exit(void)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 875b785..2e87296 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4245,7 +4245,8 @@ static int __init vmx_init(void)
 
 	set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
 
-	r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx), THIS_MODULE);
+	r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
+		     __alignof__(struct vcpu_vmx), THIS_MODULE);
 	if (r)
 		goto out3;
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ce027d5..7cb116a 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -243,7 +243,7 @@ void kvm_vcpu_uninit(struct kvm_vcpu *vcpu);
 void vcpu_load(struct kvm_vcpu *vcpu);
 void vcpu_put(struct kvm_vcpu *vcpu);
 
-int kvm_init(void *opaque, unsigned int vcpu_size,
+int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 		  struct module *module);
 void kvm_exit(void);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9ab1a77..f032806 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2178,7 +2178,7 @@ static void kvm_sched_out(struct preempt_notifier *pn,
 	kvm_arch_vcpu_put(vcpu);
 }
 
-int kvm_init(void *opaque, unsigned int vcpu_size,
+int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 		  struct module *module)
 {
 	int r;
@@ -2228,8 +2228,9 @@ int kvm_init(void *opaque, unsigned int vcpu_size,
 		goto out_free_4;
 
 	/* A kmem cache lets us meet the alignment requirements of fx_save. */
-	kvm_vcpu_cache = kmem_cache_create("kvm_vcpu", vcpu_size,
-					   __alignof__(struct kvm_vcpu),
+	if (!vcpu_align)
+		vcpu_align = __alignof__(struct kvm_vcpu);
+	kvm_vcpu_cache = kmem_cache_create("kvm_vcpu", vcpu_size, vcpu_align,
 					   0, NULL);
 	if (!kvm_vcpu_cache) {
 		r = -ENOMEM;
-- 
1.7.0.4

[PATCH 2/5] KVM: VMX: Add definition for msr autoload entry

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/vmx.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index fb9a080..4497318 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -25,6 +25,8 @@
  *
  */
 
+#include <linux/types.h>
+
 /*
  * Definitions of Primary Processor-Based VM-Execution Controls.
  */
@@ -394,6 +396,10 @@ enum vmcs_field {
 #define ASM_VMX_INVEPT		  ".byte 0x66, 0x0f, 0x38, 0x80, 0x08"
 #define ASM_VMX_INVVPID		  ".byte 0x66, 0x0f, 0x38, 0x81, 0x08"
 
-
+struct vmx_msr_entry {
+	u32 index;
+	u32 reserved;
+	u64 value;
+} __aligned(16);
 
 #endif
-- 
1.7.0.4

[PATCH 3/5] KVM: VMX: Add definitions for guest and host EFER autoswitch vmcs entries

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/vmx.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 4497318..9e6779f 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -122,6 +122,8 @@ enum vmcs_field {
 	GUEST_IA32_DEBUGCTL_HIGH        = 0x00002803,
 	GUEST_IA32_PAT			= 0x00002804,
 	GUEST_IA32_PAT_HIGH		= 0x00002805,
+	GUEST_IA32_EFER			= 0x00002806,
+	GUEST_IA32_EFER_HIGH		= 0x00002807,
 	GUEST_PDPTR0                    = 0x0000280a,
 	GUEST_PDPTR0_HIGH               = 0x0000280b,
 	GUEST_PDPTR1                    = 0x0000280c,
@@ -132,6 +134,8 @@ enum vmcs_field {
 	GUEST_PDPTR3_HIGH               = 0x00002811,
 	HOST_IA32_PAT			= 0x00002c00,
 	HOST_IA32_PAT_HIGH		= 0x00002c01,
+	HOST_IA32_EFER			= 0x00002c02,
+	HOST_IA32_EFER_HIGH		= 0x00002c03,
 	PIN_BASED_VM_EXEC_CONTROL       = 0x00004000,
 	CPU_BASED_VM_EXEC_CONTROL       = 0x00004002,
 	EXCEPTION_BITMAP                = 0x00004004,
-- 
1.7.0.4

[PATCH 4/5] KVM: VMX: Add facility to atomically switch MSRs on guest entry/exit

[Avi Kivity]

Some guest msr values cannot be used on the host (for example. EFER.NX=0),
so we need to switch them atomically during guest entry or exit.

Add a facility to program the vmx msr autoload registers accordingly.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/vmx.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 2e87296..ae22dcf 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -98,6 +98,8 @@ module_param(ple_gap, int, S_IRUGO);
 static int ple_window = KVM_VMX_DEFAULT_PLE_WINDOW;
 module_param(ple_window, int, S_IRUGO);
 
+#define NR_AUTOLOAD_MSRS 1
+
 struct vmcs {
 	u32 revision_id;
 	u32 abort;
@@ -125,6 +127,11 @@ struct vcpu_vmx {
 	u64 		      msr_guest_kernel_gs_base;
 #endif
 	struct vmcs          *vmcs;
+	struct msr_autoload {
+		unsigned nr;
+		struct vmx_msr_entry guest[NR_AUTOLOAD_MSRS];
+		struct vmx_msr_entry host[NR_AUTOLOAD_MSRS];
+	} msr_autoload;
 	struct {
 		int           loaded;
 		u16           fs_sel, gs_sel, ldt_sel;
@@ -595,6 +602,46 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
 	vmcs_write32(EXCEPTION_BITMAP, eb);
 }
 
+static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
+{
+	unsigned i;
+	struct msr_autoload *m = &vmx->msr_autoload;
+
+	for (i = 0; i < m->nr; ++i)
+		if (m->guest[i].index == msr)
+			break;
+
+	if (i == m->nr)
+		return;
+	--m->nr;
+	m->guest[i] = m->guest[m->nr];
+	m->host[i] = m->host[m->nr];
+	vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, m->nr);
+	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, m->nr);
+}
+
+static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
+				  u64 guest_val, u64 host_val)
+{
+	unsigned i;
+	struct msr_autoload *m = &vmx->msr_autoload;
+
+	for (i = 0; i < m->nr; ++i)
+		if (m->guest[i].index == msr)
+			break;
+
+	if (i == m->nr) {
+		++m->nr;
+		vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, m->nr);
+		vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, m->nr);
+	}
+
+	m->guest[i].index = msr;
+	m->guest[i].value = guest_val;
+	m->host[i].index = msr;
+	m->host[i].value = host_val;
+}
+
 static void reload_tss(void)
 {
 	/*
@@ -2470,7 +2517,9 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 	vmcs_writel(HOST_RIP, kvm_vmx_return); /* 22.2.5 */
 	vmcs_write32(VM_EXIT_MSR_STORE_COUNT, 0);
 	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, 0);
+	vmcs_write64(VM_EXIT_MSR_LOAD_ADDR, __pa(vmx->msr_autoload.host));
 	vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, 0);
+	vmcs_write64(VM_ENTRY_MSR_LOAD_ADDR, __pa(vmx->msr_autoload.guest));
 
 	rdmsr(MSR_IA32_SYSENTER_CS, host_sysenter_cs, junk);
 	vmcs_write32(HOST_IA32_SYSENTER_CS, host_sysenter_cs);
-- 
1.7.0.4

[PATCH 5/5] KVM: VMX: Atomically switch efer if EPT && !EFER.NX

[Avi Kivity]

When EPT is enabled, we cannot emulate EFER.NX=0 through the shadow page
tables.  This causes accesses through ptes with bit 63 set to succeed instead
of failing a reserved bit check.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/vmx.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ae22dcf..cc78fee 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -678,6 +678,17 @@ static bool update_transition_efer(struct vcpu_vmx *vmx, int efer_offset)
 	guest_efer |= host_efer & ignore_bits;
 	vmx->guest_msrs[efer_offset].data = guest_efer;
 	vmx->guest_msrs[efer_offset].mask = ~ignore_bits;
+
+	clear_atomic_switch_msr(vmx, MSR_EFER);
+	/* On ept, can't emulate nx, and must switch nx atomically */
+	if (enable_ept && ((vmx->vcpu.arch.efer ^ host_efer) & EFER_NX)) {
+		guest_efer = vmx->vcpu.arch.efer;
+		if (!(guest_efer & EFER_LMA))
+			guest_efer &= ~EFER_LME;
+		add_atomic_switch_msr(vmx, MSR_EFER, guest_efer, host_efer);
+		return false;
+	}
+
 	return true;
 }
 
-- 
1.7.0.4

Re: [PATCH 5/5] KVM: VMX: Atomically switch efer if EPT && !EFER.NX

[Marcelo Tosatti]

Avi,

This patch breaks WinVista.64 install.

On Wed, Apr 28, 2010 at 04:47:19PM +0300, Avi Kivity wrote:
> When EPT is enabled, we cannot emulate EFER.NX=0 through the shadow page
> tables.  This causes accesses through ptes with bit 63 set to succeed instead
> of failing a reserved bit check.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/vmx.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ae22dcf..cc78fee 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -678,6 +678,17 @@ static bool update_transition_efer(struct vcpu_vmx *vmx, int efer_offset)
>  	guest_efer |= host_efer & ignore_bits;
>  	vmx->guest_msrs[efer_offset].data = guest_efer;
>  	vmx->guest_msrs[efer_offset].mask = ~ignore_bits;
> +
> +	clear_atomic_switch_msr(vmx, MSR_EFER);
> +	/* On ept, can't emulate nx, and must switch nx atomically */
> +	if (enable_ept && ((vmx->vcpu.arch.efer ^ host_efer) & EFER_NX)) {
> +		guest_efer = vmx->vcpu.arch.efer;
> +		if (!(guest_efer & EFER_LMA))
> +			guest_efer &= ~EFER_LME;
> +		add_atomic_switch_msr(vmx, MSR_EFER, guest_efer, host_efer);
> +		return false;
> +	}
> +
>  	return true;
>  }
>  
> -- 
> 1.7.0.4

Re: [PATCH 5/5] KVM: VMX: Atomically switch efer if EPT && !EFER.NX

[Avi Kivity]

On 04/30/2010 08:37 PM, Marcelo Tosatti wrote:
> Avi,
>
> This patch breaks WinVista.64 install.
>    

Please back it out then (the entire patchset).

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 5/5] KVM: VMX: Atomically switch efer if EPT && !EFER.NX

[Avi Kivity]

On 04/30/2010 08:56 PM, Avi Kivity wrote:
> On 04/30/2010 08:37 PM, Marcelo Tosatti wrote:
>> Avi,
>>
>> This patch breaks WinVista.64 install.
>
> Please back it out then (the entire patchset).
>

v2 sent out.  Problem was switching from long mode to legacy mode.

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

Re: [PATCH 0/5] Fix EFER.NX=0 with EPT

[Marcelo Tosatti]

On Wed, Apr 28, 2010 at 04:47:14PM +0300, Avi Kivity wrote:
> Currently we run with EFER.NX=1 on the guest even if the guest value is 0.
> This is fine with shadow, since we check bit 63 when instantiating a page
> table, and fault if bit 63 is set while EFER.NX is clear.
> 
> This doesn't work with EPT, since we no longer get the change to check guest
> ptes.  So we need to run with EFER.NX=0.
> 
> This is complicated by the fact that if we switch EFER.NX on the host, we'll
> trap immediately, since some host pages are mapped with the NX bit set.  As
> a result, we need to switch the MSR atomically during guest entry and exit.
> 
> This patchset implements the complications described above.
> 
> Avi Kivity (5):
>   KVM: Let vcpu structure alignment be determined at runtime
>   KVM: VMX: Add definition for msr autoload entry
>   KVM: VMX: Add definitions for guest and host EFER autoswitch vmcs
>     entries
>   KVM: VMX: Add facility to atomically switch MSRs on guest entry/exit
>   KVM: VMX: Atomically switch efer if EPT && !EFER.NX

Applied, thanks.

Out of curiosity, did you measure the vmentry/vmexit overhead?

Re: [PATCH 0/5] Fix EFER.NX=0 with EPT

[Avi Kivity]

On 04/29/2010 02:22 AM, Marcelo Tosatti wrote:
> On Wed, Apr 28, 2010 at 04:47:14PM +0300, Avi Kivity wrote:
>    
>> Currently we run with EFER.NX=1 on the guest even if the guest value is 0.
>> This is fine with shadow, since we check bit 63 when instantiating a page
>> table, and fault if bit 63 is set while EFER.NX is clear.
>>
>> This doesn't work with EPT, since we no longer get the change to check guest
>> ptes.  So we need to run with EFER.NX=0.
>>
>> This is complicated by the fact that if we switch EFER.NX on the host, we'll
>> trap immediately, since some host pages are mapped with the NX bit set.  As
>> a result, we need to switch the MSR atomically during guest entry and exit.
>>
>>      
> Applied, thanks.
>
> Out of curiosity, did you measure the vmentry/vmexit overhead?
>    

Just did now.

   nx=0: 3384
   nx=1: 2203

Perhaps using the dedicated vmcs field is faster (but since it's 
optional, we have to support the generic autoload area).

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.