Re: [Xen-devel] [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 12.02.16 at 13:57, <tlengyel@xxxxxxxxxxx> wrote:
> On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>
>> >>> On 12.02.16 at 01:22, <tlengyel@xxxxxxxxxxx> wrote:
>> > Sending the dr7 register during vm_events is useful for various
> applications,
>> > but the current way the register value is gathered is incorrent. In this
>> > patch
>> > we extend vmx_vmcs_save so that we get the correct value.
>> >
>> > Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>
>> Iirc Andrew suggested ...
>>
>> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu *v, struct hvm_hw_cpu *c)
>> >Â Â Â __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
>> >Â Â Â __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
>> >Â Â Â __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
>> > +Â Â __vmread(GUEST_DR7, &c->dr7);
>>
>> ... just when v == current.
>>
>
> Would that check really be necessary? It would complicate the code not just
> here but the caller would need to be aware too that in that case dr7 can be
> aquired from someplace else. I don't see the harm in just saving dr7 here
> in both cases.

Maybe the solution then is for the suggested if() to have an "else"?
While, as someone said elsewhere, a few more cycles may not be
noticable, why make things slower than they need to be. Plus - what
guarantees that the VMCS field isn't stale while the guest isn't running
(perhaps it got updated but not sync-ed back yet in anticipation for
this to happen during vCPU resume)?

I would say the caller is better suited to make this choice then this function. This function is intended to save vmcs values, so it should do so regardless whether the value in it is stale or not. Then the caller can selectively choose to use the values it knows not to be stale. As for it adding cycles, the if/else check here would also add some cycles. I would guess that the performance difference between the if/else check and __vmread would be unnoticeable so I don't really see any value in doing this check here.

TamasÂ

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel