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(-)

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;
-- 
2.30.2

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