Re: [PATCH 7/7] x86/pv: Rewrite %dr6 handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/09/2023 8:50 am, Jinoh Kang wrote:
> On 9/16/23 05:36, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index dead728ce329..447edc827b3a 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1887,11 +1887,11 @@ void do_device_not_available(struct cpu_user_regs 
>> *regs)
>>  /* SAF-1-safe */
>>  void do_debug(struct cpu_user_regs *regs)
>>  {
>> -    unsigned long dr6;
>> +    unsigned long pending_dbg;
>>      struct vcpu *v = current;
>>  
>> -    /* Stash dr6 as early as possible. */
>> -    dr6 = read_debugreg(6);
>> +    /* Stash dr6 as early as possible, operating with positive polarity. */
>> +    pending_dbg = read_debugreg(6) ^ X86_DR6_DEFAULT;
> We don't reset DR6 after reading it, and there is no guarantee that the PV 
> guest
> will reset it either, so it doesn't match PENDING_DBG exactly IIRC.
>
> On the other hand, nothing will probably care about its double-accumulating
> quirk except perhaps monitor users.
>
> Do we want to document that, shadow DR6 for PV (which seems a little overkill
> if we don't trap DR6 access from PV already), or just live with that?

Different DR6's.

This is Xen responding to a real #DB (most likely from a PV guest, but
maybe from debugging activity in Xen itself), and in ...

>>  
>>      /*
>>       * At the time of writing (March 2018), on the subject of %dr6:

... this piece of context missing from the patch, Xen always writes
X86_DR6_DEFAULT back in order to clear the sticky bits.

This behaviour hasn't changed.  Xen always sees a "clean" DR6 on every
new #DB.

For the PV guest, what matters is ...

>> @@ -1963,13 +1963,13 @@ void do_debug(struct cpu_user_regs *regs)
>>           * If however we do, safety measures need to be enacted.  Use a big
>>           * hammer and clear all debug settings.
>>           */
>> -        if ( dr6 & (DR_TRAP3 | DR_TRAP2 | DR_TRAP1 | DR_TRAP0) )
>> +        if ( pending_dbg & X86_DR6_BP_MASK )
>>          {
>>              unsigned int bp, dr7 = read_debugreg(7);
>>  
>>              for ( bp = 0; bp < 4; ++bp )
>>              {
>> -                if ( (dr6 & (1u << bp)) && /* Breakpoint triggered? */
>> +                if ( (pending_dbg & (1u << bp)) && /* Breakpoint triggered? 
>> */
>>                       (dr7 & (3u << (bp * DR_ENABLE_SIZE))) && /* Enabled? */
>>                       ((dr7 & (3u << ((bp * DR_CONTROL_SIZE) + /* Insn? */
>>                                       DR_CONTROL_SHIFT))) == DR_RW_EXECUTE) )
>> @@ -1990,24 +1990,23 @@ void do_debug(struct cpu_user_regs *regs)
>>           * so ensure the message is ratelimited.
>>           */
>>          gprintk(XENLOG_WARNING,
>> -                "Hit #DB in Xen context: %04x:%p [%ps], stk %04x:%p, dr6 
>> %lx\n",
>> +                "Hit #DB in Xen context: %04x:%p [%ps], stk %04x:%p, 
>> pending_dbg %lx\n",
>>                  regs->cs, _p(regs->rip), _p(regs->rip),
>> -                regs->ss, _p(regs->rsp), dr6);
>> +                regs->ss, _p(regs->rsp), pending_dbg);
>>  
>>          return;
>>      }
>>  
>> -    /* Save debug status register where guest OS can peek at it */
>> -    v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
>> -    v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
>> -
>>      if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
>>      {
>> +        /* Save debug status register where gdbsx can peek at it */
>> +        v->arch.dr6 = x86_merge_dr6(v->domain->arch.cpu_policy,
>> +                                    v->arch.dr6, pending_dbg);
>>          domain_pause_for_debugger();
>>          return;
>>      }
>>  
>> -    pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
>> +    pv_inject_DB(pending_dbg);

... this, which will merge all new pending bits into the guest's DR6.

If the guest chooses not to clean out DR6 each time, then it will see
content accumulate.

To your earlier point of shadowing, we already have to do that.  DR6 is
just one of many registers we need to context switch for the vCPU.

PV guests, being ring-deprivieged, trap into Xen for every DR access,
and Xen handles every one of their #DB exceptions.  This is one reason
why I split the work into several parts.  PV guests are easier to get
sorted properly, and patch 1,2,5,6 are all common improvements relevant
to HVM too.

~Andrew