Re: [PATCH RFC] KVM: nSVM: Fix L1 state corruption upon return from SMM

From: Maxim Levitsky <mlevitsk@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>,
	Cathy Avery <cavery@redhat.com>,
	Emanuele Giuseppe Esposito <eesposit@redhat.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH RFC] KVM: nSVM: Fix L1 state corruption upon return from SMM
Date: Thu, 24 Jun 2021 11:20:24 +0300	[thread overview]
Message-ID: <83affeedb9a3d091bece8f5fdd5373342298dcd3.camel@redhat.com> (raw)
In-Reply-To: <82327cd1-92ca-9f6b-3af0-8215e9d21eae@redhat.com>

On Wed, 2021-06-23 at 22:37 +0200, Paolo Bonzini wrote:
> On 23/06/21 18:21, Sean Christopherson wrote:
> > On Wed, Jun 23, 2021, Sean Christopherson wrote:
> > > And I believe this hackery is necessary only because nested_svm_vmexit() isn't
> > > following the architcture in the first place.  I.e. using vmcb01 to restore
> > > host state is flat out wrong.
> > 
> > Ah, that's not true, using vmcb01 is allowed by "may store some or all host state
> > in hidden on-chip memory".
> 
> And also, "Different implementations may choose to save the hidden parts 
> of the host’s segment registers as well as the selectors".
> 
> >  From a performance perspective, I do like the SMI/RSM shenanigans.  I'm not
> > totally opposed to the trickery since I think it will break a guest if and only
> > if the L1 guest is also violating the APM.  And we're not fudging the spec thaat
> > much :-)
> 
> Yeah, that was my reasoning as well.  Any reference to "hidden on-chip 
> memory", plus the forbidding modifications of the host save area, sort 
> of implies that the processor can actually flush that hidden on-chip 
> memory for whatever reason (such as on some sleep states?!?).
> 
> Paolo
> 

Let me explain my thoughts about this again, now that I hopefully
understand this correctly:

L1 register state is stored in VMCB01 since the CPU stores it there,
every time L1 VMexits.

Now L1 is a host for L2, and therefore this is also L1 "host" state.

When we switch to L2, it is therefore natural to keep that state in
vmcb01, and not copy it to VM_HSAVE_PA area.

So when running SMM code in L1, since we must not corrupt this state,
It makes sense to back it up somewhere,
but as I said I prefer to store/migrate it out of band instead of
saving it in the guest memory, although Paolo's reasoning that CPU
might *sometimes* write VM_HSAVE_PA and that *sometimes* is allowed to be
when we enter SMM,  does make sense.
Thus I don't have a strong opinion on this anymore, as long as it works.

Something else to note, just for our information is that KVM 
these days does vmsave/vmload to VM_HSAVE_PA to store/restore 
the additional host state, something that is frowned upon in the spec, 
but there is some justification of doing this in the commit message,
citing an old spec which allowed this.
This shoudn't affect anything, but just FYI.

https://patchwork.kernel.org/project/kvm/patch/20201210174814.1122585-1-michael.roth@amd.com/

Best regards,
	Maxim Levitsky