[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |