| Message ID | 20230912232113.402347-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/pv: #DB vs %dr6 fixes, part 2 | expand |
On 13.09.2023 01:21, Andrew Cooper wrote: > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -8379,7 +8379,10 @@ x86_emulate( > if ( !mode_64bit() ) > _regs.r(ip) = (uint32_t)_regs.r(ip); > > - /* Should a singlestep #DB be raised? */ > + if ( singlestep ) > + ctxt->retire.pending_dbg |= X86_DR6_BS; We set "singlestep" about first thing in the function. Is it really correct to latch that into pending_dbg without regard to rc? (Perhaps yes, seeing the comment next to the field declaration.) > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -588,15 +588,26 @@ struct x86_emulate_ctxt > /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */ > unsigned int opcode; > > - /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ > + /* > + * Retirement state, set by the emulator (valid only on X86EMUL_OKAY/DONE). > + * > + * TODO: all this state should be input/output from the VMCS PENDING_DBG, > + * INTERRUPTIBILITY and ACTIVITIY fields. > + */ > union { > - uint8_t raw; > + unsigned long raw; > struct { > + /* > + * Accumulated %dr6 trap bits, positive polarity. Should only be > + * interpreted in the case of X86EMUL_OKAY/DONE. > + */ > + unsigned int pending_dbg; > + > 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. */ > + bool singlestep:1; /* Singlestepping was active. (TODO, merge into pending_dbg) */ > }; > } retire; > DONE has wrongly made it into here, as pointed out for patch 1. Jan
On 14/09/2023 4:04 pm, Jan Beulich wrote: > On 13.09.2023 01:21, Andrew Cooper wrote: >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -8379,7 +8379,10 @@ x86_emulate( >> if ( !mode_64bit() ) >> _regs.r(ip) = (uint32_t)_regs.r(ip); >> >> - /* Should a singlestep #DB be raised? */ >> + if ( singlestep ) >> + ctxt->retire.pending_dbg |= X86_DR6_BS; > We set "singlestep" about first thing in the function. Is it really correct > to latch that into pending_dbg without regard to rc? (Perhaps yes, seeing > the comment next to the field declaration.) Honestly, without seeing some real RTL, I'm not sure. The thing that is definitely buggy in this retire logic is saying "don't set singlestep if MovSS is pending". The correct behaviour is to set both PENDING_DBG.BS and INTERRUPTIBILITY.MOVSS, the latter of which causes #DB to be skipped on this instruction boundary. That's why we get the XSA-260 behaviour - when the SYSCALL/INT/etc completes and we hit the subsequent instruction boundary, PENDING_DBG still has values latched in it. With the DONE path being removed, and with the plan to wire in the VMCS register for emulations that occur when something is already pending, then it probably does want to be restricted back to the OKAY case. e.g. We shouldn't latch BS in EXCEPTION Case, or we'd create more XSA-260-like behaviour. I'll adjust in light of this. ~Andrew
On 9/13/23 08:21, Andrew Cooper wrote: > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h > index 698750267a90..f0e74d23c378 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -588,15 +588,26 @@ struct x86_emulate_ctxt > /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */ > unsigned int opcode; > > - /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ > + /* > + * Retirement state, set by the emulator (valid only on X86EMUL_OKAY/DONE). > + * > + * TODO: all this state should be input/output from the VMCS PENDING_DBG, > + * INTERRUPTIBILITY and ACTIVITIY fields. > + */ > union { > - uint8_t raw; > + unsigned long raw; Minor nit: this should be uint64_t for clarity. Otherwise, it's not at all clear that the raw field covers the entire union, unless you remind myself that Xen does not support 32-bit host. > struct { > + /* > + * Accumulated %dr6 trap bits, positive polarity. Should only be > + * interpreted in the case of X86EMUL_OKAY/DONE. > + */ > + unsigned int pending_dbg; > + > 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. */ > + bool singlestep:1; /* Singlestepping was active. (TODO, merge into pending_dbg) */ > }; > } retire; >
On 9/15/23 21:20, Jinoh Kang wrote: > On 9/13/23 08:21, Andrew Cooper wrote: >> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h >> index 698750267a90..f0e74d23c378 100644 >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -588,15 +588,26 @@ struct x86_emulate_ctxt >> /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */ >> unsigned int opcode; >> >> - /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ >> + /* >> + * Retirement state, set by the emulator (valid only on X86EMUL_OKAY/DONE). >> + * >> + * TODO: all this state should be input/output from the VMCS PENDING_DBG, >> + * INTERRUPTIBILITY and ACTIVITIY fields. >> + */ >> union { >> - uint8_t raw; >> + unsigned long raw; > > Minor nit: this should be uint64_t for clarity. Otherwise, it's not at all > clear that the raw field covers the entire union, unless you remind myself > that Xen does not support 32-bit host. you remind yourself*. What a weird typo to make :-(
On 15/09/2023 3:24 pm, Jinoh Kang wrote: > On 9/15/23 21:20, Jinoh Kang wrote: >> On 9/13/23 08:21, Andrew Cooper wrote: >>> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h >>> index 698750267a90..f0e74d23c378 100644 >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >>> @@ -588,15 +588,26 @@ struct x86_emulate_ctxt >>> /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */ >>> unsigned int opcode; >>> >>> - /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ >>> + /* >>> + * Retirement state, set by the emulator (valid only on X86EMUL_OKAY/DONE). >>> + * >>> + * TODO: all this state should be input/output from the VMCS PENDING_DBG, >>> + * INTERRUPTIBILITY and ACTIVITIY fields. >>> + */ >>> union { >>> - uint8_t raw; >>> + unsigned long raw; >> Minor nit: this should be uint64_t for clarity. Otherwise, it's not at all >> clear that the raw field covers the entire union, unless you remind myself >> that Xen does not support 32-bit host. > you remind yourself*. What a weird typo to make :-( For better or worse, this is form preferred by the Xen coding style. We deleted the 32bit build of the Xen more than a decade ago, and have been 64bit-only ever since. ~Andrew
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index e88245eae9fb..de707c8ec211 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -8379,7 +8379,10 @@ x86_emulate( if ( !mode_64bit() ) _regs.r(ip) = (uint32_t)_regs.r(ip); - /* Should a singlestep #DB be raised? */ + if ( singlestep ) + ctxt->retire.pending_dbg |= X86_DR6_BS; + + /* Should a singlestep #DB be raised? (BROKEN - TODO, merge into pending_dbg) */ if ( rc == X86EMUL_OKAY && singlestep && !ctxt->retire.mov_ss ) { ctxt->retire.singlestep = true; @@ -8659,6 +8662,7 @@ int x86_emulate_wrapper( { typeof(ctxt->retire) retire = ctxt->retire; + retire.pending_dbg = 0; retire.unblock_nmi = false; ASSERT(!retire.raw); } diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index 698750267a90..f0e74d23c378 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -588,15 +588,26 @@ struct x86_emulate_ctxt /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */ unsigned int opcode; - /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ + /* + * Retirement state, set by the emulator (valid only on X86EMUL_OKAY/DONE). + * + * TODO: all this state should be input/output from the VMCS PENDING_DBG, + * INTERRUPTIBILITY and ACTIVITIY fields. + */ union { - uint8_t raw; + unsigned long raw; struct { + /* + * Accumulated %dr6 trap bits, positive polarity. Should only be + * interpreted in the case of X86EMUL_OKAY/DONE. + */ + unsigned int pending_dbg; + 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. */ + bool singlestep:1; /* Singlestepping was active. (TODO, merge into pending_dbg) */ }; } retire;
Lots of this is very very broken, but we need to start somewhere. PENDING_DBG, INTERRUPTIBILITY and ACTIVITY are internal pipeline registers which Intel exposed to software in the VMCS, and AMD exposed a subset of in the VMCB. Importantly, bits set in PENDING_DBG can survive across multiple instruction boundaries if e.g. delivery of #DB is delayed by a MovSS. For now, introduce a full pending_dbg field into the retire union. This keeps the sh_page_fault() and init_context() paths working but in due course the field will want to lose the "retire" infix. In addition, set singlestep into pending_dbg as appropriate. Leave the old singlestep bitfield in place until we can adjust the callers to handle it properly. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Jinoh Kang <jinoh.kang.kr@gmail.com> --- xen/arch/x86/x86_emulate/x86_emulate.c | 6 +++++- xen/arch/x86/x86_emulate/x86_emulate.h | 17 ++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-)