[PATCH v2 0/1] KVM: correctly restore the TSC value on nested migration

[PATCH v2 0/1] KVM: correctly restore the TSC value on nested migration

[Maxim Levitsky]

This patch is a result of a long investigation I made to understand
why the nested migration more often than not makes the nested guest hang.
Sometimes the nested guest recovers and sometimes it hangs forever.

The root cause of this is that reading MSR_IA32_TSC while nested guest is
running returns its TSC value, that is (assuming no tsc scaling)
host tsc + L1 tsc offset + L2 tsc offset.

This is correct but it is a result of a nice curiosity of X86 VMX
(and apparently SVM too, according to my tests) implementation:

As a rule MSR reads done by the guest should either trap to host, or just
return host value, and therefore kvm_get_msr and friends, should basically
always return the L1 value of any msr.

Well, MSR_IA32_TSC is an exception. Intel's PRM states that when you disable
its interception, then in guest mode the host adds the TSC offset to
the read value.

I haven't found anything like that in AMD's PRM but according to the few
tests I made, it behaves the same.

However, there is no such exception when writing MSR_IA32_TSC, and this
poses a problem for nested migration.

When MSR_IA32_TSC is read, we read L2 value (smaller since L2 is started
after L1), and when we restore it after migration, the value is interpreted
as L1 value, thus resulting in huge TSC jump backward which the guest usually
really doesn't like, especially on AMD with APIC deadline timer, which
usually just doesn't fire afterward sending the guest into endless wait for it.

The proposed patch fixes this by making read of MSR_IA32_TSC depend on
'msr_info->host_initiated'

If guest reads the MSR, we add the TSC offset, but when host's qemu reads
the msr we skip that silly emulation of TSC offset, and return the real value
for the L1 guest which is host tsc + L1 offset.

This patch was tested on both SVM and VMX, and on both it fixes hangs.
On VMX since it uses VMX preemption timer for APIC deadline, the guest seems
to recover after a while without that patch.

To make sure that the nested migration happens I usually used
-overcommit cpu_pm=on but I reproduced this with just running an endless loop
in L2.

This is tested both with and without -invtsc,tsc-frequency=...

The migration was done by saving the migration stream to a file, and then
loading the qemu with '-incoming'

V2: incorporated feedback from Sean Christopherson (thanks!)

Maxim Levitsky (1):
  KVM: x86: fix MSR_IA32_TSC read for nested migration

 arch/x86/kvm/x86.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH v2 1/1] KVM: x86: fix MSR_IA32_TSC read for nested migration

[Maxim Levitsky]

MSR reads/writes should always access the L1 state, since the (nested)
hypervisor should intercept all the msrs it wants to adjust, and these
that it doesn't should be read by the guest as if the host had read it.

However IA32_TSC is an exception. Even when not intercepted, guest still
reads the value + TSC offset.
The write however does not take any TSC offset into account.

This is documented in Intel's SDM and seems also to happen on AMD as well.

This creates a problem when userspace wants to read the IA32_TSC value and then
write it. (e.g for migration)

In this case it reads L2 value but write is interpreted as an L1 value.
To fix this make the userspace initiated reads of IA32_TSC return L1 value
as well.

Huge thanks to Dave Gilbert for helping me understand this very confusing
semantic of MSR writes.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/x86.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 17f4995e80a7e..ed4314641360e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3219,9 +3219,21 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_POWER_CTL:
 		msr_info->data = vcpu->arch.msr_ia32_power_ctl;
 		break;
-	case MSR_IA32_TSC:
-		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + vcpu->arch.tsc_offset;
+	case MSR_IA32_TSC: {
+		/*
+		 * Intel SDM states that MSR_IA32_TSC read adds the TSC offset
+		 * even when not intercepted. AMD manual doesn't explicitly
+		 * state this but appears to behave the same.
+		 *
+		 * However when userspace wants to read this MSR, we should
+		 * return it's real L1 value so that its restore will be correct.
+		 */
+		u64 tsc_offset = msr_info->host_initiated ? vcpu->arch.l1_tsc_offset :
+							    vcpu->arch.tsc_offset;
+
+		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + tsc_offset;
 		break;
+	}
 	case MSR_MTRRcap:
 	case 0x200 ... 0x2ff:
 		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
-- 
2.26.2

Re: [PATCH v2 1/1] KVM: x86: fix MSR_IA32_TSC read for nested migration

[Paolo Bonzini]

On 21/09/20 18:23, Sean Christopherson wrote:
> Avoid "should" in code comments and describe what the code is doing, not what
> it should be doing.  The only exception for this is when the code has a known
> flaw/gap, e.g. "KVM should do X, but because of Y, KVM actually does Z".
> 
>> +		 * return it's real L1 value so that its restore will be correct.
> s/it's/its
> 
> Perhaps add "unconditionally" somewhere, since arch.tsc_offset can also contain
> the L1 value.  E.g. 
> 
> 		 * Unconditionally return L1's TSC offset on userspace reads
> 		 * so that userspace reads and writes always operate on L1's
> 		 * offset, e.g. to ensure deterministic behavior for migration.
> 		 */
> 

Technically the host need not restore MSR_IA32_TSC at all.  This follows
the idea of the discussion with Oliver Upton about transmitting the
state of the kvmclock heuristics to userspace, which include a (TSC,
CLOCK_MONOTONIC) pair to transmit the offset to the destination.  All
that needs to be an L1 value is then the TSC value in that pair.

I'm a bit torn over this patch.  On one hand it's an easy solution, on
the other hand it's... just wrong if KVM_GET_MSR is used for e.g.
debugging the guest.

I'll talk to Maxim and see if he can work on the kvmclock migration stuff.

Paolo

Re: [PATCH v2 1/1] KVM: x86: fix MSR_IA32_TSC read for nested migration

[Maxim Levitsky]

On Tue, 2020-09-22 at 14:50 +0200, Paolo Bonzini wrote:
> On 21/09/20 18:23, Sean Christopherson wrote:
> > Avoid "should" in code comments and describe what the code is doing, not what
> > it should be doing.  The only exception for this is when the code has a known
> > flaw/gap, e.g. "KVM should do X, but because of Y, KVM actually does Z".
> > 
> > > +		 * return it's real L1 value so that its restore will be correct.
> > s/it's/its
> > 
> > Perhaps add "unconditionally" somewhere, since arch.tsc_offset can also contain
> > the L1 value.  E.g. 
> > 
> > 		 * Unconditionally return L1's TSC offset on userspace reads
> > 		 * so that userspace reads and writes always operate on L1's
> > 		 * offset, e.g. to ensure deterministic behavior for migration.
> > 		 */
> > 
> 
> Technically the host need not restore MSR_IA32_TSC at all.  This follows
> the idea of the discussion with Oliver Upton about transmitting the
> state of the kvmclock heuristics to userspace, which include a (TSC,
> CLOCK_MONOTONIC) pair to transmit the offset to the destination.  All
> that needs to be an L1 value is then the TSC value in that pair.
> 
> I'm a bit torn over this patch.  On one hand it's an easy solution, on
> the other hand it's... just wrong if KVM_GET_MSR is used for e.g.
> debugging the guest.

Could you explain why though? After my patch, the KVM_GET_MSR will consistently
read the L1 TSC, just like all other MSRs as I explained. I guess for debugging,
this should work?

The fact that TSC reads with the guest offset is a nice exception made for the guests,
that insist on reading this msr without inteception and not using rdtsc.

Best regards,
	Maxim Levitsky

> 
> I'll talk to Maxim and see if he can work on the kvmclock migration stuff.
> 
> Paolo
> 

Re: [PATCH v2 1/1] KVM: x86: fix MSR_IA32_TSC read for nested migration

[Maxim Levitsky]

On Tue, 2020-09-22 at 17:50 +0300, Maxim Levitsky wrote:
> On Tue, 2020-09-22 at 14:50 +0200, Paolo Bonzini wrote:
> > On 21/09/20 18:23, Sean Christopherson wrote:
> > > Avoid "should" in code comments and describe what the code is doing, not what
> > > it should be doing.  The only exception for this is when the code has a known
> > > flaw/gap, e.g. "KVM should do X, but because of Y, KVM actually does Z".
> > > 
> > > > +		 * return it's real L1 value so that its restore will be correct.
> > > s/it's/its
> > > 
> > > Perhaps add "unconditionally" somewhere, since arch.tsc_offset can also contain
> > > the L1 value.  E.g. 
> > > 
> > > 		 * Unconditionally return L1's TSC offset on userspace reads
> > > 		 * so that userspace reads and writes always operate on L1's
> > > 		 * offset, e.g. to ensure deterministic behavior for migration.
> > > 		 */
> > > 
> > 
> > Technically the host need not restore MSR_IA32_TSC at all.  This follows
> > the idea of the discussion with Oliver Upton about transmitting the
> > state of the kvmclock heuristics to userspace, which include a (TSC,
> > CLOCK_MONOTONIC) pair to transmit the offset to the destination.  All
> > that needs to be an L1 value is then the TSC value in that pair.
> > 
> > I'm a bit torn over this patch.  On one hand it's an easy solution, on
> > the other hand it's... just wrong if KVM_GET_MSR is used for e.g.
> > debugging the guest.
> 
> Could you explain why though? After my patch, the KVM_GET_MSR will consistently
> read the L1 TSC, just like all other MSRs as I explained. I guess for debugging,
> this should work?
> 
> The fact that TSC reads with the guest offset is a nice exception made for the guests,
> that insist on reading this msr without inteception and not using rdtsc.
> 
> Best regards,
> 	Maxim Levitsky
> 
> > I'll talk to Maxim and see if he can work on the kvmclock migration stuff.

We talked about this on IRC and now I am also convinced that we should implement
proper TSC migration instead, so I guess I'll drop this patch and I will implement it.

Last few weeks I was digging through all the timing code, and I mostly understand it
so it shouldn't take me much time to implement it.

There is hope that this will make nested migration fully stable since, with this patch,
it still sometimes hangs. While on my AMD machine it takes about half a day of migration
cycles to reproduce this, on my Intel's laptop even with this patch I can hang the nested
guest after 10-20 cycles. The symptoms look very similar to the issue that this patch
tried to fix.
 
Maybe we should keep the *comment* I added to document this funny TSC read behavior. 
When I implement the whole thing, maybe I add a comment only version of this patch
for that.

Best regards,
	Maxim Levitsky 

> > 
> > Paolo
> > 

Re: [PATCH v2 1/1] KVM: x86: fix MSR_IA32_TSC read for nested migration

[Paolo Bonzini]

On 22/09/20 17:39, Maxim Levitsky wrote:
>>> I'll talk to Maxim and see if he can work on the kvmclock migration stuff.
> 
> We talked about this on IRC and now I am also convinced that we should implement
> proper TSC migration instead, so I guess I'll drop this patch and I will implement it.
> 
> Last few weeks I was digging through all the timing code, and I mostly understand it
> so it shouldn't take me much time to implement it.
> 
> There is hope that this will make nested migration fully stable since, with this patch,
> it still sometimes hangs. While on my AMD machine it takes about half a day of migration
> cycles to reproduce this, on my Intel's laptop even with this patch I can hang the nested
> guest after 10-20 cycles. The symptoms look very similar to the issue that this patch
> tried to fix.
>  
> Maybe we should keep the *comment* I added to document this funny TSC read behavior. 
> When I implement the whole thing, maybe I add a comment only version of this patch
> for that.

Sure, that's a good idea.

Paolo

Re: [PATCH v2 1/1] KVM: x86: fix MSR_IA32_TSC read for nested migration

[Paolo Bonzini]

On 21/09/20 12:38, Maxim Levitsky wrote:
> MSR reads/writes should always access the L1 state, since the (nested)
> hypervisor should intercept all the msrs it wants to adjust, and these
> that it doesn't should be read by the guest as if the host had read it.
> 
> However IA32_TSC is an exception. Even when not intercepted, guest still
> reads the value + TSC offset.
> The write however does not take any TSC offset into account.
> 
> This is documented in Intel's SDM and seems also to happen on AMD as well.
> 
> This creates a problem when userspace wants to read the IA32_TSC value and then
> write it. (e.g for migration)
> 
> In this case it reads L2 value but write is interpreted as an L1 value.
> To fix this make the userspace initiated reads of IA32_TSC return L1 value
> as well.
> 
> Huge thanks to Dave Gilbert for helping me understand this very confusing
> semantic of MSR writes.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 17f4995e80a7e..ed4314641360e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3219,9 +3219,21 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_POWER_CTL:
>  		msr_info->data = vcpu->arch.msr_ia32_power_ctl;
>  		break;
> -	case MSR_IA32_TSC:
> -		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + vcpu->arch.tsc_offset;
> +	case MSR_IA32_TSC: {
> +		/*
> +		 * Intel SDM states that MSR_IA32_TSC read adds the TSC offset
> +		 * even when not intercepted. AMD manual doesn't explicitly
> +		 * state this but appears to behave the same.
> +		 *
> +		 * However when userspace wants to read this MSR, we should
> +		 * return it's real L1 value so that its restore will be correct.
> +		 */
> +		u64 tsc_offset = msr_info->host_initiated ? vcpu->arch.l1_tsc_offset :
> +							    vcpu->arch.tsc_offset;
> +
> +		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + tsc_offset;
>  		break;
> +	}
>  	case MSR_MTRRcap:
>  	case 0x200 ... 0x2ff:
>  		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
> 

Applied the patch as it is doing the sanest possible thing for the
current semantics of host-initiated accesses.

Paolo

Re: [PATCH v2 1/1] KVM: x86: fix MSR_IA32_TSC read for nested migration

[Maxim Levitsky]

On Thu, 2020-09-24 at 19:33 +0200, Paolo Bonzini wrote:
> On 21/09/20 12:38, Maxim Levitsky wrote:
> > MSR reads/writes should always access the L1 state, since the (nested)
> > hypervisor should intercept all the msrs it wants to adjust, and these
> > that it doesn't should be read by the guest as if the host had read it.
> > 
> > However IA32_TSC is an exception. Even when not intercepted, guest still
> > reads the value + TSC offset.
> > The write however does not take any TSC offset into account.
> > 
> > This is documented in Intel's SDM and seems also to happen on AMD as well.
> > 
> > This creates a problem when userspace wants to read the IA32_TSC value and then
> > write it. (e.g for migration)
> > 
> > In this case it reads L2 value but write is interpreted as an L1 value.
> > To fix this make the userspace initiated reads of IA32_TSC return L1 value
> > as well.
> > 
> > Huge thanks to Dave Gilbert for helping me understand this very confusing
> > semantic of MSR writes.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/kvm/x86.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 17f4995e80a7e..ed4314641360e 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3219,9 +3219,21 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  	case MSR_IA32_POWER_CTL:
> >  		msr_info->data = vcpu->arch.msr_ia32_power_ctl;
> >  		break;
> > -	case MSR_IA32_TSC:
> > -		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + vcpu->arch.tsc_offset;
> > +	case MSR_IA32_TSC: {
> > +		/*
> > +		 * Intel SDM states that MSR_IA32_TSC read adds the TSC offset
> > +		 * even when not intercepted. AMD manual doesn't explicitly
> > +		 * state this but appears to behave the same.
> > +		 *
> > +		 * However when userspace wants to read this MSR, we should
> > +		 * return it's real L1 value so that its restore will be correct.
> > +		 */
> > +		u64 tsc_offset = msr_info->host_initiated ? vcpu->arch.l1_tsc_offset :
> > +							    vcpu->arch.tsc_offset;
> > +
> > +		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + tsc_offset;
> >  		break;
> > +	}
> >  	case MSR_MTRRcap:
> >  	case 0x200 ... 0x2ff:
> >  		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
> > 
> 
> Applied the patch as it is doing the sanest possible thing for the
> current semantics of host-initiated accesses.
> 
> Paolo
> 
Thanks!

Best regards,
	Maxim Levitsky