On Mon, Nov 14, 2016 at 03:22:02PM +0100, Paolo Bonzini wrote:
>
>
> On 14/11/2016 15:00, Marcelo Tosatti wrote:
> > On Mon, Nov 14, 2016 at 02:54:38PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 14/11/2016 13:36, Marcelo Tosatti wrote:
> >>> + /* local (running VM) restore */
> >>> + if (s->clock_valid) {
> >>> + /*
> >>> + * if host does not support reliable KVM_GET_CLOCK,
> >>> + * read kvmclock value from memory
> >>> + */
> >>> + if (!kvm_has_adjust_clock_stable()) {
> >>> + time_at_migration = kvmclock_current_nsec(s);
> >>
> >> Just assign to s->clock here...
> >
> > If kvmclock is not enabled, you want to use s->clock,
> > rather than 0.
> >
> >>> + }
> >>> + /* migration/savevm/init restore */
> >>> + } else {
> >>> + /*
> >>> + * use s->clock in case machine uses reliable
> >>> + * get clock and host where vm was executing
> >>> + * supported reliable get clock
> >>> + */
> >>> + if (!s->mach_use_reliable_get_clock ||
> >>> + !s->src_use_reliable_get_clock) {
> >>> + time_at_migration = kvmclock_current_nsec(s);
> >>
> >> ... and here, so that time_at_migration is not needed anymore.
> >
> > Same as above.
>
> You're right.
>
> >> Also here it's enough to look at s->src_user_reliable_get_clock, because
> >> if s->mach_use_reliable_get_clock is false,
> >> s->src_use_reliable_get_clock will be false as well.
> >
> > Yes, but i like the code annotation.
>
> Ah, I think we're looking at it differently.
Well, i didnt want to mix the meaning of the variables:
+ /* whether machine supports reliable KVM_GET_CLOCK */
+ bool mach_use_reliable_get_clock;
+
+ /* whether source host supported reliable KVM_GET_CLOCK */
+ bool src_use_reliable_get_clock;
See the comments on top (later if you look at the variable,
then have to think: well it has one name, but its disabled
by that other path as well, so its more than its
name,etc...).
> I'm thinking "mach_use_reliable_get_clock is just for migration,
Thats whether the machine supports it. New machines have it enabled,
olders don't.
> src_use_reliable_get_clock is the state".
Thats whether the migration source supported it.
> Perhaps you're thinking of
> enabling/disabling the whole new code for old machines?
source destination behaviour
supports supports on migration use s->clock,
on stop/cont as well
supports ~supports
on migration use s->clock,
on stop/cont read from guest mem
~support supports on migration read from guest,
on stop/cont use
kvm_get_clock/kvm_set_clock
~support ~support always read from guest memory
Thats what should happen (and thats what the patch should implement).
"support" means host supports your new KVM_GET_CLOCK/KVM_SET_CLOCK.
> What is the
> advantage?
Well its necessary to use the correct thing, otherwise
you see a time backwards event.
>
> >>> + }
> >>> + }
> >>>
> >>> - /* We can't rely on the migrated clock value, just discard it */
> >>> + /* We can't rely on the saved clock value, just discard it */
> >>> if (time_at_migration) {
> >>> s->clock = time_at_migration;
> >>
> >> [...]
> >>
> >>>
> >>> +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> >>> +{
> >>> + KVMClockState *s = opaque;
> >>> +
> >>> + /*
> >>> + * On machine types that support reliable KVM_GET_CLOCK,
> >>> + * if host kernel does provide reliable KVM_GET_CLOCK,
> >>> + * set src_use_reliable_get_clock=true so that destination
> >>> + * avoids reading kvmclock from memory.
> >>> + */
> >>> + if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable())
> >>> {
> >>> + s->src_use_reliable_get_clock = true;
> >>> + }
> >>> +
> >>> + return s->src_use_reliable_get_clock;
> >>> +}
> >>
> >> Here you can just return s->mach_use_reliable_get_clock.
> >
> > mach_use_reliable_get_clock can be true but host might not support it.
>
> Yes, but the "needed" function is only required to avoid breaking
> pc-i440fx-2.7 and earlier.
"needed" is required so that the migration between:
SRC DEST BEHAVIOUR
~support supports on migration read from guest,
on stop/cont use
kvm_get_clock/kvm_set_clock
Destination does not use KVM_GET_CLOCK value (which is
broken and should not be used).
> If you return true here, you can still
> migrate a "false" value for src_use_reliable_get_clock.
But the source only uses a reliable KVM_GET_CLOCK if
both conditions are true.
And the subsection is only needed if the source
uses a reliable KVM_GET_CLOCK.
> >> To set
> >> s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look
> >> at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.
> >
> > KVM_CLOCK_TSC_STABLE bit in the kvmclock structure !=
> > KVM_GET_CLOCK returns reliable value, right?
>
> It is the same as "is using masterclock", which is actually a stricter
> condition than the KVM_CHECK_EXTENSION return value. The right check to
> use is whether masterclock is in use,
Actually its "has a reliable KVM_GET_CLOCK" (which returns
get_kernel_clock() + (rdtsc() - tsc_timestamp),
"broken KVM_GET_CLOCK" = get_kernel_clock()
> and then the idea is to treat
> clock,src_use_reliable_get_clock as one tuple that is updated atomically.
>
> Paolo
Hum, not sure i get this...
