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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.09.2023 14:19, Andrew Cooper wrote:
> 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.

Since I assume we're talking about the tail of _hvm_emulate_one(), my
problem is that I cannot see how setting X86_DR6_BS would lead to a
problem there. Plus you don't touch x86/hvm/ at all in the series, and
the pending_dbg field gets newly introduced in the patch here. Hence it
looks to me as if for HVM the field could take any value, without
breaking the code. But then, from you explicitly pointing out a problem,
I can only infer that I'm overlooking something.

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

Of course.

Jan