Xen project Mailing List

On Mon, Feb 15, 2016 at 9:48 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 15.02.16 at 17:27, <tlengyel@xxxxxxxxxxx> wrote:
> 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.

That's a valid point, but while I agree it nevertheless only makes
me ...

> 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.

... ask to then tweak the caller to overwrite the DR7 value with the
known non-stale one in the v != current case.

All paths that end up using this dr7 value in vm_event have v==current, so right now there is no caller to this function using dr7 where v!=current. Future callers where v!=current could do so indeed.

Tamas

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