| Message ID | e90fbb85a6f1cc34167f8b7f908c3dd01b694f9b.1375971992.git.jan.kiszka@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 08, 2013 at 04:26:30PM +0200, Jan Kiszka wrote: > 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. > Hmm, I do not see how nested ept mmu depends on guests EFER.NX setting. It just sets mmu->nx to true. > 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 6c42518..363fe19 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -7727,11 +7727,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) > @@ -7741,6 +7736,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 -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-09-02 15:16, Gleb Natapov wrote: > On Thu, Aug 08, 2013 at 04:26:30PM +0200, Jan Kiszka wrote: >> 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. >> > Hmm, I do not see how nested ept mmu depends on guests EFER.NX setting. > It just sets mmu->nx to true. Don't ask me for the details behind this, but update_permission_bitmask called by kvm_init_shadow_ept_mmu is using it e.g. And the "before-after" effect was clearly visible when L2 and L1 were using different NX settings. Maybe Arthur can write a test for this. Jan > >> 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 6c42518..363fe19 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -7727,11 +7727,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) >> @@ -7741,6 +7736,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 > > -- > Gleb. >
On Mon, Sep 02, 2013 at 07:58:30PM +0200, Jan Kiszka wrote: > On 2013-09-02 15:16, Gleb Natapov wrote: > > On Thu, Aug 08, 2013 at 04:26:30PM +0200, Jan Kiszka wrote: > >> 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. > >> > > Hmm, I do not see how nested ept mmu depends on guests EFER.NX setting. > > It just sets mmu->nx to true. > > Don't ask me for the details behind this, but update_permission_bitmask > called by kvm_init_shadow_ept_mmu is using it e.g. And the It uses it only in !ept case though and never looks at a guest setting as far as I can tell. Is it possible that this was an artifact of all nEPT code and the latest one does not need this patch? > "before-after" effect was clearly visible when L2 and L1 were using > different NX settings. Maybe Arthur can write a test for this. > > Jan > > > > >> 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 6c42518..363fe19 100644 > >> --- a/arch/x86/kvm/vmx.c > >> +++ b/arch/x86/kvm/vmx.c > >> @@ -7727,11 +7727,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) > >> @@ -7741,6 +7736,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 > > > > -- > > Gleb. > > > > -- > Siemens AG, Corporate Technology, CT RTC ITP SES-DE > Corporate Competence Center Embedded Linux -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-09-02 20:09, Gleb Natapov wrote: > On Mon, Sep 02, 2013 at 07:58:30PM +0200, Jan Kiszka wrote: >> On 2013-09-02 15:16, Gleb Natapov wrote: >>> On Thu, Aug 08, 2013 at 04:26:30PM +0200, Jan Kiszka wrote: >>>> 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. >>>> >>> Hmm, I do not see how nested ept mmu depends on guests EFER.NX setting. >>> It just sets mmu->nx to true. >> >> Don't ask me for the details behind this, but update_permission_bitmask >> called by kvm_init_shadow_ept_mmu is using it e.g. And the > It uses it only in !ept case though and never looks at a guest setting > as far as I can tell. Is it possible that this was an artifact of all > nEPT code and the latest one does not need this patch? Hmm, possibly. Let me recheck, hope I can find the reproduction pattern again... Jan
On 2013-09-02 20:20, Jan Kiszka wrote: > On 2013-09-02 20:09, Gleb Natapov wrote: >> On Mon, Sep 02, 2013 at 07:58:30PM +0200, Jan Kiszka wrote: >>> On 2013-09-02 15:16, Gleb Natapov wrote: >>>> On Thu, Aug 08, 2013 at 04:26:30PM +0200, Jan Kiszka wrote: >>>>> 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. >>>>> >>>> Hmm, I do not see how nested ept mmu depends on guests EFER.NX setting. >>>> It just sets mmu->nx to true. >>> >>> Don't ask me for the details behind this, but update_permission_bitmask >>> called by kvm_init_shadow_ept_mmu is using it e.g. And the >> It uses it only in !ept case though and never looks at a guest setting >> as far as I can tell. Is it possible that this was an artifact of all >> nEPT code and the latest one does not need this patch? > > Hmm, possibly. Let me recheck, hope I can find the reproduction pattern > again... Yeah, drop it. Things work fine without it, and I just found an old version of nEPT where kvm_init_shadow_EPT_mmu did this: context->nx = is_nx(vcpu); /* TODO: ? */ Obviously, this patch was a workaround for that issue. Jan
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 6c42518..363fe19 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -7727,11 +7727,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) @@ -7741,6 +7736,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.
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(-)