Xen devel - Jun 2013 - [PATCH v2] xen: fix initialization of wallclock time for PVHVM on migration

Call update_domain_wallclock_time on hvm_latch_shinfo_size even if
the bitness of the guest has already been set, this fixes the problem
with the wallclock not being set for PVHVM guests on resume from
migration.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
---
Since this is a bug fix, I think it is suitable for inclusion in the
4.3 release, and backported to older releases.
---
 xen/arch/x86/hvm/hvm.c |   20 ++++++--------------
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a962ce2..0dcfd81 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3404,21 +3404,13 @@ static void hvm_latch_shinfo_size(struct domain *d)
      */
     if ( current->domain == d ) {
         new_has_32bit = (hvm_guest_x86_mode(current) != 8);
-        if (new_has_32bit != d->arch.has_32bit_shinfo) {
+        if (new_has_32bit != d->arch.has_32bit_shinfo)
             d->arch.has_32bit_shinfo = new_has_32bit;
-            /*
-             * Make sure that the timebase in the shared info
-             * structure is correct for its new bit-ness.  We should
-             * arguably try to convert the other fields as well, but
-             * that's much more problematic (e.g. what do you do if
-             * you're going from 64 bit to 32 bit and there's an event
-             * channel pending which doesn't exist in the 32 bit
-             * version?).  Just setting the wallclock time seems to be
-             * sufficient for everything we do, even if it is a bit of
-             * a hack.
-             */
-            update_domain_wallclock_time(d);
-        }
+        /*
+         * Make sure that the timebase in the shared info
+         * structure is correct.
+         */
+        update_domain_wallclock_time(d);
     }
 }
 
-- 
1.7.7.5 (Apple Git-26)


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

On 11/06/2013 17:41, "Roger Pau Monne" <roger.pau@citrix.com>
wrote:
> Call update_domain_wallclock_time on hvm_latch_shinfo_size even if
> the bitness of the guest has already been set, this fixes the problem
> with the wallclock not being set for PVHVM guests on resume from
> migration.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
May as well write directly into d->arch.has_32bit_shinfo and get rid of
new_has_32bit. But apart from that:

Acked-by: Keir Fraser <keir@xen.org>
> ---
> Since this is a bug fix, I think it is suitable for inclusion in the
> 4.3 release, and backported to older releases.
> ---
>  xen/arch/x86/hvm/hvm.c |   20 ++++++--------------
>  1 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index a962ce2..0dcfd81 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3404,21 +3404,13 @@ static void hvm_latch_shinfo_size(struct domain *d)
>       */
>      if ( current->domain == d ) {
>          new_has_32bit = (hvm_guest_x86_mode(current) != 8);
> -        if (new_has_32bit != d->arch.has_32bit_shinfo) {
> +        if (new_has_32bit != d->arch.has_32bit_shinfo)
>              d->arch.has_32bit_shinfo = new_has_32bit;
> -            /*
> -             * Make sure that the timebase in the shared info
> -             * structure is correct for its new bit-ness.  We should
> -             * arguably try to convert the other fields as well, but
> -             * that''s much more problematic (e.g. what do you do
if
> -             * you''re going from 64 bit to 32 bit and
there''s an event
> -             * channel pending which doesn''t exist in the 32 bit
> -             * version?).  Just setting the wallclock time seems to be
> -             * sufficient for everything we do, even if it is a bit of
> -             * a hack.
> -             */
> -            update_domain_wallclock_time(d);
> -        }
> +        /*
> +         * Make sure that the timebase in the shared info
> +         * structure is correct.
> +         */
> +        update_domain_wallclock_time(d);
>      }
>  }
>

On 11/06/13 18:02, Keir Fraser wrote:
> On 11/06/2013 17:41, "Roger Pau Monne"
<roger.pau@citrix.com> wrote:
>
>> Call update_domain_wallclock_time on hvm_latch_shinfo_size even if
>> the bitness of the guest has already been set, this fixes the problem
>> with the wallclock not being set for PVHVM guests on resume from
>> migration.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Jan Beulich <JBeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> May as well write directly into d->arch.has_32bit_shinfo and get rid of
> new_has_32bit. But apart from that:
>
> Acked-by: Keir Fraser <keir@xen.org>
Yikes - we have had a patch in the XenServer patch queue for donkeys
years which implements the same fix as this.

I had still not manage to decide whether it was a gross hack which we
needed to discard or whether it needed upstreaming.

I guess this answers the question.

~Andrew
>
>> ---
>> Since this is a bug fix, I think it is suitable for inclusion in the
>> 4.3 release, and backported to older releases.
>> ---
>>  xen/arch/x86/hvm/hvm.c |   20 ++++++--------------
>>  1 files changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index a962ce2..0dcfd81 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3404,21 +3404,13 @@ static void hvm_latch_shinfo_size(struct domain
*d)
>>       */
>>      if ( current->domain == d ) {
>>          new_has_32bit = (hvm_guest_x86_mode(current) != 8);
>> -        if (new_has_32bit != d->arch.has_32bit_shinfo) {
>> +        if (new_has_32bit != d->arch.has_32bit_shinfo)
>>              d->arch.has_32bit_shinfo = new_has_32bit;
>> -            /*
>> -             * Make sure that the timebase in the shared info
>> -             * structure is correct for its new bit-ness.  We should
>> -             * arguably try to convert the other fields as well, but
>> -             * that''s much more problematic (e.g. what do you
do if
>> -             * you''re going from 64 bit to 32 bit and
there''s an event
>> -             * channel pending which doesn''t exist in the 32
bit
>> -             * version?).  Just setting the wallclock time seems to be
>> -             * sufficient for everything we do, even if it is a bit of
>> -             * a hack.
>> -             */
>> -            update_domain_wallclock_time(d);
>> -        }
>> +        /*
>> +         * Make sure that the timebase in the shared info
>> +         * structure is correct.
>> +         */
>> +        update_domain_wallclock_time(d);
>>      }
>>  }
>>  
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 11.06.13 at 20:09, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 11/06/13 18:02, Keir Fraser wrote:
>> On 11/06/2013 17:41, "Roger Pau Monne"
<roger.pau@citrix.com> wrote:
>>
>>> Call update_domain_wallclock_time on hvm_latch_shinfo_size even if
>>> the bitness of the guest has already been set, this fixes the
problem
>>> with the wallclock not being set for PVHVM guests on resume from
>>> migration.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Cc: Jan Beulich <JBeulich@suse.com>
>>> Cc: Keir Fraser <keir@xen.org>
>>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> May as well write directly into d->arch.has_32bit_shinfo and get rid
of
>> new_has_32bit. But apart from that:
>>
>> Acked-by: Keir Fraser <keir@xen.org>
> 
> Yikes - we have had a patch in the XenServer patch queue for donkeys
> years which implements the same fix as this.
> 
> I had still not manage to decide whether it was a gross hack which we
> needed to discard or whether it needed upstreaming.
> 
> I guess this answers the question.
Sounds like an ack/review then...

Jan

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

>>> On 11.06.13 at 19:02, Keir Fraser <keir.xen@gmail.com> wrote:
> On 11/06/2013 17:41, "Roger Pau Monne"
<roger.pau@citrix.com> wrote:
> 
>> Call update_domain_wallclock_time on hvm_latch_shinfo_size even if
>> the bitness of the guest has already been set, this fixes the problem
>> with the wallclock not being set for PVHVM guests on resume from
>> migration.
>> 
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Jan Beulich <JBeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> 
> May as well write directly into d->arch.has_32bit_shinfo and get rid of
> new_has_32bit.
Yeah, I certainly will want to do that as I commit it, even if it
grows the patch size.

Jan

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

On 11/06/2013 19:09, "Andrew Cooper" <andrew.cooper3@citrix.com>
wrote:
> On 11/06/13 18:02, Keir Fraser wrote:
>> On 11/06/2013 17:41, "Roger Pau Monne"
<roger.pau@citrix.com> wrote:
>> 
>>> Call update_domain_wallclock_time on hvm_latch_shinfo_size even if
>>> the bitness of the guest has already been set, this fixes the
problem
>>> with the wallclock not being set for PVHVM guests on resume from
>>> migration.
>>> 
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Cc: Jan Beulich <JBeulich@suse.com>
>>> Cc: Keir Fraser <keir@xen.org>
>>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> May as well write directly into d->arch.has_32bit_shinfo and get rid
of
>> new_has_32bit. But apart from that:
>> 
>> Acked-by: Keir Fraser <keir@xen.org>
> 
> Yikes - we have had a patch in the XenServer patch queue for donkeys
> years which implements the same fix as this.
> 
> I had still not manage to decide whether it was a gross hack which we
> needed to discard or whether it needed upstreaming.
> 
> I guess this answers the question.
I believe the morally right(er) patch would be bigger and more fragile. This
is a bit of a hack, but the whole idea of inferring shared-info bitness from
sampling some cpu''s execution mode is a bit odd (but necessary).

 -- Keir
> ~Andrew
> 
>> 
>>> ---
>>> Since this is a bug fix, I think it is suitable for inclusion in
the
>>> 4.3 release, and backported to older releases.
>>> ---
>>>  xen/arch/x86/hvm/hvm.c |   20 ++++++--------------
>>>  1 files changed, 6 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index a962ce2..0dcfd81 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3404,21 +3404,13 @@ static void hvm_latch_shinfo_size(struct
domain *d)
>>>       */
>>>      if ( current->domain == d ) {
>>>          new_has_32bit = (hvm_guest_x86_mode(current) != 8);
>>> -        if (new_has_32bit != d->arch.has_32bit_shinfo) {
>>> +        if (new_has_32bit != d->arch.has_32bit_shinfo)
>>>              d->arch.has_32bit_shinfo = new_has_32bit;
>>> -            /*
>>> -             * Make sure that the timebase in the shared info
>>> -             * structure is correct for its new bit-ness.  We
should
>>> -             * arguably try to convert the other fields as well,
but
>>> -             * that''s much more problematic (e.g. what do
you do if
>>> -             * you''re going from 64 bit to 32 bit and
there''s an event
>>> -             * channel pending which doesn''t exist in the
32 bit
>>> -             * version?).  Just setting the wallclock time seems
to be
>>> -             * sufficient for everything we do, even if it is a
bit of
>>> -             * a hack.
>>> -             */
>>> -            update_domain_wallclock_time(d);
>>> -        }
>>> +        /*
>>> +         * Make sure that the timebase in the shared info
>>> +         * structure is correct.
>>> +         */
>>> +        update_domain_wallclock_time(d);
>>>      }
>>>  }
>>>  
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

On 11/06/13 17:41, Roger Pau Monne wrote:
> Call update_domain_wallclock_time on hvm_latch_shinfo_size even if
> the bitness of the guest has already been set, this fixes the problem
> with the wallclock not being set for PVHVM guests on resume from
> migration.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Re the release:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Since this is a bug fix, I think it is suitable for inclusion in the
> 4.3 release, and backported to older releases.
> ---
>   xen/arch/x86/hvm/hvm.c |   20 ++++++--------------
>   1 files changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index a962ce2..0dcfd81 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3404,21 +3404,13 @@ static void hvm_latch_shinfo_size(struct domain *d)
>        */
>       if ( current->domain == d ) {
>           new_has_32bit = (hvm_guest_x86_mode(current) != 8);
> -        if (new_has_32bit != d->arch.has_32bit_shinfo) {
> +        if (new_has_32bit != d->arch.has_32bit_shinfo)
>               d->arch.has_32bit_shinfo = new_has_32bit;
> -            /*
> -             * Make sure that the timebase in the shared info
> -             * structure is correct for its new bit-ness.  We should
> -             * arguably try to convert the other fields as well, but
> -             * that's much more problematic (e.g. what do you do if
> -             * you're going from 64 bit to 32 bit and there's an
event
> -             * channel pending which doesn't exist in the 32 bit
> -             * version?).  Just setting the wallclock time seems to be
> -             * sufficient for everything we do, even if it is a bit of
> -             * a hack.
> -             */
> -            update_domain_wallclock_time(d);
> -        }
> +        /*
> +         * Make sure that the timebase in the shared info
> +         * structure is correct.
> +         */
> +        update_domain_wallclock_time(d);
>       }
>   }
>   

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

On 12/06/13 08:41, Jan Beulich wrote:
>>>> On 11.06.13 at 20:09, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> On 11/06/13 18:02, Keir Fraser wrote:
>>> On 11/06/2013 17:41, "Roger Pau Monne"
<roger.pau@citrix.com> wrote:
>>>
>>>> Call update_domain_wallclock_time on hvm_latch_shinfo_size even
if
>>>> the bitness of the guest has already been set, this fixes the
problem
>>>> with the wallclock not being set for PVHVM guests on resume
from
>>>> migration.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> Cc: Jan Beulich <JBeulich@suse.com>
>>>> Cc: Keir Fraser <keir@xen.org>
>>>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>>> May as well write directly into d->arch.has_32bit_shinfo and get
rid of
>>> new_has_32bit. But apart from that:
>>>
>>> Acked-by: Keir Fraser <keir@xen.org>
>> Yikes - we have had a patch in the XenServer patch queue for donkeys
>> years which implements the same fix as this.
>>
>> I had still not manage to decide whether it was a gross hack which we
>> needed to discard or whether it needed upstreaming.
>>
>> I guess this answers the question.
> Sounds like an ack/review then...
>
> Jan
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

On closer inspection our patch is not actually so similar, but it still
is achieving the same effect using a rather more convoluted method.

I am rather embarrased to say that the patch in our queue completely
abuses HVM_PARAM_32BIT, value 8 (which is why that value is curiously
missing upstream, given HVM_PARAM_VIRIDIAN at 9)

~Andrew

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

On 12/06/13 10:36, Keir Fraser wrote:
> On 11/06/2013 19:09, "Andrew Cooper"
<andrew.cooper3@citrix.com> wrote:
>
>> On 11/06/13 18:02, Keir Fraser wrote:
>>> On 11/06/2013 17:41, "Roger Pau Monne"
<roger.pau@citrix.com> wrote:
>>>
>>>> Call update_domain_wallclock_time on hvm_latch_shinfo_size even
if
>>>> the bitness of the guest has already been set, this fixes the
problem
>>>> with the wallclock not being set for PVHVM guests on resume
from
>>>> migration.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> Cc: Jan Beulich <JBeulich@suse.com>
>>>> Cc: Keir Fraser <keir@xen.org>
>>>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>>> May as well write directly into d->arch.has_32bit_shinfo and get
rid of
>>> new_has_32bit. But apart from that:
>>>
>>> Acked-by: Keir Fraser <keir@xen.org>
>> Yikes - we have had a patch in the XenServer patch queue for donkeys
>> years which implements the same fix as this.
>>
>> I had still not manage to decide whether it was a gross hack which we
>> needed to discard or whether it needed upstreaming.
>>
>> I guess this answers the question.
> I believe the morally right(er) patch would be bigger and more fragile.
This
> is a bit of a hack, but the whole idea of inferring shared-info bitness
from
> sampling some cpu''s execution mode is a bit odd (but necessary).
>
>  -- Keir
The way XenServer does this is that the windows drivers explicitly make
a hypercall to set their intended bitness using HVM_PARAM_32BIT, value
8, but it is itself a gross hack.

~Andrew
>
>> ~Andrew
>>
>>>> ---
>>>> Since this is a bug fix, I think it is suitable for inclusion
in the
>>>> 4.3 release, and backported to older releases.
>>>> ---
>>>>  xen/arch/x86/hvm/hvm.c |   20 ++++++--------------
>>>>  1 files changed, 6 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>> index a962ce2..0dcfd81 100644
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -3404,21 +3404,13 @@ static void
hvm_latch_shinfo_size(struct domain *d)
>>>>       */
>>>>      if ( current->domain == d ) {
>>>>          new_has_32bit = (hvm_guest_x86_mode(current) != 8);
>>>> -        if (new_has_32bit != d->arch.has_32bit_shinfo) {
>>>> +        if (new_has_32bit != d->arch.has_32bit_shinfo)
>>>>              d->arch.has_32bit_shinfo = new_has_32bit;
>>>> -            /*
>>>> -             * Make sure that the timebase in the shared info
>>>> -             * structure is correct for its new bit-ness.  We
should
>>>> -             * arguably try to convert the other fields as
well, but
>>>> -             * that''s much more problematic (e.g.
what do you do if
>>>> -             * you''re going from 64 bit to 32 bit and
there''s an event
>>>> -             * channel pending which doesn''t exist in
the 32 bit
>>>> -             * version?).  Just setting the wallclock time
seems to be
>>>> -             * sufficient for everything we do, even if it is
a bit of
>>>> -             * a hack.
>>>> -             */
>>>> -            update_domain_wallclock_time(d);
>>>> -        }
>>>> +        /*
>>>> +         * Make sure that the timebase in the shared info
>>>> +         * structure is correct.
>>>> +         */
>>>> +        update_domain_wallclock_time(d);
>>>>      }
>>>>  }
>>>>  
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>