Re: [PATCH 3/3] Fix TSC MSR read in nested SVM

[Joerg Roedel <joro@8bytes.org>]

On Fri, Aug 05, 2011 at 12:32:24PM -0300, Marcelo Tosatti wrote:
> On Wed, Aug 03, 2011 at 06:00:52PM -0300, Marcelo Tosatti wrote:
> > On Wed, Aug 03, 2011 at 01:34:58AM -0700, Zachary Amsden wrote:
> > > Caution: this requires more care.
> > > 
> > > Pretty sure this breaks userspace suspend at the cost of supporting a
> > > not-so-reasonable hardware feature.
> > > 
> > > On Tue, Aug 2, 2011 at 5:55 AM, Nadav Har'El <nyh@il.ibm.com> wrote:
> > > > 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 <nyh@il.ibm.com>
> > > > ---
> > > >  arch/x86/kvm/svm.c |    4 +---
> > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > >
> > > > --- .before/arch/x86/kvm/svm.c  2011-08-02 15:51:02.000000000 +0300
> > > > +++ .after/arch/x86/kvm/svm.c   2011-08-02 15:51:02.000000000 +0300
> > > > @@ -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.
> > 
> > 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.
> 
> Joerg, can you review and ack Nadav's SVM patch? TIA

Yes, sorry for the delay. I'll give it a review and test today.

	Joerg