All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: SEV: improve the code readability for ASID management
@ 2021-07-31  1:13 Mingwei Zhang
  2021-08-02 13:38 ` Joerg Roedel
  2021-08-02 16:17 ` Sean Christopherson
  0 siblings, 2 replies; 6+ messages in thread
From: Mingwei Zhang @ 2021-07-31  1:13 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Mingwei Zhang, Tom Lendacky,
	Marc Orr, David Rientjes, Alper Gun, Dionna Glaze, Vipin Sharma

KVM SEV code uses bitmaps to manage ASID states. ASID 0 was always skipped
because it is never used by VM. Thus, ASID value and its bitmap postion
always has an 'offset-by-1' relationship.

Both SEV and SEV-ES shares the ASID space, thus KVM uses a dynamic range
[min_asid, max_asid] to handle SEV and SEV-ES ASIDs separately.

Existing code mixes the usage of ASID value and its bitmap position by
using the same variable called 'min_asid'.

Fix the min_asid usage: ensure that its usage is consistent with its name;
adjust its value before using it as a bitmap position. Add comments on ASID
bitmap allocation to clarify the skipping-ASID-0 property.

Fixes: 80675b3ad45f (KVM: SVM: Update ASID allocation to support SEV-ES guests)
Signed-off-by: Mingwei Zhang <mizhang@google.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Marc Orr <marcorr@google.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Alper Gun <alpergun@google.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Vipin Sharma <vipinsh@google.com>
Ce: Peter Gonda <pgonda@google.com>
---
 arch/x86/kvm/svm/sev.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 8d36f0c73071..e3902283cbf7 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -80,7 +80,7 @@ static int sev_flush_asids(int min_asid, int max_asid)
 	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_asid, min_asid);
+	pos = find_next_bit(sev_reclaim_asid_bitmap, max_asid, min_asid - 1);
 	if (pos >= max_asid)
 		return -EBUSY;
 
@@ -142,10 +142,10 @@ static int sev_asid_new(struct kvm_sev_info *sev)
 	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
 	 * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
 	 */
-	min_asid = sev->es_active ? 0 : min_sev_asid - 1;
+	min_asid = sev->es_active ? 1 : min_sev_asid;
 	max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid;
 again:
-	pos = find_next_zero_bit(sev_asid_bitmap, max_sev_asid, min_asid);
+	pos = find_next_zero_bit(sev_asid_bitmap, max_sev_asid, min_asid - 1);
 	if (pos >= max_asid) {
 		if (retry && __sev_recycle_asids(min_asid, max_asid)) {
 			retry = false;
@@ -1854,7 +1854,10 @@ void __init sev_hardware_setup(void)
 	min_sev_asid = edx;
 	sev_me_mask = 1UL << (ebx & 0x3f);
 
-	/* Initialize SEV ASID bitmaps */
+	/*
+	 * Initialize SEV ASID bitmaps. Note: ASID 0 is skipped since it is
+	 * never used by any VM, thus: ASID value == ASID position + 1;
+	 */
 	sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
 	if (!sev_asid_bitmap)
 		goto out;
@@ -1904,7 +1907,7 @@ void sev_hardware_teardown(void)
 		return;
 
 	/* No need to take sev_bitmap_lock, all VMs have been destroyed. */
-	sev_flush_asids(0, max_sev_asid);
+	sev_flush_asids(1, max_sev_asid);
 
 	bitmap_free(sev_asid_bitmap);
 	bitmap_free(sev_reclaim_asid_bitmap);
-- 
2.32.0.554.ge1b32706d8-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SEV: improve the code readability for ASID management
  2021-07-31  1:13 [PATCH] KVM: SEV: improve the code readability for ASID management Mingwei Zhang
@ 2021-08-02 13:38 ` Joerg Roedel
  2021-08-02 16:00   ` Mingwei Zhang
  2021-08-02 16:17 ` Sean Christopherson
  1 sibling, 1 reply; 6+ messages in thread
From: Joerg Roedel @ 2021-08-02 13:38 UTC (permalink / raw)
  To: Mingwei Zhang
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, kvm, linux-kernel, Tom Lendacky, Marc Orr,
	David Rientjes, Alper Gun, Dionna Glaze, Vipin Sharma

On Fri, Jul 30, 2021 at 06:13:04PM -0700, Mingwei Zhang wrote:
> Fix the min_asid usage: ensure that its usage is consistent with its name;
> adjust its value before using it as a bitmap position. Add comments on ASID
> bitmap allocation to clarify the skipping-ASID-0 property.
> 
> Fixes: 80675b3ad45f (KVM: SVM: Update ASID allocation to support SEV-ES guests)

This looks more like an optimization to me, or does this fix any real
bug?


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SEV: improve the code readability for ASID management
  2021-08-02 13:38 ` Joerg Roedel
@ 2021-08-02 16:00   ` Mingwei Zhang
  0 siblings, 0 replies; 6+ messages in thread
From: Mingwei Zhang @ 2021-08-02 16:00 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, kvm, LKML, Tom Lendacky, Marc Orr, David Rientjes,
	Alper Gun, Dionna Glaze, Vipin Sharma

Right, it does not fix any bugs but improves readability. I will
remove this primitive.

On Mon, Aug 2, 2021 at 6:38 AM Joerg Roedel <joro@8bytes.org> wrote:
>
> On Fri, Jul 30, 2021 at 06:13:04PM -0700, Mingwei Zhang wrote:
> > Fix the min_asid usage: ensure that its usage is consistent with its name;
> > adjust its value before using it as a bitmap position. Add comments on ASID
> > bitmap allocation to clarify the skipping-ASID-0 property.
> >
> > Fixes: 80675b3ad45f (KVM: SVM: Update ASID allocation to support SEV-ES guests)
>
> This looks more like an optimization to me, or does this fix any real
> bug?
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SEV: improve the code readability for ASID management
  2021-07-31  1:13 [PATCH] KVM: SEV: improve the code readability for ASID management Mingwei Zhang
  2021-08-02 13:38 ` Joerg Roedel
@ 2021-08-02 16:17 ` Sean Christopherson
  2021-08-02 16:52   ` Paolo Bonzini
  1 sibling, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2021-08-02 16:17 UTC (permalink / raw)
  To: Mingwei Zhang
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Tom Lendacky, Marc Orr,
	David Rientjes, Alper Gun, Dionna Glaze, Vipin Sharma

On Fri, Jul 30, 2021, Mingwei Zhang wrote:
> KVM SEV code uses bitmaps to manage ASID states. ASID 0 was always skipped
> because it is never used by VM. Thus, ASID value and its bitmap postion
> always has an 'offset-by-1' relationship.

That's not necessarily a bad thing, assuming the bitmap is properly sized.

> Both SEV and SEV-ES shares the ASID space, thus KVM uses a dynamic range
> [min_asid, max_asid] to handle SEV and SEV-ES ASIDs separately.
> 
> Existing code mixes the usage of ASID value and its bitmap position by
> using the same variable called 'min_asid'.
> 
> Fix the min_asid usage: ensure that its usage is consistent with its name;
> adjust its value before using it as a bitmap position. Add comments on ASID
> bitmap allocation to clarify the skipping-ASID-0 property.
> 
> Fixes: 80675b3ad45f (KVM: SVM: Update ASID allocation to support SEV-ES guests)

As Joerg commented, Fixes: is not appropriate unless there's an actual bug being
addressed.  And based on the shortlog's "improve the code readability", I would
expect a pure refactoring, i.e. something's got to give.  AFAICT, this is a pure
refactoring, so the Fixes: should be dropped.

> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Alper Gun <alpergun@google.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Vipin Sharma <vipinsh@google.com>
> Ce: Peter Gonda <pgonda@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 8d36f0c73071..e3902283cbf7 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -80,7 +80,7 @@ static int sev_flush_asids(int min_asid, int max_asid)
>  	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_asid, min_asid);
> +	pos = find_next_bit(sev_reclaim_asid_bitmap, max_asid, min_asid - 1);
>  	if (pos >= max_asid)
>  		return -EBUSY;
>  
> @@ -142,10 +142,10 @@ static int sev_asid_new(struct kvm_sev_info *sev)
>  	 * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
>  	 * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
>  	 */
> -	min_asid = sev->es_active ? 0 : min_sev_asid - 1;
> +	min_asid = sev->es_active ? 1 : min_sev_asid;
>  	max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid;
>  again:
> -	pos = find_next_zero_bit(sev_asid_bitmap, max_sev_asid, min_asid);
> +	pos = find_next_zero_bit(sev_asid_bitmap, max_sev_asid, min_asid - 1);

IMO, this is only marginally better, as the checks against max_asid are still
misleading, and the "pos + 1" + "min_asid - 1" interaction is subtle.

>  	if (pos >= max_asid) {

This is the check that's misleading/confusing.

Rather than adjusting the bitmap index, what about simply umping the bitmap size?
IIRC, current CPUs have 512 ASIDs, counting ASID 0, i.e. bumping the size won't
consume any additional memory.  And if it does, the cost is 8 bytes...

It'd be a bigger refactoring, but it should completely eliminate the mod-by-1
shenanigans, e.g. a partial patch could look like

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 62926f1a5f7b..7bcdc34546d7 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -64,6 +64,7 @@ static DEFINE_MUTEX(sev_bitmap_lock);
 unsigned int max_sev_asid;
 static unsigned int min_sev_asid;
 static unsigned long sev_me_mask;
+static unsigned int nr_asids;
 static unsigned long *sev_asid_bitmap;
 static unsigned long *sev_reclaim_asid_bitmap;

@@ -81,8 +82,8 @@ static int sev_flush_asids(int min_asid, int max_asid)
        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_asid, min_asid);
-       if (pos >= max_asid)
+       pos = find_next_bit(sev_reclaim_asid_bitmap, nr_asids, min_asid);
+       if (pos > max_asid)
                return -EBUSY;

        /*
@@ -115,8 +116,8 @@ static bool __sev_recycle_asids(int min_asid, int max_asid)

        /* The flush process will flush all reclaimable SEV and SEV-ES ASIDs */
        bitmap_xor(sev_asid_bitmap, sev_asid_bitmap, sev_reclaim_asid_bitmap,
-                  max_sev_asid);
-       bitmap_zero(sev_reclaim_asid_bitmap, max_sev_asid);
+                  nr_asids);
+       bitmap_zero(sev_reclaim_asid_bitmap, nr_asids);

        return true;
 }
@@ -143,11 +144,11 @@ static int sev_asid_new(struct kvm_sev_info *sev)
         * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
         * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
         */
-       min_asid = sev->es_active ? 0 : min_sev_asid - 1;
+       min_asid = sev->es_active ? 1 : min_sev_asid;
        max_asid = sev->es_active ? min_sev_asid - 1 : max_sev_asid;
 again:
-       pos = find_next_zero_bit(sev_asid_bitmap, max_sev_asid, min_asid);
-       if (pos >= max_asid) {
+       pos = find_next_zero_bit(sev_asid_bitmap, sev_asid_bitmap_size, min_asid);
+       if (pos > max_asid) {
                if (retry && __sev_recycle_asids(min_asid, max_asid)) {
                        retry = false;
                        goto again;
@@ -161,7 +162,7 @@ static int sev_asid_new(struct kvm_sev_info *sev)

        mutex_unlock(&sev_bitmap_lock);

-       return pos + 1;
+       return pos;
@@ -1855,12 +1942,17 @@ void __init sev_hardware_setup(void)
        min_sev_asid = edx;
        sev_me_mask = 1UL << (ebx & 0x3f);
 
-       /* Initialize SEV ASID bitmaps */
-       sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
+       /*
+        * Initialize SEV ASID bitmaps.  Allocate space for ASID 0 in the
+        * bitmap, even though it's never used, so that the bitmap is indexed
+        * by the actual ASID.
+        */
+       nr_asids = max_sev_asid + 1;
+       sev_asid_bitmap = bitmap_zalloc(nr_asids, GFP_KERNEL);
        if (!sev_asid_bitmap)
                goto out;
 
-       sev_reclaim_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
+       sev_reclaim_asid_bitmap = bitmap_zalloc(nr_asids, GFP_KERNEL);
        if (!sev_reclaim_asid_bitmap) {
                bitmap_free(sev_asid_bitmap);
                sev_asid_bitmap = NULL;

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SEV: improve the code readability for ASID management
  2021-08-02 16:17 ` Sean Christopherson
@ 2021-08-02 16:52   ` Paolo Bonzini
  2021-08-02 17:25     ` Mingwei Zhang
  0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2021-08-02 16:52 UTC (permalink / raw)
  To: Sean Christopherson, Mingwei Zhang
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel, Tom Lendacky, Marc Orr, David Rientjes, Alper Gun,
	Dionna Glaze, Vipin Sharma

On 02/08/21 18:17, Sean Christopherson wrote:
> 
> Rather than adjusting the bitmap index, what about simply umping the bitmap size?
> IIRC, current CPUs have 512 ASIDs, counting ASID 0, i.e. bumping the size won't
> consume any additional memory.  And if it does, the cost is 8 bytes...
> 
> It'd be a bigger refactoring, but it should completely eliminate the mod-by-1
> shenanigans, e.g. a partial patch could look like

This is also okay by me if Mingwei agrees, of course.  I have already 
queued his patch, but I can replace it with one using a nr_asids-sized 
bitmap too.

Paolo


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SEV: improve the code readability for ASID management
  2021-08-02 16:52   ` Paolo Bonzini
@ 2021-08-02 17:25     ` Mingwei Zhang
  0 siblings, 0 replies; 6+ messages in thread
From: Mingwei Zhang @ 2021-08-02 17:25 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, LKML, Tom Lendacky, Marc Orr, David Rientjes,
	Alper Gun, Dionna Glaze, Vipin Sharma

Hi Paolo,

Thanks. I think Sean's suggestion makes sense. I will update it with
that one and remove the 'fixes' line.

Regards
-Mingwei

On Mon, Aug 2, 2021 at 9:53 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/08/21 18:17, Sean Christopherson wrote:
> >
> > Rather than adjusting the bitmap index, what about simply umping the bitmap size?
> > IIRC, current CPUs have 512 ASIDs, counting ASID 0, i.e. bumping the size won't
> > consume any additional memory.  And if it does, the cost is 8 bytes...
> >
> > It'd be a bigger refactoring, but it should completely eliminate the mod-by-1
> > shenanigans, e.g. a partial patch could look like
>
> This is also okay by me if Mingwei agrees, of course.  I have already
> queued his patch, but I can replace it with one using a nr_asids-sized
> bitmap too.
>
> Paolo
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-02 17:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-31  1:13 [PATCH] KVM: SEV: improve the code readability for ASID management Mingwei Zhang
2021-08-02 13:38 ` Joerg Roedel
2021-08-02 16:00   ` Mingwei Zhang
2021-08-02 16:17 ` Sean Christopherson
2021-08-02 16:52   ` Paolo Bonzini
2021-08-02 17:25     ` Mingwei Zhang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.