| Message ID | 20170112022829.15140-1-srutherford@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
----- Original Message ----- > From: "Steve Rutherford" <srutherford@google.com> > To: kvm@vger.kernel.org > Cc: pbonzini@redhat.com, dvyukov@google.com, rkrcmar@redhat.com, ppandit@redhat.com, kernellwp@gmail.com > Sent: Thursday, January 12, 2017 3:28:29 AM > Subject: [PATCH] KVM: x86: Introduce segmented_write_std > > Introduces segemented_write_std. > > Switches from emulated reads/writes to standard read/writes in fxsave, > fxrstor, sgdt, and sidt. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Steve Rutherford <srutherford@google.com> > --- > arch/x86/kvm/emulate.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 2b8349a2b14b..ad258aa0b302 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -819,6 +819,20 @@ static int segmented_read_std(struct x86_emulate_ctxt > *ctxt, > return ctxt->ops->read_std(ctxt, linear, data, size, &ctxt->exception); > } > > +static int segmented_write_std(struct x86_emulate_ctxt *ctxt, > + struct segmented_address addr, > + void *data, > + unsigned int size) > +{ > + int rc; > + ulong linear; > + > + rc = linearize(ctxt, addr, size, true, &linear); > + if (rc != X86EMUL_CONTINUE) > + return rc; > + return ctxt->ops->write_std(ctxt, linear, data, size, &ctxt->exception); > +} > + > /* > * Prefetch the remaining bytes of the instruction without crossing page > * boundary if they are not in fetch_cache yet. > @@ -3686,8 +3700,8 @@ static int emulate_store_desc_ptr(struct > x86_emulate_ctxt *ctxt, > } > /* Disable writeback. */ > ctxt->dst.type = OP_NONE; > - return segmented_write(ctxt, ctxt->dst.addr.mem, > - &desc_ptr, 2 + ctxt->op_bytes); > + return segmented_write_std(ctxt, ctxt->dst.addr.mem, > + &desc_ptr, 2 + ctxt->op_bytes); > } > > static int em_sgdt(struct x86_emulate_ctxt *ctxt) > @@ -3933,7 +3947,7 @@ static int em_fxsave(struct x86_emulate_ctxt *ctxt) > else > size = offsetof(struct fxregs_state, xmm_space[0]); > > - return segmented_write(ctxt, ctxt->memop.addr.mem, &fx_state, size); > + return segmented_write_std(ctxt, ctxt->memop.addr.mem, &fx_state, size); > } > > static int fxrstor_fixup(struct x86_emulate_ctxt *ctxt, > @@ -3975,7 +3989,7 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt) > if (rc != X86EMUL_CONTINUE) > return rc; > > - rc = segmented_read(ctxt, ctxt->memop.addr.mem, &fx_state, 512); > + rc = segmented_read_std(ctxt, ctxt->memop.addr.mem, &fx_state, 512); > if (rc != X86EMUL_CONTINUE) > return rc; > > -- > 2.11.0.390.gc69c2f50cf-goog > > Queued for 4.10, thanks. At least fxsave/fxrstor is not in any released version, but that was close. I owe Dmitry some (more) beer. Paolo -- 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
Why attempt to emulate these instructions at all, if we're not going to handle a data access to emulated/special memory? It seems that one of the following three cases must hold: 1) The data accessed by the instruction is emulated/special memory. 2) The instruction was fetched from emulated/special memory. 3) The instruction has been modified since the VM-exit. The proposed patch is incorrect for case (1). Case (2) violates the emulator's assumptions outlined in kvm_emulate.h. Case (3) seems best handled by simply re-entering VMX non-root mode. On Thu, Jan 12, 2017 at 5:40 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > ----- Original Message ----- >> From: "Steve Rutherford" <srutherford@google.com> >> To: kvm@vger.kernel.org >> Cc: pbonzini@redhat.com, dvyukov@google.com, rkrcmar@redhat.com, ppandit@redhat.com, kernellwp@gmail.com >> Sent: Thursday, January 12, 2017 3:28:29 AM >> Subject: [PATCH] KVM: x86: Introduce segmented_write_std >> >> Introduces segemented_write_std. >> >> Switches from emulated reads/writes to standard read/writes in fxsave, >> fxrstor, sgdt, and sidt. >> >> Reported-by: Dmitry Vyukov <dvyukov@google.com> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Steve Rutherford <srutherford@google.com> >> --- >> arch/x86/kvm/emulate.c | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c >> index 2b8349a2b14b..ad258aa0b302 100644 >> --- a/arch/x86/kvm/emulate.c >> +++ b/arch/x86/kvm/emulate.c >> @@ -819,6 +819,20 @@ static int segmented_read_std(struct x86_emulate_ctxt >> *ctxt, >> return ctxt->ops->read_std(ctxt, linear, data, size, &ctxt->exception); >> } >> >> +static int segmented_write_std(struct x86_emulate_ctxt *ctxt, >> + struct segmented_address addr, >> + void *data, >> + unsigned int size) >> +{ >> + int rc; >> + ulong linear; >> + >> + rc = linearize(ctxt, addr, size, true, &linear); >> + if (rc != X86EMUL_CONTINUE) >> + return rc; >> + return ctxt->ops->write_std(ctxt, linear, data, size, &ctxt->exception); >> +} >> + >> /* >> * Prefetch the remaining bytes of the instruction without crossing page >> * boundary if they are not in fetch_cache yet. >> @@ -3686,8 +3700,8 @@ static int emulate_store_desc_ptr(struct >> x86_emulate_ctxt *ctxt, >> } >> /* Disable writeback. */ >> ctxt->dst.type = OP_NONE; >> - return segmented_write(ctxt, ctxt->dst.addr.mem, >> - &desc_ptr, 2 + ctxt->op_bytes); >> + return segmented_write_std(ctxt, ctxt->dst.addr.mem, >> + &desc_ptr, 2 + ctxt->op_bytes); >> } >> >> static int em_sgdt(struct x86_emulate_ctxt *ctxt) >> @@ -3933,7 +3947,7 @@ static int em_fxsave(struct x86_emulate_ctxt *ctxt) >> else >> size = offsetof(struct fxregs_state, xmm_space[0]); >> >> - return segmented_write(ctxt, ctxt->memop.addr.mem, &fx_state, size); >> + return segmented_write_std(ctxt, ctxt->memop.addr.mem, &fx_state, size); >> } >> >> static int fxrstor_fixup(struct x86_emulate_ctxt *ctxt, >> @@ -3975,7 +3989,7 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt) >> if (rc != X86EMUL_CONTINUE) >> return rc; >> >> - rc = segmented_read(ctxt, ctxt->memop.addr.mem, &fx_state, 512); >> + rc = segmented_read_std(ctxt, ctxt->memop.addr.mem, &fx_state, 512); >> if (rc != X86EMUL_CONTINUE) >> return rc; >> >> -- >> 2.11.0.390.gc69c2f50cf-goog >> >> > > Queued for 4.10, thanks. At least fxsave/fxrstor is not in any > released version, but that was close. I owe Dmitry some (more) beer. > > Paolo > -- > 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 -- 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 20/01/2017 17:55, Jim Mattson wrote: > Why attempt to emulate these instructions at all, if we're not going > to handle a data access to emulated/special memory? > > It seems that one of the following three cases must hold: > > 1) The data accessed by the instruction is emulated/special memory. > 2) The instruction was fetched from emulated/special memory. > 3) The instruction has been modified since the VM-exit. 4) The processor is in big real mode and you don't have unrestricted guest support in your processor (or you disabled EPT). Paolo > The proposed patch is incorrect for case (1). Case (2) violates the > emulator's assumptions outlined in kvm_emulate.h. Case (3) seems best > handled by simply re-entering VMX non-root mode. -- 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
2017-01-20 18:09+0100, Paolo Bonzini: > On 20/01/2017 17:55, Jim Mattson wrote: >> Why attempt to emulate these instructions at all, if we're not going >> to handle a data access to emulated/special memory? >> >> It seems that one of the following three cases must hold: >> >> 1) The data accessed by the instruction is emulated/special memory. >> 2) The instruction was fetched from emulated/special memory. >> 3) The instruction has been modified since the VM-exit. > > 4) The processor is in big real mode and you don't have unrestricted > guest support in your processor (or you disabled EPT). What about marking instructions that are not expected to access emulated memory? For now, we could WARN_ONCE if they do, which would pave a way to make unrestricted guest mandatory. Then we would drop instructions that were not needed with the hope that they won't be. (This would imply mandatory EPT. Also a benefit, IMO.) Westmere (the architecture to introduce unrestricted guest) is from 2010, which makes it close to being endangered by expired extended warranties. -- 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
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 2b8349a2b14b..ad258aa0b302 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -819,6 +819,20 @@ static int segmented_read_std(struct x86_emulate_ctxt *ctxt, return ctxt->ops->read_std(ctxt, linear, data, size, &ctxt->exception); } +static int segmented_write_std(struct x86_emulate_ctxt *ctxt, + struct segmented_address addr, + void *data, + unsigned int size) +{ + int rc; + ulong linear; + + rc = linearize(ctxt, addr, size, true, &linear); + if (rc != X86EMUL_CONTINUE) + return rc; + return ctxt->ops->write_std(ctxt, linear, data, size, &ctxt->exception); +} + /* * Prefetch the remaining bytes of the instruction without crossing page * boundary if they are not in fetch_cache yet. @@ -3686,8 +3700,8 @@ static int emulate_store_desc_ptr(struct x86_emulate_ctxt *ctxt, } /* Disable writeback. */ ctxt->dst.type = OP_NONE; - return segmented_write(ctxt, ctxt->dst.addr.mem, - &desc_ptr, 2 + ctxt->op_bytes); + return segmented_write_std(ctxt, ctxt->dst.addr.mem, + &desc_ptr, 2 + ctxt->op_bytes); } static int em_sgdt(struct x86_emulate_ctxt *ctxt) @@ -3933,7 +3947,7 @@ static int em_fxsave(struct x86_emulate_ctxt *ctxt) else size = offsetof(struct fxregs_state, xmm_space[0]); - return segmented_write(ctxt, ctxt->memop.addr.mem, &fx_state, size); + return segmented_write_std(ctxt, ctxt->memop.addr.mem, &fx_state, size); } static int fxrstor_fixup(struct x86_emulate_ctxt *ctxt, @@ -3975,7 +3989,7 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt) if (rc != X86EMUL_CONTINUE) return rc; - rc = segmented_read(ctxt, ctxt->memop.addr.mem, &fx_state, 512); + rc = segmented_read_std(ctxt, ctxt->memop.addr.mem, &fx_state, 512); if (rc != X86EMUL_CONTINUE) return rc;
Introduces segemented_write_std. Switches from emulated reads/writes to standard read/writes in fxsave, fxrstor, sgdt, and sidt. Reported-by: Dmitry Vyukov <dvyukov@google.com> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Steve Rutherford <srutherford@google.com> --- arch/x86/kvm/emulate.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)