On Mon, Feb 15, 2016 at 10:06 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 15.02.16 at 17:55, <tleng...@novetta.com> wrote: > > On Mon, Feb 15, 2016 at 9:48 AM, Jan Beulich <jbeul...@suse.com> wrote: > > > >> >>> On 15.02.16 at 17:27, <tleng...@novetta.com> wrote: > >> > On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich <jbeul...@suse.com> > wrote: > >> > > >> >> >>> On 12.02.16 at 13:57, <tleng...@novetta.com> wrote: > >> >> > On Feb 12, 2016 02:12, "Jan Beulich" <jbeul...@suse.com> wrote: > >> >> >> > >> >> >> >>> On 12.02.16 at 01:22, <tleng...@novetta.com> 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.coop...@citrix.com> > >> >> >> > >> >> >> 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)? I don't have an answer to that but I don't think it's very significant. My original intention was to introduce a separate hvm function to do this saving of dr7 which was voted against by Andrew. I personally don't have a use for dr7 at the moment either way. Tamas
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
