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

[PATCH v2 1/2] 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Copy back outside domain pause.
---
 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 b6df23a..6d3fe91 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -856,13 +856,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);
             domain_unpause(d);
-            copyback = 1;
+            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
+                ret = -EFAULT;
         }
         break;
 
-- 
1.7.10.4

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

[Jan Beulich]

>>> On 26.05.15 at 13:14, <ian.campbell@citrix.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -856,13 +856,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);
>              domain_unpause(d);
> -            copyback = 1;
> +            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
> +                ret = -EFAULT;
>          }
>          break;
>  

I have to admit that I don't see the point of this change when patch
2 basically undoes it all.

Jan

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

[Ian Campbell]

On Tue, 2015-05-26 at 13:25 +0100, Jan Beulich wrote:
> >>> On 26.05.15 at 13:14, <ian.campbell@citrix.com> wrote:
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -856,13 +856,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);
> >              domain_unpause(d);
> > -            copyback = 1;
> > +            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
> > +                ret = -EFAULT;
> >          }
> >          break;
> >  
> 
> I have to admit that I don't see the point of this change when patch
> 2 basically undoes it all.

I thought so at first but the restructuring in the second patch was
large enough that I didn't want to mix it in with an actual functional
change. Plus the second patch does more than undo it, it removes "info."
from the domctl access.

Ian.

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

[Jan Beulich]

>>> On 26.05.15 at 14:56, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-05-26 at 13:25 +0100, Jan Beulich wrote:
>> >>> On 26.05.15 at 13:14, <ian.campbell@citrix.com> wrote:
>> > --- a/xen/arch/x86/domctl.c
>> > +++ b/xen/arch/x86/domctl.c
>> > @@ -856,13 +856,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);
>> >              domain_unpause(d);
>> > -            copyback = 1;
>> > +            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
>> > +                ret = -EFAULT;
>> >          }
>> >          break;
>> >  
>> 
>> I have to admit that I don't see the point of this change when patch
>> 2 basically undoes it all.
> 
> I thought so at first but the restructuring in the second patch was
> large enough that I didn't want to mix it in with an actual functional
> change. Plus the second patch does more than undo it, it removes "info."
> from the domctl access.

Hmm, yeah. I guess in that case the fix (patch 1) could have been
to drop out_info, and the cleanup (patch 2) to eliminate struct
xen_guest_tsc_info. But anyway, feel free to commit as is, taking
this as an ack for patch 2 even in its current shape.

Jan

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

[Ian Campbell]

On Tue, 2015-05-26 at 14:11 +0100, Jan Beulich wrote:
> >>> On 26.05.15 at 14:56, <ian.campbell@citrix.com> wrote:
> > On Tue, 2015-05-26 at 13:25 +0100, Jan Beulich wrote:
> >> >>> On 26.05.15 at 13:14, <ian.campbell@citrix.com> wrote:
> >> > --- a/xen/arch/x86/domctl.c
> >> > +++ b/xen/arch/x86/domctl.c
> >> > @@ -856,13 +856,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);
> >> >              domain_unpause(d);
> >> > -            copyback = 1;
> >> > +            if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
> >> > +                ret = -EFAULT;
> >> >          }
> >> >          break;
> >> >  
> >> 
> >> I have to admit that I don't see the point of this change when patch
> >> 2 basically undoes it all.
> > 
> > I thought so at first but the restructuring in the second patch was
> > large enough that I didn't want to mix it in with an actual functional
> > change. Plus the second patch does more than undo it, it removes "info."
> > from the domctl access.
> 
> Hmm, yeah. I guess in that case the fix (patch 1) could have been
> to drop out_info, and the cleanup (patch 2) to eliminate struct
> xen_guest_tsc_info. But anyway, feel free to commit as is, taking
> this as an ack for patch 2 even in its current shape.

Done, thanks.