Re: [PATCH 4/7] x86/pv: Drop priv_op_ctxt.bpmatch and use pending_dbg instead

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/09/2023 3:42 pm, Jinoh Kang wrote:
> On 9/16/23 23:00, Andrew Cooper wrote:
>> On 16/09/2023 8:36 am, Jinoh Kang wrote:
> (snip)
>
>>> These two hunks look like a behavioral change in singlestep mode.
>>>
>>> This is actually a fix, assuming the emulator previously did not handle
>>> 'rep {in,out}s' in singlestep mode correctly, since it now checks for
>>> PENDING_DBG.BS in addition to PENDING_DBG.B[0-4].
>> The emulator should handle this correctly already.  I had been meaning
>> to test this, but hadn't so far - guess I should fix that.
>>
>> x86_emulate.c line 511 in get_rep_prefix() sets max_reps to 1 if
>> SingleStepping is active.
> Thanks for informing.  Although that EFLAGS.TF check in the macro now
> makes me--almost reflexively--imagine all sort of creative pathological
> cases, like "mov ss, ax; rep ins"...

Yeah isn't x86 fun...

That's why it's important to accumulate in pending_dbg, which does hold
the breakpoint recognition across point where the blocked-by-movss state
inhibits the generation of #DB, and causes it to generate #DB on the
subsequent instruction boundary.

>> This in turn causes the emulator to use the io_{read,write}() hook
>> rather than the rep hook.
> Right.  (Frankly that part of code has too many branches to be readable.
> Also the "presumably most efficient" part of the comment hints at perf
> optimization sans any profiling attempts...)

Yeah it's not the greatest code.  I cant remember the exact history
there, but IIRC we used to handle the rep looping entirely in
x86_emulate() and there were no rep hooks.

There are two cases where the perf hit did get noticed.  REP OUTSB to
the qemu/bochs/Xen debug port, and Windows which does a REP MOVSB across
the various bits of PCI MMIO.

>> This is important, because singlestepping becoming pending is normally
>> evaluated at the end of the instruction.  i.e. in this example it
>> wouldn't show up in pending_dbg (yet).
>>
>> What definitely is broken here is the recognition of a data breakpoint
>> on the memory operand of the INS/OUTS instruction, but it's broken
>> everywhere else for PV guest emulation too, so needs to go on the TODO list.
> (Another thing definitely broken here is the recognition of I/O breakpoints
>  post the first iteration.  Maybe it would be beneficial to do differential
>  testing between the {read,write}_io slowpath and rep_{in,out}s fastpath.)

PV, or HVM guest?

IO breakpoint recognition is missing generally in HVM guests.  I opened
https://gitlab.com/xen-project/xen/-/work_items/171 yesterday for it,
because it's not 30s work to fix.

But, IO breakpoints are invariant of REP.  The IO port doesn't change,
so the breakpoint(s) either match on every iteration, or not at all. 
(Of course, this doesn't mean that Xen is handling the recognition
correctly.)

In this patch, there is IO breakpoint recognition on all 4 of the PV IO
port hooks, so I think it ought to work properly, whether it's the first
iteration or not.

Obviously if not, I've got another bug to figure out...


It's worth saying that patch 2 does fix a bug (an X86EMUL_OKAY/DONE
confusion), but I'm pretty sure it was only a singlestep on admin-ok PV
IO ports which would be skipped, not the breakpoints.

~Andrew