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

[Jinoh Kang <jinoh.kang.kr@xxxxxxxxx>]

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"...

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

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

> 
>> If this is the case, (at least) this part of the patch looks like a stable
>> candidate.  You might want to edit the commit message to reflect that.
> 
> We're going to try and get all the %dr6 handling issues sorted, then
> decide whether to backport the lot or not.  It will entirely depend on
> how invasive the fixes end up being, but I hope they'll be ok to backport.
> 
>> (Ideally all the HWBP handling should be part of the emulator logic, but
>>  I don't see an easy way to generalize the PV-specific logic.  It could
>>  be its own patch anyway.)
> 
> The emulator does have HWBP handling for HVM guests, because that's
> architectural behaviour to look in the TSS.

I was under such impression since I didn't immediately notice I/O
breakpoint handling in ins/outs path; maybe I haven't looked into it deeper...

> 
> PV guests are the odd-ones-out with non-standard behaviour.
> 
> ~Andrew

-- 
Sincerely,
Jinoh Kang