From: WANG Xuerui <[email protected]>
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
From: WANG Xuerui <[email protected]>
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 <[email protected]>
---
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
From: WANG Xuerui <[email protected]>
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 <[email protected]>
---
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
From: WANG Xuerui <[email protected]>
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 <[email protected]>
---
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
Hi, Xuerui,
On Wed, Feb 14, 2024 at 6:16 PM WANG Xuerui <[email protected]> wrote:
>
> From: WANG Xuerui <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
Hi, Xuerui,
On Wed, Feb 14, 2024 at 6:16 PM WANG Xuerui <[email protected]> wrote:
>
> From: WANG Xuerui <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
>