Message ID | 58ED1413020000780014FF9C@prv-mh.provo.novell.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 11/04/17 16:36, Jan Beulich wrote: > No matter that we emulate IRET for (guest) real more only right now, we > should get its effect on (virtual) NMI delivery right. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Adjust x86_emulate_wrapper() accordingly. > > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e > memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes); > } > > - if ( rc != X86EMUL_OKAY ) > - return rc; > + new_intr_shadow = hvmemul_ctxt->intr_shadow; > > - if ( hvmemul_ctxt->ctxt.retire.singlestep ) > - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + /* > + * IRET, if valid in the given context, clears NMI blocking > + * (irrespective of rc). > + */ > + if ( hvmemul_ctxt->ctxt.retire.unblock_nmi ) > + new_intr_shadow &= ~HVM_INTR_SHADOW_NMI; > > - new_intr_shadow = hvmemul_ctxt->intr_shadow; > + if ( rc == X86EMUL_OKAY ) > + { On further thought, given the assertion, you don't need to introduce this check, and can avoid the block indentation. It should make the patch rather smaller. > + if ( hvmemul_ctxt->ctxt.retire.singlestep ) > + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > > - /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */ > - if ( hvmemul_ctxt->ctxt.retire.mov_ss ) > - new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS; > - else > - new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS; > - > - /* STI instruction toggles STI shadow, else we just clear it. */ > - if ( hvmemul_ctxt->ctxt.retire.sti ) > - new_intr_shadow ^= HVM_INTR_SHADOW_STI; > - else > - new_intr_shadow &= ~HVM_INTR_SHADOW_STI; > + /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */ > + if ( hvmemul_ctxt->ctxt.retire.mov_ss ) > + new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS; > + else > + new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS; > + > + /* STI instruction toggles STI shadow, else we just clear it. */ > + if ( hvmemul_ctxt->ctxt.retire.sti ) > + new_intr_shadow ^= HVM_INTR_SHADOW_STI; > + else > + new_intr_shadow &= ~HVM_INTR_SHADOW_STI; > + } > > if ( hvmemul_ctxt->intr_shadow != new_intr_shadow ) > { > @@ -1981,13 +1988,11 @@ static int _hvm_emulate_one(struct hvm_e > hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow); > } > > - if ( hvmemul_ctxt->ctxt.retire.hlt && > + if ( rc == X86EMUL_OKAY && hvmemul_ctxt->ctxt.retire.hlt && > !hvm_local_events_need_delivery(curr) ) Similarly, you don't need the OKAY check here. Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > - { > hvm_hlt(regs->eflags); > - } > > - return X86EMUL_OKAY; > + return rc; > } > > int hvm_emulate_one( > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -4002,6 +4002,7 @@ x86_emulate( > uint32_t mask = X86_EFLAGS_VIP | X86_EFLAGS_VIF | X86_EFLAGS_VM; > > fail_if(!in_realmode(ctxt, ops)); > + ctxt->retire.unblock_nmi = true; > if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), > &eip, op_bytes, ctxt, ops)) || > (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), > @@ -7918,9 +7919,17 @@ int x86_emulate_wrapper( > > rc = x86_emulate(ctxt, ops); > > - /* Retire flags should only be set for successful instruction emulation. */ > + /* > + * Most retire flags should only be set for successful instruction > + * emulation. > + */ > if ( rc != X86EMUL_OKAY ) > - ASSERT(ctxt->retire.raw == 0); > + { > + typeof(ctxt->retire) retire = ctxt->retire; > + > + retire.unblock_nmi = false; > + ASSERT(!retire.raw); > + } > > /* All cases returning X86EMUL_EXCEPTION should have fault semantics. */ > if ( rc == X86EMUL_EXCEPTION ) > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -490,6 +490,7 @@ struct x86_emulate_ctxt > bool hlt:1; /* Instruction HLTed. */ > bool mov_ss:1; /* Instruction sets MOV-SS irq shadow. */ > bool sti:1; /* Instruction sets STI irq shadow. */ > + bool unblock_nmi:1; /* Instruction clears NMI blocking. */ > bool singlestep:1; /* Singlestepping was active. */ > }; > } retire; > >
>>> On 11.04.17 at 18:09, <andrew.cooper3@citrix.com> wrote: > On 11/04/17 16:36, Jan Beulich wrote: >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e >> memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes); >> } >> >> - if ( rc != X86EMUL_OKAY ) >> - return rc; >> + new_intr_shadow = hvmemul_ctxt->intr_shadow; >> >> - if ( hvmemul_ctxt->ctxt.retire.singlestep ) >> - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); >> + /* >> + * IRET, if valid in the given context, clears NMI blocking >> + * (irrespective of rc). >> + */ >> + if ( hvmemul_ctxt->ctxt.retire.unblock_nmi ) >> + new_intr_shadow &= ~HVM_INTR_SHADOW_NMI; >> >> - new_intr_shadow = hvmemul_ctxt->intr_shadow; >> + if ( rc == X86EMUL_OKAY ) >> + { > > On further thought, given the assertion, you don't need to introduce > this check, and can avoid the block indentation. It should make the > patch rather smaller. Hmm, I did consider this, but I feel uneasy doing so, as it leaves production builds in potentially bad shape (in case we have a path we don't ever execute in any of the routine testing done, but which someone nevertheless ends up coming down in a released product). Paul, you're the maintainer of the code, do you have an opinion either way? Jan
On 12/04/2017 08:43, Jan Beulich wrote: >>>> On 11.04.17 at 18:09, <andrew.cooper3@citrix.com> wrote: >> On 11/04/17 16:36, Jan Beulich wrote: >>> --- a/xen/arch/x86/hvm/emulate.c >>> +++ b/xen/arch/x86/hvm/emulate.c >>> @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e >>> memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes); >>> } >>> >>> - if ( rc != X86EMUL_OKAY ) >>> - return rc; >>> + new_intr_shadow = hvmemul_ctxt->intr_shadow; >>> >>> - if ( hvmemul_ctxt->ctxt.retire.singlestep ) >>> - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); >>> + /* >>> + * IRET, if valid in the given context, clears NMI blocking >>> + * (irrespective of rc). >>> + */ >>> + if ( hvmemul_ctxt->ctxt.retire.unblock_nmi ) >>> + new_intr_shadow &= ~HVM_INTR_SHADOW_NMI; >>> >>> - new_intr_shadow = hvmemul_ctxt->intr_shadow; >>> + if ( rc == X86EMUL_OKAY ) >>> + { >> On further thought, given the assertion, you don't need to introduce >> this check, and can avoid the block indentation. It should make the >> patch rather smaller. > Hmm, I did consider this, but I feel uneasy doing so, as it leaves > production builds in potentially bad shape (in case we have a path > we don't ever execute in any of the routine testing done, but > which someone nevertheless ends up coming down in a released > product). Paul, you're the maintainer of the code, do you have an > opinion either way? Actually, on yet further thought, the reset nature of these conditions are actually necessary on the != OKAY path. Consider "mov %eax, %ss; ud2a". Even on raising an exception for the ud2a, the SS shadow needs dropping. ~Andrew
>>> On 12.04.17 at 10:06, <andrew.cooper3@citrix.com> wrote: > On 12/04/2017 08:43, Jan Beulich wrote: >>>>> On 11.04.17 at 18:09, <andrew.cooper3@citrix.com> wrote: >>> On 11/04/17 16:36, Jan Beulich wrote: >>>> --- a/xen/arch/x86/hvm/emulate.c >>>> +++ b/xen/arch/x86/hvm/emulate.c >>>> @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e >>>> memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes); >>>> } >>>> >>>> - if ( rc != X86EMUL_OKAY ) >>>> - return rc; >>>> + new_intr_shadow = hvmemul_ctxt->intr_shadow; >>>> >>>> - if ( hvmemul_ctxt->ctxt.retire.singlestep ) >>>> - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); >>>> + /* >>>> + * IRET, if valid in the given context, clears NMI blocking >>>> + * (irrespective of rc). >>>> + */ >>>> + if ( hvmemul_ctxt->ctxt.retire.unblock_nmi ) >>>> + new_intr_shadow &= ~HVM_INTR_SHADOW_NMI; >>>> >>>> - new_intr_shadow = hvmemul_ctxt->intr_shadow; >>>> + if ( rc == X86EMUL_OKAY ) >>>> + { >>> On further thought, given the assertion, you don't need to introduce >>> this check, and can avoid the block indentation. It should make the >>> patch rather smaller. >> Hmm, I did consider this, but I feel uneasy doing so, as it leaves >> production builds in potentially bad shape (in case we have a path >> we don't ever execute in any of the routine testing done, but >> which someone nevertheless ends up coming down in a released >> product). Paul, you're the maintainer of the code, do you have an >> opinion either way? > > Actually, on yet further thought, the reset nature of these conditions > are actually necessary on the != OKAY path. > > Consider "mov %eax, %ss; ud2a". Even on raising an exception for the > ud2a, the SS shadow needs dropping. Hmm, indeed (for EXCEPTION at least, not so sure about the others; quite certainly not for RETRY). Along the lines of the above this would then call for an "else" to the newly added "if ( rc == X86EMUL_OKAY )" though. Jan
--- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes); } - if ( rc != X86EMUL_OKAY ) - return rc; + new_intr_shadow = hvmemul_ctxt->intr_shadow; - if ( hvmemul_ctxt->ctxt.retire.singlestep ) - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + /* + * IRET, if valid in the given context, clears NMI blocking + * (irrespective of rc). + */ + if ( hvmemul_ctxt->ctxt.retire.unblock_nmi ) + new_intr_shadow &= ~HVM_INTR_SHADOW_NMI; - new_intr_shadow = hvmemul_ctxt->intr_shadow; + if ( rc == X86EMUL_OKAY ) + { + if ( hvmemul_ctxt->ctxt.retire.singlestep ) + hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); - /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */ - if ( hvmemul_ctxt->ctxt.retire.mov_ss ) - new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS; - else - new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS; - - /* STI instruction toggles STI shadow, else we just clear it. */ - if ( hvmemul_ctxt->ctxt.retire.sti ) - new_intr_shadow ^= HVM_INTR_SHADOW_STI; - else - new_intr_shadow &= ~HVM_INTR_SHADOW_STI; + /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */ + if ( hvmemul_ctxt->ctxt.retire.mov_ss ) + new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS; + else + new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS; + + /* STI instruction toggles STI shadow, else we just clear it. */ + if ( hvmemul_ctxt->ctxt.retire.sti ) + new_intr_shadow ^= HVM_INTR_SHADOW_STI; + else + new_intr_shadow &= ~HVM_INTR_SHADOW_STI; + } if ( hvmemul_ctxt->intr_shadow != new_intr_shadow ) { @@ -1981,13 +1988,11 @@ static int _hvm_emulate_one(struct hvm_e hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow); } - if ( hvmemul_ctxt->ctxt.retire.hlt && + if ( rc == X86EMUL_OKAY && hvmemul_ctxt->ctxt.retire.hlt && !hvm_local_events_need_delivery(curr) ) - { hvm_hlt(regs->eflags); - } - return X86EMUL_OKAY; + return rc; } int hvm_emulate_one( --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -4002,6 +4002,7 @@ x86_emulate( uint32_t mask = X86_EFLAGS_VIP | X86_EFLAGS_VIF | X86_EFLAGS_VM; fail_if(!in_realmode(ctxt, ops)); + ctxt->retire.unblock_nmi = true; if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), &eip, op_bytes, ctxt, ops)) || (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), @@ -7918,9 +7919,17 @@ int x86_emulate_wrapper( rc = x86_emulate(ctxt, ops); - /* Retire flags should only be set for successful instruction emulation. */ + /* + * Most retire flags should only be set for successful instruction + * emulation. + */ if ( rc != X86EMUL_OKAY ) - ASSERT(ctxt->retire.raw == 0); + { + typeof(ctxt->retire) retire = ctxt->retire; + + retire.unblock_nmi = false; + ASSERT(!retire.raw); + } /* All cases returning X86EMUL_EXCEPTION should have fault semantics. */ if ( rc == X86EMUL_EXCEPTION ) --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -490,6 +490,7 @@ struct x86_emulate_ctxt bool hlt:1; /* Instruction HLTed. */ bool mov_ss:1; /* Instruction sets MOV-SS irq shadow. */ bool sti:1; /* Instruction sets STI irq shadow. */ + bool unblock_nmi:1; /* Instruction clears NMI blocking. */ bool singlestep:1; /* Singlestepping was active. */ }; } retire;