[PATCH for-6.8 0/5] KVM: LoongArch: Fix wrong CPUCFG ID handling

[PATCH for-6.8 0/5] KVM: LoongArch: Fix wrong CPUCFG ID handling

[WANG Xuerui]

From: WANG Xuerui <git@xen0n.name>

Hi,

While trying to add loongarch to the Rust kvm-bindings crate, I
accidentally discovered faulty logic in the handling of CPUCFG IDs
("leaves" for those more familiar with x86), that could result in
incorrectly accepting every possible int for the ID; fortunately it is
6.8 material that hasn't seen a release yet, so a fix is possible.

The first two patches contain the fix, while the rest are general
drive-by refactoring and comment cleanups.

(As it is currently the Chinese holiday season, it is probably best for
this series to go through the kvm tree, instead of the loongarch one
even though they seem to like prefer collecting every loongarch patch.)

WANG Xuerui (5):
  KVM: LoongArch: Fix input value checking of _kvm_get_cpucfg
  KVM: LoongArch: Fix kvm_check_cpucfg incorrectly accepting bad CPUCFG
    IDs
  KVM: LoongArch: Rename _kvm_get_cpucfg to _kvm_get_cpucfg_mask
  KVM: LoongArch: Streamline control flow of kvm_check_cpucfg
  KVM: LoongArch: Clean up comments of _kvm_get_cpucfg_mask and
    kvm_check_cpucfg

 arch/loongarch/kvm/vcpu.c | 72 ++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 43 deletions(-)

-- 
2.43.0

[PATCH for-6.8 1/5] KVM: LoongArch: Fix input value checking of _kvm_get_cpucfg

[WANG Xuerui]

From: WANG Xuerui <git@xen0n.name>

The range check for the CPUCFG ID is wrong (should have been a ||
instead of &&), and pointless, because the default case a few lines
below already serves to deny all unhandled cases.

Furthermore, the juggling of the temp return value is unnecessary,
because it is semantically equivalent and more readable to just
return at every switch case's end. This is done too to avoid potential
bugs in the future related to the unwanted complexity.

Fixes: db1ecca22edf ("LoongArch: KVM: Add LSX (128bit SIMD) support")
Signed-off-by: WANG Xuerui <git@xen0n.name>
---
 arch/loongarch/kvm/vcpu.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index 27701991886d..c4a592623da6 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -300,11 +300,6 @@ static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val)
 
 static int _kvm_get_cpucfg(int id, u64 *v)
 {
-	int ret = 0;
-
-	if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
-		return -EINVAL;
-
 	switch (id) {
 	case 2:
 		/* Return CPUCFG2 features which have been supported by KVM */
@@ -324,12 +319,10 @@ static int _kvm_get_cpucfg(int id, u64 *v)
 		if (cpu_has_lasx)
 			*v |= CPUCFG2_LASX;
 
-		break;
+		return 0;
 	default:
-		ret = -EINVAL;
-		break;
+		return -EINVAL;
 	}
-	return ret;
 }
 
 static int kvm_check_cpucfg(int id, u64 val)
-- 
2.43.0

[PATCH for-6.8 2/5] KVM: LoongArch: Fix kvm_check_cpucfg incorrectly accepting bad CPUCFG IDs

[WANG Xuerui]

From: WANG Xuerui <git@xen0n.name>

The return value of _kvm_get_cpucfg is meant to be checked, but this
was not done, so bad CPUCFG IDs wrongly fall back to the default case
and 0 is returned.

Check the return value to fix the UAPI behavior.

While at it, also remove the redundant range check, because
_kvm_get_cpucfg already rejects all unrecognized input CPUCFG IDs.
It was also wrong and no-op, in the same way as the former identical
check in _kvm_get_cpucfg.

Fixes: db1ecca22edf ("LoongArch: KVM: Add LSX (128bit SIMD) support")
Signed-off-by: WANG Xuerui <git@xen0n.name>
---
 arch/loongarch/kvm/vcpu.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index c4a592623da6..cc2332b056ba 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -328,12 +328,10 @@ static int _kvm_get_cpucfg(int id, u64 *v)
 static int kvm_check_cpucfg(int id, u64 val)
 {
 	u64 mask;
-	int ret = 0;
-
-	if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
-		return -EINVAL;
+	int ret;
 
-	if (_kvm_get_cpucfg(id, &mask))
+	ret = _kvm_get_cpucfg(id, &mask);
+	if (ret)
 		return ret;
 
 	switch (id) {
-- 
2.43.0

[PATCH for-6.8 3/5] KVM: LoongArch: Rename _kvm_get_cpucfg to _kvm_get_cpucfg_mask

[WANG Xuerui]

From: WANG Xuerui <git@xen0n.name>

The function is not actually a getter of guest CPUCFG, but rather
validation of the input CPUCFG ID plus information about the supported
bit flags of that CPUCFG leaf. So rename it to avoid confusion.

Signed-off-by: WANG Xuerui <git@xen0n.name>
---
 arch/loongarch/kvm/vcpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index cc2332b056ba..e973500611b4 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -298,7 +298,7 @@ static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val)
 	return ret;
 }
 
-static int _kvm_get_cpucfg(int id, u64 *v)
+static int _kvm_get_cpucfg_mask(int id, u64 *v)
 {
 	switch (id) {
 	case 2:
@@ -330,7 +330,7 @@ static int kvm_check_cpucfg(int id, u64 val)
 	u64 mask;
 	int ret;
 
-	ret = _kvm_get_cpucfg(id, &mask);
+	ret = _kvm_get_cpucfg_mask(id, &mask);
 	if (ret)
 		return ret;
 
@@ -557,7 +557,7 @@ static int kvm_loongarch_get_cpucfg_attr(struct kvm_vcpu *vcpu,
 	uint64_t val;
 	uint64_t __user *uaddr = (uint64_t __user *)attr->addr;
 
-	ret = _kvm_get_cpucfg(attr->attr, &val);
+	ret = _kvm_get_cpucfg_mask(attr->attr, &val);
 	if (ret)
 		return ret;
 
-- 
2.43.0

[PATCH for-6.8 4/5] KVM: LoongArch: Streamline control flow of kvm_check_cpucfg

[WANG Xuerui]

From: WANG Xuerui <git@xen0n.name>

All the checks currently done in kvm_check_cpucfg can be realized with
early returns, so just do that to avoid extra cognitive burden related
to the return value handling.

The default branch is unreachable because of the earlier validation by
_kvm_get_cpucfg_mask, so mark it as such too to make things clearer.

Signed-off-by: WANG Xuerui <git@xen0n.name>
---
 arch/loongarch/kvm/vcpu.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index e973500611b4..9e108ffaba30 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -339,24 +339,23 @@ static int kvm_check_cpucfg(int id, u64 val)
 		/* CPUCFG2 features checking */
 		if (val & ~mask)
 			/* The unsupported features should not be set */
-			ret = -EINVAL;
-		else if (!(val & CPUCFG2_LLFTP))
+			return -EINVAL;
+		if (!(val & CPUCFG2_LLFTP))
 			/* The LLFTP must be set, as guest must has a constant timer */
-			ret = -EINVAL;
-		else if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
+			return -EINVAL;
+		if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
 			/* Single and double float point must both be set when enable FP */
-			ret = -EINVAL;
-		else if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
+			return -EINVAL;
+		if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
 			/* FP should be set when enable LSX */
-			ret = -EINVAL;
-		else if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
+			return -EINVAL;
+		if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
 			/* LSX, FP should be set when enable LASX, and FP has been checked before. */
-			ret = -EINVAL;
-		break;
+			return -EINVAL;
+		return 0;
 	default:
-		break;
+		unreachable();
 	}
-	return ret;
 }
 
 static int kvm_get_one_reg(struct kvm_vcpu *vcpu,
-- 
2.43.0

Re: [PATCH for-6.8 4/5] KVM: LoongArch: Streamline control flow of kvm_check_cpucfg

[Huacai Chen]

Hi, Xuerui,

On Wed, Feb 14, 2024 at 6:16 PM WANG Xuerui <kernel@xen0n.name> wrote:
>
> From: WANG Xuerui <git@xen0n.name>
>
> All the checks currently done in kvm_check_cpucfg can be realized with
> early returns, so just do that to avoid extra cognitive burden related
> to the return value handling.
>
> The default branch is unreachable because of the earlier validation by
> _kvm_get_cpucfg_mask, so mark it as such too to make things clearer.
>
> Signed-off-by: WANG Xuerui <git@xen0n.name>
> ---
>  arch/loongarch/kvm/vcpu.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index e973500611b4..9e108ffaba30 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -339,24 +339,23 @@ static int kvm_check_cpucfg(int id, u64 val)
>                 /* CPUCFG2 features checking */
>                 if (val & ~mask)
>                         /* The unsupported features should not be set */
> -                       ret = -EINVAL;
> -               else if (!(val & CPUCFG2_LLFTP))
> +                       return -EINVAL;
> +               if (!(val & CPUCFG2_LLFTP))
>                         /* The LLFTP must be set, as guest must has a constant timer */
> -                       ret = -EINVAL;
> -               else if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
> +                       return -EINVAL;
> +               if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
>                         /* Single and double float point must both be set when enable FP */
> -                       ret = -EINVAL;
> -               else if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
> +                       return -EINVAL;
> +               if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
>                         /* FP should be set when enable LSX */
> -                       ret = -EINVAL;
> -               else if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
> +                       return -EINVAL;
> +               if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
>                         /* LSX, FP should be set when enable LASX, and FP has been checked before. */
> -                       ret = -EINVAL;
> -               break;
> +                       return -EINVAL;
> +               return 0;
>         default:
> -               break;
> +               unreachable();
Maybe BUG() is better than unreachable()?

Huacai
>         }
> -       return ret;
>  }
>
>  static int kvm_get_one_reg(struct kvm_vcpu *vcpu,
> --
> 2.43.0
>

[PATCH for-6.8 5/5] KVM: LoongArch: Clean up comments of _kvm_get_cpucfg_mask and kvm_check_cpucfg

[WANG Xuerui]

From: WANG Xuerui <git@xen0n.name>

Remove comments that are merely restatement of the code nearby, and
paraphrase the rest so they read more natural for English speakers (that
lack understanding of Chinese grammar). No functional changes.

Signed-off-by: WANG Xuerui <git@xen0n.name>
---
 arch/loongarch/kvm/vcpu.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index 9e108ffaba30..ff51d6ba59aa 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -302,20 +302,14 @@ static int _kvm_get_cpucfg_mask(int id, u64 *v)
 {
 	switch (id) {
 	case 2:
-		/* Return CPUCFG2 features which have been supported by KVM */
+		/* CPUCFG2 features unconditionally supported by KVM */
 		*v = CPUCFG2_FP     | CPUCFG2_FPSP  | CPUCFG2_FPDP     |
 		     CPUCFG2_FPVERS | CPUCFG2_LLFTP | CPUCFG2_LLFTPREV |
 		     CPUCFG2_LAM;
-		/*
-		 * If LSX is supported by CPU, it is also supported by KVM,
-		 * as we implement it.
-		 */
+		/* If LSX is supported by the host, then it is also supported by KVM */
 		if (cpu_has_lsx)
 			*v |= CPUCFG2_LSX;
-		/*
-		 * if LASX is supported by CPU, it is also supported by KVM,
-		 * as we implement it.
-		 */
+		/* Same with LASX */
 		if (cpu_has_lasx)
 			*v |= CPUCFG2_LASX;
 
@@ -336,21 +330,23 @@ static int kvm_check_cpucfg(int id, u64 val)
 
 	switch (id) {
 	case 2:
-		/* CPUCFG2 features checking */
 		if (val & ~mask)
-			/* The unsupported features should not be set */
+			/* Unsupported features should not be set */
 			return -EINVAL;
 		if (!(val & CPUCFG2_LLFTP))
-			/* The LLFTP must be set, as guest must has a constant timer */
+			/* Guests must have a constant timer */
 			return -EINVAL;
 		if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
-			/* Single and double float point must both be set when enable FP */
+			/* Single and double float point must both be set when FP is enabled */
 			return -EINVAL;
 		if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
-			/* FP should be set when enable LSX */
+			/* LSX is architecturally defined to imply FP */
 			return -EINVAL;
 		if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
-			/* LSX, FP should be set when enable LASX, and FP has been checked before. */
+			/*
+			 * LASX is architecturally defined to imply LSX and FP
+			 * FP is checked just above
+			 */
 			return -EINVAL;
 		return 0;
 	default:
-- 
2.43.0

Re: [PATCH for-6.8 5/5] KVM: LoongArch: Clean up comments of _kvm_get_cpucfg_mask and kvm_check_cpucfg

[Huacai Chen]

Hi, Xuerui,

On Wed, Feb 14, 2024 at 6:16 PM WANG Xuerui <kernel@xen0n.name> wrote:
>
> From: WANG Xuerui <git@xen0n.name>
>
> Remove comments that are merely restatement of the code nearby, and
> paraphrase the rest so they read more natural for English speakers (that
> lack understanding of Chinese grammar). No functional changes.
>
> Signed-off-by: WANG Xuerui <git@xen0n.name>
> ---
>  arch/loongarch/kvm/vcpu.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 9e108ffaba30..ff51d6ba59aa 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -302,20 +302,14 @@ static int _kvm_get_cpucfg_mask(int id, u64 *v)
>  {
>         switch (id) {
>         case 2:
> -               /* Return CPUCFG2 features which have been supported by KVM */
> +               /* CPUCFG2 features unconditionally supported by KVM */
>                 *v = CPUCFG2_FP     | CPUCFG2_FPSP  | CPUCFG2_FPDP     |
>                      CPUCFG2_FPVERS | CPUCFG2_LLFTP | CPUCFG2_LLFTPREV |
>                      CPUCFG2_LAM;
> -               /*
> -                * If LSX is supported by CPU, it is also supported by KVM,
> -                * as we implement it.
> -                */
> +               /* If LSX is supported by the host, then it is also supported by KVM */
>                 if (cpu_has_lsx)
>                         *v |= CPUCFG2_LSX;
> -               /*
> -                * if LASX is supported by CPU, it is also supported by KVM,
> -                * as we implement it.
> -                */
> +               /* Same with LASX */
Consider a full description "If LASX is supported by the host, then it
is also supported by KVM"?

>                 if (cpu_has_lasx)
>                         *v |= CPUCFG2_LASX;
>
> @@ -336,21 +330,23 @@ static int kvm_check_cpucfg(int id, u64 val)
>
>         switch (id) {
>         case 2:
> -               /* CPUCFG2 features checking */
>                 if (val & ~mask)
> -                       /* The unsupported features should not be set */
> +                       /* Unsupported features should not be set */
>                         return -EINVAL;
>                 if (!(val & CPUCFG2_LLFTP))
> -                       /* The LLFTP must be set, as guest must has a constant timer */
> +                       /* Guests must have a constant timer */
>                         return -EINVAL;
>                 if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
> -                       /* Single and double float point must both be set when enable FP */
> +                       /* Single and double float point must both be set when FP is enabled */
>                         return -EINVAL;
>                 if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
> -                       /* FP should be set when enable LSX */
> +                       /* LSX is architecturally defined to imply FP */
>                         return -EINVAL;
>                 if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
> -                       /* LSX, FP should be set when enable LASX, and FP has been checked before. */
> +                       /*
> +                        * LASX is architecturally defined to imply LSX and FP
> +                        * FP is checked just above
I think "LASX is architecturally defined to imply LSX and FP" is enough here.

> +                        */
>                         return -EINVAL;
>                 return 0;
>         default:
And I prefer to squash the last two patches together.

Huacai

> --
> 2.43.0
>
>