Re: [PATCH 2/7] x86/emul: Fix and extend #DB trap handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/09/2023 12:30 pm, Jan Beulich wrote:
> On 18.09.2023 11:24, Andrew Cooper wrote:
>> On 15/09/2023 9:36 pm, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -8379,13 +8379,6 @@ x86_emulate(
>>>      if ( !mode_64bit() )
>>>          _regs.r(ip) = (uint32_t)_regs.r(ip);
>>>  
>>> -    /* Should a singlestep #DB be raised? */
>>> -    if ( rc == X86EMUL_OKAY && singlestep && !ctxt->retire.mov_ss )
>>> -    {
>>> -        ctxt->retire.singlestep = true;
>>> -        ctxt->retire.sti = false;
>>> -    }
>>> -
>>>      if ( rc != X86EMUL_DONE )
>>>          *ctxt->regs = _regs;
>>>      else
>>> @@ -8394,6 +8387,19 @@ x86_emulate(
>>>          rc = X86EMUL_OKAY;
>>>      }
>>>  
>>> +    /* Should a singlestep #DB be raised? */
>>> +    if ( rc == X86EMUL_OKAY && singlestep )
>>> +    {
>>> +        ctxt->retire.pending_dbg |= X86_DR6_BS;
>>> +
>>> +        /* BROKEN - TODO, merge into pending_dbg. */
>>> +        if ( !ctxt->retire.mov_ss )
>>> +        {
>>> +            ctxt->retire.singlestep = true;
>>> +            ctxt->retire.sti = false;
>>> +        }
>> I occurs to me that setting X86_DR6_BS outside of the !mov_ss case will
>> break HVM (when HVM swaps from singlestep to pending_dbg) until one of
>> the further TODOs is addressed.
>>
>> This will need bringing back within the conditional to avoid regressions
>> in the short term.
> I'm afraid I don't understand this: Isn't the purpose to latch state no
> matter whether it'll be consumed right away, or only on the next insn?

Yes, that is the intention in the longterm.

But in the short term, where I'm doing just enough to fix the %dr6 bits,
putting this unconditionally in PENDING_DBG will break the emulation of
mov-to-ss until the bigger todo of "wire INTERRUPTIBILITY/ACTIVITY state
into the emulation context" is complete.

The latter is definitely too big to fit into 4.18, and I can't
intentionally regress mov-to-ss in a series we intend to backport.

~Andrew