| Message ID | 1490202597-5926-1-git-send-email-lprosek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-03-22 18:09+0100, Ladi Prosek: > The nested_ept_enabled flag introduced in commit 7ca29de2136 was not > computed correctly. We are interested only in L1's EPT state, not the > the combined L0+L1 value. > > In particular, if L0 uses EPT but L1 does not, nested_ept_enabled must > be false to make sure that PDPSTRs are loaded based on CR3 as usual, > because the special case described in 26.3.2.4 Loading Page-Directory- > Pointer-Table Entries does not apply. > > Reported-by: Wanpeng Li <wanpeng.li@hotmail.com> > Signed-off-by: Ladi Prosek <lprosek@redhat.com> Adding a standardized "Fixes:" tag even if you mention the commit in text allows simpler automated backports to stable kernels, Fixes: 7ca29de21362 ("KVM: nVMX: fix CR3 load if L2 uses PAE paging and EPT") > --- > arch/x86/kvm/vmx.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 98e82ee..a525c72 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -10105,8 +10105,11 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, > SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | > SECONDARY_EXEC_APIC_REGISTER_VIRT); > if (nested_cpu_has(vmcs12, > - CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)) > + CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)) { > exec_control |= vmcs12->secondary_vm_exec_control; > + nested_ept_enabled = (vmcs12->secondary_vm_exec_control & > + SECONDARY_EXEC_ENABLE_EPT) != 0; I'd prefer to get rid of nested_ept_enabled and directly call 'nested_cpu_has_ept(vmcs12)' instead, like the other place in prepare_vmcs02() that asks whether we have nested ept. The change looks correct, Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> Thanks. > + } > > if (exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY) { > vmcs_write64(EOI_EXIT_BITMAP0, > @@ -10121,8 +10124,6 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, > vmcs12->guest_intr_status); > } > > - nested_ept_enabled = (exec_control & SECONDARY_EXEC_ENABLE_EPT) != 0; > - > /* > * Write an illegal value to APIC_ACCESS_ADDR. Later, > * nested_get_vmcs12_pages will either fix it up or > -- > 2.7.4 >
On Wed, Mar 22, 2017 at 6:42 PM, Radim Krčmář <rkrcmar@redhat.com> wrote: > 2017-03-22 18:09+0100, Ladi Prosek: >> The nested_ept_enabled flag introduced in commit 7ca29de2136 was not >> computed correctly. We are interested only in L1's EPT state, not the >> the combined L0+L1 value. >> >> In particular, if L0 uses EPT but L1 does not, nested_ept_enabled must >> be false to make sure that PDPSTRs are loaded based on CR3 as usual, >> because the special case described in 26.3.2.4 Loading Page-Directory- >> Pointer-Table Entries does not apply. >> >> Reported-by: Wanpeng Li <wanpeng.li@hotmail.com> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com> > > Adding a standardized "Fixes:" tag even if you mention the commit in > text allows simpler automated backports to stable kernels, > > Fixes: 7ca29de21362 ("KVM: nVMX: fix CR3 load if L2 uses PAE paging and EPT") > >> --- >> arch/x86/kvm/vmx.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 98e82ee..a525c72 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -10105,8 +10105,11 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, >> SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | >> SECONDARY_EXEC_APIC_REGISTER_VIRT); >> if (nested_cpu_has(vmcs12, >> - CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)) >> + CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)) { >> exec_control |= vmcs12->secondary_vm_exec_control; >> + nested_ept_enabled = (vmcs12->secondary_vm_exec_control & >> + SECONDARY_EXEC_ENABLE_EPT) != 0; > > I'd prefer to get rid of nested_ept_enabled and directly call > 'nested_cpu_has_ept(vmcs12)' instead, like the other place in > prepare_vmcs02() that asks whether we have nested ept. Oops, I completely missed that. I'll send a v2, thanks! > The change looks correct, > > Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > > Thanks. > >> + } >> >> if (exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY) { >> vmcs_write64(EOI_EXIT_BITMAP0, >> @@ -10121,8 +10124,6 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, >> vmcs12->guest_intr_status); >> } >> >> - nested_ept_enabled = (exec_control & SECONDARY_EXEC_ENABLE_EPT) != 0; >> - >> /* >> * Write an illegal value to APIC_ACCESS_ADDR. Later, >> * nested_get_vmcs12_pages will either fix it up or >> -- >> 2.7.4 >>
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 98e82ee..a525c72 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10105,8 +10105,11 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | SECONDARY_EXEC_APIC_REGISTER_VIRT); if (nested_cpu_has(vmcs12, - CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)) + CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)) { exec_control |= vmcs12->secondary_vm_exec_control; + nested_ept_enabled = (vmcs12->secondary_vm_exec_control & + SECONDARY_EXEC_ENABLE_EPT) != 0; + } if (exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY) { vmcs_write64(EOI_EXIT_BITMAP0, @@ -10121,8 +10124,6 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, vmcs12->guest_intr_status); } - nested_ept_enabled = (exec_control & SECONDARY_EXEC_ENABLE_EPT) != 0; - /* * Write an illegal value to APIC_ACCESS_ADDR. Later, * nested_get_vmcs12_pages will either fix it up or
The nested_ept_enabled flag introduced in commit 7ca29de2136 was not computed correctly. We are interested only in L1's EPT state, not the the combined L0+L1 value. In particular, if L0 uses EPT but L1 does not, nested_ept_enabled must be false to make sure that PDPSTRs are loaded based on CR3 as usual, because the special case described in 26.3.2.4 Loading Page-Directory- Pointer-Table Entries does not apply. Reported-by: Wanpeng Li <wanpeng.li@hotmail.com> Signed-off-by: Ladi Prosek <lprosek@redhat.com> --- arch/x86/kvm/vmx.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)