Re: [PATCH v2] xen: fix initialization of wallclock time for PVHVM on migration

[Keir Fraser <keir.xen@gmail.com>]

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
>