| Message ID | 20230912232113.402347-2-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: > We long ago fixed the emulator to not inject exceptions behind our back. > Therefore, assert that that a PV event (including interrupts, because that > would be buggy too) isn't pending, rather than skipping the #DB injection if > one is. > > On the other hand, the io_emul() stubs which use X86EMUL_DONE rather than > X86EMUL_OKAY may have pending breakpoints to inject after the IO access is > complete, not to mention a pending singlestep. If you look at the uses of X86EMUL_DONE you'll see that this error code is not intended to ever come back from the emulator. It's solely used to communicate between hooks and the core emulator. Therefore I think this part of the description and the added case label are wrong here. With them dropped again ... > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 14/09/2023 3:40 pm, Jan Beulich wrote: > On 13.09.2023 01:21, Andrew Cooper wrote: >> We long ago fixed the emulator to not inject exceptions behind our back. >> Therefore, assert that that a PV event (including interrupts, because that >> would be buggy too) isn't pending, rather than skipping the #DB injection if >> one is. >> >> On the other hand, the io_emul() stubs which use X86EMUL_DONE rather than >> X86EMUL_OKAY may have pending breakpoints to inject after the IO access is >> complete, not to mention a pending singlestep. > If you look at the uses of X86EMUL_DONE you'll see that this error code is > not intended to ever come back from the emulator. It's solely used to > communicate between hooks and the core emulator. Therefore I think this > part of the description and the added case label are wrong here. With them > dropped again ... Oh. I see that now you've pointed it out, but it's far from clear. I'd suggest that we extend the the debug wrapper for x86_emulate() with an assertion to this effect. It also has a knock-on effect in later patches. With the DONE part dropped, this probably wants merging into patch 4. Thoughts? ~Andrew
On 14.09.2023 16:49, Andrew Cooper wrote: > On 14/09/2023 3:40 pm, Jan Beulich wrote: >> On 13.09.2023 01:21, Andrew Cooper wrote: >>> We long ago fixed the emulator to not inject exceptions behind our back. >>> Therefore, assert that that a PV event (including interrupts, because that >>> would be buggy too) isn't pending, rather than skipping the #DB injection if >>> one is. >>> >>> On the other hand, the io_emul() stubs which use X86EMUL_DONE rather than >>> X86EMUL_OKAY may have pending breakpoints to inject after the IO access is >>> complete, not to mention a pending singlestep. >> If you look at the uses of X86EMUL_DONE you'll see that this error code is >> not intended to ever come back from the emulator. It's solely used to >> communicate between hooks and the core emulator. Therefore I think this >> part of the description and the added case label are wrong here. With them >> dropped again ... > > Oh. I see that now you've pointed it out, but it's far from clear. > > I'd suggest that we extend the the debug wrapper for x86_emulate() with > an assertion to this effect. It also has a knock-on effect in later > patches. > > With the DONE part dropped, this probably wants merging into patch 4. > Thoughts? Not sure. Even then the patch here looks to still make sense on its own. I don't mind the folding, I guess, but for the moment the two R-b are only on the individual patches. Jan
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index 142bc4818cb5..257891a2a2dd 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -1358,14 +1358,18 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs) switch ( rc ) { case X86EMUL_OKAY: + case X86EMUL_DONE: + ASSERT(!curr->arch.pv.trap_bounce.flags); + if ( ctxt.ctxt.retire.singlestep ) ctxt.bpmatch |= DR_STEP; + if ( ctxt.bpmatch ) { curr->arch.dr6 |= ctxt.bpmatch | DR_STATUS_RESERVED_ONE; - if ( !(curr->arch.pv.trap_bounce.flags & TBF_EXCEPTION) ) - pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); + pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); } + /* fall through */ case X86EMUL_RETRY: return EXCRET_fault_fixed;
We long ago fixed the emulator to not inject exceptions behind our back. Therefore, assert that that a PV event (including interrupts, because that would be buggy too) isn't pending, rather than skipping the #DB injection if one is. On the other hand, the io_emul() stubs which use X86EMUL_DONE rather than X86EMUL_OKAY may have pending breakpoints to inject after the IO access is complete, not to mention a pending singlestep. 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/pv/emul-priv-op.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)