[PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2924 bytes --]

By moving the call to update_vcpu_system_time() out of schedule() into
arch-specific context switch code, the original problem of the function
accessing the wrong domain's address space goes away (obvious even from
patch context, as update_runstate_area() does similar copying).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Regardless of the code freeze I'd still like to propose this for
inclusion in 4.3, mainly based on the fact that this got disabled late
in the 4.0 release cycle with the expectation that it would get
re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
least on x86-64, the necessary hypervisor independent support code, it
would be odd to not leverage this on Xen.

--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -232,6 +232,9 @@ static void schedule_tail(struct vcpu *p
 
     if ( prev != current )
         update_runstate_area(current);
+
+    /* Ensure that the vcpu has an up-to-date time base. */
+    update_vcpu_system_time(current);
 }
 
 static void continue_new_vcpu(struct vcpu *prev)
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -966,11 +966,6 @@ arch_do_vcpu_op(
 
     switch ( cmd )
     {
-    /*
-     * XXX Disable for 4.0.0: __update_vcpu_system_time() writes to the given
-     * virtual address even when running in another domain's address space.
-     */
-#if 0
     case VCPUOP_register_vcpu_time_memory_area:
     {
         struct vcpu_register_time_memory_area area;
@@ -989,7 +984,6 @@ arch_do_vcpu_op(
 
         break;
     }
-#endif
 
     case VCPUOP_get_physid:
     {
@@ -1457,6 +1451,9 @@ void context_switch(struct vcpu *prev, s
     if (prev != next)
         update_runstate_area(next);
 
+    /* Ensure that the vcpu has an up-to-date time base. */
+    update_vcpu_system_time(next);
+
     schedule_tail(next);
     BUG();
 }
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3174,6 +3174,7 @@ static long hvm_vcpu_op(
     case VCPUOP_set_singleshot_timer:
     case VCPUOP_stop_singleshot_timer:
     case VCPUOP_register_vcpu_info:
+    case VCPUOP_register_vcpu_time_memory_area:
         rc = do_vcpu_op(cmd, vcpuid, arg);
         break;
     default:
@@ -3232,6 +3233,7 @@ static long hvm_vcpu_op_compat32(
     case VCPUOP_set_singleshot_timer:
     case VCPUOP_stop_singleshot_timer:
     case VCPUOP_register_vcpu_info:
+    case VCPUOP_register_vcpu_time_memory_area:
         rc = compat_vcpu_op(cmd, vcpuid, arg);
         break;
     default:
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1231,8 +1231,6 @@ static void schedule(void)
     if ( next_slice.migrated )
         evtchn_move_pirqs(next);
 
-    /* Ensure that the domain has an up-to-date time base. */
-    update_vcpu_system_time(next);
     vcpu_periodic_timer_work(next);
 
     context_switch(prev, next);




[-- Attachment #2: reenable-vcpu-time-area.patch --]
[-- Type: text/plain, Size: 2974 bytes --]

x86: re-enable VCPUOP_register_vcpu_time_memory_area

By moving the call to update_vcpu_system_time() out of schedule() into
arch-specific context switch code, the original problem of the function
accessing the wrong domain's address space goes away (obvious even from
patch context, as update_runstate_area() does similar copying).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Regardless of the code freeze I'd still like to propose this for
inclusion in 4.3, mainly based on the fact that this got disabled late
in the 4.0 release cycle with the expectation that it would get
re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
least on x86-64, the necessary hypervisor independent support code, it
would be odd to not leverage this on Xen.

--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -232,6 +232,9 @@ static void schedule_tail(struct vcpu *p
 
     if ( prev != current )
         update_runstate_area(current);
+
+    /* Ensure that the vcpu has an up-to-date time base. */
+    update_vcpu_system_time(current);
 }
 
 static void continue_new_vcpu(struct vcpu *prev)
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -966,11 +966,6 @@ arch_do_vcpu_op(
 
     switch ( cmd )
     {
-    /*
-     * XXX Disable for 4.0.0: __update_vcpu_system_time() writes to the given
-     * virtual address even when running in another domain's address space.
-     */
-#if 0
     case VCPUOP_register_vcpu_time_memory_area:
     {
         struct vcpu_register_time_memory_area area;
@@ -989,7 +984,6 @@ arch_do_vcpu_op(
 
         break;
     }
-#endif
 
     case VCPUOP_get_physid:
     {
@@ -1457,6 +1451,9 @@ void context_switch(struct vcpu *prev, s
     if (prev != next)
         update_runstate_area(next);
 
+    /* Ensure that the vcpu has an up-to-date time base. */
+    update_vcpu_system_time(next);
+
     schedule_tail(next);
     BUG();
 }
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3174,6 +3174,7 @@ static long hvm_vcpu_op(
     case VCPUOP_set_singleshot_timer:
     case VCPUOP_stop_singleshot_timer:
     case VCPUOP_register_vcpu_info:
+    case VCPUOP_register_vcpu_time_memory_area:
         rc = do_vcpu_op(cmd, vcpuid, arg);
         break;
     default:
@@ -3232,6 +3233,7 @@ static long hvm_vcpu_op_compat32(
     case VCPUOP_set_singleshot_timer:
     case VCPUOP_stop_singleshot_timer:
     case VCPUOP_register_vcpu_info:
+    case VCPUOP_register_vcpu_time_memory_area:
         rc = compat_vcpu_op(cmd, vcpuid, arg);
         break;
     default:
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1231,8 +1231,6 @@ static void schedule(void)
     if ( next_slice.migrated )
         evtchn_move_pirqs(next);
 
-    /* Ensure that the domain has an up-to-date time base. */
-    update_vcpu_system_time(next);
     vcpu_periodic_timer_work(next);
 
     context_switch(prev, next);

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

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

Re: [PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[Keir Fraser]

On 16/05/2013 09:47, "Jan Beulich" <JBeulich@suse.com> wrote:

> By moving the call to update_vcpu_system_time() out of schedule() into
> arch-specific context switch code, the original problem of the function
> accessing the wrong domain's address space goes away (obvious even from
> patch context, as update_runstate_area() does similar copying).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Keir Fraser <keir@xen.org>

> ---
> Regardless of the code freeze I'd still like to propose this for
> inclusion in 4.3, mainly based on the fact that this got disabled late
> in the 4.0 release cycle with the expectation that it would get
> re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
> least on x86-64, the necessary hypervisor independent support code, it
> would be odd to not leverage this on Xen.
> 
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -232,6 +232,9 @@ static void schedule_tail(struct vcpu *p
>  
>      if ( prev != current )
>          update_runstate_area(current);
> +
> +    /* Ensure that the vcpu has an up-to-date time base. */
> +    update_vcpu_system_time(current);
>  }
>  
>  static void continue_new_vcpu(struct vcpu *prev)
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -966,11 +966,6 @@ arch_do_vcpu_op(
>  
>      switch ( cmd )
>      {
> -    /*
> -     * XXX Disable for 4.0.0: __update_vcpu_system_time() writes to the given
> -     * virtual address even when running in another domain's address space.
> -     */
> -#if 0
>      case VCPUOP_register_vcpu_time_memory_area:
>      {
>          struct vcpu_register_time_memory_area area;
> @@ -989,7 +984,6 @@ arch_do_vcpu_op(
>  
>          break;
>      }
> -#endif
>  
>      case VCPUOP_get_physid:
>      {
> @@ -1457,6 +1451,9 @@ void context_switch(struct vcpu *prev, s
>      if (prev != next)
>          update_runstate_area(next);
>  
> +    /* Ensure that the vcpu has an up-to-date time base. */
> +    update_vcpu_system_time(next);
> +
>      schedule_tail(next);
>      BUG();
>  }
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3174,6 +3174,7 @@ static long hvm_vcpu_op(
>      case VCPUOP_set_singleshot_timer:
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
> +    case VCPUOP_register_vcpu_time_memory_area:
>          rc = do_vcpu_op(cmd, vcpuid, arg);
>          break;
>      default:
> @@ -3232,6 +3233,7 @@ static long hvm_vcpu_op_compat32(
>      case VCPUOP_set_singleshot_timer:
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
> +    case VCPUOP_register_vcpu_time_memory_area:
>          rc = compat_vcpu_op(cmd, vcpuid, arg);
>          break;
>      default:
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1231,8 +1231,6 @@ static void schedule(void)
>      if ( next_slice.migrated )
>          evtchn_move_pirqs(next);
>  
> -    /* Ensure that the domain has an up-to-date time base. */
> -    update_vcpu_system_time(next);
>      vcpu_periodic_timer_work(next);
>  
>      context_switch(prev, next);
> 
> 
> 

Re: [PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[Jan Beulich]

>>> On 16.05.13 at 11:44, Keir Fraser <keir.xen@gmail.com> wrote:
> On 16/05/2013 09:47, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> By moving the call to update_vcpu_system_time() out of schedule() into
>> arch-specific context switch code, the original problem of the function
>> accessing the wrong domain's address space goes away (obvious even from
>> patch context, as update_runstate_area() does similar copying).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Keir Fraser <keir@xen.org>
> 
>> ---
>> Regardless of the code freeze I'd still like to propose this for
>> inclusion in 4.3, mainly based on the fact that this got disabled late
>> in the 4.0 release cycle with the expectation that it would get
>> re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
>> least on x86-64, the necessary hypervisor independent support code, it
>> would be odd to not leverage this on Xen.

George?

>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -232,6 +232,9 @@ static void schedule_tail(struct vcpu *p
>>  
>>      if ( prev != current )
>>          update_runstate_area(current);
>> +
>> +    /* Ensure that the vcpu has an up-to-date time base. */
>> +    update_vcpu_system_time(current);
>>  }
>>  
>>  static void continue_new_vcpu(struct vcpu *prev)
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -966,11 +966,6 @@ arch_do_vcpu_op(
>>  
>>      switch ( cmd )
>>      {
>> -    /*
>> -     * XXX Disable for 4.0.0: __update_vcpu_system_time() writes to the 
> given
>> -     * virtual address even when running in another domain's address space.
>> -     */
>> -#if 0
>>      case VCPUOP_register_vcpu_time_memory_area:
>>      {
>>          struct vcpu_register_time_memory_area area;
>> @@ -989,7 +984,6 @@ arch_do_vcpu_op(
>>  
>>          break;
>>      }
>> -#endif
>>  
>>      case VCPUOP_get_physid:
>>      {
>> @@ -1457,6 +1451,9 @@ void context_switch(struct vcpu *prev, s
>>      if (prev != next)
>>          update_runstate_area(next);
>>  
>> +    /* Ensure that the vcpu has an up-to-date time base. */
>> +    update_vcpu_system_time(next);
>> +
>>      schedule_tail(next);
>>      BUG();
>>  }
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3174,6 +3174,7 @@ static long hvm_vcpu_op(
>>      case VCPUOP_set_singleshot_timer:
>>      case VCPUOP_stop_singleshot_timer:
>>      case VCPUOP_register_vcpu_info:
>> +    case VCPUOP_register_vcpu_time_memory_area:
>>          rc = do_vcpu_op(cmd, vcpuid, arg);
>>          break;
>>      default:
>> @@ -3232,6 +3233,7 @@ static long hvm_vcpu_op_compat32(
>>      case VCPUOP_set_singleshot_timer:
>>      case VCPUOP_stop_singleshot_timer:
>>      case VCPUOP_register_vcpu_info:
>> +    case VCPUOP_register_vcpu_time_memory_area:
>>          rc = compat_vcpu_op(cmd, vcpuid, arg);
>>          break;
>>      default:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1231,8 +1231,6 @@ static void schedule(void)
>>      if ( next_slice.migrated )
>>          evtchn_move_pirqs(next);
>>  
>> -    /* Ensure that the domain has an up-to-date time base. */
>> -    update_vcpu_system_time(next);
>>      vcpu_periodic_timer_work(next);
>>  
>>      context_switch(prev, next);
>> 
>> 
>> 

Re: [PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[Ian Campbell]

On Thu, 2013-05-16 at 09:47 +0100, Jan Beulich wrote:
> By moving the call to update_vcpu_system_time() out of schedule() into
> arch-specific context switch code, the original problem of the function
> accessing the wrong domain's address space goes away (obvious even from
> patch context, as update_runstate_area() does similar copying).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
> Regardless of the code freeze I'd still like to propose this for
> inclusion in 4.3, mainly based on the fact that this got disabled late
> in the 4.0 release cycle with the expectation that it would get
> re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
> least on x86-64, the necessary hypervisor independent support code, it
> would be odd to not leverage this on Xen.

I went looking for a call to VCPUOP_register_vcpu_time_memory_area in
3.10-rc1 and can't find it -- what have I missed?

Re: [PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[Jan Beulich]

>>> On 16.05.13 at 16:40, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Thu, 2013-05-16 at 09:47 +0100, Jan Beulich wrote:
>> Regardless of the code freeze I'd still like to propose this for
>> inclusion in 4.3, mainly based on the fact that this got disabled late
>> in the 4.0 release cycle with the expectation that it would get
>> re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
>> least on x86-64, the necessary hypervisor independent support code, it
>> would be odd to not leverage this on Xen.
> 
> I went looking for a call to VCPUOP_register_vcpu_time_memory_area in
> 3.10-rc1 and can't find it -- what have I missed?

The words "hypervisor independent" in my explanation. Iirc Jeremy
had a patch, and the non-Xen pieces got extracted from it for KVM
(in 3.8 I think). The Xen parts would need to be recovered and
pushed upstream. The way I did this for our kernel isn't even coming
close to what would be needed for upstreaming.

Jan

Re: [PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[Matt Wilson]

On Thu, May 16, 2013 at 04:01:33PM +0100, Jan Beulich wrote:
> >>> On 16.05.13 at 16:40, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Thu, 2013-05-16 at 09:47 +0100, Jan Beulich wrote:
> >> Regardless of the code freeze I'd still like to propose this for
> >> inclusion in 4.3, mainly based on the fact that this got disabled late
> >> in the 4.0 release cycle with the expectation that it would get
> >> re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
> >> least on x86-64, the necessary hypervisor independent support code, it
> >> would be odd to not leverage this on Xen.
> > 
> > I went looking for a call to VCPUOP_register_vcpu_time_memory_area in
> > 3.10-rc1 and can't find it -- what have I missed?
> 
> The words "hypervisor independent" in my explanation. Iirc Jeremy
> had a patch, and the non-Xen pieces got extracted from it for KVM
> (in 3.8 I think). The Xen parts would need to be recovered and
> pushed upstream. The way I did this for our kernel isn't even coming
> close to what would be needed for upstreaming.

Does this look like the right bit of old code? 
  https://lkml.org/lkml/2009/10/5/391

--msw

Re: [PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[Jan Beulich]

>>> On 16.05.13 at 18:15, Matt Wilson <msw@amazon.com> wrote:
> On Thu, May 16, 2013 at 04:01:33PM +0100, Jan Beulich wrote:
>> >>> On 16.05.13 at 16:40, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > On Thu, 2013-05-16 at 09:47 +0100, Jan Beulich wrote:
>> >> Regardless of the code freeze I'd still like to propose this for
>> >> inclusion in 4.3, mainly based on the fact that this got disabled late
>> >> in the 4.0 release cycle with the expectation that it would get
>> >> re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
>> >> least on x86-64, the necessary hypervisor independent support code, it
>> >> would be odd to not leverage this on Xen.
>> > 
>> > I went looking for a call to VCPUOP_register_vcpu_time_memory_area in
>> > 3.10-rc1 and can't find it -- what have I missed?
>> 
>> The words "hypervisor independent" in my explanation. Iirc Jeremy
>> had a patch, and the non-Xen pieces got extracted from it for KVM
>> (in 3.8 I think). The Xen parts would need to be recovered and
>> pushed upstream. The way I did this for our kernel isn't even coming
>> close to what would be needed for upstreaming.
> 
> Does this look like the right bit of old code? 
>   https://lkml.org/lkml/2009/10/5/391 

Yes, it does. But from a cursory look it'll need quite a bit up updating/
polishing.

Jan

Re: [PATCH] x86: re-enable VCPUOP_register_vcpu_time_memory_area

[Ian Campbell]

On Thu, 2013-05-16 at 17:15 +0100, Matt Wilson wrote:
> On Thu, May 16, 2013 at 04:01:33PM +0100, Jan Beulich wrote:
> > >>> On 16.05.13 at 16:40, Ian Campbell <ian.campbell@citrix.com> wrote:
> > > On Thu, 2013-05-16 at 09:47 +0100, Jan Beulich wrote:
> > >> Regardless of the code freeze I'd still like to propose this for
> > >> inclusion in 4.3, mainly based on the fact that this got disabled late
> > >> in the 4.0 release cycle with the expectation that it would get
> > >> re-enabled soon after. Now that upstream Linux also has, as of 3.8 at
> > >> least on x86-64, the necessary hypervisor independent support code, it
> > >> would be odd to not leverage this on Xen.
> > > 
> > > I went looking for a call to VCPUOP_register_vcpu_time_memory_area in
> > > 3.10-rc1 and can't find it -- what have I missed?
> > 
> > The words "hypervisor independent" in my explanation. Iirc Jeremy
> > had a patch, and the non-Xen pieces got extracted from it for KVM
> > (in 3.8 I think). The Xen parts would need to be recovered and
> > pushed upstream. The way I did this for our kernel isn't even coming
> > close to what would be needed for upstreaming.
> 
> Does this look like the right bit of old code? 
>   https://lkml.org/lkml/2009/10/5/391

That looks like the xen bit but it seems incomplete -- I expect there is
a generic precursor somewhere.

The interesting upstream commits seem to be these two, although there
are various subsequent fixups as well.
        
        commit 3dc4f7cfb7441e5e0fed3a02fc81cdaabd28300a
        Author: Marcelo Tosatti <mtosatti@redhat.com>
        Date:   Tue Nov 27 23:28:56 2012 -0200
        
            x86: kvm guest: pvclock vsyscall support
            
            Hook into generic pvclock vsyscall code, with the aim to
            allow userspace to have visibility into pvclock data.
            
            Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
        
        commit 71056ae22d43f58d7e0f793af18ace2eaf5b74eb
        Author: Marcelo Tosatti <mtosatti@redhat.com>
        Date:   Tue Nov 27 23:28:55 2012 -0200
        
            x86: pvclock: generic pvclock vsyscall initialization
            
            Originally from Jeremy Fitzhardinge.
            
            Introduce generic, non hypervisor specific, pvclock initialization
            routines.
            
            Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Ian.