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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan