KVM: x86: fix pvclock guest stopped flag reporting

KVM: x86: fix pvclock guest stopped flag reporting

[Marcelo Tosatti]

kvm_guest_time_update unconditionally clears hv_clock.flags field, 
so the notification never reaches the guest.

Fix it by allowing PVCLOCK_GUEST_STOPPED to passthrough.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3a53bcc..e8ce10f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1217,7 +1217,9 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 	vcpu->hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
 	vcpu->last_kernel_ns = kernel_ns;
 	vcpu->last_guest_tsc = tsc_timestamp;
-	vcpu->hv_clock.flags = 0;
+	/* only support PVCLOCK_GUEST_STOPPED flag ATM */
+	if (vcpu->hv_clock.flags != PVCLOCK_GUEST_STOPPED)
+		vcpu->hv_clock.flags = 0;
 
 	/*
 	 * The interface expects us to write an even number signaling that the

Re: KVM: x86: fix pvclock guest stopped flag reporting

[Avi Kivity]

On 08/02/2012 06:33 PM, Marcelo Tosatti wrote:
> 
> kvm_guest_time_update unconditionally clears hv_clock.flags field, 
> so the notification never reaches the guest.
> 
> Fix it by allowing PVCLOCK_GUEST_STOPPED to passthrough.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3a53bcc..e8ce10f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1217,7 +1217,9 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
>  	vcpu->hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
>  	vcpu->last_kernel_ns = kernel_ns;
>  	vcpu->last_guest_tsc = tsc_timestamp;
> -	vcpu->hv_clock.flags = 0;
> +	/* only support PVCLOCK_GUEST_STOPPED flag ATM */
> +	if (vcpu->hv_clock.flags != PVCLOCK_GUEST_STOPPED)
> +		vcpu->hv_clock.flags = 0;
>  

Seems a little risky.  Should we store the flag in a separate bool and
mix it in instead of RMWing it?


-- 
error compiling committee.c: too many arguments to function

Re: KVM: x86: fix pvclock guest stopped flag reporting

[Marcelo Tosatti]

On Thu, Aug 02, 2012 at 06:42:25PM +0300, Avi Kivity wrote:
> On 08/02/2012 06:33 PM, Marcelo Tosatti wrote:
> > 
> > kvm_guest_time_update unconditionally clears hv_clock.flags field, 
> > so the notification never reaches the guest.
> > 
> > Fix it by allowing PVCLOCK_GUEST_STOPPED to passthrough.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 3a53bcc..e8ce10f 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1217,7 +1217,9 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
> >  	vcpu->hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
> >  	vcpu->last_kernel_ns = kernel_ns;
> >  	vcpu->last_guest_tsc = tsc_timestamp;
> > -	vcpu->hv_clock.flags = 0;
> > +	/* only support PVCLOCK_GUEST_STOPPED flag ATM */
> > +	if (vcpu->hv_clock.flags != PVCLOCK_GUEST_STOPPED)
> > +		vcpu->hv_clock.flags = 0;
> >  
> 
> Seems a little risky.  Should we store the flag in a separate bool and
> mix it in instead of RMWing it?

This is not guest memory. Its a host copy, the actual data is copied
(safely via the version mechanism) below in that function.

Are you ok with it now?

Re: KVM: x86: fix pvclock guest stopped flag reporting

[Avi Kivity]

On 08/02/2012 06:43 PM, Marcelo Tosatti wrote:
> On Thu, Aug 02, 2012 at 06:42:25PM +0300, Avi Kivity wrote:
>> On 08/02/2012 06:33 PM, Marcelo Tosatti wrote:
>> > 
>> > kvm_guest_time_update unconditionally clears hv_clock.flags field, 
>> > so the notification never reaches the guest.
>> > 
>> > Fix it by allowing PVCLOCK_GUEST_STOPPED to passthrough.
>> > 
>> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>> > 
>> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> > index 3a53bcc..e8ce10f 100644
>> > --- a/arch/x86/kvm/x86.c
>> > +++ b/arch/x86/kvm/x86.c
>> > @@ -1217,7 +1217,9 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
>> >  	vcpu->hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
>> >  	vcpu->last_kernel_ns = kernel_ns;
>> >  	vcpu->last_guest_tsc = tsc_timestamp;
>> > -	vcpu->hv_clock.flags = 0;
>> > +	/* only support PVCLOCK_GUEST_STOPPED flag ATM */
>> > +	if (vcpu->hv_clock.flags != PVCLOCK_GUEST_STOPPED)
>> > +		vcpu->hv_clock.flags = 0;
>> >  
>> 
>> Seems a little risky.  Should we store the flag in a separate bool and
>> mix it in instead of RMWing it?
> 
> This is not guest memory. Its a host copy, the actual data is copied
> (safely via the version mechanism) below in that function.
> 
> Are you ok with it now?
> 

Yeah.  Though why are we writing flags at all then?

We could write it one during init and leave it there (not that it matters).

Anyway, patch is fine.

-- 
error compiling committee.c: too many arguments to function

Re: KVM: x86: fix pvclock guest stopped flag reporting

[Marcelo Tosatti]

On Thu, Aug 02, 2012 at 12:43:58PM -0300, Marcelo Tosatti wrote:
> On Thu, Aug 02, 2012 at 06:42:25PM +0300, Avi Kivity wrote:
> > On 08/02/2012 06:33 PM, Marcelo Tosatti wrote:
> > > 
> > > kvm_guest_time_update unconditionally clears hv_clock.flags field, 
> > > so the notification never reaches the guest.
> > > 
> > > Fix it by allowing PVCLOCK_GUEST_STOPPED to passthrough.
> > > 
> > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 3a53bcc..e8ce10f 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -1217,7 +1217,9 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
> > >  	vcpu->hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
> > >  	vcpu->last_kernel_ns = kernel_ns;
> > >  	vcpu->last_guest_tsc = tsc_timestamp;
> > > -	vcpu->hv_clock.flags = 0;
> > > +	/* only support PVCLOCK_GUEST_STOPPED flag ATM */
> > > +	if (vcpu->hv_clock.flags != PVCLOCK_GUEST_STOPPED)
> > > +		vcpu->hv_clock.flags = 0;
> > >  
> > 
> > Seems a little risky.  Should we store the flag in a separate bool and
> > mix it in instead of RMWing it?
> 
> This is not guest memory. Its a host copy, the actual data is copied
> (safely via the version mechanism) below in that function.
> 
> Are you ok with it now?

The patch is still incomplete as we must clear the flag in our copy
if the guest clears it. 

KVM: x86: fix pvclock guest stopped flag reporting (v2)

[Marcelo Tosatti]

kvm_guest_time_update unconditionally clears hv_clock.flags field,
so the notification never reaches the guest.

Fix it by allowing PVCLOCK_GUEST_STOPPED to passthrough.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 48e7131..e775547 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -420,6 +420,8 @@ struct kvm_vcpu_arch {
 	unsigned int hw_tsc_khz;
 	unsigned int time_offset;
 	struct page *time_page;
+	/* set guest stopped flag in pvclock flags field */
+	bool pvclock_set_guest_stopped_request;
 
 	struct {
 		u64 msr_val;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a87c82a..ddcf8ad 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1136,6 +1136,7 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 	unsigned long this_tsc_khz;
 	s64 kernel_ns, max_kernel_ns;
 	u64 tsc_timestamp;
+	u8 pvclock_flags;
 
 	/* Keep irq disabled to prevent changes to the clock */
 	local_irq_save(flags);
@@ -1217,7 +1218,14 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 	vcpu->hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
 	vcpu->last_kernel_ns = kernel_ns;
 	vcpu->last_guest_tsc = tsc_timestamp;
-	vcpu->hv_clock.flags = 0;
+
+	pvclock_flags = 0;
+	if (vcpu->pvclock_set_guest_stopped_request) {
+		pvclock_flags |= PVCLOCK_GUEST_STOPPED;
+		vcpu->pvclock_set_guest_stopped_request = false;
+	}
+
+	vcpu->hv_clock.flags = pvclock_flags;
 
 	/*
 	 * The interface expects us to write an even number signaling that the
@@ -2628,10 +2636,9 @@ static int kvm_vcpu_ioctl_x86_set_xcrs(struct kvm_vcpu *vcpu,
  */
 static int kvm_set_guest_paused(struct kvm_vcpu *vcpu)
 {
-	struct pvclock_vcpu_time_info *src = &vcpu->arch.hv_clock;
 	if (!vcpu->arch.time_page)
 		return -EINVAL;
-	src->flags |= PVCLOCK_GUEST_STOPPED;
+	vcpu->arch.pvclock_set_guest_stopped_request = true;
 	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 	return 0;
 }

Re: KVM: x86: fix pvclock guest stopped flag reporting (v2)

[Amit Shah]

On (Fri) 03 Aug 2012 [15:57:49], Marcelo Tosatti wrote:
> 
> kvm_guest_time_update unconditionally clears hv_clock.flags field,
> so the notification never reaches the guest.
> 
> Fix it by allowing PVCLOCK_GUEST_STOPPED to passthrough.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Reviewed-by: Amit Shah <amit.shah@redhat.com>

		Amit

Re: KVM: x86: fix pvclock guest stopped flag reporting (v2)

[Eric B Munson]

On 2012-08-03 14:57, Marcelo Tosatti wrote:
> kvm_guest_time_update unconditionally clears hv_clock.flags field,
> so the notification never reaches the guest.
>
> Fix it by allowing PVCLOCK_GUEST_STOPPED to passthrough.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>

Reviewed-by: Eric B Munson <emunson@mgebm.net>