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:   Wed, 20 Jul 2022 21:34:10 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
Cc:     Maxim Levitsky <mlevitsk@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH] KVM: nSVM: Pull CS.Base from actual VMCB12 for soft
 int/ex re-injection
Message-ID: <Yth00gK0DWjukLgq@google.com>
References: <4caa0f67589ae3c22c311ee0e6139496902f2edc.1658159083.git.maciej.szmigiero@oracle.com>
 <7458497a8694ba0fbabee28eabf557e6e4406fbe.camel@redhat.com>
 <d311c92a-d753-3584-d662-7d82b2fc1e50@maciej.szmigiero.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d311c92a-d753-3584-d662-7d82b2fc1e50@maciej.szmigiero.name>
Precedence: bulk

On Wed, Jul 20, 2022, Maciej S. Szmigiero wrote:
> On 20.07.2022 10:43, Maxim Levitsky wrote:
> > On Mon, 2022-07-18 at 17:47 +0200, Maciej S. Szmigiero wrote:
> > > Fixes: 6ef88d6e36c2 ("KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction")
> > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > > ---
> > >  ?arch/x86/kvm/svm/nested.c | 9 +++++----
> > >  ?1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > index adf4120b05d90..23252ab821941 100644
> > > --- a/arch/x86/kvm/svm/nested.c
> > > +++ b/arch/x86/kvm/svm/nested.c
> > > @@ -639,7 +639,8 @@ static bool is_evtinj_nmi(u32 evtinj)
> > >  ?}
> > >  ?static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
> > > -???????????????????????????????????????? unsigned long vmcb12_rip)
> > > +???????????????????????????????????????? unsigned long vmcb12_rip,
> > > +???????????????????????????????????????? unsigned long vmcb12_csbase)
> > 
> > Honestly I don't like that nested_vmcb02_prepare_control starts to grow its parameter list,
> > because it kind of defeats the purpose of vmcb12 cache we added back then.
> > 
> > I think that it is better to add csbase/rip to vmcb_save_area_cached,
> > but I am not 100% sure. What do you think?
> 
> This function has only 3 parameters now, so they fit well into registers
> without taking any extra memory (even assuming it won't get inlined).
> 
> If in the future more parameters need to be added to this function
> (which may or may not happen) then they all can be moved to, for example,
> vmcb_ctrl_area_cached.

I don't think Maxim is concerned about the size, rather that we have a dedicated
struct for snapshotting select save state and aren't using it.

IIRC, I deliberately avoided using the "cache" because the main/original purpose
of the cache is to avoid TOCTOU issues.  And because RIP and CS.base aren't checked,
there's no need to throw them in the cache.