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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/06/13 10:36, Keir Fraser wrote:
> On 11/06/2013 19:09, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx> wrote:
>
>> On 11/06/13 18:02, Keir Fraser wrote:
>>> On 11/06/2013 17:41, "Roger Pau Monne" <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>> Cc: Jan Beulich <JBeulich@xxxxxxxx>
>>>> Cc: Keir Fraser <keir@xxxxxxx>
>>>> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>>> 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@xxxxxxx>
>> 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@xxxxxxxxxxxxx
>>> http://lists.xen.org/xen-devel
>


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