Re: [PATCH v2 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

["Lengyel, Tamas" <tlengyel@novetta.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2965 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 4471 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel