Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 Jan 2021 09:19:19 -0800
From:   Sean Christopherson <seanjc@google.com>
To:     Tom Lendacky <thomas.lendacky@amd.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        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, Borislav Petkov <bp@suse.de>,
        Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [PATCH v2 14/14] KVM: SVM: Skip SEV cache flush if no ASIDs have
 been used
Message-ID: <YAHOl/ghOZKJcI1o@google.com>
References: <20210114003708.3798992-1-seanjc@google.com>
 <20210114003708.3798992-15-seanjc@google.com>
 <fa048460-5517-b689-3b82-a269e1ff8ea6@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fa048460-5517-b689-3b82-a269e1ff8ea6@amd.com>
Precedence: bulk

On Fri, Jan 15, 2021, Tom Lendacky wrote:
> On 1/13/21 6:37 PM, Sean Christopherson wrote:
> > Skip SEV's expensive WBINVD and DF_FLUSH if there are no SEV ASIDs
> > waiting to be reclaimed, e.g. if SEV was never used.  This "fixes" an
> > issue where the DF_FLUSH fails during hardware teardown if the original
> > SEV_INIT failed.  Ideally, SEV wouldn't be marked as enabled in KVM if
> > SEV_INIT fails, but that's a problem for another day.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/kvm/svm/sev.c | 22 ++++++++++------------
> >   1 file changed, 10 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 23a4bead4a82..e71bc742d8da 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -56,9 +56,14 @@ struct enc_region {
> >   	unsigned long size;
> >   };
> > -static int sev_flush_asids(void)
> > +static int sev_flush_asids(int min_asid, int max_asid)
> >   {
> > -	int ret, error = 0;
> > +	int ret, pos, error = 0;
> > +
> > +	/* Check if there are any ASIDs to reclaim before performing a flush */
> > +	pos = find_next_bit(sev_reclaim_asid_bitmap, max_sev_asid, min_asid);
> > +	if (pos >= max_asid)
> > +		return -EBUSY;
> >   	/*
> >   	 * DEACTIVATE will clear the WBINVD indicator causing DF_FLUSH to fail,
> > @@ -80,14 +85,7 @@ static int sev_flush_asids(void)
> >   /* Must be called with the sev_bitmap_lock held */
> >   static bool __sev_recycle_asids(int min_asid, int max_asid)
> >   {
> > -	int pos;
> > -
> > -	/* Check if there are any ASIDs to reclaim before performing a flush */
> > -	pos = find_next_bit(sev_reclaim_asid_bitmap, max_sev_asid, min_asid);
> > -	if (pos >= max_asid)
> > -		return false;
> > -
> > -	if (sev_flush_asids())
> > +	if (sev_flush_asids(min_asid, max_asid))
> >   		return false;
> >   	/* The flush process will flush all reclaimable SEV and SEV-ES ASIDs */
> > @@ -1323,10 +1321,10 @@ void sev_hardware_teardown(void)
> >   	if (!sev_enabled)
> >   		return;
> > +	sev_flush_asids(0, max_sev_asid);
> 
> I guess you could have called __sev_recycle_asids(0, max_sev_asid) here and
> left things unchanged up above. It would do the extra bitmap_xor() and
> bitmap_zero() operations, though. What do you think?

IMO, calling "recycle" from the teardown flow would be confusing without a
comment to explain that it's the flush that we really care about, and at that
point it's hard to argue against calling "flush" directly.

The cost of the extra operations is almost certainly negligible, but similar to
above it will leave readers wonder why the teardown flow bothers to xor/zero
the bitmap, only to immediately free it.

> Also, maybe a comment about not needing the bitmap lock because this is
> during teardown.

Ya, I'll add that.