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: <8a5984a6c85dd1cc25fb5837df3e9eba82f4e014.camel@redhat.com>
Subject: Re: [PATCH v2 10/11] KVM: x86: SVM: use smram structs
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, x86@kernel.org,
        Kees Cook <keescook@chromium.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        linux-kernel@vger.kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        Borislav Petkov <bp@alien8.de>, Joerg Roedel <joro@8bytes.org>,
        Ingo Molnar <mingo@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>
Date:   Thu, 21 Jul 2022 14:54:38 +0300
In-Reply-To: <YtibYud8NY8bgcPZ@google.com>
References: <20220621150902.46126-1-mlevitsk@redhat.com>
         <20220621150902.46126-11-mlevitsk@redhat.com> <YtibYud8NY8bgcPZ@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 Thu, 2022-07-21 at 00:18 +0000, Sean Christopherson wrote:
> On Tue, Jun 21, 2022, Maxim Levitsky wrote:
> > This removes the last user of put_smstate/GET_SMSTATE so
> > remove these functions as well.
> > 
> > Also add a sanity check that we don't attempt to enter the SMM
> > on non long mode capable guest CPU with a running nested guest.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  6 ------
> >  arch/x86/kvm/svm/svm.c          | 28 +++++++++++++++++-----------
> >  2 files changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 1038ccb7056a39..9e8467be96b4e6 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -2057,12 +2057,6 @@ static inline int kvm_cpu_get_apicid(int mps_cpu)
> >  #endif
> >  }
> >  
> > -#define put_smstate(type, buf, offset, val)                      \
> > -	*(type *)((buf) + (offset) - 0x7e00) = val
> > -
> > -#define GET_SMSTATE(type, buf, offset)		\
> > -	(*(type *)((buf) + (offset) - 0x7e00))
> > -
> >  int kvm_cpu_dirty_log_size(void);
> >  
> >  int memslot_rmap_alloc(struct kvm_memory_slot *slot, unsigned long npages);
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 136298cfb3fb57..8dcbbe839bef36 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -4399,6 +4399,7 @@ static int svm_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection)
> >  
> >  static int svm_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
> >  {
> > +	struct kvm_smram_state_64 *smram = (struct kvm_smram_state_64 *)smstate;
> >  	struct vcpu_svm *svm = to_svm(vcpu);
> >  	struct kvm_host_map map_save;
> >  	int ret;
> > @@ -4406,10 +4407,17 @@ static int svm_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
> >  	if (!is_guest_mode(vcpu))
> >  		return 0;
> >  
> > -	/* FED8h - SVM Guest */
> > -	put_smstate(u64, smstate, 0x7ed8, 1);
> > -	/* FEE0h - SVM Guest VMCB Physical Address */
> > -	put_smstate(u64, smstate, 0x7ee0, svm->nested.vmcb12_gpa);
> > +	/*
> > +	 * 32 bit SMRAM format doesn't preserve EFER and SVM state.
> > +	 * SVM should not be enabled by the userspace without marking
> > +	 * the CPU as at least long mode capable.
> > +	 */
> > +
> > +	if (!guest_cpuid_has(vcpu, X86_FEATURE_SVM))
> > +		return 1;
> 
> Isn't this is an independent bug fix?  I.e. should be in its own patch?

Yes to some extent, patch was small so I didn't bother, but I don't mind splitting it up.

Best regards,
	Maxim Levitsky

> 
> > +
> > +	smram->svm_guest_flag = 1;
> > +	smram->svm_guest_vmcb_gpa = svm->nested.vmcb12_gpa;
> >  
> >  	svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
> >  	svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];