Re: [PATCH v2 3/8] x86/emul: Add pending_dbg field to x86_event

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/08/2023 2:39 pm, Jan Beulich wrote:
> On 24.08.2023 17:26, Jinoh Kang wrote:
>> @@ -62,9 +63,16 @@ void pv_inject_event(const struct x86_event *event)
>>              error_code |= PFEC_user_mode;
>>  
>>          trace_pv_page_fault(event->cr2, error_code);
>> -    }
>> -    else
>> +        break;
>> +
>> +    case X86_EXC_DB:
>> +        curr->arch.dr6 |= event->pending_dbg;
>> +        /* Fallthrough */
> I guess I have another question here, perhaps more to Andrew: How come
> this is just an OR? Not only with some of the bits having inverted sense
> and earlier logic being ...
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1989,17 +1989,17 @@ void do_debug(struct cpu_user_regs *regs)
>>          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);
> ... an OR and an AND, but also with ...
>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -78,7 +78,10 @@ struct x86_event {
>>      uint8_t       type;         /* X86_EVENTTYPE_* */
>>      uint8_t       insn_len;     /* Instruction length */
>>      int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
>> -    unsigned long cr2;          /* Only for X86_EXC_PF h/w exception */
>> +    union {
>> +        unsigned long cr2;         /* #PF */
>> +        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) 
>> */

As a tangent, since I wrote the original series, there's #NM and
MSR_XFD_ERR which needs to fit into this union for AMX support.

Sadly, the only AMX hardware on the market right now has an errata where
XFD_ERR doesn't behave properly here.

> ... the comment here saying "positive polarity", which I understand
> to mean that inverted bits need inverting by the consumer of this
> field. If this is solely because none of the inverted bits are
> supported for PV, then I guess this wants a comment at the use site
> (not the least because it would need adjusting as soon as one such
> would become supported).

Part of this patch is (or was) introducing pending_dbg with no logical
change, but as I said, I don't think I had the original series split up
quite correctly either.


This field is more than just the inversion.  It needs to match the
semantics of the VMCS PENDING_DBG field, which is architectural but
otherwise hidden pipeline state, similar to the segment descriptor
cache.  The other necessary property is the (lack of) stickiness of bits
in the pending_dbg field.

All of that said, having talked to some pipeline people recent, I think
pending_dbg needs to be elsewhere.  Or perhaps a second copy elsewhere.

Some bits stay pending in pending_dbg across multiple instructions. 
This is how we get the MovSS-delays-breakpoints property that is a
security disaster elsewhere.

The problem with this is that we can't get at the pipeline pending_dbg
state for PV guests (where we've only got an architectural #DB to work
with) or for SVM guests (where this state isn't presented in the VMCB
despite existing internally).

~Andrew