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;
Message-ID: <a3a4e22f3f2fd0b8582f233d0c34c8460f0dae6f.camel@redhat.com>
Subject: Re: [PATCH v3 13/13] KVM: x86: emulator/smm: preserve interrupt
 shadow in SMRAM
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.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>
Date:   Thu, 25 Aug 2022 13:13:28 +0300
In-Reply-To: <Ywa5K3qVO0kDfTW9@google.com>
References: <20220803155011.43721-1-mlevitsk@redhat.com>
         <20220803155011.43721-14-mlevitsk@redhat.com> <Ywa5K3qVO0kDfTW9@google.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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).

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.

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.

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