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
