From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Lengyel, Tamas" Subject: Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save Date: Mon, 15 Feb 2016 09:55:00 -0700 Message-ID: References: <1455236525-14866-1-git-send-email-tlengyel@novetta.com> <56BDB01C02000078000D14CC@prv-mh.provo.novell.com> <56BE019102000078000D1745@prv-mh.provo.novell.com> <56C20F5002000078000D24D6@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3929036510463823983==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aVMQk-0006wM-RJ for xen-devel@lists.xenproject.org; Mon, 15 Feb 2016 16:55:23 +0000 Received: by mail-vk0-f54.google.com with SMTP id e6so111831105vkh.2 for ; Mon, 15 Feb 2016 08:55:20 -0800 (PST) In-Reply-To: <56C20F5002000078000D24D6@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , Kevin Tian , Keir Fraser , Jun Nakajima , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org --===============3929036510463823983== Content-Type: multipart/alternative; boundary=001a11438ee8caa5cc052bd1e175 --001a11438ee8caa5cc052bd1e175 Content-Type: text/plain; charset=UTF-8 On Mon, Feb 15, 2016 at 9:48 AM, Jan Beulich wrote: > >>> On 15.02.16 at 17:27, wrote: > > On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich wrote: > > > >> >>> On 12.02.16 at 13:57, wrote: > >> > On Feb 12, 2016 02:12, "Jan Beulich" wrote: > >> >> > >> >> >>> On 12.02.16 at 01:22, 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 > >> >> > >> >> 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 --001a11438ee8caa5cc052bd1e175 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Mon, Feb 15, 2016 at 9:48 AM, Jan Beulich <JBeulich@suse.com>= ; wrote:
>>> On 15.02.16 at 17:27, <tlengyel@novetta.com> wrote:
> On Fri, Feb 12, 2016 at 8:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>
>> >>> On 12.02.16 at 13:57, <tlengyel@novetta.com> wrote:
>> > On Feb 12, 2016 02:12, "Jan Beulich" <JBeulich@suse.com> wrote:
>> >>
>> >> >>> On 12.02.16 at 01:22, <tlengyel@novetta.com> wrote:
>> >> > Sending the dr7 register during vm_events is useful = for various
>> > applications,
>> >> > but the current way the register value is gathered i= s incorrent. In
>> this
>> >> > patch
>> >> > we extend vmx_vmcs_save so that we get the correct v= alue.
>> >> >
>> >> > Suggested-by: Andrew Cooper <andrew.cooper3@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)
>> >> >=C2=A0 =C2=A0 =C2=A0 __vmread(GUEST_SYSENTER_CS, &= ;c->sysenter_cs);
>> >> >=C2=A0 =C2=A0 =C2=A0 __vmread(GUEST_SYSENTER_ESP, &am= p;c->sysenter_esp);
>> >> >=C2=A0 =C2=A0 =C2=A0 __vmread(GUEST_SYSENTER_EIP, &am= p;c->sysenter_eip);
>> >> > +=C2=A0 =C2=A0 __vmread(GUEST_DR7, &c->dr7);<= br> >> >>
>> >> ... just when v =3D=3D 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 c= ase 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 - wha= t
>> 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 f= or
>> this to happen during vCPU resume)?
>>
>
> I would say the caller is better suited to make this choice then this<= br> > 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 onl= y 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 wo= uld
> 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 !=3D current case.

All p= aths that end up using this dr7 value in vm_event have v=3D=3Dcurrent, so r= ight now there is no caller to this function using dr7 where v!=3Dcurrent. = Future callers where v!=3Dcurrent could do so indeed.

Tam= as

--001a11438ee8caa5cc052bd1e175-- --===============3929036510463823983== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============3929036510463823983==--