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

["Roger Pau Monné" <roger.pau@citrix.com>]

On 11/06/13 17:45, Keir Fraser wrote:
> On 11/06/2013 16:05, "Keir Fraser" <keir.xen@gmail.com> wrote:
> 
>> On 11/06/2013 15:16, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>>>> Would it be OK to call
>>>> update_domain_wallclock_time unconditionally on
>>>> hvm_hypercall_page_initialise?
>>>
>>> The primary question is - why is what we have not enough for you?
>>> In particular I would expect that the call from arch_set_info_guest()
>>> (for vCPU 0) should do what you want. Or wait, this is covering PV
>>> only. So yes, with the description change I would then withdraw my
>>> NACK - apparently no-one really used the shared info wall clock
>>> time in a HVM guest so far (or it going wrong post-resume wasn't
>>> noticed).
>>>
>>> I would, however, prefer the if() immediately preceding the patch
>>> context to be pulled out past the domain_lock()ed region, convert it
>>> to switch(), and add your code. That was, eventual other post-
>>> processing for the various map spaces has a consistent, easily
>>> extensible home.
>>
>> I apparently made a fix for this to work on initial boot of a 32-bit PVHVM
>> guest back in September (a change in hvmloader to not zero the wc fields in
>> shared_info). But I agree I now can't see why it works... But it surely does
>> as it was tested to do so by Konrad.
>>
>> A bit more digging required...
> 
> Hmm I can't find any confirmation that my patch actually *did* work. :( I'm
> sure I remember testing it though!
> 
> My suggestion is we do indeed remove the inner if() in latch_shinfo_size().
> Ie. Call update_domain_wallclock_time() even if shinfo size has apparently
> not changed. 
> 
> We only latch shinfo size on hypercall page initialisation and on setup of
> the callback irq. They are start-of-day/resume operations, so removing the
> if() should have no bad side effect that I can see. If nothing else it
> should make this wallclock-field setup more robust.

So it would be better to call update_domain_wallclock_time
unconditionally on latch_shinfo_size rather than doing it on
XENMAPSPACE_shared_info?

Conceptially it makes more sense IMHO to do it in the call to
XENMAPSPACE_shared_info.