Nested TSC handling

KVM assumed in several places that reading the TSC MSR returns the value for L1. This is incorrect, because when L2 is running, the correct TSC read exit emulation is to return L2's value.

We therefore add a new x86_ops function, read_l1_tsc, to use in places that specifically need to read the L1 TSC, NOT the TSC of the current level of guest.

Note that one change, of one line in kvm_arch_vcpu_load, is made redundant by a different patch sent by Zachary Amsden (and not yet applied):
kvm_arch_vcpu_load() should not read the guest TSC, and if it didn't, of course we didn't have to change the call of kvm_get_msr() to read_l1_tsc().

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    2 ++

 arch/x86/kvm/svm.c              |    9 +++++++++
 arch/x86/kvm/vmx.c              |   17 +++++++++++++++++
 arch/x86/kvm/x86.c              |    8 ++++----
 4 files changed, 32 insertions(+), 4 deletions(-)

arch/x86/include/asm/kvm_host.h      2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/include/asm/kvm_host.h     2011-08-02 15:51:02.000000000 +0300 +2 -0

@@ -636,6 +636,8 @@ struct kvm_x86_ops {
                               struct x86_instruction_info *info,
                               enum x86_intercept_stage stage);
 
+       u64 (*read_l1_tsc)(struct kvm_vcpu *vcpu);
+
        const struct trace_print_flags *exit_reasons_str;
  };
 

arch/x86/kvm/vmx.c   2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/kvm/vmx.c  2011-08-02 15:51:02.000000000 +0300 +17 -0

@@ -1748,6 +1748,21 @@ static u64 guest_read_tsc(void)
  }
 
  /*
+ * Like guest_read_tsc, but always returns L1's notion of the timestamp
+ * counter, even if a nested guest (L2) is currently running.
+ */
+u64 vmx_read_l1_tsc(struct kvm_vcpu *vcpu)
+{
+       u64 host_tsc, tsc_offset;
+
+       rdtscll(host_tsc);
+       tsc_offset = is_guest_mode(vcpu) ?
+               to_vmx(vcpu)->nested.vmcs01_tsc_offset :
+               vmcs_read64(TSC_OFFSET);
+       return host_tsc + tsc_offset;
+}
+
+/*
  * Empty call-back. Needs to be implemented when VMX enables the SET_TSC_KHZ
  * ioctl. In this case the call-back should update internal vmx state to make
  * the changes effective.

@@ -7059,6 +7074,8 @@ static struct kvm_x86_ops vmx_x86_ops =
        .set_tdp_cr3 = vmx_set_cr3,
 
        .check_intercept = vmx_check_intercept,
+
+       .read_l1_tsc = vmx_read_l1_tsc,
  };
 
  static int __init vmx_init(void)

arch/x86/kvm/svm.c   2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/kvm/svm.c  2011-08-02 15:51:02.000000000 +0300 +9 -0

@@ -2894,6 +2894,13 @@ static int cr8_write_interception(struct
        return 0;
  }
 
+u64 svm_read_l1_tsc(struct kvm_vcpu *vcpu)
+{
+       struct vmcb *vmcb = get_host_vmcb(to_svm(vcpu));
+       return vmcb->control.tsc_offset +
+               svm_scale_tsc(vcpu, native_read_tsc());
+}
+
  static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
  {
        struct vcpu_svm *svm = to_svm(vcpu);

@@ -4243,6 +4250,8 @@ static struct kvm_x86_ops svm_x86_ops =
        .set_tdp_cr3 = set_tdp_cr3,
 
        .check_intercept = svm_check_intercept,
+
+       .read_l1_tsc = svm_read_l1_tsc,
  };
 
  static int __init svm_init(void)

arch/x86/kvm/x86.c   2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/kvm/x86.c  2011-08-02 15:51:02.000000000 +0300 +2 -2

@@ -1098,7 +1098,7 @@ static int kvm_guest_time_update(struct
 
        /* Keep irq disabled to prevent changes to the clock */
        local_irq_save(flags);
-       kvm_get_msr(v, MSR_IA32_TSC, &tsc_timestamp);
+       tsc_timestamp = kvm_x86_ops->read_l1_tsc(v);
        kernel_ns = get_kernel_ns();
        this_tsc_khz = vcpu_tsc_khz(v);
        if (unlikely(this_tsc_khz == 0)) {

@@ -2215,7 +2215,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu
                s64 tsc_delta;
                u64 tsc;
 
-               kvm_get_msr(vcpu, MSR_IA32_TSC, &tsc);
+               tsc = kvm_x86_ops->read_l1_tsc(vcpu);

               tsc_delta = !vcpu->arch.last_guest_tsc ? 0 :
                            tsc - vcpu->arch.last_guest_tsc;

arch/x86/kvm/x86.c   2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/kvm/x86.c  2011-08-02 15:51:02.000000000 +0300 +2 -2

@@ -2239,7 +2239,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *
  {
        kvm_x86_ops->vcpu_put(vcpu);
        kvm_put_guest_fpu(vcpu);
-       kvm_get_msr(vcpu, MSR_IA32_TSC, &vcpu->arch.last_guest_tsc);
+       vcpu->arch.last_guest_tsc = kvm_x86_ops->read_l1_tsc(vcpu);
  }
 
  static int is_efer_nx(void)

@@ -5722,7 +5722,7 @@ static int vcpu_enter_guest(struct kvm_v
        if (hw_breakpoint_active())
                hw_breakpoint_restore();
 
-       kvm_get_msr(vcpu, MSR_IA32_TSC, &vcpu->arch.last_guest_tsc);
+       vcpu->arch.last_guest_tsc = kvm_x86_ops->read_l1_tsc(vcpu);
 
        vcpu->mode = OUTSIDE_GUEST_MODE;
        smp_wmb();

This patch fixes two corner cases in nested (L2) handling of TSC-related issues:

1. Somewhat suprisingly, according to the Intel spec, if L1 allows WRMSR to the TSC MSR without an exit, then this should set L1's TSC value itself - not offset by vmcs12.TSC_OFFSET (like was wrongly done in the previous code).

2. Allow L1 to disable the TSC_OFFSETING control, and then correctly ignore the vmcs12.TSC_OFFSET.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/vmx.c |   31 +++++++++++++++++++++----------

 1 file changed, 21 insertions(+), 10 deletions(-)

arch/x86/kvm/vmx.c   2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/kvm/vmx.c  2011-08-02 15:51:02.000000000 +0300 +6 -7

@@ -1777,15 +1777,23 @@ static void vmx_set_tsc_khz(struct kvm_v
  */
  static void vmx_write_tsc_offset(struct kvm_vcpu *vcpu, u64 offset)
  {
-       vmcs_write64(TSC_OFFSET, offset);
-       if (is_guest_mode(vcpu))
+       if (is_guest_mode(vcpu)) {
                /*
-                * We're here if L1 chose not to trap the TSC MSR. Since
-                * prepare_vmcs12() does not copy tsc_offset, we need to also
-                * set the vmcs12 field here.
+                * We're here if L1 chose not to trap WRMSR to TSC. According
+                * to the spec, this should set L1's TSC; The offset that L1
+                * set for L2 remains unchanged, and still needs to be added
+                * to the newly set TSC to get L2's TSC.
                 */
-               get_vmcs12(vcpu)->tsc_offset = offset -
-                       to_vmx(vcpu)->nested.vmcs01_tsc_offset;
+               struct vmcs12 *vmcs12;

...

arch/x86/kvm/vmx.c static void prepare_vmcs02(struct kvm_vc · line 6541

Quoted 8 lines

arch/x86/kvm/vmx.c +5 -2

@@ -6529,8 +6537,11 @@ static void prepare_vmcs02(struct kvm_vc
-       vmcs_write64(TSC_OFFSET,
-               vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
+       if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
+               vmcs_write64(TSC_OFFSET,
+                       vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
+       else
+               vmcs_write64(TSC_OFFSET, vmx->nested.vmcs01_tsc_offset);

I need more context here... where do you apply the adjustment?

The offset should be added to the vmcs01_tsc_offset only (but also written into the hardware VMCS, which should not be preserved when the guest exits).

This is correct. You should always restore the L1 offset when exiting if it might have changed. This implies also that you must update vmx->nested.vmcs01_tsc_offset if you receive a call to vmx_adjust_tsc_offset while L2 is running, which is why I wanted to see more context above.

When the TSC MSR is read by an L2 guest (when L1 allowed this MSR to be read without exit), we need to return L2's notion of the TSC, not L1's.

The current code incorrectly returned L1 TSC, because svm_get_msr() was also used in x86.c where this was assumed, but now that these places call the new svm_read_l1_tsc(), the MSR read can be fixed.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/svm.c |    4 +---

 1 file changed, 1 insertion(+), 3 deletions(-)

Pretty sure this breaks userspace suspend at the cost of supporting a not-so-reasonable hardware feature.
...
This is correct. Now you properly return the correct MSR value for the TSC to the guest in all cases.

However, there is a BIG PROBLEM (yes, it is that bad). Sorry I did not think of it before.

When the guest gets suspended and frozen by userspace, to be restarted later, what qemu is going to do is come along and read all of the MSRs as part of the saved state. One of those happens to be the TSC MSR.

In the end, it may not be worth the hassle to support this mode of operation that to our current knowledge, is in fact, unused. I would

Caution: this requires more care.

Pretty sure this breaks userspace suspend at the cost of supporting a not-so-reasonable hardware feature.

Quoted 16 lines · Nadav Har'El

When the TSC MSR is read by an L2 guest (when L1 allowed this MSR to be read without exit), we need to return L2's notion of the TSC, not L1's.

The current code incorrectly returned L1 TSC, because svm_get_msr() was also used in x86.c where this was assumed, but now that these places call the new svm_read_l1_tsc(), the MSR read can be fixed.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/svm.c |    4 +---

 1 file changed, 1 insertion(+), 3 deletions(-)

arch/x86/kvm/svm.c   2011-08-02 15:51:02.000000000 +0300 static int svm_get_msr(struct kvm_vcpu * · line 2913

Quoted 10 lines

arch/x86/kvm/svm.c   2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/kvm/svm.c  2011-08-02 15:51:02.000000000 +0300 +1 -3

@@ -2907,9 +2907,7 @@ static int svm_get_msr(struct kvm_vcpu *
 
        switch (ecx) {
        case MSR_IA32_TSC: {
-               struct vmcb *vmcb = get_host_vmcb(svm);
-
-               *data = vmcb->control.tsc_offset +
+               *data = svm->vmcb->control.tsc_offset +
                        svm_scale_tsc(vcpu, native_read_tsc());
 
                break;

This is correct. Now you properly return the correct MSR value for the TSC to the guest in all cases.

However, there is a BIG PROBLEM (yes, it is that bad). Sorry I did not think of it before.

When the guest gets suspended and frozen by userspace, to be restarted later, what qemu is going to do is come along and read all of the MSRs as part of the saved state. One of those happens to be the TSC MSR.

Since you can't guarantee suspend will take place when only L1 is running, you may have mixed L1/L2 TSC MSRs now being returned to userspace. Now, when you resume this guest, you will have mixed L1/L2 TSC values written into the MSRs.

Those will almost certainly fail to be matched by the TSC offset matching code, and thus, with multiple VCPUs, you will end up with slightly unsynchronized TSCs, and with that, all the problems associated with unstable TSC come back to haunt you again. Basically, all bets for stability are off.

Apart from recording the L1 and L2 TSC through separate MSRs, there isn't a good way to solve this. Walking through all the steps again, I'm pretty sure this is why I thought it would be better for L0 kvm_get_msr() to return the L1 values even if L2 was running.

In the end, it may not be worth the hassle to support this mode of operation that to our current knowledge, is in fact, unused. I would much prefer to see a bad virtualization of a known insecure and unused guest mode than to have a well used feature such as L1 guest suspend / resume continue to cause clock de-synchronization.

Zach

Quoted 49 lines · Zachary Amsden

Caution: this requires more care.

Pretty sure this breaks userspace suspend at the cost of supporting a not-so-reasonable hardware feature.

Quoted 16 lines · Nadav Har'El

When the TSC MSR is read by an L2 guest (when L1 allowed this MSR to be read without exit), we need to return L2's notion of the TSC, not L1's.

The current code incorrectly returned L1 TSC, because svm_get_msr() was also used in x86.c where this was assumed, but now that these places call the new svm_read_l1_tsc(), the MSR read can be fixed.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/svm.c |    4 +---

 1 file changed, 1 insertion(+), 3 deletions(-)

arch/x86/kvm/svm.c   2011-08-02 15:51:02.000000000 +0300 static int svm_get_msr(struct kvm_vcpu * · line 2913

Quoted 10 lines

arch/x86/kvm/svm.c   2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/kvm/svm.c  2011-08-02 15:51:02.000000000 +0300 +1 -3

@@ -2907,9 +2907,7 @@ static int svm_get_msr(struct kvm_vcpu *
 
        switch (ecx) {
        case MSR_IA32_TSC: {
-               struct vmcb *vmcb = get_host_vmcb(svm);
-
-               *data = vmcb->control.tsc_offset +
+               *data = svm->vmcb->control.tsc_offset +
                        svm_scale_tsc(vcpu, native_read_tsc());
 
                break;

This is correct. Now you properly return the correct MSR value for the TSC to the guest in all cases.

However, there is a BIG PROBLEM (yes, it is that bad). Sorry I did not think of it before.

When the guest gets suspended and frozen by userspace, to be restarted later, what qemu is going to do is come along and read all of the MSRs as part of the saved state. One of those happens to be the TSC MSR.

Since you can't guarantee suspend will take place when only L1 is running, you may have mixed L1/L2 TSC MSRs now being returned to userspace. Now, when you resume this guest, you will have mixed L1/L2 TSC values written into the MSRs.

Quoted 5 lines

Those will almost certainly fail to be matched by the TSC offset matching code, and thus, with multiple VCPUs, you will end up with slightly unsynchronized TSCs, and with that, all the problems associated with unstable TSC come back to haunt you again. Basically, all bets for stability are off.

TSC synchronization is the least of your problems if you attempt to save/restore state a guest while any vcpu is in L2 mode.

So to keep consistency between the remaining MSRs, i agree with Nadav's patch. Apparently SVM's original patches to return L1 TSC were aimed at fixing the problem VMX is facing now, which is fixed by introduction read_l1_tsc helpers.

Quoted 59 lines · Marcelo Tosatti

Quoted 49 lines · Zachary Amsden

Caution: this requires more care.

Pretty sure this breaks userspace suspend at the cost of supporting a not-so-reasonable hardware feature.

Quoted 16 lines · Nadav Har'El

When the TSC MSR is read by an L2 guest (when L1 allowed this MSR to be read without exit), we need to return L2's notion of the TSC, not L1's.

The current code incorrectly returned L1 TSC, because svm_get_msr() was also used in x86.c where this was assumed, but now that these places call the new svm_read_l1_tsc(), the MSR read can be fixed.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/svm.c |    4 +---

 1 file changed, 1 insertion(+), 3 deletions(-)

arch/x86/kvm/svm.c   2011-08-02 15:51:02.000000000 +0300 static int svm_get_msr(struct kvm_vcpu * · line 2913

Quoted 10 lines

arch/x86/kvm/svm.c   2011-08-02 15:51:02.000000000 +0300 renamed from arch/x86/kvm/svm.c  2011-08-02 15:51:02.000000000 +0300 +1 -3

@@ -2907,9 +2907,7 @@ static int svm_get_msr(struct kvm_vcpu *
 
        switch (ecx) {
        case MSR_IA32_TSC: {
-               struct vmcb *vmcb = get_host_vmcb(svm);
-
-               *data = vmcb->control.tsc_offset +
+               *data = svm->vmcb->control.tsc_offset +
                        svm_scale_tsc(vcpu, native_read_tsc());
 
                break;

This is correct. Now you properly return the correct MSR value for the TSC to the guest in all cases.

However, there is a BIG PROBLEM (yes, it is that bad). Sorry I did not think of it before.

When the guest gets suspended and frozen by userspace, to be restarted later, what qemu is going to do is come along and read all of the MSRs as part of the saved state. One of those happens to be the TSC MSR.

Since you can't guarantee suspend will take place when only L1 is running, you may have mixed L1/L2 TSC MSRs now being returned to userspace. Now, when you resume this guest, you will have mixed L1/L2 TSC values written into the MSRs.

Quoted 5 lines

Those will almost certainly fail to be matched by the TSC offset matching code, and thus, with multiple VCPUs, you will end up with slightly unsynchronized TSCs, and with that, all the problems associated with unstable TSC come back to haunt you again. Basically, all bets for stability are off.

TSC synchronization is the least of your problems if you attempt to save/restore state a guest while any vcpu is in L2 mode.

Quoted 4 lines

So to keep consistency between the remaining MSRs, i agree with Nadav's patch. Apparently SVM's original patches to return L1 TSC were aimed at fixing the problem VMX is facing now, which is fixed by introduction read_l1_tsc helpers.

Joerg, can you review and ack Nadav's SVM patch? TIA

Quoted 4 lines

So to keep consistency between the remaining MSRs, i agree with Nadav's patch. Apparently SVM's original patches to return L1 TSC were aimed at fixing the problem VMX is facing now, which is fixed by introduction read_l1_tsc helpers.

Joerg, can you review and ack Nadav's SVM patch? TIA

Quoted 1 line

Joerg, can you review and ack Nadav's SVM patch? TIA

Tested-by Joerg Roedel <[email protected]>

Acked-by Joerg Roedel <[email protected]>

Reviewed and tested (didn't apply cleanly, but was easy to fix that up). Looks all good so far.

On Sun, Aug 07, 2011, Joerg Roedel wrote about "Re: [PATCH 3/3] Fix TSC MSR read in nested SVM":

Quoted 7 lines

Quoted 1 line

Joerg, can you review and ack Nadav's SVM patch? TIA

Tested-by Joerg Roedel <[email protected]>

Acked-by Joerg Roedel <[email protected]>

Reviewed and tested (didn't apply cleanly, but was easy to fix that up). Looks all good so far.

Hi guys, I'm a bit confused how we want to proceed from here.

The patches which I sent appear to fix the original bug (as confirmed by the two people who reported it), but Zachary warned that it would break migration of nested SVM while L2 is running. I asked whether migration works at all while L2 is running (without exiting to L1 first) - and Marcelo suggested that it doesn't.

The following are patches I propose for fixing the bug discovered by Bandan Das and discussed in the "Nested VMX - L1 hangs on running L2" thread.

The first patch should fix the originally-reported bug, as explained in the aforementioned thread: A new x86_op read_l1_tsc() is called L1's TSC is needed, instead of assuming that calling kvm_read_msr() will do that (because this has to return L2's TSC when a nested guest is running).

The second and third patches fix relatively-unimportant corner cases in nested VMX and nested SVM TSC handling.

I'd appreciate it if the people who noticed this bug can verify that these patches indeed solve it for them.

Patch statistics:
-----------------

 arch/x86/include/asm/kvm_host.h |    2 +
 arch/x86/kvm/svm.c              |   13 ++++++--
 arch/x86/kvm/vmx.c              |   48 +++++++++++++++++++++++-------
 arch/x86/kvm/x86.c              |    8 ++---
 4 files changed, 54 insertions(+), 17 deletions(-)

--
Nadav Har'El
IBM Haifa Research Lab

Quoted 8 lines · Nadav Har'El

The following are patches I propose for fixing the bug discovered by Bandan Das and discussed in the "Nested VMX - L1 hangs on running L2" thread.

The first patch should fix the originally-reported bug, as explained in the aforementioned thread: A new x86_op read_l1_tsc() is called L1's TSC is needed, instead of assuming that calling kvm_read_msr() will do that (because this has to return L2's TSC when a nested guest is running).

Quoted 5 lines

The second and third patches fix relatively-unimportant corner cases in nested VMX and nested SVM TSC handling.

I'd appreciate it if the people who noticed this bug can verify that these patches indeed solve it for them.

I can verify that these patches work for me. Thanks Nadav and others for looking into this!

I have cc'ed Jan and Matt in case they would be interested in trying out these patches.

Quoted 12 lines

Patch statistics:
-----------------

arch/x86/include/asm/kvm_host.h |    2 +
arch/x86/kvm/svm.c              |   13 ++++++--
arch/x86/kvm/vmx.c              |   48 +++++++++++++++++++++++-------
arch/x86/kvm/x86.c              |    8 ++---
4 files changed, 54 insertions(+), 17 deletions(-)

--
Nadav Har'El
IBM Haifa Research Lab

The following are patches I propose for fixing the bug discovered by Bandan Das and discussed in the "Nested VMX - L1 hangs on running L2" thread.

The first patch should fix the originally-reported bug, as explained in the aforementioned thread: A new x86_op read_l1_tsc() is called L1's TSC is needed, instead of assuming that calling kvm_read_msr() will do that (because this has to return L2's TSC when a nested guest is running).

The second and third patches fix relatively-unimportant corner cases in nested VMX and nested SVM TSC handling.

I'd appreciate it if the people who noticed this bug can verify that these patches indeed solve it for them.