[PATCH v2 0/8] KVM: nVMX: Enable unrestricted guest mode and fix some nEPT issues

[PATCH v2 0/8] KVM: nVMX: Enable unrestricted guest mode and fix some nEPT issues

[Jan Kiszka]

These patches apply on top of nEPT v7.

Changes in v2:
 - rebased over v7
 - added patch to fix VM_ENTRY_IA32E_MODE reporting

Jan Kiszka (8):
  KVM: nEPT: Advertise WB type EPTP
  KVM: nVMX: Fix up VM_ENTRY_IA32E_MODE control feature reporting
  KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in
    load_vmcs12_host_state
  KVM: nVMX: Enable unrestricted guest mode support
  KVM: nVMX: Fix guest CR3 read-back on VM-exit
  KVM: nVMX: Load nEPT state after EFER
  KVM: nVMX: Implement support for EFER saving on VM-exit
  KVM: nVMX: Update mmu.base_role.nxe after EFER loading on
    VM-entry/exit

 arch/x86/kvm/vmx.c |   50 +++++++++++++++++++++++++++++++++++---------------
 1 files changed, 35 insertions(+), 15 deletions(-)

-- 
1.7.3.4

[PATCH v2 1/8] KVM: nEPT: Advertise WB type EPTP

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

At least WB must be possible.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 00af1ef..773b54d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2251,8 +2251,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	if (enable_ept) {
 		/* nested EPT: emulate EPT also to L1 */
 		nested_vmx_secondary_ctls_high |= SECONDARY_EXEC_ENABLE_EPT;
-		nested_vmx_ept_caps = VMX_EPT_PAGE_WALK_4_BIT;
-		nested_vmx_ept_caps |= VMX_EPT_INVEPT_BIT;
+		nested_vmx_ept_caps = VMX_EPT_PAGE_WALK_4_BIT |
+			 VMX_EPTP_WB_BIT | VMX_EPT_INVEPT_BIT;
 		nested_vmx_ept_caps &= vmx_capability.ept;
 		/*
 		 * Since invept is completely emulated we support both global
-- 
1.7.3.4

[PATCH v2 2/8] KVM: nVMX: Fix up VM_ENTRY_IA32E_MODE control feature reporting

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Do not report that we can enter the guest in 64-bit mode if the host is
32-bit only. This is not supported by KVM.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 773b54d..c810f9b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2212,9 +2212,13 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
 	nested_vmx_entry_ctls_low = VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR;
 	nested_vmx_entry_ctls_high &=
-		VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_IA32E_MODE;
+#ifdef CONFIG_X86_64
+		VM_ENTRY_IA32E_MODE |
+#endif
+		VM_ENTRY_LOAD_IA32_PAT;
 	nested_vmx_entry_ctls_high |= (VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR |
 				       VM_ENTRY_LOAD_IA32_EFER);
+
 	/* cpu-based controls */
 	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
 		nested_vmx_procbased_ctls_low, nested_vmx_procbased_ctls_high);
-- 
1.7.3.4

[PATCH v2 3/8] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the
state transition that may prevent loading L1's cr0.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c810f9b..21b19c0 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8183,7 +8183,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 	 * fpu_active (which may have changed).
 	 * Note that vmx_set_cr0 refers to efer set above.
 	 */
-	kvm_set_cr0(vcpu, vmcs12->host_cr0);
+	vmx_set_cr0(vcpu, vmcs12->host_cr0);
 	/*
 	 * If we did fpu_activate()/fpu_deactivate() during L2's run, we need
 	 * to apply the same changes to L1's vmcs. We just set cr0 correctly,
-- 
1.7.3.4

[PATCH v2 4/8] KVM: nVMX: Enable unrestricted guest mode support

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Now that we provide EPT support, there is no reason to torture our
guests by hiding the relieving unrestricted guest mode feature. We just
need to relax CR0 checks for always-on bits as PE and PG can now be
switched off.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 21b19c0..b482d47 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2250,6 +2250,7 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	nested_vmx_secondary_ctls_low = 0;
 	nested_vmx_secondary_ctls_high &=
 		SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
+		SECONDARY_EXEC_UNRESTRICTED_GUEST |
 		SECONDARY_EXEC_WBINVD_EXITING;
 
 	if (enable_ept) {
@@ -4873,6 +4874,17 @@ vmx_patch_hypercall(struct kvm_vcpu *vcpu, unsigned char *hypercall)
 	hypercall[2] = 0xc1;
 }
 
+static bool nested_cr0_valid(struct vmcs12 *vmcs12, unsigned long val)
+{
+	unsigned long always_on = VMXON_CR0_ALWAYSON;
+
+	if (nested_vmx_secondary_ctls_high &
+		SECONDARY_EXEC_UNRESTRICTED_GUEST &&
+	    nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST))
+		always_on &= ~(X86_CR0_PE | X86_CR0_PG);
+	return (val & always_on) == always_on;
+}
+
 /* called to set cr0 as appropriate for a mov-to-cr0 exit. */
 static int handle_set_cr0(struct kvm_vcpu *vcpu, unsigned long val)
 {
@@ -4891,9 +4903,7 @@ static int handle_set_cr0(struct kvm_vcpu *vcpu, unsigned long val)
 		val = (val & ~vmcs12->cr0_guest_host_mask) |
 			(vmcs12->guest_cr0 & vmcs12->cr0_guest_host_mask);
 
-		/* TODO: will have to take unrestricted guest mode into
-		 * account */
-		if ((val & VMXON_CR0_ALWAYSON) != VMXON_CR0_ALWAYSON)
+		if (!nested_cr0_valid(vmcs12, val))
 			return 1;
 
 		if (kvm_set_cr0(vcpu, val))
@@ -7858,7 +7868,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 		return 1;
 	}
 
-	if (((vmcs12->guest_cr0 & VMXON_CR0_ALWAYSON) != VMXON_CR0_ALWAYSON) ||
+	if (!nested_cr0_valid(vmcs12, vmcs12->guest_cr0) ||
 	    ((vmcs12->guest_cr4 & VMXON_CR4_ALWAYSON) != VMXON_CR4_ALWAYSON)) {
 		nested_vmx_entry_failure(vcpu, vmcs12,
 			EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT);
-- 
1.7.3.4

[PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

If nested EPT is enabled, the L2 guest may change CR3 without any exits.
We therefore have to read the current value from the VMCS when switching
to L1. However, if paging wasn't enabled, L0 tracks L2's CR3, and
GUEST_CR3 rather contains the real-mode identity map. So we need to
retrieve CR3 from the architectural state after conditionally updating
it - and this is what kvm_read_cr3 does.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b482d47..09666aa 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8106,7 +8106,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	 * Additionally, restore L2's PDPTR to vmcs12.
 	 */
 	if (enable_ept) {
-		vmcs12->guest_cr3 = vmcs_read64(GUEST_CR3);
+		vmcs12->guest_cr3 = kvm_read_cr3(vcpu);
 		vmcs12->guest_pdptr0 = vmcs_read64(GUEST_PDPTR0);
 		vmcs12->guest_pdptr1 = vmcs_read64(GUEST_PDPTR1);
 		vmcs12->guest_pdptr2 = vmcs_read64(GUEST_PDPTR2);
-- 
1.7.3.4

[PATCH v2 6/8] KVM: nVMX: Load nEPT state after EFER

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

We need to update EFER.NX before building the nEPT state via
nested_ept_init_mmu_context. Otherwise, we risk to create an MMU context
that claims to have NX disabled while the guest EPT used NX. This will
cause spurious faults for L2.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 09666aa..8f69203 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -7733,11 +7733,6 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 		vmx_flush_tlb(vcpu);
 	}
 
-	if (nested_cpu_has_ept(vmcs12)) {
-		kvm_mmu_unload(vcpu);
-		nested_ept_init_mmu_context(vcpu);
-	}
-
 	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER)
 		vcpu->arch.efer = vmcs12->guest_ia32_efer;
 	else if (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE)
@@ -7747,6 +7742,11 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	/* Note: modifies VM_ENTRY/EXIT_CONTROLS and GUEST/HOST_IA32_EFER */
 	vmx_set_efer(vcpu, vcpu->arch.efer);
 
+	if (nested_cpu_has_ept(vmcs12)) {
+		kvm_mmu_unload(vcpu);
+		nested_ept_init_mmu_context(vcpu);
+	}
+
 	/*
 	 * This sets GUEST_CR0 to vmcs12->guest_cr0, with possibly a modified
 	 * TS bit (for lazy fpu) and bits which we consider mandatory enabled.
-- 
1.7.3.4

[PATCH v2 7/8] KVM: nVMX: Implement support for EFER saving on VM-exit

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Implement and advertise VM_EXIT_SAVE_IA32_EFER. L0 traps EFER writes
unconditionally, so we always find the current L2 value in the
architectural state.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 8f69203..99a4a70 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2204,7 +2204,7 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 	nested_vmx_exit_ctls_high = 0;
 #endif
 	nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
-				      VM_EXIT_LOAD_IA32_EFER);
+		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
 
 	/* entry controls */
 	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
@@ -8122,6 +8122,8 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	vmcs12->guest_ia32_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
 	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
 		vmcs12->guest_ia32_pat = vmcs_read64(GUEST_IA32_PAT);
+	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER)
+		vmcs12->guest_ia32_efer = vcpu->arch.efer;
 	vmcs12->guest_sysenter_cs = vmcs_read32(GUEST_SYSENTER_CS);
 	vmcs12->guest_sysenter_esp = vmcs_readl(GUEST_SYSENTER_ESP);
 	vmcs12->guest_sysenter_eip = vmcs_readl(GUEST_SYSENTER_EIP);
-- 
1.7.3.4

[PATCH v2 8/8] KVM: nVMX: Update mmu.base_role.nxe after EFER loading on VM-entry/exit

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This job is normally performed by the architectural EFER set service
which we cannot use as it prevents transitions that are valid when
switching between L1 and L2. So open-code the update of base_role.nxe
after changing EFER on VM-entry and exit.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/vmx.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 99a4a70..44494ed 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -7741,6 +7741,8 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 		vcpu->arch.efer &= ~(EFER_LMA | EFER_LME);
 	/* Note: modifies VM_ENTRY/EXIT_CONTROLS and GUEST/HOST_IA32_EFER */
 	vmx_set_efer(vcpu, vcpu->arch.efer);
+	vcpu->arch.mmu.base_role.nxe =
+		(vcpu->arch.efer & EFER_NX) && !enable_ept;
 
 	if (nested_cpu_has_ept(vmcs12)) {
 		kvm_mmu_unload(vcpu);
@@ -8185,6 +8187,8 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 	else
 		vcpu->arch.efer &= ~(EFER_LMA | EFER_LME);
 	vmx_set_efer(vcpu, vcpu->arch.efer);
+	vcpu->arch.mmu.base_role.nxe =
+		(vcpu->arch.efer & EFER_NX) && !enable_ept;
 
 	kvm_register_write(vcpu, VCPU_REGS_RSP, vmcs12->host_rsp);
 	kvm_register_write(vcpu, VCPU_REGS_RIP, vmcs12->host_rip);
-- 
1.7.3.4

Re: [PATCH v2 2/8] KVM: nVMX: Fix up VM_ENTRY_IA32E_MODE control feature reporting

[Gleb Natapov]

On Tue, Aug 06, 2013 at 10:39:56AM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Do not report that we can enter the guest in 64-bit mode if the host is
> 32-bit only. This is not supported by KVM.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Gleb Natapov <gleb@redhat.com>

> ---
>  arch/x86/kvm/vmx.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 773b54d..c810f9b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2212,9 +2212,13 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>  	/* If bit 55 of VMX_BASIC is off, bits 0-8 and 12 must be 1. */
>  	nested_vmx_entry_ctls_low = VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR;
>  	nested_vmx_entry_ctls_high &=
> -		VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_IA32E_MODE;
> +#ifdef CONFIG_X86_64
> +		VM_ENTRY_IA32E_MODE |
> +#endif
> +		VM_ENTRY_LOAD_IA32_PAT;
>  	nested_vmx_entry_ctls_high |= (VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR |
>  				       VM_ENTRY_LOAD_IA32_EFER);
> +
>  	/* cpu-based controls */
>  	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS,
>  		nested_vmx_procbased_ctls_low, nested_vmx_procbased_ctls_high);
> -- 
> 1.7.3.4

--
			Gleb.

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Gleb Natapov]

On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> If nested EPT is enabled, the L2 guest may change CR3 without any exits.
> We therefore have to read the current value from the VMCS when switching
> to L1. However, if paging wasn't enabled, L0 tracks L2's CR3, and
> GUEST_CR3 rather contains the real-mode identity map. So we need to
> retrieve CR3 from the architectural state after conditionally updating
> it - and this is what kvm_read_cr3 does.
> 
I have a headache from trying to think about it already, but shouldn't
L1 be the one who setups identity map for L2? I traced what
vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not see
different values in real mode.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/kvm/vmx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b482d47..09666aa 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8106,7 +8106,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  	 * Additionally, restore L2's PDPTR to vmcs12.
>  	 */
>  	if (enable_ept) {
> -		vmcs12->guest_cr3 = vmcs_read64(GUEST_CR3);
> +		vmcs12->guest_cr3 = kvm_read_cr3(vcpu);
>  		vmcs12->guest_pdptr0 = vmcs_read64(GUEST_PDPTR0);
>  		vmcs12->guest_pdptr1 = vmcs_read64(GUEST_PDPTR1);
>  		vmcs12->guest_pdptr2 = vmcs_read64(GUEST_PDPTR2);
> -- 
> 1.7.3.4

--
			Gleb.

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Jan Kiszka]

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

On 2013-08-06 12:12, Gleb Natapov wrote:
> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> If nested EPT is enabled, the L2 guest may change CR3 without any exits.
>> We therefore have to read the current value from the VMCS when switching
>> to L1. However, if paging wasn't enabled, L0 tracks L2's CR3, and
>> GUEST_CR3 rather contains the real-mode identity map. So we need to
>> retrieve CR3 from the architectural state after conditionally updating
>> it - and this is what kvm_read_cr3 does.
>>
> I have a headache from trying to think about it already, but shouldn't
> L1 be the one who setups identity map for L2? I traced what
> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not see
> different values in real mode.

Did you try with my patches applied and unrestricted guest mode in use?

Jan

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/vmx.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index b482d47..09666aa 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -8106,7 +8106,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>>  	 * Additionally, restore L2's PDPTR to vmcs12.
>>  	 */
>>  	if (enable_ept) {
>> -		vmcs12->guest_cr3 = vmcs_read64(GUEST_CR3);
>> +		vmcs12->guest_cr3 = kvm_read_cr3(vcpu);
>>  		vmcs12->guest_pdptr0 = vmcs_read64(GUEST_PDPTR0);
>>  		vmcs12->guest_pdptr1 = vmcs_read64(GUEST_PDPTR1);
>>  		vmcs12->guest_pdptr2 = vmcs_read64(GUEST_PDPTR2);
>> -- 
>> 1.7.3.4
> 
> --
> 			Gleb.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Gleb Natapov]

On Tue, Aug 06, 2013 at 12:25:55PM +0200, Jan Kiszka wrote:
> On 2013-08-06 12:12, Gleb Natapov wrote:
> > On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> If nested EPT is enabled, the L2 guest may change CR3 without any exits.
> >> We therefore have to read the current value from the VMCS when switching
> >> to L1. However, if paging wasn't enabled, L0 tracks L2's CR3, and
> >> GUEST_CR3 rather contains the real-mode identity map. So we need to
> >> retrieve CR3 from the architectural state after conditionally updating
> >> it - and this is what kvm_read_cr3 does.
> >>
> > I have a headache from trying to think about it already, but shouldn't
> > L1 be the one who setups identity map for L2? I traced what
> > vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not see
> > different values in real mode.
> 
> Did you try with my patches applied and unrestricted guest mode in use?
> 
No, for that I need to setup nested environment on the machine that
support unrestricted guest first :)

> Jan
> 
> > 
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  arch/x86/kvm/vmx.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index b482d47..09666aa 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -8106,7 +8106,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> >>  	 * Additionally, restore L2's PDPTR to vmcs12.
> >>  	 */
> >>  	if (enable_ept) {
> >> -		vmcs12->guest_cr3 = vmcs_read64(GUEST_CR3);
> >> +		vmcs12->guest_cr3 = kvm_read_cr3(vcpu);
> >>  		vmcs12->guest_pdptr0 = vmcs_read64(GUEST_PDPTR0);
> >>  		vmcs12->guest_pdptr1 = vmcs_read64(GUEST_PDPTR1);
> >>  		vmcs12->guest_pdptr2 = vmcs_read64(GUEST_PDPTR2);
> >> -- 
> >> 1.7.3.4
> > 
> > --
> > 			Gleb.
> > 
> 
> 



--
			Gleb.

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Gleb Natapov]

On Tue, Aug 06, 2013 at 01:31:03PM +0300, Gleb Natapov wrote:
> On Tue, Aug 06, 2013 at 12:25:55PM +0200, Jan Kiszka wrote:
> > On 2013-08-06 12:12, Gleb Natapov wrote:
> > > On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
> > >> From: Jan Kiszka <jan.kiszka@siemens.com>
> > >>
> > >> If nested EPT is enabled, the L2 guest may change CR3 without any exits.
> > >> We therefore have to read the current value from the VMCS when switching
> > >> to L1. However, if paging wasn't enabled, L0 tracks L2's CR3, and
> > >> GUEST_CR3 rather contains the real-mode identity map. So we need to
> > >> retrieve CR3 from the architectural state after conditionally updating
> > >> it - and this is what kvm_read_cr3 does.
> > >>
> > > I have a headache from trying to think about it already, but shouldn't
> > > L1 be the one who setups identity map for L2? I traced what
> > > vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not see
> > > different values in real mode.
> > 
> > Did you try with my patches applied and unrestricted guest mode in use?
> > 
> No, for that I need to setup nested environment on the machine that
> support unrestricted guest first :)
> 
Did that. I see unrestricted guest is enabled in L1, but still do not
see different values.

--
			Gleb.

RE: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-08-06:
> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> If nested EPT is enabled, the L2 guest may change CR3 without any exits.
>> We therefore have to read the current value from the VMCS when
>> switching to L1. However, if paging wasn't enabled, L0 tracks L2's
>> CR3, and
>> GUEST_CR3 rather contains the real-mode identity map. So we need to
>> retrieve CR3 from the architectural state after conditionally
>> updating it - and this is what kvm_read_cr3 does.
>> 
> I have a headache from trying to think about it already, but shouldn't
> L1 be the one who setups identity map for L2? I traced what
> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not see
Here is my understanding:
In vmx_set_cr3(), if enabled ept, it will check whether target vcpu is enabling paging. When L2 running in real mode, then target vcpu is not enabling paging and it will use L0's identity map for L2. If you read GUEST_CR3 from VMCS, then you may get the L2's identity map not L1's.

> different values in real mode.
> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/vmx.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index
>> b482d47..09666aa 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -8106,7 +8106,7 @@ static void prepare_vmcs12(struct kvm_vcpu
> *vcpu, struct vmcs12 *vmcs12)
>>  	 * Additionally, restore L2's PDPTR to vmcs12.
>>  	 */
>>  	if (enable_ept) {
>> -		vmcs12->guest_cr3 = vmcs_read64(GUEST_CR3);
>> +		vmcs12->guest_cr3 = kvm_read_cr3(vcpu);
>>  		vmcs12->guest_pdptr0 = vmcs_read64(GUEST_PDPTR0);
>>  		vmcs12->guest_pdptr1 = vmcs_read64(GUEST_PDPTR1);
>>  		vmcs12->guest_pdptr2 = vmcs_read64(GUEST_PDPTR2);
>> --
>> 1.7.3.4
>


Best regards,
Yang

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Gleb Natapov]

On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-08-06:
> > On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >> 
> >> If nested EPT is enabled, the L2 guest may change CR3 without any exits.
> >> We therefore have to read the current value from the VMCS when
> >> switching to L1. However, if paging wasn't enabled, L0 tracks L2's
> >> CR3, and
> >> GUEST_CR3 rather contains the real-mode identity map. So we need to
> >> retrieve CR3 from the architectural state after conditionally
> >> updating it - and this is what kvm_read_cr3 does.
> >> 
> > I have a headache from trying to think about it already, but shouldn't
> > L1 be the one who setups identity map for L2? I traced what
> > vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not see
> Here is my understanding:
> In vmx_set_cr3(), if enabled ept, it will check whether target vcpu is enabling paging. When L2 running in real mode, then target vcpu is not enabling paging and it will use L0's identity map for L2. If you read GUEST_CR3 from VMCS, then you may get the L2's identity map not L1's.
> 
Yes, but why it makes sense to use L0 identity map for L2? I didn't see
different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because L0
and L1 use the same identity map address. When I changed identity
address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
indeed different, but the real CR3 L2 uses points to L0 identity map. If I
zero L1 identity map page L2 still works.

--
			Gleb.

RE: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-08-06:
> On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-08-06:
>>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>> 
>>>> If nested EPT is enabled, the L2 guest may change CR3 without any exits.
>>>> We therefore have to read the current value from the VMCS when
>>>> switching to L1. However, if paging wasn't enabled, L0 tracks
>>>> L2's CR3, and
>>>> GUEST_CR3 rather contains the real-mode identity map. So we need
>>>> to retrieve CR3 from the architectural state after conditionally
>>>> updating it - and this is what kvm_read_cr3 does.
>>>> 
>>> I have a headache from trying to think about it already, but
>>> shouldn't
>>> L1 be the one who setups identity map for L2? I traced what
>>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
>>> see
>> Here is my understanding:
>> In vmx_set_cr3(), if enabled ept, it will check whether target vcpu
>> is enabling
> paging. When L2 running in real mode, then target vcpu is not enabling
> paging and it will use L0's identity map for L2. If you read GUEST_CR3
> from VMCS, then you may get the L2's identity map not L1's.
>> 
> Yes, but why it makes sense to use L0 identity map for L2? I didn't see
> different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because L0
> and L1 use the same identity map address. When I changed identity
> address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
> indeed different, but the real CR3 L2 uses points to L0 identity map. If
> I zero L1 identity map page L2 still works.
>
If L2 in real mode, then L2PA == L1PA. So L0's identity map also works if L2 is in real mode.

Best regards,
Yang

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Gleb Natapov]

On Tue, Aug 06, 2013 at 02:12:51PM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-08-06:
> > On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-08-06:
> >>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> 
> >>>> If nested EPT is enabled, the L2 guest may change CR3 without any exits.
> >>>> We therefore have to read the current value from the VMCS when
> >>>> switching to L1. However, if paging wasn't enabled, L0 tracks
> >>>> L2's CR3, and
> >>>> GUEST_CR3 rather contains the real-mode identity map. So we need
> >>>> to retrieve CR3 from the architectural state after conditionally
> >>>> updating it - and this is what kvm_read_cr3 does.
> >>>> 
> >>> I have a headache from trying to think about it already, but
> >>> shouldn't
> >>> L1 be the one who setups identity map for L2? I traced what
> >>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
> >>> see
> >> Here is my understanding:
> >> In vmx_set_cr3(), if enabled ept, it will check whether target vcpu
> >> is enabling
> > paging. When L2 running in real mode, then target vcpu is not enabling
> > paging and it will use L0's identity map for L2. If you read GUEST_CR3
> > from VMCS, then you may get the L2's identity map not L1's.
> >> 
> > Yes, but why it makes sense to use L0 identity map for L2? I didn't see
> > different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because L0
> > and L1 use the same identity map address. When I changed identity
> > address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
> > indeed different, but the real CR3 L2 uses points to L0 identity map. If
> > I zero L1 identity map page L2 still works.
> >
> If L2 in real mode, then L2PA == L1PA. So L0's identity map also works if L2 is in real mode.
> 
That not the point. It may work accidentally for kvm on kvm, but what
if other hypervisor plays different tricks and builds different ident map
for its guest?
 
--
			Gleb.

RE: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-08-06:
> On Tue, Aug 06, 2013 at 02:12:51PM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-08-06:
>>> On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-08-06:
>>>>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> 
>>>>>> If nested EPT is enabled, the L2 guest may change CR3 without any
>>>>>> exits. We therefore have to read the current value from the VMCS
>>>>>> when switching to L1. However, if paging wasn't enabled, L0 tracks
>>>>>> L2's CR3, and GUEST_CR3 rather contains the real-mode identity map.
>>>>>> So we need to retrieve CR3 from the architectural state after
>>>>>> conditionally updating it - and this is what kvm_read_cr3 does.
>>>>>> 
>>>>> I have a headache from trying to think about it already, but
>>>>> shouldn't
>>>>> L1 be the one who setups identity map for L2? I traced what
>>>>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
>>>>> see
>>>> Here is my understanding:
>>>> In vmx_set_cr3(), if enabled ept, it will check whether target
>>>> vcpu is enabling
>>> paging. When L2 running in real mode, then target vcpu is not
>>> enabling paging and it will use L0's identity map for L2. If you
>>> read GUEST_CR3 from VMCS, then you may get the L2's identity map
>>> not
> L1's.
>>>> 
>>> Yes, but why it makes sense to use L0 identity map for L2? I didn't
>>> see different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because
>>> L0 and L1 use the same identity map address. When I changed identity
>>> address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
>>> indeed different, but the real CR3 L2 uses points to L0 identity map.
>>> If I zero L1 identity map page L2 still works.
>>> 
>> If L2 in real mode, then L2PA == L1PA. So L0's identity map also works
>> if L2 is in real mode.
>> 
> That not the point. It may work accidentally for kvm on kvm, but what
> if other hypervisor plays different tricks and builds different ident map for its guest?
Yes, if other hypervisor doesn't build the 1:1 mapping for its guest, it will fail to work. But I cannot imagine what kind of hypervisor will do this and what the purpose is.
Anyway, current logic is definitely wrong. It should use L1's identity map instead L0's.

Best regards,
Yang

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Jan Kiszka]

On 2013-08-06 17:04, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-08-06:
>> On Tue, Aug 06, 2013 at 02:12:51PM +0000, Zhang, Yang Z wrote:
>>> Gleb Natapov wrote on 2013-08-06:
>>>> On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>
>>>>>>> If nested EPT is enabled, the L2 guest may change CR3 without any
>>>>>>> exits. We therefore have to read the current value from the VMCS
>>>>>>> when switching to L1. However, if paging wasn't enabled, L0 tracks
>>>>>>> L2's CR3, and GUEST_CR3 rather contains the real-mode identity map.
>>>>>>> So we need to retrieve CR3 from the architectural state after
>>>>>>> conditionally updating it - and this is what kvm_read_cr3 does.
>>>>>>>
>>>>>> I have a headache from trying to think about it already, but
>>>>>> shouldn't
>>>>>> L1 be the one who setups identity map for L2? I traced what
>>>>>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
>>>>>> see
>>>>> Here is my understanding:
>>>>> In vmx_set_cr3(), if enabled ept, it will check whether target
>>>>> vcpu is enabling
>>>> paging. When L2 running in real mode, then target vcpu is not
>>>> enabling paging and it will use L0's identity map for L2. If you
>>>> read GUEST_CR3 from VMCS, then you may get the L2's identity map
>>>> not
>> L1's.
>>>>>
>>>> Yes, but why it makes sense to use L0 identity map for L2? I didn't
>>>> see different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because
>>>> L0 and L1 use the same identity map address. When I changed identity
>>>> address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
>>>> indeed different, but the real CR3 L2 uses points to L0 identity map.
>>>> If I zero L1 identity map page L2 still works.
>>>>
>>> If L2 in real mode, then L2PA == L1PA. So L0's identity map also works
>>> if L2 is in real mode.
>>>
>> That not the point. It may work accidentally for kvm on kvm, but what
>> if other hypervisor plays different tricks and builds different ident map for its guest?
> Yes, if other hypervisor doesn't build the 1:1 mapping for its guest, it will fail to work. But I cannot imagine what kind of hypervisor will do this and what the purpose is.
> Anyway, current logic is definitely wrong. It should use L1's identity map instead L0's.

So something like this is rather needed?

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 44494ed..60a3644 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 	if (enable_ept) {
 		eptp = construct_eptp(cr3);
 		vmcs_write64(EPT_POINTER, eptp);
-		guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
-			vcpu->kvm->arch.ept_identity_map_addr;
+		if (is_paging(vcpu) || is_guest_mode(vcpu))
+			guest_cr3 = kvm_read_cr3(vcpu) :
+		else
+			guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
 		ept_load_pdptrs(vcpu);
 	}
 

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Gleb Natapov]

On Tue, Aug 06, 2013 at 05:48:54PM +0200, Jan Kiszka wrote:
> On 2013-08-06 17:04, Zhang, Yang Z wrote:
> > Gleb Natapov wrote on 2013-08-06:
> >> On Tue, Aug 06, 2013 at 02:12:51PM +0000, Zhang, Yang Z wrote:
> >>> Gleb Natapov wrote on 2013-08-06:
> >>>> On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
> >>>>> Gleb Natapov wrote on 2013-08-06:
> >>>>>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
> >>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>
> >>>>>>> If nested EPT is enabled, the L2 guest may change CR3 without any
> >>>>>>> exits. We therefore have to read the current value from the VMCS
> >>>>>>> when switching to L1. However, if paging wasn't enabled, L0 tracks
> >>>>>>> L2's CR3, and GUEST_CR3 rather contains the real-mode identity map.
> >>>>>>> So we need to retrieve CR3 from the architectural state after
> >>>>>>> conditionally updating it - and this is what kvm_read_cr3 does.
> >>>>>>>
> >>>>>> I have a headache from trying to think about it already, but
> >>>>>> shouldn't
> >>>>>> L1 be the one who setups identity map for L2? I traced what
> >>>>>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
> >>>>>> see
> >>>>> Here is my understanding:
> >>>>> In vmx_set_cr3(), if enabled ept, it will check whether target
> >>>>> vcpu is enabling
> >>>> paging. When L2 running in real mode, then target vcpu is not
> >>>> enabling paging and it will use L0's identity map for L2. If you
> >>>> read GUEST_CR3 from VMCS, then you may get the L2's identity map
> >>>> not
> >> L1's.
> >>>>>
> >>>> Yes, but why it makes sense to use L0 identity map for L2? I didn't
> >>>> see different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because
> >>>> L0 and L1 use the same identity map address. When I changed identity
> >>>> address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
> >>>> indeed different, but the real CR3 L2 uses points to L0 identity map.
> >>>> If I zero L1 identity map page L2 still works.
> >>>>
> >>> If L2 in real mode, then L2PA == L1PA. So L0's identity map also works
> >>> if L2 is in real mode.
> >>>
> >> That not the point. It may work accidentally for kvm on kvm, but what
> >> if other hypervisor plays different tricks and builds different ident map for its guest?
> > Yes, if other hypervisor doesn't build the 1:1 mapping for its guest, it will fail to work. But I cannot imagine what kind of hypervisor will do this and what the purpose is.
> > Anyway, current logic is definitely wrong. It should use L1's identity map instead L0's.
> 
> So something like this is rather needed?
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 44494ed..60a3644 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>  	if (enable_ept) {
>  		eptp = construct_eptp(cr3);
>  		vmcs_write64(EPT_POINTER, eptp);
> -		guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
> -			vcpu->kvm->arch.ept_identity_map_addr;
> +		if (is_paging(vcpu) || is_guest_mode(vcpu))
> +			guest_cr3 = kvm_read_cr3(vcpu) :
> +		else
> +			guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
>  		ept_load_pdptrs(vcpu);
>  	}
>  
That what I am thinking, will think about it some more tomorrow.

--
			Gleb.

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Jan Kiszka]

On 2013-08-06 17:53, Gleb Natapov wrote:
> On Tue, Aug 06, 2013 at 05:48:54PM +0200, Jan Kiszka wrote:
>> On 2013-08-06 17:04, Zhang, Yang Z wrote:
>>> Gleb Natapov wrote on 2013-08-06:
>>>> On Tue, Aug 06, 2013 at 02:12:51PM +0000, Zhang, Yang Z wrote:
>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>> On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
>>>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>>>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>
>>>>>>>>> If nested EPT is enabled, the L2 guest may change CR3 without any
>>>>>>>>> exits. We therefore have to read the current value from the VMCS
>>>>>>>>> when switching to L1. However, if paging wasn't enabled, L0 tracks
>>>>>>>>> L2's CR3, and GUEST_CR3 rather contains the real-mode identity map.
>>>>>>>>> So we need to retrieve CR3 from the architectural state after
>>>>>>>>> conditionally updating it - and this is what kvm_read_cr3 does.
>>>>>>>>>
>>>>>>>> I have a headache from trying to think about it already, but
>>>>>>>> shouldn't
>>>>>>>> L1 be the one who setups identity map for L2? I traced what
>>>>>>>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
>>>>>>>> see
>>>>>>> Here is my understanding:
>>>>>>> In vmx_set_cr3(), if enabled ept, it will check whether target
>>>>>>> vcpu is enabling
>>>>>> paging. When L2 running in real mode, then target vcpu is not
>>>>>> enabling paging and it will use L0's identity map for L2. If you
>>>>>> read GUEST_CR3 from VMCS, then you may get the L2's identity map
>>>>>> not
>>>> L1's.
>>>>>>>
>>>>>> Yes, but why it makes sense to use L0 identity map for L2? I didn't
>>>>>> see different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because
>>>>>> L0 and L1 use the same identity map address. When I changed identity
>>>>>> address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
>>>>>> indeed different, but the real CR3 L2 uses points to L0 identity map.
>>>>>> If I zero L1 identity map page L2 still works.
>>>>>>
>>>>> If L2 in real mode, then L2PA == L1PA. So L0's identity map also works
>>>>> if L2 is in real mode.
>>>>>
>>>> That not the point. It may work accidentally for kvm on kvm, but what
>>>> if other hypervisor plays different tricks and builds different ident map for its guest?
>>> Yes, if other hypervisor doesn't build the 1:1 mapping for its guest, it will fail to work. But I cannot imagine what kind of hypervisor will do this and what the purpose is.
>>> Anyway, current logic is definitely wrong. It should use L1's identity map instead L0's.
>>
>> So something like this is rather needed?
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 44494ed..60a3644 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>>  	if (enable_ept) {
>>  		eptp = construct_eptp(cr3);
>>  		vmcs_write64(EPT_POINTER, eptp);
>> -		guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
>> -			vcpu->kvm->arch.ept_identity_map_addr;
>> +		if (is_paging(vcpu) || is_guest_mode(vcpu))
>> +			guest_cr3 = kvm_read_cr3(vcpu) :
>> +		else
>> +			guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
>>  		ept_load_pdptrs(vcpu);
>>  	}
>>  
> That what I am thinking, will think about it some more tomorrow.

OK, and I'll feed it into a local test.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Gleb Natapov]

On Tue, Aug 06, 2013 at 05:57:02PM +0200, Jan Kiszka wrote:
> On 2013-08-06 17:53, Gleb Natapov wrote:
> > On Tue, Aug 06, 2013 at 05:48:54PM +0200, Jan Kiszka wrote:
> >> On 2013-08-06 17:04, Zhang, Yang Z wrote:
> >>> Gleb Natapov wrote on 2013-08-06:
> >>>> On Tue, Aug 06, 2013 at 02:12:51PM +0000, Zhang, Yang Z wrote:
> >>>>> Gleb Natapov wrote on 2013-08-06:
> >>>>>> On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
> >>>>>>> Gleb Natapov wrote on 2013-08-06:
> >>>>>>>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
> >>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>>
> >>>>>>>>> If nested EPT is enabled, the L2 guest may change CR3 without any
> >>>>>>>>> exits. We therefore have to read the current value from the VMCS
> >>>>>>>>> when switching to L1. However, if paging wasn't enabled, L0 tracks
> >>>>>>>>> L2's CR3, and GUEST_CR3 rather contains the real-mode identity map.
> >>>>>>>>> So we need to retrieve CR3 from the architectural state after
> >>>>>>>>> conditionally updating it - and this is what kvm_read_cr3 does.
> >>>>>>>>>
> >>>>>>>> I have a headache from trying to think about it already, but
> >>>>>>>> shouldn't
> >>>>>>>> L1 be the one who setups identity map for L2? I traced what
> >>>>>>>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
> >>>>>>>> see
> >>>>>>> Here is my understanding:
> >>>>>>> In vmx_set_cr3(), if enabled ept, it will check whether target
> >>>>>>> vcpu is enabling
> >>>>>> paging. When L2 running in real mode, then target vcpu is not
> >>>>>> enabling paging and it will use L0's identity map for L2. If you
> >>>>>> read GUEST_CR3 from VMCS, then you may get the L2's identity map
> >>>>>> not
> >>>> L1's.
> >>>>>>>
> >>>>>> Yes, but why it makes sense to use L0 identity map for L2? I didn't
> >>>>>> see different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because
> >>>>>> L0 and L1 use the same identity map address. When I changed identity
> >>>>>> address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
> >>>>>> indeed different, but the real CR3 L2 uses points to L0 identity map.
> >>>>>> If I zero L1 identity map page L2 still works.
> >>>>>>
> >>>>> If L2 in real mode, then L2PA == L1PA. So L0's identity map also works
> >>>>> if L2 is in real mode.
> >>>>>
> >>>> That not the point. It may work accidentally for kvm on kvm, but what
> >>>> if other hypervisor plays different tricks and builds different ident map for its guest?
> >>> Yes, if other hypervisor doesn't build the 1:1 mapping for its guest, it will fail to work. But I cannot imagine what kind of hypervisor will do this and what the purpose is.
> >>> Anyway, current logic is definitely wrong. It should use L1's identity map instead L0's.
> >>
> >> So something like this is rather needed?
> >>
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index 44494ed..60a3644 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
> >>  	if (enable_ept) {
> >>  		eptp = construct_eptp(cr3);
> >>  		vmcs_write64(EPT_POINTER, eptp);
> >> -		guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
> >> -			vcpu->kvm->arch.ept_identity_map_addr;
> >> +		if (is_paging(vcpu) || is_guest_mode(vcpu))
> >> +			guest_cr3 = kvm_read_cr3(vcpu) :
> >> +		else
> >> +			guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
> >>  		ept_load_pdptrs(vcpu);
> >>  	}
> >>  
> > That what I am thinking, will think about it some more tomorrow.
> 
> OK, and I'll feed it into a local test.
> 
Thought about is some more. So without nested unrestricted guest (nUG)
is_paging() will always be true (since without nUG guest entry is not
possible otherwise) and guest's cr3 will be used, but with nUG identity
map is not used (that is why L2 still works even though wrong identity
map pointer is assigned to cr3), so the code here just corrupts nested
guest's cr3 for no reason and that is why you had to use kvm_read_cr3()
in prepare_vmcs12() to get correct cr3 value. The patch above should be
used instead of original one IMO. How is testing going?

--
			Gleb.

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Jan Kiszka]

On 2013-08-07 14:39, Gleb Natapov wrote:
> On Tue, Aug 06, 2013 at 05:57:02PM +0200, Jan Kiszka wrote:
>> On 2013-08-06 17:53, Gleb Natapov wrote:
>>> On Tue, Aug 06, 2013 at 05:48:54PM +0200, Jan Kiszka wrote:
>>>> On 2013-08-06 17:04, Zhang, Yang Z wrote:
>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>> On Tue, Aug 06, 2013 at 02:12:51PM +0000, Zhang, Yang Z wrote:
>>>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>>>> On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
>>>>>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>>>>>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>
>>>>>>>>>>> If nested EPT is enabled, the L2 guest may change CR3 without any
>>>>>>>>>>> exits. We therefore have to read the current value from the VMCS
>>>>>>>>>>> when switching to L1. However, if paging wasn't enabled, L0 tracks
>>>>>>>>>>> L2's CR3, and GUEST_CR3 rather contains the real-mode identity map.
>>>>>>>>>>> So we need to retrieve CR3 from the architectural state after
>>>>>>>>>>> conditionally updating it - and this is what kvm_read_cr3 does.
>>>>>>>>>>>
>>>>>>>>>> I have a headache from trying to think about it already, but
>>>>>>>>>> shouldn't
>>>>>>>>>> L1 be the one who setups identity map for L2? I traced what
>>>>>>>>>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
>>>>>>>>>> see
>>>>>>>>> Here is my understanding:
>>>>>>>>> In vmx_set_cr3(), if enabled ept, it will check whether target
>>>>>>>>> vcpu is enabling
>>>>>>>> paging. When L2 running in real mode, then target vcpu is not
>>>>>>>> enabling paging and it will use L0's identity map for L2. If you
>>>>>>>> read GUEST_CR3 from VMCS, then you may get the L2's identity map
>>>>>>>> not
>>>>>> L1's.
>>>>>>>>>
>>>>>>>> Yes, but why it makes sense to use L0 identity map for L2? I didn't
>>>>>>>> see different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because
>>>>>>>> L0 and L1 use the same identity map address. When I changed identity
>>>>>>>> address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
>>>>>>>> indeed different, but the real CR3 L2 uses points to L0 identity map.
>>>>>>>> If I zero L1 identity map page L2 still works.
>>>>>>>>
>>>>>>> If L2 in real mode, then L2PA == L1PA. So L0's identity map also works
>>>>>>> if L2 is in real mode.
>>>>>>>
>>>>>> That not the point. It may work accidentally for kvm on kvm, but what
>>>>>> if other hypervisor plays different tricks and builds different ident map for its guest?
>>>>> Yes, if other hypervisor doesn't build the 1:1 mapping for its guest, it will fail to work. But I cannot imagine what kind of hypervisor will do this and what the purpose is.
>>>>> Anyway, current logic is definitely wrong. It should use L1's identity map instead L0's.
>>>>
>>>> So something like this is rather needed?
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index 44494ed..60a3644 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>>>>  	if (enable_ept) {
>>>>  		eptp = construct_eptp(cr3);
>>>>  		vmcs_write64(EPT_POINTER, eptp);
>>>> -		guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
>>>> -			vcpu->kvm->arch.ept_identity_map_addr;
>>>> +		if (is_paging(vcpu) || is_guest_mode(vcpu))
>>>> +			guest_cr3 = kvm_read_cr3(vcpu) :
>>>> +		else
>>>> +			guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
>>>>  		ept_load_pdptrs(vcpu);
>>>>  	}
>>>>  
>>> That what I am thinking, will think about it some more tomorrow.
>>
>> OK, and I'll feed it into a local test.
>>
> Thought about is some more. So without nested unrestricted guest (nUG)
> is_paging() will always be true (since without nUG guest entry is not
> possible otherwise) and guest's cr3 will be used, but with nUG identity
> map is not used (that is why L2 still works even though wrong identity
> map pointer is assigned to cr3), so the code here just corrupts nested
> guest's cr3 for no reason and that is why you had to use kvm_read_cr3()
> in prepare_vmcs12() to get correct cr3 value. The patch above should be
> used instead of original one IMO. How is testing going?

Yes, testing worked fine. I've queued above patch and will send it out
within the next round.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Paolo Bonzini]

On 08/07/2013 02:46 PM, Jan Kiszka wrote:
> On 2013-08-07 14:39, Gleb Natapov wrote:
>> On Tue, Aug 06, 2013 at 05:57:02PM +0200, Jan Kiszka wrote:
>>> On 2013-08-06 17:53, Gleb Natapov wrote:
>>>> On Tue, Aug 06, 2013 at 05:48:54PM +0200, Jan Kiszka wrote:
>>>>> On 2013-08-06 17:04, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>>> On Tue, Aug 06, 2013 at 02:12:51PM +0000, Zhang, Yang Z wrote:
>>>>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>>>>> On Tue, Aug 06, 2013 at 11:44:41AM +0000, Zhang, Yang Z wrote:
>>>>>>>>>> Gleb Natapov wrote on 2013-08-06:
>>>>>>>>>>> On Tue, Aug 06, 2013 at 10:39:59AM +0200, Jan Kiszka wrote:
>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>>
>>>>>>>>>>>> If nested EPT is enabled, the L2 guest may change CR3 without any
>>>>>>>>>>>> exits. We therefore have to read the current value from the VMCS
>>>>>>>>>>>> when switching to L1. However, if paging wasn't enabled, L0 tracks
>>>>>>>>>>>> L2's CR3, and GUEST_CR3 rather contains the real-mode identity map.
>>>>>>>>>>>> So we need to retrieve CR3 from the architectural state after
>>>>>>>>>>>> conditionally updating it - and this is what kvm_read_cr3 does.
>>>>>>>>>>>>
>>>>>>>>>>> I have a headache from trying to think about it already, but
>>>>>>>>>>> shouldn't
>>>>>>>>>>> L1 be the one who setups identity map for L2? I traced what
>>>>>>>>>>> vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) return here and do not
>>>>>>>>>>> see
>>>>>>>>>> Here is my understanding:
>>>>>>>>>> In vmx_set_cr3(), if enabled ept, it will check whether target
>>>>>>>>>> vcpu is enabling
>>>>>>>>> paging. When L2 running in real mode, then target vcpu is not
>>>>>>>>> enabling paging and it will use L0's identity map for L2. If you
>>>>>>>>> read GUEST_CR3 from VMCS, then you may get the L2's identity map
>>>>>>>>> not
>>>>>>> L1's.
>>>>>>>>>>
>>>>>>>>> Yes, but why it makes sense to use L0 identity map for L2? I didn't
>>>>>>>>> see different vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) values because
>>>>>>>>> L0 and L1 use the same identity map address. When I changed identity
>>>>>>>>> address L1 configures vmcs_read64(GUEST_CR3)/kvm_read_cr3(vcpu) are
>>>>>>>>> indeed different, but the real CR3 L2 uses points to L0 identity map.
>>>>>>>>> If I zero L1 identity map page L2 still works.
>>>>>>>>>
>>>>>>>> If L2 in real mode, then L2PA == L1PA. So L0's identity map also works
>>>>>>>> if L2 is in real mode.
>>>>>>>>
>>>>>>> That not the point. It may work accidentally for kvm on kvm, but what
>>>>>>> if other hypervisor plays different tricks and builds different ident map for its guest?
>>>>>> Yes, if other hypervisor doesn't build the 1:1 mapping for its guest, it will fail to work. But I cannot imagine what kind of hypervisor will do this and what the purpose is.
>>>>>> Anyway, current logic is definitely wrong. It should use L1's identity map instead L0's.
>>>>>
>>>>> So something like this is rather needed?
>>>>>
>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>> index 44494ed..60a3644 100644
>>>>> --- a/arch/x86/kvm/vmx.c
>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>> @@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>>>>>   	if (enable_ept) {
>>>>>   		eptp = construct_eptp(cr3);
>>>>>   		vmcs_write64(EPT_POINTER, eptp);
>>>>> -		guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
>>>>> -			vcpu->kvm->arch.ept_identity_map_addr;
>>>>> +		if (is_paging(vcpu) || is_guest_mode(vcpu))
>>>>> +			guest_cr3 = kvm_read_cr3(vcpu) :
>>>>> +		else
>>>>> +			guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
>>>>>   		ept_load_pdptrs(vcpu);
>>>>>   	}
>>>>>
>>>> That what I am thinking, will think about it some more tomorrow.
>>>
>>> OK, and I'll feed it into a local test.
>>>
>> Thought about is some more. So without nested unrestricted guest (nUG)
>> is_paging() will always be true (since without nUG guest entry is not
>> possible otherwise) and guest's cr3 will be used, but with nUG identity
>> map is not used (that is why L2 still works even though wrong identity
>> map pointer is assigned to cr3), so the code here just corrupts nested
>> guest's cr3 for no reason and that is why you had to use kvm_read_cr3()
>> in prepare_vmcs12() to get correct cr3 value. The patch above should be
>> used instead of original one IMO. How is testing going?
>
> Yes, testing worked fine. I've queued above patch and will send it out
> within the next round.

Just reply here with the commit message you desire and Signed-off-by, so 
I can queue it for people who wish to play with nEPT.

Paolo

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Gleb Natapov]

On Wed, Aug 07, 2013 at 03:32:37PM +0200, Paolo Bonzini wrote:
> >>>>>diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >>>>>index 44494ed..60a3644 100644
> >>>>>--- a/arch/x86/kvm/vmx.c
> >>>>>+++ b/arch/x86/kvm/vmx.c
> >>>>>@@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
> >>>>>  	if (enable_ept) {
> >>>>>  		eptp = construct_eptp(cr3);
> >>>>>  		vmcs_write64(EPT_POINTER, eptp);
> >>>>>-		guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
> >>>>>-			vcpu->kvm->arch.ept_identity_map_addr;
> >>>>>+		if (is_paging(vcpu) || is_guest_mode(vcpu))
> >>>>>+			guest_cr3 = kvm_read_cr3(vcpu) :
> >>>>>+		else
> >>>>>+			guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
> >>>>>  		ept_load_pdptrs(vcpu);
> >>>>>  	}
> >>>>>
> >>>>That what I am thinking, will think about it some more tomorrow.
> >>>
> >>>OK, and I'll feed it into a local test.
> >>>
> >>Thought about is some more. So without nested unrestricted guest (nUG)
> >>is_paging() will always be true (since without nUG guest entry is not
> >>possible otherwise) and guest's cr3 will be used, but with nUG identity
> >>map is not used (that is why L2 still works even though wrong identity
> >>map pointer is assigned to cr3), so the code here just corrupts nested
> >>guest's cr3 for no reason and that is why you had to use kvm_read_cr3()
> >>in prepare_vmcs12() to get correct cr3 value. The patch above should be
> >>used instead of original one IMO. How is testing going?
> >
> >Yes, testing worked fine. I've queued above patch and will send it out
> >within the next round.
> 
> Just reply here with the commit message you desire and
> Signed-off-by, so I can queue it for people who wish to play with
> nEPT.
> 
I would love to have a comment there too :)

--
			Gleb.

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Paolo Bonzini]

On 08/07/2013 03:38 PM, Gleb Natapov wrote:
> On Wed, Aug 07, 2013 at 03:32:37PM +0200, Paolo Bonzini wrote:
>>>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>>>> index 44494ed..60a3644 100644
>>>>>>> --- a/arch/x86/kvm/vmx.c
>>>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>>>> @@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>>>>>>>   	if (enable_ept) {
>>>>>>>   		eptp = construct_eptp(cr3);
>>>>>>>   		vmcs_write64(EPT_POINTER, eptp);
>>>>>>> -		guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
>>>>>>> -			vcpu->kvm->arch.ept_identity_map_addr;
>>>>>>> +		if (is_paging(vcpu) || is_guest_mode(vcpu))
>>>>>>> +			guest_cr3 = kvm_read_cr3(vcpu) :
>>>>>>> +		else
>>>>>>> +			guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
>>>>>>>   		ept_load_pdptrs(vcpu);
>>>>>>>   	}
>>>>>>>
>>>>>> That what I am thinking, will think about it some more tomorrow.
>>>>>
>>>>> OK, and I'll feed it into a local test.
>>>>>
>>>> Thought about is some more. So without nested unrestricted guest (nUG)
>>>> is_paging() will always be true (since without nUG guest entry is not
>>>> possible otherwise) and guest's cr3 will be used, but with nUG identity
>>>> map is not used (that is why L2 still works even though wrong identity
>>>> map pointer is assigned to cr3), so the code here just corrupts nested
>>>> guest's cr3 for no reason and that is why you had to use kvm_read_cr3()
>>>> in prepare_vmcs12() to get correct cr3 value. The patch above should be
>>>> used instead of original one IMO. How is testing going?
>>>
>>> Yes, testing worked fine. I've queued above patch and will send it out
>>> within the next round.
>>
>> Just reply here with the commit message you desire and
>> Signed-off-by, so I can queue it for people who wish to play with
>> nEPT.
>
> I would love to have a comment there too :)

Ok, then it can wait since it is only needed with nested unrestricted 
guest.  On the other hand, it should come before patch 4 on the next 
submission.

Paolo

Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Jan Kiszka]

On 2013-08-07 15:54, Paolo Bonzini wrote:
> On 08/07/2013 03:38 PM, Gleb Natapov wrote:
>> On Wed, Aug 07, 2013 at 03:32:37PM +0200, Paolo Bonzini wrote:
>>>>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>>>>> index 44494ed..60a3644 100644
>>>>>>>> --- a/arch/x86/kvm/vmx.c
>>>>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>>>>> @@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu
>>>>>>>> *vcpu, unsigned long cr3)
>>>>>>>>       if (enable_ept) {
>>>>>>>>           eptp = construct_eptp(cr3);
>>>>>>>>           vmcs_write64(EPT_POINTER, eptp);
>>>>>>>> -        guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
>>>>>>>> -            vcpu->kvm->arch.ept_identity_map_addr;
>>>>>>>> +        if (is_paging(vcpu) || is_guest_mode(vcpu))
>>>>>>>> +            guest_cr3 = kvm_read_cr3(vcpu) :
>>>>>>>> +        else
>>>>>>>> +            guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
>>>>>>>>           ept_load_pdptrs(vcpu);
>>>>>>>>       }
>>>>>>>>
>>>>>>> That what I am thinking, will think about it some more tomorrow.
>>>>>>
>>>>>> OK, and I'll feed it into a local test.
>>>>>>
>>>>> Thought about is some more. So without nested unrestricted guest (nUG)
>>>>> is_paging() will always be true (since without nUG guest entry is not
>>>>> possible otherwise) and guest's cr3 will be used, but with nUG
>>>>> identity
>>>>> map is not used (that is why L2 still works even though wrong identity
>>>>> map pointer is assigned to cr3), so the code here just corrupts nested
>>>>> guest's cr3 for no reason and that is why you had to use
>>>>> kvm_read_cr3()
>>>>> in prepare_vmcs12() to get correct cr3 value. The patch above
>>>>> should be
>>>>> used instead of original one IMO. How is testing going?
>>>>
>>>> Yes, testing worked fine. I've queued above patch and will send it out
>>>> within the next round.
>>>
>>> Just reply here with the commit message you desire and
>>> Signed-off-by, so I can queue it for people who wish to play with
>>> nEPT.
>>
>> I would love to have a comment there too :)
> 
> Ok, then it can wait since it is only needed with nested unrestricted
> guest.

Yes, it's related to that feature.

> On the other hand, it should come before patch 4 on the next
> submission.

I'll reorder the whole series, moving the feature enabling to the end.
The ordering still reflects more the history than the dependencies.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 0/8] KVM: nVMX: Enable unrestricted guest mode and fix some nEPT issues

[Paolo Bonzini]

On 08/06/2013 10:39 AM, Jan Kiszka wrote:
> These patches apply on top of nEPT v7.
>
> Changes in v2:
>   - rebased over v7
>   - added patch to fix VM_ENTRY_IA32E_MODE reporting
>
> Jan Kiszka (8):
>    KVM: nEPT: Advertise WB type EPTP
>    KVM: nVMX: Fix up VM_ENTRY_IA32E_MODE control feature reporting
>    KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in
>      load_vmcs12_host_state
>    KVM: nVMX: Enable unrestricted guest mode support
>    KVM: nVMX: Fix guest CR3 read-back on VM-exit
>    KVM: nVMX: Load nEPT state after EFER
>    KVM: nVMX: Implement support for EFER saving on VM-exit
>    KVM: nVMX: Update mmu.base_role.nxe after EFER loading on
>      VM-entry/exit
>
>   arch/x86/kvm/vmx.c |   50 +++++++++++++++++++++++++++++++++++---------------
>   1 files changed, 35 insertions(+), 15 deletions(-)
>

I'm applying 1-2.  Thanks,

Paolo