| Message ID | 20210823114618.1184209-3-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: few more SMM fixes | expand |
On Mon, Aug 23, 2021, Maxim Levitsky wrote: > KVM_REQ_GET_NESTED_STATE_PAGES is also used with VM entries that happen > on exit from SMM mode, and in this case PDPTRS must be always reloaded. > > Thanks to Sean Christopherson for pointing this out. > > Fixes: 0f85722341b0 ("KVM: nVMX: delay loading of PDPTRs to KVM_REQ_GET_NESTED_STATE_PAGES") > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/x86/kvm/vmx/vmx.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index fada1055f325..4194fbf5e5d6 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -7504,6 +7504,13 @@ static int vmx_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) > } > > if (vmx->nested.smm.guest_mode) { > + > + /* Exit from the SMM to the non root mode also uses Just "Exit from SMM to non-root mode", i.e. drop the "the". Multi-line comments should look like: /* * Exit from SMM ... though oddly checkpatch doesn't complain about that. That said, for the comment, it'd be more helpful to explain why the PDPTRs should not come from userspace. Something like: /* * Always reload the guest's version of the PDPTRs when exiting * from SMM to non-root. If KVM_SET_SREGS2 stuffs PDPTRs while * SMM is active, that state is specifically for SMM context. * On RSM, all guest state is pulled from its architectural * location, whatever that may be. */ Though typing that makes me wonder if this is fixing the wrong thing. It seems like pdptrs_from_userspace shouldn't be set when SMM is active, though I suppose there's a potential ordering issue between KVM_SET_SREGS2 and KVM_SET_VCPU_EVENTS. Bummer. > + * the KVM_REQ_GET_NESTED_STATE_PAGES request, > + * but in this case the pdptrs must be always reloaded > + */ > + vcpu->arch.pdptrs_from_userspace = false; > + > ret = nested_vmx_enter_non_root_mode(vcpu, false); > if (ret) > return ret; > -- > 2.26.3 >
On Thu, 2021-09-09 at 00:55 +0000, Sean Christopherson wrote: > On Mon, Aug 23, 2021, Maxim Levitsky wrote: > > KVM_REQ_GET_NESTED_STATE_PAGES is also used with VM entries that happen > > on exit from SMM mode, and in this case PDPTRS must be always reloaded. > > > > Thanks to Sean Christopherson for pointing this out. > > > > Fixes: 0f85722341b0 ("KVM: nVMX: delay loading of PDPTRs to KVM_REQ_GET_NESTED_STATE_PAGES") > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index fada1055f325..4194fbf5e5d6 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -7504,6 +7504,13 @@ static int vmx_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) > > } > > > > if (vmx->nested.smm.guest_mode) { > > + > > + /* Exit from the SMM to the non root mode also uses > > Just "Exit from SMM to non-root mode", i.e. drop the "the". > > Multi-line comments should look like: > > /* > * Exit from SMM ... > > though oddly checkpatch doesn't complain about that. > > That said, for the comment, it'd be more helpful to explain why the PDPTRs should > not come from userspace. Something like: > > /* > * Always reload the guest's version of the PDPTRs when exiting > * from SMM to non-root. If KVM_SET_SREGS2 stuffs PDPTRs while > * SMM is active, that state is specifically for SMM context. > * On RSM, all guest state is pulled from its architectural > * location, whatever that may be. > */ > > Though typing that makes me wonder if this is fixing the wrong thing. It seems > like pdptrs_from_userspace shouldn't be set when SMM is active, though I suppose > there's a potential ordering issue between KVM_SET_SREGS2 and KVM_SET_VCPU_EVENTS. > Bummer. > > > + * the KVM_REQ_GET_NESTED_STATE_PAGES request, > > + * but in this case the pdptrs must be always reloaded > > + */ > > + vcpu->arch.pdptrs_from_userspace = false; > > + > > ret = nested_vmx_enter_non_root_mode(vcpu, false); > > if (ret) > > return ret; > > -- > > 2.26.3 > > I went with your suggestion in the patch 3, and dropped this patch. Thanks! Best regards, Maxim Levitsky
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index fada1055f325..4194fbf5e5d6 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7504,6 +7504,13 @@ static int vmx_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) } if (vmx->nested.smm.guest_mode) { + + /* Exit from the SMM to the non root mode also uses + * the KVM_REQ_GET_NESTED_STATE_PAGES request, + * but in this case the pdptrs must be always reloaded + */ + vcpu->arch.pdptrs_from_userspace = false; + ret = nested_vmx_enter_non_root_mode(vcpu, false); if (ret) return ret;
KVM_REQ_GET_NESTED_STATE_PAGES is also used with VM entries that happen on exit from SMM mode, and in this case PDPTRS must be always reloaded. Thanks to Sean Christopherson for pointing this out. Fixes: 0f85722341b0 ("KVM: nVMX: delay loading of PDPTRs to KVM_REQ_GET_NESTED_STATE_PAGES") Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/vmx/vmx.c | 7 +++++++ 1 file changed, 7 insertions(+)