| Message ID | 20210114003708.3798992-15-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: Misc SEV cleanups | expand |
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? Also, maybe a comment about not needing the bitmap lock because this is during teardown. Thanks, Tom > + > bitmap_free(sev_asid_bitmap); > bitmap_free(sev_reclaim_asid_bitmap); > - > - sev_flush_asids(); > } > > int sev_cpu_init(struct svm_cpu_data *sd) >
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.
On 14/01/21 01:37, 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); > + > bitmap_free(sev_asid_bitmap); > bitmap_free(sev_reclaim_asid_bitmap); > - > - sev_flush_asids(); > } > > int sev_cpu_init(struct svm_cpu_data *sd) > I can't find 00/14 in my inbox, so: queued 1-3 and 6-14, thanks. Paolo
On Thu, Jan 28, 2021, Paolo Bonzini wrote:
> I can't find 00/14 in my inbox, so: queued 1-3 and 6-14, thanks.
If it's not too late, v3 has a few tweaks that would be nice to have, as well as
a new patch to remove the CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT dependency.
https://lkml.kernel.org/r/20210122202144.2756381-1-seanjc@google.com
On 28/01/21 17:29, Sean Christopherson wrote: > On Thu, Jan 28, 2021, Paolo Bonzini wrote: >> I can't find 00/14 in my inbox, so: queued 1-3 and 6-14, thanks. > > If it's not too late, v3 has a few tweaks that would be nice to have, as well as > a new patch to remove the CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT dependency. > > https://lkml.kernel.org/r/20210122202144.2756381-1-seanjc@google.com Yes, will do (I had done all of them myself except the comment in sev_hardware_teardown() but it's better to match what was sent to LKML). Paolo
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); + bitmap_free(sev_asid_bitmap); bitmap_free(sev_reclaim_asid_bitmap); - - sev_flush_asids(); } int sev_cpu_init(struct svm_cpu_data *sd)
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(-)