| Message ID | 1455236525-14866-1-git-send-email-tlengyel@novetta.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/12/2016 02:22 AM, Tamas K Lengyel 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@citrix.com> > Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com> > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/arch/x86/hvm/vmx/vmx.c | 1 + > 1 file changed, 1 insertion(+) Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> 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 is incorrent. In this > patch > we extend vmx_vmcs_save so that we get the correct value. > > 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) > __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. Jan
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 is incorrent. In this > > patch > > we extend vmx_vmcs_save so that we get the correct value. > > > > 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) > > __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. Tamas
>>> 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 is incorrent. In this >> > patch >> > we extend vmx_vmcs_save so that we get the correct value. >> > >> > 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) >> > __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)? Jan
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 is incorrent. In > this > >> > patch > >> > we extend vmx_vmcs_save so that we get the correct value. > >> > > >> > 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) > >> > __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. 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. Tamas
>>> 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 is incorrent. In >> this >> >> > patch >> >> > we extend vmx_vmcs_save so that we get the correct value. >> >> > >> >> > 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) >> >> > __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. Jan
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 is incorrent. In > >> this > >> >> > patch > >> >> > we extend vmx_vmcs_save so that we get the correct value. > >> >> > > >> >> > 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) > >> >> > __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
>>> On 15.02.16 at 17:55, <tlengyel@novetta.com> wrote: > 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 is incorrent. In >> >> this >> >> >> > patch >> >> >> > we extend vmx_vmcs_save so that we get the correct value. >> >> >> > >> >> >> > 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) >> >> >> > __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
On Mon, Feb 15, 2016 at 10:06 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 15.02.16 at 17:55, <tlengyel@novetta.com> wrote: > > 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 is > incorrent. In > >> >> this > >> >> >> > patch > >> >> >> > we extend vmx_vmcs_save so that we get the correct value. > >> >> >> > > >> >> >> > 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) > >> >> >> > __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
>>> On 15.02.16 at 18:17, <tlengyel@novetta.com> wrote: > On Mon, Feb 15, 2016 at 10:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 15.02.16 at 17:55, <tlengyel@novetta.com> wrote: >> > 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. Andrew, thoughts? (As usual, a single such unnecessary addition may not be noticable, but once we set a precedent, more might follow, until we get into the noticable range.) Jan
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index b9f340c..ae05794 100644 --- 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); c->pending_event = 0; c->error_code = 0;
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@citrix.com> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com> Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/hvm/vmx/vmx.c | 1 + 1 file changed, 1 insertion(+)