[PATCH] xen: x86: copy back tsc info, not pointer to tsc info in domctl

[PATCH] xen: x86: copy back tsc info, not pointer to tsc info in domctl

[Ian Campbell]

In 38b37ed82705 "x86/domctl: cleanup", XEN_DOMCTL_gettscinfo was
changed to use the standard copyback mechanism.

However the output TSC Info is a guerst handle, i.e. a pointer to the
location for the information, copyback just copies the unchanged
pointer back.

Switch back to fetching the details into a local struct and explicitly
copying it back.

This caused test failures in the Cambridge instance of osstest, but
not for some reason in the production instance.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/x86/domctl.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 684259b..e7da735 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -703,13 +703,16 @@ long arch_do_domctl(
             ret = -EINVAL;
         else
         {
+            xen_guest_tsc_info_t info = { 0 };
+
             domain_pause(d);
-            tsc_get_info(d, &domctl->u.tsc_info.info.tsc_mode,
-                         &domctl->u.tsc_info.info.elapsed_nsec,
-                         &domctl->u.tsc_info.info.gtsc_khz,
-                         &domctl->u.tsc_info.info.incarnation);
+            tsc_get_info(d, &info.tsc_mode,
+                            &info.elapsed_nsec,
+                            &info.gtsc_khz,
+                            &info.incarnation);
+            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
+                ret = -EFAULT;
             domain_unpause(d);
-            copyback = 1;
         }
         break;
 
-- 
1.7.10.4

Re: [PATCH] xen: x86: copy back tsc info, not pointer to tsc info in domctl

[Andrew Cooper]

On 26/05/15 11:04, Ian Campbell wrote:
> In 38b37ed82705 "x86/domctl: cleanup", XEN_DOMCTL_gettscinfo was
> changed to use the standard copyback mechanism.
>
> However the output TSC Info is a guerst handle, i.e. a pointer to the
> location for the information, copyback just copies the unchanged
> pointer back.
>
> Switch back to fetching the details into a local struct and explicitly
> copying it back.
>
> This caused test failures in the Cambridge instance of osstest, but
> not for some reason in the production instance.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Oops - I feel very silly for not noticing that.  My testing also didn't
notice this.

> ---
>  xen/arch/x86/domctl.c |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 684259b..e7da735 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -703,13 +703,16 @@ long arch_do_domctl(
>              ret = -EINVAL;
>          else
>          {
> +            xen_guest_tsc_info_t info = { 0 };
> +
>              domain_pause(d);
> -            tsc_get_info(d, &domctl->u.tsc_info.info.tsc_mode,
> -                         &domctl->u.tsc_info.info.elapsed_nsec,
> -                         &domctl->u.tsc_info.info.gtsc_khz,
> -                         &domctl->u.tsc_info.info.incarnation);
> +            tsc_get_info(d, &info.tsc_mode,
> +                            &info.elapsed_nsec,
> +                            &info.gtsc_khz,
> +                            &info.incarnation);
> +            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
> +                ret = -EFAULT;
>              domain_unpause(d);

The copy_to_guest() can be after the domain_unpause().

Alternatively, being a domctl, we can change tsc_info to not be a guest
handle.  There is plenty of space inside the union, and it saves a
further memory access.

~Andrew

Re: [PATCH] xen: x86: copy back tsc info, not pointer to tsc info in domctl

[Ian Campbell]

On Tue, 2015-05-26 at 11:13 +0100, Andrew Cooper wrote:
> On 26/05/15 11:04, Ian Campbell wrote:
> > In 38b37ed82705 "x86/domctl: cleanup", XEN_DOMCTL_gettscinfo was
> > changed to use the standard copyback mechanism.
> >
> > However the output TSC Info is a guerst handle, i.e. a pointer to the
> > location for the information, copyback just copies the unchanged
> > pointer back.
> >
> > Switch back to fetching the details into a local struct and explicitly
> > copying it back.
> >
> > This caused test failures in the Cambridge instance of osstest, but
> > not for some reason in the production instance.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Oops - I feel very silly for not noticing that.  My testing also didn't
> notice this.

It's pretty subtle.

My best guess is that the h/w in Cambridge tends to be older and
therefore perhaps doesn't all have synchronous TSCs or something.

> > ---
> >  xen/arch/x86/domctl.c |   13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > index 684259b..e7da735 100644
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -703,13 +703,16 @@ long arch_do_domctl(
> >              ret = -EINVAL;
> >          else
> >          {
> > +            xen_guest_tsc_info_t info = { 0 };
> > +
> >              domain_pause(d);
> > -            tsc_get_info(d, &domctl->u.tsc_info.info.tsc_mode,
> > -                         &domctl->u.tsc_info.info.elapsed_nsec,
> > -                         &domctl->u.tsc_info.info.gtsc_khz,
> > -                         &domctl->u.tsc_info.info.incarnation);
> > +            tsc_get_info(d, &info.tsc_mode,
> > +                            &info.elapsed_nsec,
> > +                            &info.gtsc_khz,
> > +                            &info.incarnation);
> > +            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
> > +                ret = -EFAULT;
> >              domain_unpause(d);
> 
> The copy_to_guest() can be after the domain_unpause().

So it can. 

> Alternatively, being a domctl, we can change tsc_info to not be a guest
> handle.  There is plenty of space inside the union, and it saves a
> further memory access.

If you prefer, sure.

Perhaps fix then cleanup in a separate patch?

Ian.

Re: [PATCH] xen: x86: copy back tsc info, not pointer to tsc info in domctl

[Andrew Cooper]

On 26/05/15 11:18, Ian Campbell wrote:
> On Tue, 2015-05-26 at 11:13 +0100, Andrew Cooper wrote:
>> On 26/05/15 11:04, Ian Campbell wrote:
>>> In 38b37ed82705 "x86/domctl: cleanup", XEN_DOMCTL_gettscinfo was
>>> changed to use the standard copyback mechanism.
>>>
>>> However the output TSC Info is a guerst handle, i.e. a pointer to the
>>> location for the information, copyback just copies the unchanged
>>> pointer back.
>>>
>>> Switch back to fetching the details into a local struct and explicitly
>>> copying it back.
>>>
>>> This caused test failures in the Cambridge instance of osstest, but
>>> not for some reason in the production instance.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Oops - I feel very silly for not noticing that.  My testing also didn't
>> notice this.
> It's pretty subtle.
>
> My best guess is that the h/w in Cambridge tends to be older and
> therefore perhaps doesn't all have synchronous TSCs or something.
>
>>> ---
>>>  xen/arch/x86/domctl.c |   13 ++++++++-----
>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>> index 684259b..e7da735 100644
>>> --- a/xen/arch/x86/domctl.c
>>> +++ b/xen/arch/x86/domctl.c
>>> @@ -703,13 +703,16 @@ long arch_do_domctl(
>>>              ret = -EINVAL;
>>>          else
>>>          {
>>> +            xen_guest_tsc_info_t info = { 0 };
>>> +
>>>              domain_pause(d);
>>> -            tsc_get_info(d, &domctl->u.tsc_info.info.tsc_mode,
>>> -                         &domctl->u.tsc_info.info.elapsed_nsec,
>>> -                         &domctl->u.tsc_info.info.gtsc_khz,
>>> -                         &domctl->u.tsc_info.info.incarnation);
>>> +            tsc_get_info(d, &info.tsc_mode,
>>> +                            &info.elapsed_nsec,
>>> +                            &info.gtsc_khz,
>>> +                            &info.incarnation);
>>> +            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
>>> +                ret = -EFAULT;
>>>              domain_unpause(d);
>> The copy_to_guest() can be after the domain_unpause().
> So it can. 
>
>> Alternatively, being a domctl, we can change tsc_info to not be a guest
>> handle.  There is plenty of space inside the union, and it saves a
>> further memory access.
> If you prefer, sure.
>
> Perhaps fix then cleanup in a separate patch?

Can do.

For this patch (subject to the reordering), Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

>
> Ian.
>
>