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. Tamas
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
