Linux Virtualization - Oct 2018 - [patch 00/11] x86/vdso: Cleanups, simmplifications and CLOCK_TAI support

On Thu, Oct 04, 2018 at 03:15:32PM -0700, Andy Lutomirski
wrote:
> For better or for worse, I'm trying to understand this code.  So far,
> I've come up with this patch:
> 
>
https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/vdso-tglx&id=14fd71e12b1c4492a06f368f75041f263e6862bf
> 
> Is it correct, or am I missing some subtlety?
The master clock, when initialized, has a pair

masterclockvalues=(TSC value, time-of-day data).

When updating the guest clock, we only update relative to (TSC value)
that was read on masterclock initialization.

See the following comment on x86.c:

/*
 *
 * Assuming a stable TSC across physical CPUS, and a stable TSC
 * across virtual CPUs, the following condition is possible.
 * Each numbered line represents an event visible to both
 * CPUs at the next numbered event.
...

When updating the "masterclockvalues" pair, all vcpus are 
stopped.

On Sat, Oct 6, 2018 at 1:29 PM Marcelo Tosatti <mtosatti at redhat.com>
wrote:
>
> On Thu, Oct 04, 2018 at 03:15:32PM -0700, Andy Lutomirski wrote:
> > For better or for worse, I'm trying to understand this code.  So
far,
> > I've come up with this patch:
> >
> >
https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/vdso-tglx&id=14fd71e12b1c4492a06f368f75041f263e6862bf
> >
> > Is it correct, or am I missing some subtlety?
>
> The master clock, when initialized, has a pair
>
> masterclockvalues=(TSC value, time-of-day data).
>
> When updating the guest clock, we only update relative to (TSC value)
> that was read on masterclock initialization.
I don't see the problem.  The masterclock data is updated here:

    host_tsc_clocksource = kvm_get_time_and_clockread(
                    &ka->master_kernel_ns,
                    &ka->master_cycle_now);

kvm_get_time_and_clockread() gets those values from
do_monotonic_boot(), which, barring bugs, should cause
get_kvmclock_ns() to return exactly the same thing as
ktime_get_boot_ns() + ka->kvmclock_offset, albeit in a rather
roundabout manner.

So what am I missing?  Is there actually something wrong with my patch?

>
> See the following comment on x86.c:
I read that comment, and it's not obvious to me how it's related.

On Sat, Oct 06, 2018 at 03:28:05PM -0700, Andy Lutomirski
wrote:
> On Sat, Oct 6, 2018 at 1:29 PM Marcelo Tosatti <mtosatti at
redhat.com> wrote:
> >
> > On Thu, Oct 04, 2018 at 03:15:32PM -0700, Andy Lutomirski wrote:
> > > For better or for worse, I'm trying to understand this code. 
So far,
> > > I've come up with this patch:
> > >
> > >
https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/vdso-tglx&id=14fd71e12b1c4492a06f368f75041f263e6862bf
> > >
> > > Is it correct, or am I missing some subtlety?
> >
> > The master clock, when initialized, has a pair
> >
> > masterclockvalues=(TSC value, time-of-day data).
> >
> > When updating the guest clock, we only update relative to (TSC value)
> > that was read on masterclock initialization.
> 
> I don't see the problem.  The masterclock data is updated here:
> 
>     host_tsc_clocksource = kvm_get_time_and_clockread(
>                     &ka->master_kernel_ns,
>                     &ka->master_cycle_now);
> 
> kvm_get_time_and_clockread() gets those values from
> do_monotonic_boot(), which, barring bugs, should cause
> get_kvmclock_ns() to return exactly the same thing as
> ktime_get_boot_ns() + ka->kvmclock_offset, albeit in a rather
> roundabout manner.
> 
> So what am I missing?  Is there actually something wrong with my patch?
For the bug mentioned in the comment not to happen, you must only read
TSC and add it as offset to (TSC value, time-of-day data).

Its more than "a roundabout manner".

Read the comment again.
> 
> 
> >
> > See the following comment on x86.c:
> 
> I read that comment, and it's not obvious to me how it's related.