* [PATCH v2 1/2] xen: x86: copy back tsc info, not pointer to tsc info in domctl
@ 2015-05-26 11:14 Ian Campbell
2015-05-26 12:25 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Ian Campbell @ 2015-05-26 11:14 UTC (permalink / raw)
To: jbeulich, andrew.cooper3; +Cc: Ian Campbell, xen-devel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] xen: x86: copy back tsc info, not pointer to tsc info in domctl
2015-05-26 11:14 [PATCH v2 1/2] xen: x86: copy back tsc info, not pointer to tsc info in domctl Ian Campbell
@ 2015-05-26 12:25 ` Jan Beulich
2015-05-26 12:56 ` Ian Campbell
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2015-05-26 12:25 UTC (permalink / raw)
To: Ian Campbell; +Cc: andrew.cooper3, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] xen: x86: copy back tsc info, not pointer to tsc info in domctl
2015-05-26 12:25 ` Jan Beulich
@ 2015-05-26 12:56 ` Ian Campbell
2015-05-26 13:11 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Ian Campbell @ 2015-05-26 12:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] xen: x86: copy back tsc info, not pointer to tsc info in domctl
2015-05-26 12:56 ` Ian Campbell
@ 2015-05-26 13:11 ` Jan Beulich
2015-05-26 13:48 ` Ian Campbell
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2015-05-26 13:11 UTC (permalink / raw)
To: Ian Campbell; +Cc: andrew.cooper3, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] xen: x86: copy back tsc info, not pointer to tsc info in domctl
2015-05-26 13:11 ` Jan Beulich
@ 2015-05-26 13:48 ` Ian Campbell
0 siblings, 0 replies; 5+ messages in thread
From: Ian Campbell @ 2015-05-26 13:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-05-26 13:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-26 11:14 [PATCH v2 1/2] xen: x86: copy back tsc info, not pointer to tsc info in domctl Ian Campbell
2015-05-26 12:25 ` Jan Beulich
2015-05-26 12:56 ` Ian Campbell
2015-05-26 13:11 ` Jan Beulich
2015-05-26 13:48 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.