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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Well, first of all the vm_event consumers of this data are secondary.
The primary consumer is the VM save logic, which runs with
v != current. It just so happens that hvm_save_cpu_ctxt() already
ignores that field and uses v->arch.debugreg[7] instead. Hence
we're back to square one: How much of an overhead is the extra
VMREAD (the data gathered by which the primary consumer has no
use for)?

Jan


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