Re: [PATCH 3/5] x86/emul: Add a pending_dbg field to x86_emulate_ctxt.retire

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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