[PATCH] add missing guest clock update removed by e48672fa25e879f7ae21785c7efd187738139593

[PATCH] add missing guest clock update removed by e48672fa25e879f7ae21785c7efd187738139593

[Nikola Ciprich]

e48672fa25e879f7ae21785c7efd187738139593 removed kvm_request_guest_time_update(vcpu);
this breaks 32bit SMP guests using virtio-clock.
thus add unconditional call to kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu) to fix the
problem.

Signed-off-by: Nikola Ciprich <nikola.ciprich@linuxbox.cz>
---
 arch/x86/kvm/x86.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bcc0efc..4c27144 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2091,6 +2091,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	}
 
 	kvm_x86_ops->vcpu_load(vcpu, cpu);
+	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 	if (unlikely(vcpu->cpu != cpu) || check_tsc_unstable()) {
 		/* Make sure TSC doesn't go backwards */
 		s64 tsc_delta = !vcpu->arch.last_host_tsc ? 0 :


-- 
-------------------------------------
Ing. Nikola CIPRICH
LinuxBox.cz, s.r.o.
28. rijna 168, 709 01 Ostrava

tel.:   +420 596 603 142
fax:    +420 596 621 273
mobil:  +420 777 093 799
www.linuxbox.cz

mobil servis: +420 737 238 656
email servis: servis@linuxbox.cz
-------------------------------------

Re: [PATCH] add missing guest clock update removed by e48672fa25e879f7ae21785c7efd187738139593

[Zachary Amsden]

On 03/07/2011 05:18 AM, Nikola Ciprich wrote:
> e48672fa25e879f7ae21785c7efd187738139593 removed kvm_request_guest_time_update(vcpu);
> this breaks 32bit SMP guests using virtio-clock.
> thus add unconditional call to kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu) to fix the
> problem.
>
> Signed-off-by: Nikola Ciprich<nikola.ciprich@linuxbox.cz>
> ---
>   arch/x86/kvm/x86.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index bcc0efc..4c27144 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2091,6 +2091,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   	}
>
>   	kvm_x86_ops->vcpu_load(vcpu, cpu);
> +	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>   	if (unlikely(vcpu->cpu != cpu) || check_tsc_unstable()) {
>   		/* Make sure TSC doesn't go backwards */
>   		s64 tsc_delta = !vcpu->arch.last_host_tsc ? 0 :
>
>
>    

Can you try moving the kvm_make_request() inside the if conditional and 
see if it that also fixes it?

Re: [PATCH] add missing guest clock update removed by e48672fa25e879f7ae21785c7efd187738139593

[Nikola Ciprich]

> Can you try moving the kvm_make_request() inside the if conditional and  
> see if it that also fixes it?

yes, changing to:
if (unlikely(vcpu->cpu != cpu) || check_tsc_unstable()) {
	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
	/* Make sure TSC doesn't go backwards */
	s64 tsc_delta = !vcpu->arch.last_host_tsc ? 0 :

is also OK.

what about changing:
if (check_tsc_unstable()) {
	kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);                                                                                                               
	vcpu->arch.tsc_catchup = 1;
	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
}                                                                                                                                                                       

to:

if (check_tsc_unstable()) {
	kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);                                                                                                               
	vcpu->arch.tsc_catchup = 1;
}                                                                                                                                                                       
kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

this fixes thinks for me as well..
n.
?


> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
-------------------------------------
Ing. Nikola CIPRICH
LinuxBox.cz, s.r.o.
28. rijna 168, 709 01 Ostrava

tel.:   +420 596 603 142
fax:    +420 596 621 273
mobil:  +420 777 093 799
www.linuxbox.cz

mobil servis: +420 737 238 656
email servis: servis@linuxbox.cz
-------------------------------------

Re: [PATCH] add missing guest clock update removed by e48672fa25e879f7ae21785c7efd187738139593

[Zachary Amsden]

On 03/09/2011 02:30 PM, Nikola Ciprich wrote:
>> Can you try moving the kvm_make_request() inside the if conditional and
>> see if it that also fixes it?
>>      
> yes, changing to:
> if (unlikely(vcpu->cpu != cpu) || check_tsc_unstable()) {
> 	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> 	/* Make sure TSC doesn't go backwards */
> 	s64 tsc_delta = !vcpu->arch.last_host_tsc ? 0 :
>
> is also OK.
>
> what about changing:
> if (check_tsc_unstable()) {
> 	kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);
> 	vcpu->arch.tsc_catchup = 1;
> 	kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> }
>
> to:
>
> if (check_tsc_unstable()) {
> 	kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);
> 	vcpu->arch.tsc_catchup = 1;
> }
> kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>
> this fixes thinks for me as well..
> n.
> ?
>    

Can you send a patch which does that?  I think this is the correct fix.

Thanks,

Zach

[PATCHv2] fix regression caused by e48672fa25e879f7ae21785c7efd187738139593

[Nikola Ciprich]

commit 387b9f97750444728962b236987fbe8ee8cc4f8c moved kvm_request_guest_time_update(vcpu),
breaking 32bit SMP guests using kvm-clock. Fix this by moving (new) clock update function
to proper place.

Signed-off-by: Nikola Ciprich <nikola.ciprich@linuxbox.cz>
---
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4c27144..ba3f76f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2101,8 +2101,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		if (check_tsc_unstable()) {
 			kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);
 			vcpu->arch.tsc_catchup = 1;
-			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 		}
+		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 		if (vcpu->cpu != cpu)
 			kvm_migrate_timers(vcpu);
 		vcpu->cpu = cpu;

-- 
-------------------------------------
Ing. Nikola CIPRICH
LinuxBox.cz, s.r.o.
28. rijna 168, 709 01 Ostrava

tel.:   +420 596 603 142
fax:    +420 596 621 273
mobil:  +420 777 093 799
www.linuxbox.cz

mobil servis: +420 737 238 656
email servis: servis@linuxbox.cz
-------------------------------------

Re: [PATCHv2] fix regression caused by e48672fa25e879f7ae21785c7efd187738139593

[Zachary Amsden]

On 03/09/2011 05:36 PM, Nikola Ciprich wrote:
> commit 387b9f97750444728962b236987fbe8ee8cc4f8c moved kvm_request_guest_time_update(vcpu),
> breaking 32bit SMP guests using kvm-clock. Fix this by moving (new) clock update function
> to proper place.
>
> Signed-off-by: Nikola Ciprich<nikola.ciprich@linuxbox.cz>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4c27144..ba3f76f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2101,8 +2101,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   		if (check_tsc_unstable()) {
>   			kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);
>   			vcpu->arch.tsc_catchup = 1;
> -			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>   		}
> +		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>   		if (vcpu->cpu != cpu)
>   			kvm_migrate_timers(vcpu);
>   		vcpu->cpu = cpu;
>
>    
ACK

Re: [PATCHv2] fix regression caused by e48672fa25e879f7ae21785c7efd187738139593

[Avi Kivity]

On 03/10/2011 12:36 AM, Nikola Ciprich wrote:
> commit 387b9f97750444728962b236987fbe8ee8cc4f8c moved kvm_request_guest_time_update(vcpu),
> breaking 32bit SMP guests using kvm-clock. Fix this by moving (new) clock update function
> to proper place.
>

Applied, thanks.

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

Re: [PATCHv2] fix regression caused by e48672fa25e879f7ae21785c7efd187738139593

[Zachary Amsden]

[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]

On 03/09/2011 05:36 PM, Nikola Ciprich wrote:
> commit 387b9f97750444728962b236987fbe8ee8cc4f8c moved kvm_request_guest_time_update(vcpu),
> breaking 32bit SMP guests using kvm-clock. Fix this by moving (new) clock update function
> to proper place.
>
> Signed-off-by: Nikola Ciprich<nikola.ciprich@linuxbox.cz>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4c27144..ba3f76f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2101,8 +2101,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   		if (check_tsc_unstable()) {
>   			kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);
>   			vcpu->arch.tsc_catchup = 1;
> -			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>   		}
> +		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>   		if (vcpu->cpu != cpu)
>   			kvm_migrate_timers(vcpu);
>   		vcpu->cpu = cpu;
>
>    


So something bothers me still about this bug.  What you did correctly 
restores the old behavior - but it shouldn't be fixing a bug.

The only reason you need to schedule an update for the KVM clock area is 
if a new VCPU has been created, you have an unstable TSC.. or something 
changes the VM's kvmclock offset.

So this change could in fact be hiding an underlying bug - either an 
unstable TSC is not being properly reported, the KVM clock offset is 
being changed, we are missing a KVM clock update for secondary VCPUs - 
or something else we don't yet understand is going on.

Nikola, can you try the patch below, which reverts your change and 
attempts to fix other possible sources of the problem, and see if it 
still reproduces?

Thanks,

Zach

[-- Attachment #2: kvm-regres-test.patch --]
[-- Type: text/plain, Size: 1102 bytes --]

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 58f517b..42618fb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2127,8 +2127,10 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		if (check_tsc_unstable()) {
 			kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);
 			vcpu->arch.tsc_catchup = 1;
+			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 		}
-		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
+		if (vcpu->cpu == -1)
+			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 		if (vcpu->cpu != cpu)
 			kvm_migrate_timers(vcpu);
 		vcpu->cpu = cpu;
@@ -3534,6 +3536,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		struct kvm_clock_data user_ns;
 		u64 now_ns;
 		s64 delta;
+		struct kvm_vcpu *vcpu;
+		int i;
 
 		r = -EFAULT;
 		if (copy_from_user(&user_ns, argp, sizeof(user_ns)))
@@ -3549,6 +3553,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		delta = user_ns.clock - now_ns;
 		local_irq_enable();
 		kvm->arch.kvmclock_offset = delta;
+		kvm_for_each_vcpu(i, vcpu, kvm)
+			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 		break;
 	}
 	case KVM_GET_CLOCK: {

Re: [PATCHv2] fix regression caused by e48672fa25e879f7ae21785c7efd187738139593

[Nikola Ciprich]

> So something bothers me still about this bug.  What you did correctly  
> restores the old behavior - but it shouldn't be fixing a bug.
>
> The only reason you need to schedule an update for the KVM clock area is  
> if a new VCPU has been created, you have an unstable TSC.. or something  
> changes the VM's kvmclock offset.
>
> So this change could in fact be hiding an underlying bug - either an  
> unstable TSC is not being properly reported, the KVM clock offset is  
> being changed, we are missing a KVM clock update for secondary VCPUs -  
> or something else we don't yet understand is going on.
>
> Nikola, can you try the patch below, which reverts your change and  
> attempts to fix other possible sources of the problem, and see if it  
> still reproduces?
with Your patch, 32b SMP guests boot fine as well, without it, they
don't, so if You consider this better fix, I can acknowledge it as
working :)
n.

>
> Thanks,
>
> Zach

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 58f517b..42618fb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2127,8 +2127,10 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  		if (check_tsc_unstable()) {
>  			kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta);
>  			vcpu->arch.tsc_catchup = 1;
> +			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>  		}
> -		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> +		if (vcpu->cpu == -1)
> +			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>  		if (vcpu->cpu != cpu)
>  			kvm_migrate_timers(vcpu);
>  		vcpu->cpu = cpu;
> @@ -3534,6 +3536,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		struct kvm_clock_data user_ns;
>  		u64 now_ns;
>  		s64 delta;
> +		struct kvm_vcpu *vcpu;
> +		int i;
>  
>  		r = -EFAULT;
>  		if (copy_from_user(&user_ns, argp, sizeof(user_ns)))
> @@ -3549,6 +3553,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		delta = user_ns.clock - now_ns;
>  		local_irq_enable();
>  		kvm->arch.kvmclock_offset = delta;
> +		kvm_for_each_vcpu(i, vcpu, kvm)
> +			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>  		break;
>  	}
>  	case KVM_GET_CLOCK: {


-- 
-------------------------------------
Ing. Nikola CIPRICH
LinuxBox.cz, s.r.o.
28. rijna 168, 709 01 Ostrava

tel.:   +420 596 603 142
fax:    +420 596 621 273
mobil:  +420 777 093 799

www.linuxbox.cz

mobil servis: +420 737 238 656
email servis: servis@linuxbox.cz
-------------------------------------

Re: [PATCHv2] fix regression caused by e48672fa25e879f7ae21785c7efd187738139593

[Nikola Ciprich]

Hello Zachary,
what is the current status, are You going to post this patch to Avi?
I'd like to see one (or both) in stable eventually, I think it's good candidate..
BR
nik


-------------------------------------
Ing. Nikola CIPRICH
LinuxBox.cz, s.r.o.
28. rijna 168, 709 01 Ostrava

tel.:   +420 596 603 142
fax:    +420 596 621 273
mobil:  +420 777 093 799

www.linuxbox.cz

mobil servis: +420 737 238 656
email servis: servis@linuxbox.cz
-------------------------------------

Re: [PATCHv2] fix regression caused by e48672fa25e879f7ae21785c7efd187738139593

[Zachary Amsden]

On 04/11/2011 12:12 PM, Nikola Ciprich wrote:
> Hello Zachary,
> what is the current status, are You going to post this patch to Avi?
> I'd like to see one (or both) in stable eventually, I think it's good candidate..
> BR
> nik
>    

I think for upstream the newer patch is the way to go, but I would like 
to see it get some exposure first as it did change locking I think in 
one place.

I'm travelling right now with limited time and network access, difficult 
to resend that patch, but Avi, can you apply it to your tree?

Thanks,

Zach