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

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

[Roger Pau Monne]

The initial values of the wallclock time in the shared info page are
set for PVHVM guests when the hypercall page is initialized, since the
hypercall page is not reinitialized on resume, the hypervisor
wallclock time is not properly set on resume.

Fix it by forcing an update of the wallclock values when the shared
info page is mapped.

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/mm.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5123860..1591528 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4629,6 +4629,14 @@ static int xenmem_add_to_physmap_once(
         put_gfn(d, gfn);
     domain_unlock(d);
 
+    if ( !rc && xatp->space == XENMAPSPACE_shared_info )
+        /*
+         * Force an update of the wallclock values in the shared info
+         * page, since in the PVHVM resume path the hypercall page is
+         * not reinitialized.
+         */
+        update_domain_wallclock_time(d);
+
     return rc;
 }
 
-- 
1.7.7.5 (Apple Git-26)


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

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

[Jan Beulich]

>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
> The initial values of the wallclock time in the shared info page are
> set for PVHVM guests when the hypercall page is initialized, since the
> hypercall page is not reinitialized on resume, the hypervisor
> wallclock time is not properly set on resume.
> 
> Fix it by forcing an update of the wallclock values when the shared
> info page is mapped.

NACK - this is a guest side bug. After migration, a guest _has_ to
re-init the hypercall page, as it may have got migrated between
a VMX and an SVM machine, and the hypercall instructions are
different between them.

Jan

> 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/mm.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 5123860..1591528 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4629,6 +4629,14 @@ static int xenmem_add_to_physmap_once(
>          put_gfn(d, gfn);
>      domain_unlock(d);
>  
> +    if ( !rc && xatp->space == XENMAPSPACE_shared_info )
> +        /*
> +         * Force an update of the wallclock values in the shared info
> +         * page, since in the PVHVM resume path the hypercall page is
> +         * not reinitialized.
> +         */
> +        update_domain_wallclock_time(d);
> +
>      return rc;
>  }
>  
> -- 
> 1.7.7.5 (Apple Git-26)



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

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

[George Dunlap]

On 06/11/2013 12:59 PM, Jan Beulich wrote:
>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> The initial values of the wallclock time in the shared info page are
>> set for PVHVM guests when the hypercall page is initialized, since the
>> hypercall page is not reinitialized on resume, the hypervisor
>> wallclock time is not properly set on resume.
>>
>> Fix it by forcing an update of the wallclock values when the shared
>> info page is mapped.
>
> NACK - this is a guest side bug. After migration, a guest _has_ to
> re-init the hypercall page, as it may have got migrated between
> a VMX and an SVM machine, and the hypercall instructions are
> different between them.

Does migrating from VMX to SVM actually work?

In any case, if Linux does it, then FreeBSD should do it too. (I assume 
this came up because you're working on the FreeBSD PVHVM port?)

  -George

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

[Jan Beulich]

>>> On 11.06.13 at 14:01, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 06/11/2013 12:59 PM, Jan Beulich wrote:
>>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> The initial values of the wallclock time in the shared info page are
>>> set for PVHVM guests when the hypercall page is initialized, since the
>>> hypercall page is not reinitialized on resume, the hypervisor
>>> wallclock time is not properly set on resume.
>>>
>>> Fix it by forcing an update of the wallclock values when the shared
>>> info page is mapped.
>>
>> NACK - this is a guest side bug. After migration, a guest _has_ to
>> re-init the hypercall page, as it may have got migrated between
>> a VMX and an SVM machine, and the hypercall instructions are
>> different between them.
> 
> Does migrating from VMX to SVM actually work?

If you sufficiently feature-restrict both hosts, it should work. At
least it was made work years ago iirc (see the sysenter/syscall
emulation code in the x86 emulator - the only case where this
may be needed is when doing cross vendor migration).

Jan

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

[Ian Campbell]

On Tue, 2013-06-11 at 13:12 +0100, Jan Beulich wrote:
> >>> On 11.06.13 at 14:01, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> > On 06/11/2013 12:59 PM, Jan Beulich wrote:
> >>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
> >>> The initial values of the wallclock time in the shared info page are
> >>> set for PVHVM guests when the hypercall page is initialized, since the
> >>> hypercall page is not reinitialized on resume, the hypervisor
> >>> wallclock time is not properly set on resume.
> >>>
> >>> Fix it by forcing an update of the wallclock values when the shared
> >>> info page is mapped.
> >>
> >> NACK - this is a guest side bug. After migration, a guest _has_ to
> >> re-init the hypercall page, as it may have got migrated between
> >> a VMX and an SVM machine, and the hypercall instructions are
> >> different between them.
> > 
> > Does migrating from VMX to SVM actually work?
> 
> If you sufficiently feature-restrict both hosts, it should work. At
> least it was made work years ago iirc (see the sysenter/syscall
> emulation code in the x86 emulator - the only case where this
> may be needed is when doing cross vendor migration).

Doesn't the presence of that emulation contradict the requirement to
reinit the hypercall page?

My recollection is that although it could be made to work the
performance would suffer horribly precisely due to this emulation.

Ian.

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

[Jan Beulich]

>>> On 11.06.13 at 15:59, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-06-11 at 13:12 +0100, Jan Beulich wrote:
>> >>> On 11.06.13 at 14:01, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> > On 06/11/2013 12:59 PM, Jan Beulich wrote:
>> >>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> >>> The initial values of the wallclock time in the shared info page are
>> >>> set for PVHVM guests when the hypercall page is initialized, since the
>> >>> hypercall page is not reinitialized on resume, the hypervisor
>> >>> wallclock time is not properly set on resume.
>> >>>
>> >>> Fix it by forcing an update of the wallclock values when the shared
>> >>> info page is mapped.
>> >>
>> >> NACK - this is a guest side bug. After migration, a guest _has_ to
>> >> re-init the hypercall page, as it may have got migrated between
>> >> a VMX and an SVM machine, and the hypercall instructions are
>> >> different between them.
>> > 
>> > Does migrating from VMX to SVM actually work?
>> 
>> If you sufficiently feature-restrict both hosts, it should work. At
>> least it was made work years ago iirc (see the sysenter/syscall
>> emulation code in the x86 emulator - the only case where this
>> may be needed is when doing cross vendor migration).
> 
> Doesn't the presence of that emulation contradict the requirement to
> reinit the hypercall page?

Definitely not - the emulation deals with system call instructions,
while the hypercall page holds hypercall instructions (which have
no emulation anywhere iirc).

> My recollection is that although it could be made to work the
> performance would suffer horribly precisely due to this emulation.

Performance isn't going to be good, sure. It's more for some kind
of emergency I think. Wasn't it Christoph Egger who implemented
this? Maybe he recalls more details than I do...

Jan

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

[Egger, Christoph]

On 11.06.13 16:04, Jan Beulich wrote:
>>>> On 11.06.13 at 15:59, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Tue, 2013-06-11 at 13:12 +0100, Jan Beulich wrote:
>>>>>> On 11.06.13 at 14:01, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>>> On 06/11/2013 12:59 PM, Jan Beulich wrote:
>>>>>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>>>> The initial values of the wallclock time in the shared info page are
>>>>>> set for PVHVM guests when the hypercall page is initialized, since the
>>>>>> hypercall page is not reinitialized on resume, the hypervisor
>>>>>> wallclock time is not properly set on resume.
>>>>>>
>>>>>> Fix it by forcing an update of the wallclock values when the shared
>>>>>> info page is mapped.
>>>>>
>>>>> NACK - this is a guest side bug. After migration, a guest _has_ to
>>>>> re-init the hypercall page, as it may have got migrated between
>>>>> a VMX and an SVM machine, and the hypercall instructions are
>>>>> different between them.
>>>>
>>>> Does migrating from VMX to SVM actually work?
>>>
>>> If you sufficiently feature-restrict both hosts, it should work. At
>>> least it was made work years ago iirc (see the sysenter/syscall
>>> emulation code in the x86 emulator - the only case where this
>>> may be needed is when doing cross vendor migration).
>>
>> Doesn't the presence of that emulation contradict the requirement to
>> reinit the hypercall page?
> 
> Definitely not - the emulation deals with system call instructions,
> while the hypercall page holds hypercall instructions (which have
> no emulation anywhere iirc).

Back at that time, yes migration from VMX to SVM and vice versa gotta
work. However, there are always things to consider:

1) Hypervisor side: When the guest OS uses MSRs that do not exist
on the destination machine must be emulated.
2) feature-set: Make sure both OS and application do not use
instructions (i.e. 3DNow, AVX, etc.) that isn't available on
destination side. You can archieve that by either setting cpuid
properly or emulate the missing instructions in the hypervisor.
The latter one will decrease performance.

>> My recollection is that although it could be made to work the
>> performance would suffer horribly precisely due to this emulation.
> 
> Performance isn't going to be good, sure. It's more for some kind
> of emergency I think. Wasn't it Christoph Egger who implemented
> this? Maybe he recalls more details than I do...

sysenter/syscall emulation comes into play with 32bit applications
in a 64bit guest OS. Yes, performance decreases but how much impact
this actually has on the end-user side depends on
a) the syscall itself (i.e. glibc caches the getpid syscall so the
performance penality shouldn't even be measureable while other syscalls
like read/write may suffer)
b) how often the syscall is actually in use

Christoph

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

[Roger Pau Monné]

On 11/06/13 13:59, Jan Beulich wrote:
>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> The initial values of the wallclock time in the shared info page are
>> set for PVHVM guests when the hypercall page is initialized, since the
>> hypercall page is not reinitialized on resume, the hypervisor
>> wallclock time is not properly set on resume.
>>
>> Fix it by forcing an update of the wallclock values when the shared
>> info page is mapped.
> 
> NACK - this is a guest side bug. After migration, a guest _has_ to
> re-init the hypercall page, as it may have got migrated between
> a VMX and an SVM machine, and the hypercall instructions are
> different between them.

AFAICS Linux doesn't seem to re-init the hypercall page, see
xen_arch_hvm_post_suspend at arch/x86/xen/suspend.c, but maybe I'm
missing something.

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

[konrad wilk]

On 6/11/2013 8:17 AM, Roger Pau Monné wrote:
> On 11/06/13 13:59, Jan Beulich wrote:
>>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> The initial values of the wallclock time in the shared info page are
>>> set for PVHVM guests when the hypercall page is initialized, since the
>>> hypercall page is not reinitialized on resume, the hypervisor
>>> wallclock time is not properly set on resume.
>>>
>>> Fix it by forcing an update of the wallclock values when the shared
>>> info page is mapped.
>> NACK - this is a guest side bug. After migration, a guest _has_ to
>> re-init the hypercall page, as it may have got migrated between
>> a VMX and an SVM machine, and the hypercall instructions are
>> different between them.
> AFAICS Linux doesn't seem to re-init the hypercall page, see
> xen_arch_hvm_post_suspend at arch/x86/xen/suspend.c, but maybe I'm
> missing something.
You are right. It does not re-init. It is the first time I heard that it 
has to re-init too.

In the past some steps were taken towards re-initing the shared page - 
that (on PVHVM) flat out died during migration (32-bit). Never figured 
out why and Olaf didn't have the time to dig in this.
(git commit 9d02b43dee0d7fb18dfb13a00915550b1a3daa9f).




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

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

[Jan Beulich]

>>> On 11.06.13 at 14:17, Roger Pau Monné<roger.pau@citrix.com> wrote:
> On 11/06/13 13:59, Jan Beulich wrote:
>>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> The initial values of the wallclock time in the shared info page are
>>> set for PVHVM guests when the hypercall page is initialized, since the
>>> hypercall page is not reinitialized on resume, the hypervisor
>>> wallclock time is not properly set on resume.
>>>
>>> Fix it by forcing an update of the wallclock values when the shared
>>> info page is mapped.
>> 
>> NACK - this is a guest side bug. After migration, a guest _has_ to
>> re-init the hypercall page, as it may have got migrated between
>> a VMX and an SVM machine, and the hypercall instructions are
>> different between them.
> 
> AFAICS Linux doesn't seem to re-init the hypercall page, see
> xen_arch_hvm_post_suspend at arch/x86/xen/suspend.c, but maybe I'm
> missing something.

I just looked at the unmodified_drivers/ tree, and there this re-init
is being done.

Jan

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

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

[Roger Pau Monné]

On 11/06/13 13:59, Jan Beulich wrote:
>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> The initial values of the wallclock time in the shared info page are
>> set for PVHVM guests when the hypercall page is initialized, since the
>> hypercall page is not reinitialized on resume, the hypervisor
>> wallclock time is not properly set on resume.
>>
>> Fix it by forcing an update of the wallclock values when the shared
>> info page is mapped.
> 
> NACK - this is a guest side bug. After migration, a guest _has_ to
> re-init the hypercall page, as it may have got migrated between
> a VMX and an SVM machine, and the hypercall instructions are
> different between them.

I've re-inited the hypercall page on resume, but the hypervisor
wallclock is still 0.

The call to update_domain_wallclock_time in hvm_latch_shinfo_size is
gated, and doesn't get called on the resume path. Would it be OK to call
update_domain_wallclock_time unconditionally on
hvm_hypercall_page_initialise?

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

[Jan Beulich]

>>> On 11.06.13 at 15:38, Roger Pau Monné<roger.pau@citrix.com> wrote:
> On 11/06/13 13:59, Jan Beulich wrote:
>>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> The initial values of the wallclock time in the shared info page are
>>> set for PVHVM guests when the hypercall page is initialized, since the
>>> hypercall page is not reinitialized on resume, the hypervisor
>>> wallclock time is not properly set on resume.
>>>
>>> Fix it by forcing an update of the wallclock values when the shared
>>> info page is mapped.
>> 
>> NACK - this is a guest side bug. After migration, a guest _has_ to
>> re-init the hypercall page, as it may have got migrated between
>> a VMX and an SVM machine, and the hypercall instructions are
>> different between them.
> 
> I've re-inited the hypercall page on resume, but the hypervisor
> wallclock is still 0.
> 
> The call to update_domain_wallclock_time in hvm_latch_shinfo_size is
> gated, and doesn't get called on the resume path.

And it really isn't meant to deal with resume anyway. I simply based
my original response on your description of the change, which now
it looks like was misleading.

> 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.

Jan

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

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

[Konrad Rzeszutek Wilk]

[-- Attachment #1.1: Type: text/plain, Size: 793 bytes --]

> 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).
>

It was. Olaf posted a patch that would re-init the shared page for PVHVM
guest
(for kexec to work) and it stopped migration on 32-bit guest doing
migration.

Olaf and me never got enough time to actually dig in this to figure out why
it was
busted :-(

[-- Attachment #1.2: Type: text/html, Size: 1188 bytes --]

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

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

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

[Keir Fraser]

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...

 -- Keir

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

[Keir Fraser]

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.

 -- Keir

>  -- Keir
> 
> 

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

[Roger Pau Monné]

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.

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

[Keir Fraser]

On 11/06/2013 16:59, "Roger Pau Monné" <roger.pau@citrix.com> wrote:

>> 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.

I would still make the fix in latch_shinfo_size() and perhaps add an extra
call to latch_shinfo_size() from the call to XENMAPSPACE_shared_info. But
actually I am sure you will find it unnecessary and at this point for Xen
4.3 I think the smallest possible patch wins.

 -- Keir

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

[Roger Pau Monné]

On 11/06/13 18:12, Keir Fraser wrote:
> On 11/06/2013 16:59, "Roger Pau Monné" <roger.pau@citrix.com> wrote:
> 
>>> 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.
> 
> I would still make the fix in latch_shinfo_size() and perhaps add an extra
> call to latch_shinfo_size() from the call to XENMAPSPACE_shared_info. But
> actually I am sure you will find it unnecessary and at this point for Xen
> 4.3 I think the smallest possible patch wins.

ACK, will resend the patch.

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

[Jan Beulich]

>>> On 11.06.13 at 17:45, Keir Fraser <keir.xen@gmail.com> 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.

I can't see anything bad with this either, so let's go that route if
it fixes Roger's problem.

Jan

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

[Konrad Rzeszutek Wilk]

On Tue, Jun 11, 2013 at 04:45:29PM +0100, 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!

I do remember that it did work. The issue was :
 Bug 14519356 - SYSTEM TIME DRIFTS DRASTICALLY FOR GUEST WITH UEK2 KERNEL AFTER MIGRATION 


 Easily reproduced using save.restore.
@ .
@ [root@localhost ~]# uname -a
@ Linux localhost.localdomain 2.6.39-300.7.0.el6uek.i686 #1 SMP Thu Sep 6
@ 12:47:30 EDT 2012 i686 i686 i386 GNU/Linux
@ [root@localhost ~]# date
@ Tue Sep 11 22:38:19 PDT 2012
@ [root@localhost ~]# uptime
@  09:24:43 up 15590 days, 17:16,  3 users,  load average: 2.75, 1.21, 0.47
@ [root@localhost ~]#
@ [root@localhost ~]# date
@ Mon Apr 14 09:24:51 PDT 1919
@ [root@localhost ~]# 

So the date went back to 1919.

.. and the reason was the the shared page (which is not re-inited by
the kernel) ends up with a large negative delta causing it to go back
in time.

> 
> 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.
> 
>  -- Keir
> 
> >  -- Keir
> > 
> > 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

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

[Roger Pau Monné]

On 12/06/13 15:50, Konrad Rzeszutek Wilk wrote:
> On Tue, Jun 11, 2013 at 04:45:29PM +0100, 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!
> 
> I do remember that it did work. The issue was :
>  Bug 14519356 - SYSTEM TIME DRIFTS DRASTICALLY FOR GUEST WITH UEK2 KERNEL AFTER MIGRATION 
> 
> 
>  Easily reproduced using save.restore.
> @ .
> @ [root@localhost ~]# uname -a
> @ Linux localhost.localdomain 2.6.39-300.7.0.el6uek.i686 #1 SMP Thu Sep 6
> @ 12:47:30 EDT 2012 i686 i686 i386 GNU/Linux
> @ [root@localhost ~]# date
> @ Tue Sep 11 22:38:19 PDT 2012
> @ [root@localhost ~]# uptime
> @  09:24:43 up 15590 days, 17:16,  3 users,  load average: 2.75, 1.21, 0.47
> @ [root@localhost ~]#
> @ [root@localhost ~]# date
> @ Mon Apr 14 09:24:51 PDT 1919
> @ [root@localhost ~]# 
> 
> So the date went back to 1919.
> 
> .. and the reason was the the shared page (which is not re-inited by
> the kernel) ends up with a large negative delta causing it to go back
> in time.

What I saw on recent Linux kernels (I think it was 3.9) is that the
kernel switches the clocksource from Xen to another available
clocksource when it sees an unreasonable drift, so it is not such a big
problem with upstream kernels.

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

[Keir Fraser]

On 11/06/2013 14:38, "Roger Pau Monné" <roger.pau@citrix.com> wrote:

> On 11/06/13 13:59, Jan Beulich wrote:
>>>>> On 11.06.13 at 12:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> The initial values of the wallclock time in the shared info page are
>>> set for PVHVM guests when the hypercall page is initialized, since the
>>> hypercall page is not reinitialized on resume, the hypervisor
>>> wallclock time is not properly set on resume.
>>> 
>>> Fix it by forcing an update of the wallclock values when the shared
>>> info page is mapped.
>> 
>> NACK - this is a guest side bug. After migration, a guest _has_ to
>> re-init the hypercall page, as it may have got migrated between
>> a VMX and an SVM machine, and the hypercall instructions are
>> different between them.
> 
> I've re-inited the hypercall page on resume, but the hypervisor
> wallclock is still 0.
> 
> The call to update_domain_wallclock_time in hvm_latch_shinfo_size is
> gated, and doesn't get called on the resume path. Would it be OK to call
> update_domain_wallclock_time unconditionally on
> hvm_hypercall_page_initialise?

Yes, just posted to confirm this is probably a good fix.

By the by, we also latch_shinfo_size() on setup of HVM_PARAM_CALLBACK_IRQ,
and you surely do *that* on PVHVM resume already. So although also
re-initialising the hypercall page is also a good idea on the resume path,
even guests without that fix should benefit from making the call in
latch_shinfo_size() unconditional.

 -- Keir