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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
