| Message ID | 1480513841-7565-11-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/30/2016 08:50 AM, Andrew Cooper wrote: > The common case is already using fault semantics out of x86_emulate(), as that > is how VT-x/SVM expects to inject the event (given suitable hardware support). > > However, x86_emulate() returning X86EMUL_EXCEPTION and also completing a > register writeback is problematic for callers. > > Switch the logic to always using fault semantics, and leave svm_inject_trap() > to fix up %eip if necessary. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Tim Deegan <tim@xen.org> > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: > @@ -1242,27 +1242,38 @@ static void svm_inject_event(const struct x86_event *event) > eventinj.fields.v = 1; > eventinj.fields.vector = _event.vector; > > - /* Refer to AMD Vol 2: System Programming, 15.20 Event Injection. */ > + /* > + * Refer to AMD Vol 2: System Programming, 15.20 Event Injection. > + * > + * On hardware lacking NextRIP support, and all hardware in the case of > + * icebp, software events with trap semantics need emulating, so %eip in > + * the trap frame points after the instruction. > + * > + * The x86 emulator (if requested by the x86_swint_emulate_* choice) will > + * have performed checks such as presence/dpl/etc and believes that the > + * event injection will succeed without faulting. > + * > + * The x86 emulator will always provide fault semantics for software > + * events, with _trap.insn_len set appropriately. If the injection > + * requires emulation, move %eip forwards at this point. > + */ > switch ( _event.type ) > { > case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */ > - /* > - * Software interrupts (type 4) cannot be properly injected if the > - * processor doesn't support NextRIP. Without NextRIP, the emulator > - * will have performed DPL and presence checks for us, and will have > - * moved eip forward if appropriate. > - */ > if ( cpu_has_svm_nrips ) > vmcb->nextrip = regs->eip + _event.insn_len; > + else > + regs->eip += _event.insn_len; Please use ->rip here and below (and perhaps also in the comment). > eventinj.fields.type = X86_EVENTTYPE_SW_INTERRUPT; > break; > > case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */ > /* > - * icebp's injection must always be emulated. Software injection help > - * in x86_emulate has moved eip forward, but NextRIP (if used) still > - * needs setting or execution will resume from 0. > + * icebp's injection must always be emulated, as hardware does not > + * special case HW_EXCEPTION with vector 1 (#DB) as having trap > + * semantics. > */ > + regs->eip += _event.insn_len; > if ( cpu_has_svm_nrips ) > vmcb->nextrip = regs->eip; > eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION; > @@ -1270,16 +1281,13 @@ static void svm_inject_event(const struct x86_event *event) > > case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */ > /* > - * The AMD manual states that .type=3 (HW exception), .vector=3 or 4, > - * will perform DPL checks. Experimentally, DPL and presence checks > - * are indeed performed, even without NextRIP support. > - * > - * However without NextRIP support, the event injection still needs > - * fully emulating to get the correct eip in the trap frame, yet get > - * the correct faulting eip should a fault occur. > + * Hardware special cases HW_EXCEPTION with vectors 3 and 4 as having > + * trap semantics, and will perform DPL checks. > */ > if ( cpu_has_svm_nrips ) > vmcb->nextrip = regs->eip + _event.insn_len; > + else > + regs->eip += _event.insn_len; > eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION; > break; By moving the adding to reg->rip here, you bypass x86_emulate()'s zeroing of the upper 32 bits outside of 64-bit mode, so you now need to replicate it below here. > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -1694,8 +1694,6 @@ static int inject_swint(enum x86_swint_type type, > goto raise_exn; > } > } > - > - ctxt->regs->eip += insn_len; > } > > rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt); I think for the patch description to be correct, this call's return value needs to be altered, for inject_swint() to return EXCEPTION when OKAY comes back here (which iirc you do in a later patch when you eliminate this hook). > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -64,7 +64,13 @@ enum x86_swint_type { > x86_swint_int, /* 0xcd $n */ > }; > > -/* How much help is required with software event injection? */ > +/* > + * How much help is required with software event injection? > + * > + * All software events return from x86_emulate() with X86EMUL_EXCEPTION and > + * fault-like semantics. This just controls whether the emulator performs > + * presence/dpl/etc checks and possibly raises excepions instead. exceptions Jan
On 01/12/16 10:53, Jan Beulich wrote: > >> eventinj.fields.type = X86_EVENTTYPE_SW_INTERRUPT; >> break; >> >> case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */ >> /* >> - * icebp's injection must always be emulated. Software injection help >> - * in x86_emulate has moved eip forward, but NextRIP (if used) still >> - * needs setting or execution will resume from 0. >> + * icebp's injection must always be emulated, as hardware does not >> + * special case HW_EXCEPTION with vector 1 (#DB) as having trap >> + * semantics. >> */ >> + regs->eip += _event.insn_len; >> if ( cpu_has_svm_nrips ) >> vmcb->nextrip = regs->eip; >> eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION; >> @@ -1270,16 +1281,13 @@ static void svm_inject_event(const struct x86_event *event) >> >> case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */ >> /* >> - * The AMD manual states that .type=3 (HW exception), .vector=3 or 4, >> - * will perform DPL checks. Experimentally, DPL and presence checks >> - * are indeed performed, even without NextRIP support. >> - * >> - * However without NextRIP support, the event injection still needs >> - * fully emulating to get the correct eip in the trap frame, yet get >> - * the correct faulting eip should a fault occur. >> + * Hardware special cases HW_EXCEPTION with vectors 3 and 4 as having >> + * trap semantics, and will perform DPL checks. >> */ >> if ( cpu_has_svm_nrips ) >> vmcb->nextrip = regs->eip + _event.insn_len; >> + else >> + regs->eip += _event.insn_len; >> eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION; >> break; > By moving the adding to reg->rip here, you bypass x86_emulate()'s > zeroing of the upper 32 bits outside of 64-bit mode, so you now need > to replicate it below here. Hmm - this was actually already broken with the nextrip adjustment. I will introduce a fixup at the end of this switch statement covering both fields. > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -1694,8 +1694,6 @@ static int inject_swint(enum x86_swint_type type, >> goto raise_exn; >> } >> } >> - >> - ctxt->regs->eip += insn_len; >> } >> >> rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt); > I think for the patch description to be correct, this call's return value > needs to be altered, for inject_swint() to return EXCEPTION when > OKAY comes back here (which iirc you do in a later patch when you > eliminate this hook). At the moment, the sole user is swint: rc = inject_swint(swint_type, (uint8_t)src.val, _regs.eip - ctxt->regs->eip, ctxt, ops) ? : X86EMUL_EXCEPTION; goto done; so the description is correct. We currently have a uniform pattern of the injection functions returning OKAY, and being converted to EXCEPTION by the caller. If this wants fixing at all, it wants fixing later when switching to using x86_emul_software_event(). > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -64,7 +64,13 @@ enum x86_swint_type { >> x86_swint_int, /* 0xcd $n */ >> }; >> >> -/* How much help is required with software event injection? */ >> +/* >> + * How much help is required with software event injection? >> + * >> + * All software events return from x86_emulate() with X86EMUL_EXCEPTION and >> + * fault-like semantics. This just controls whether the emulator performs >> + * presence/dpl/etc checks and possibly raises excepions instead. > exceptions I had already noticed and fixed this. ~Andrew
>>> On 01.12.16 at 12:15, <andrew.cooper3@citrix.com> wrote: > On 01/12/16 10:53, Jan Beulich wrote: >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >>> @@ -1694,8 +1694,6 @@ static int inject_swint(enum x86_swint_type type, >>> goto raise_exn; >>> } >>> } >>> - >>> - ctxt->regs->eip += insn_len; >>> } >>> >>> rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt); >> I think for the patch description to be correct, this call's return value >> needs to be altered, for inject_swint() to return EXCEPTION when >> OKAY comes back here (which iirc you do in a later patch when you >> eliminate this hook). > > At the moment, the sole user is > > swint: > rc = inject_swint(swint_type, (uint8_t)src.val, > _regs.eip - ctxt->regs->eip, > ctxt, ops) ? : X86EMUL_EXCEPTION; > goto done; > > so the description is correct. Oh - I should have checked the caller. Sorry for the noise. Jan
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 912d871..65eeab7 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1209,7 +1209,7 @@ static void svm_inject_event(const struct x86_event *event) struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb; eventinj_t eventinj = vmcb->eventinj; struct x86_event _event = *event; - const struct cpu_user_regs *regs = guest_cpu_user_regs(); + struct cpu_user_regs *regs = guest_cpu_user_regs(); switch ( _event.vector ) { @@ -1242,27 +1242,38 @@ static void svm_inject_event(const struct x86_event *event) eventinj.fields.v = 1; eventinj.fields.vector = _event.vector; - /* Refer to AMD Vol 2: System Programming, 15.20 Event Injection. */ + /* + * Refer to AMD Vol 2: System Programming, 15.20 Event Injection. + * + * On hardware lacking NextRIP support, and all hardware in the case of + * icebp, software events with trap semantics need emulating, so %eip in + * the trap frame points after the instruction. + * + * The x86 emulator (if requested by the x86_swint_emulate_* choice) will + * have performed checks such as presence/dpl/etc and believes that the + * event injection will succeed without faulting. + * + * The x86 emulator will always provide fault semantics for software + * events, with _trap.insn_len set appropriately. If the injection + * requires emulation, move %eip forwards at this point. + */ switch ( _event.type ) { case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */ - /* - * Software interrupts (type 4) cannot be properly injected if the - * processor doesn't support NextRIP. Without NextRIP, the emulator - * will have performed DPL and presence checks for us, and will have - * moved eip forward if appropriate. - */ if ( cpu_has_svm_nrips ) vmcb->nextrip = regs->eip + _event.insn_len; + else + regs->eip += _event.insn_len; eventinj.fields.type = X86_EVENTTYPE_SW_INTERRUPT; break; case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */ /* - * icebp's injection must always be emulated. Software injection help - * in x86_emulate has moved eip forward, but NextRIP (if used) still - * needs setting or execution will resume from 0. + * icebp's injection must always be emulated, as hardware does not + * special case HW_EXCEPTION with vector 1 (#DB) as having trap + * semantics. */ + regs->eip += _event.insn_len; if ( cpu_has_svm_nrips ) vmcb->nextrip = regs->eip; eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION; @@ -1270,16 +1281,13 @@ static void svm_inject_event(const struct x86_event *event) case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */ /* - * The AMD manual states that .type=3 (HW exception), .vector=3 or 4, - * will perform DPL checks. Experimentally, DPL and presence checks - * are indeed performed, even without NextRIP support. - * - * However without NextRIP support, the event injection still needs - * fully emulating to get the correct eip in the trap frame, yet get - * the correct faulting eip should a fault occur. + * Hardware special cases HW_EXCEPTION with vectors 3 and 4 as having + * trap semantics, and will perform DPL checks. */ if ( cpu_has_svm_nrips ) vmcb->nextrip = regs->eip + _event.insn_len; + else + regs->eip += _event.insn_len; eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION; break; diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index e4643a3..8a1f1f5 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1694,8 +1694,6 @@ static int inject_swint(enum x86_swint_type type, goto raise_exn; } } - - ctxt->regs->eip += insn_len; } rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt); diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index f84ced2..fc28976 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -64,7 +64,13 @@ enum x86_swint_type { x86_swint_int, /* 0xcd $n */ }; -/* How much help is required with software event injection? */ +/* + * How much help is required with software event injection? + * + * All software events return from x86_emulate() with X86EMUL_EXCEPTION and + * fault-like semantics. This just controls whether the emulator performs + * presence/dpl/etc checks and possibly raises excepions instead. + */ enum x86_swint_emulation { x86_swint_emulate_none, /* Hardware supports all software injection properly */ x86_swint_emulate_icebp,/* Help needed with `icebp` (0xf1) */
The common case is already using fault semantics out of x86_emulate(), as that is how VT-x/SVM expects to inject the event (given suitable hardware support). However, x86_emulate() returning X86EMUL_EXCEPTION and also completing a register writeback is problematic for callers. Switch the logic to always using fault semantics, and leave svm_inject_trap() to fix up %eip if necessary. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> v3: * New --- xen/arch/x86/hvm/svm/svm.c | 44 ++++++++++++++++++++-------------- xen/arch/x86/x86_emulate/x86_emulate.c | 2 -- xen/arch/x86/x86_emulate/x86_emulate.h | 8 ++++++- 3 files changed, 33 insertions(+), 21 deletions(-)