Re: [PATCH] x86/kvm: fix condition to update kvm master clocks

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Jun 21, 2016 at 05:40:32PM +0300, Roman Kagan wrote:
> On Mon, Jun 20, 2016 at 06:29:10PM -0300, Marcelo Tosatti wrote:
> > On Mon, Jun 20, 2016 at 08:22:49PM +0300, Roman Kagan wrote:
> > > The per-vcpu hv_clock is updated when the vcpu processes
> > > KVM_REQ_CLOCK_UPDATE request.
> > 
> > Yes.
> > 
> > > Once kvm_gen_update_masterclock() sets KVM_REQ_CLOCK_UPDATE and
> > > clears KVM_REQ_MCLOCK_INPROGRESS for all vcpus, one vcpu can process the
> > > requests, enter the guest, and read another vcpu's hv_clock, before that
> > > other vcpu had a chance to process its KVM_REQ_CLOCK_UPDATE request.
> > 
> > Yes. But guest code should be reading its local kvmclock area:
> > 
> >                 /*
> >                  * Test we're still on the cpu as well as the version.
> >                  * We could have been migrated just after the first
> >                  * vgetcpu but before fetching the version, so we
> >                  * wouldn't notice a version change.
> >                  */
> >                 cpu1 = __getcpu() & VGETCPU_CPU_MASK;
> > 
> > (vclock_gettime.c)
> 
> This code is from an older version.  The latest always reads the clock
> of the CPU #0:
> 
>         /*
>          * Note: The kernel and hypervisor must guarantee that cpu ID
>          * number maps 1:1 to per-CPU pvclock time info.
>          *
>          * Because the hypervisor is entirely unaware of guest userspace
>          * preemption, it cannot guarantee that per-CPU pvclock time
>          * info is updated if the underlying CPU changes or that that
>          * version is increased whenever underlying CPU changes.
>          *
>          * On KVM, we are guaranteed that pvti updates for any vCPU are
>          * atomic as seen by *all* vCPUs.  This is an even stronger
>          * guarantee than we get with a normal seqlock.
>          *
>          * On Xen, we don't appear to have that guarantee, but Xen still
>          * supplies a valid seqlock using the version field.
>          *
>          * We only do pvclock vdso timing at all if
>          * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
>          * mean that all vCPUs have matching pvti and that the TSC is
>          * synced, so we can just look at vCPU 0's pvti.
>          */

In that case vCPU-N (that has its local kvmclock updated), will read vCPU-0's 
(which does not have its kvmclock area updated).

vCPU-0 kvmclock area either contains the old copy of kvmclock values,
or the new copy.

That reading of vCPU-0 is protected by the version check, therefore it
is safe.

> > > Is there anything that prevents this?
> > 
> > Guest code confirming both version and cpu do not change across 
> > a kvmclock read. Other than this, no.
> 
> So is the code reading another vcpu's hv_clock wrong?

Its fine. 

What you can't do is to:

	at vcpu-3:

	read kvmclock area of local vcpu.
	read kvmclock area of remote vcpu. 

And compare the values.

What you can do is:

	at vcpu-1:
	read kvmclock area of local vcpu.

	at vcpu-3:
	read kvmclock area of local vcpu.

And compare these reads. They should be monotonic.

Or only read from a single vcpu, thats also monotonic.