Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 Aug 2022 15:44:26 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     kvm@vger.kernel.org, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        linux-kernel@vger.kernel.org, Wanpeng Li <wanpengli@tencent.com>,
        Ingo Molnar <mingo@redhat.com>, x86@kernel.org,
        Jim Mattson <jmattson@google.com>,
        Kees Cook <keescook@chromium.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>, Joerg Roedel <joro@8bytes.org>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v3 13/13] KVM: x86: emulator/smm: preserve interrupt
 shadow in SMRAM
Message-ID: <YweY2oiE1BVy1Ruu@google.com>
References: <20220803155011.43721-1-mlevitsk@redhat.com>
 <20220803155011.43721-14-mlevitsk@redhat.com>
 <Ywa5K3qVO0kDfTW9@google.com>
 <a3a4e22f3f2fd0b8582f233d0c34c8460f0dae6f.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a3a4e22f3f2fd0b8582f233d0c34c8460f0dae6f.camel@redhat.com>
Precedence: bulk

On Thu, Aug 25, 2022, Maxim Levitsky wrote:
> On Wed, 2022-08-24 at 23:50 +0000, Sean Christopherson wrote:
> > On Wed, Aug 03, 2022, Maxim Levitsky wrote:
> > > @@ -518,7 +519,8 @@ struct kvm_smram_state_32 {
> > >  	u32 reserved1[62];
> > >  	u32 smbase;
> > >  	u32 smm_revision;
> > > -	u32 reserved2[5];
> > > +	u32 reserved2[4];
> > > +	u32 int_shadow; /* KVM extension */
> > 
> > Looking at this with fresh(er) eyes, I agree with Jim: KVM shouldn't add its own
> > fields in SMRAM.  There's no need to use vmcb/vmcs memory either, just add fields
> > in kvm_vcpu_arch to save/restore the state across SMI/RSM, and then borrow VMX's
> > approach of supporting migration by adding flags to do out-of-band migration,
> > e.g. KVM_STATE_NESTED_SMM_STI_BLOCKING and KVM_STATE_NESTED_SMM_MOV_SS_BLOCKING.
> > 
> > 	/* SMM state that's not saved in SMRAM. */
> > 	struct {
> > 		struct {
> > 			u8 interruptibility;
> > 		} smm;
> > 	} nested;
> > 
> > That'd finally give us an excuse to move nested_run_pending to common code too :-)
> > 
> Paolo told me that he wants it to be done this way (save the state in the
> smram).

Paolo, what's the motivation for using SMRAM?  I don't see any obvious advantage
for KVM.  QEMU apparently would need to migrate interrupt.shadow, but QEMU should
be doing that anyways, no?

> My first version of this patch was actually saving the state in kvm internal
> state, I personally don't mind that much if to do it this way or another.
> 
> But note that I can't use nested state - the int shadow thing has nothing to
> do with nesting.

Oh, duh.

> I think that 'struct kvm_vcpu_events' is the right place for this, and in fact it already
> has interrupt.shadow (which btw Qemu doesn't migrate...)
> 
> My approach was to use upper 4 bits of 'interrupt.shadow' since it is hightly unlikely
> that we will ever see more that 16 different interrupt shadows.

Heh, unless we ensure STI+MOVSS are mutually exclusive... s/16/4, because
KVM_X86_SHADOW_INT_* are currently treated as masks, not values.

Pedantry aside, using interrupt.shadow definitely seems like the way to go.  We
wouldn't even technically need to use the upper four bits since the bits are KVM
controlled and not hardware-defined, though I agree that using bits 5 and 6 would
give us more flexibility if we ever need to convert the masks to values.

> It would be a bit more clean to put it into the 'smi' substruct, but we already
> have the 'triple_fault' afterwards 
> 
> (but I think that this was very recent addition - maybe it is not too late?)
> 
> A new 'KVM_VCPUEVENT_VALID_SMM_SHADOW' flag can be added to the struct to indicate the
> extra bits if you want.
> 
> Best regards,
> 	Maxim Levitsky
> 
> 
>