Re: [Xen-devel] [PATCH 02/11] x86/vmx: Don't clobber %dr6 while debugging state is lazy

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/06/18 14:50, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3696,6 +3696,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>               */
>>              __vmread(EXIT_QUALIFICATION, &exit_qualification);
>>              HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>> +            __restore_debug_registers(v);
>>              write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
> The change is certainly correct as is, but I'd still like to put out for
> discussion the alternative option:
>
>     if ( v->arch.hvm_vcpu.flag_dr_dirty )
>         write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>     else
>         v->arch.debugreg[6] = exit_qualification | DR_STATUS_RESERVED_ONE;
>
> After all the guest may know it's single stepping, and may not care to
> read DR6 at all.

All of this code changes across the series (so this specific suggestion
is incorrect), but to the recommendation in general...

I considered this, particularly when looking at the AMD side where %dr6
lives in the VMCB, and is unrelated to the dirty state of the other

The first thing a #DB handler will do is read %dr6 and reset it (as
recommended by both manuals), so forcing the %dr state to dirty (and
therefore clearing %dr interception) is a direct benefit to the guests
subsequent performance.


TBH, I'm still not terribly happy with these accessors, but I was
planning to defer reworking how the lazy state handing works.  We've got
ad-hoc lazy logic for each type of state, and its about to get very very
complicated with guest_{rd,wr}msr().

I'm planning to build a more generic piece of state-syncing logic where
every piece of emulation / access (particularly remote access code) code
starts with vcpu_sync_state(state_$X) which takes care of pulling that
specific piece of state out of hardware (if necessary) and into struct
vcpu, and vcpu_state_dirtied(state_$X) which identifies that state in
struct vcpu has been modified.

The idea is to make all the emulation and access code agnostic to how a
certain piece of state is stored, whether that be in VMCS/VMCB, in
hardware registers for current context, switched on entry/exit, or fully
emulated.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel