On 14/09/2023 5:06 pm, Jan Beulich wrote:
> On 13.09.2023 01:21, Andrew Cooper wrote:
>> --- a/xen/arch/x86/include/asm/domain.h
>> +++ b/xen/arch/x86/include/asm/domain.h
>> @@ -729,6 +729,18 @@ static inline void pv_inject_hw_exception(unsigned int
>> vector, int errcode)
>> pv_inject_event(&event);
>> }
>>
>> +static inline void pv_inject_DB(unsigned long pending_dbg)
>> +{
>> + struct x86_event event = {
>> + .vector = X86_EXC_DB,
>> + .type = X86_EVENTTYPE_HW_EXCEPTION,
>> + .error_code = X86_EVENT_NO_EC,
>> + .pending_dbg = pending_dbg,
> This being a sub-field of an unnamed union, the build will break (also
> in pv_inject_page_fault() then, for cr2 being switched at the same time)
> once again for old enough gcc.
I'm sick and tired of utterly obsolete compiler bugs stopping us writing
good code.
It will break HVM #PF too, and I'll fix it for now as these patches need
backporting, but I've got a very strong mind to intentionally break it
next time this comes up in staging.
>> --- a/xen/arch/x86/pv/emulate.c
>> +++ b/xen/arch/x86/pv/emulate.c
>> @@ -71,11 +71,9 @@ void pv_emul_instruction_done(struct cpu_user_regs *regs,
>> unsigned long rip)
>> {
>> regs->rip = rip;
>> regs->eflags &= ~X86_EFLAGS_RF;
>> +
>> if ( regs->eflags & X86_EFLAGS_TF )
>> - {
>> - current->arch.dr6 |= DR_STEP | DR_STATUS_RESERVED_ONE;
>> - pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
>> - }
>> + pv_inject_DB(X86_DR6_BS);
>> }
> This (not your change, the construct) looks bogus at the first and second
> glance, because of looking at EFLAGS.TF after emulation, when the initial
> state of TF matters. It is only correct (at the third, closer look) because
> the function presently is used only from paths not altering the guest's
> EFLAGS. Do you think it would make sense to add a comment at this occasion?
It is buggy yes, but if you notice, so is SVM's __update_guest_eip() and
VMX's update_guest_eip().
And remember that while for most instructions it's the initial state
that matters, it's the final state that matters for SYSCALL/SYSRET, and
each of LRET/IRET/ERET{U,S} have oddities that aren't currently
expressed in the emulator.
I'll leave a todo for now. This is a problem that can only reasonably
be fixed by unifying the intercept and emulation paths into a coherent
model of the instruction cycle.
~Andrew
