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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Jinoh Kang" <jinoh.kang.kr@gmail.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 5/5] x86/pv: Rewrite %dr6 handling
Date: Thu, 14 Sep 2023 19:39:06 +0100	[thread overview]
Message-ID: <3e9e1bc0-3d11-9283-b053-0e5ad171b3f1@citrix.com> (raw)
In-Reply-To: <7c847e46-6890-5511-dda9-b16e8b0ac7ab@suse.com>

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