[PATCH v3 0/5] Fix LoongArch coverity error and cpu name bug

[PATCH v3 0/5] Fix LoongArch coverity error and cpu name bug

[Xiaojuan Yang]

This series fix some coverity errors and loongarch_cpu_class_by_name function
for LoongArch virt machine.

Only the loongarch_pch_pic patch(number 2/5) need to be reviewed in this v3
verison, and other patches have been reviewed.

Changes for v3:

1. In loongarch_pch_pic file, We should not use 'unsigned long'
   type as argument when we use find_first_bit(), and we use
   ctz64() to replace find_first_bit() to fix this bug.
2. It is not standard to use '1ULL << irq' to generate a irq mask.
   So, we replace it with 'MAKE_64BIT_MASK(irq, 1)'.
3. Rewrite commit comments for op_helper patch(number 5/5).

Changes for v2:

1. Use MAKE_64BIT_MASK(shift, len) to replace 'xxx << shift'.
2. Use ARRAY_SIZE(arrqy) to get the array size.
3. Add the assertion that 'cpu_model' resolve to a class of the 
   appropriate type.


Changes for v1:

1. Fix coverity errors such as out-of-bounds, integer overflow,
   cond_at_most, etc.
2. Fix loongarch_cpu_class_by_name function.


Please help review
Thanks.

Xiaojuan Yang (5):
  target/loongarch/cpu: Fix cpu_class_by_name function
  hw/intc/loongarch_pch_pic: Fix bugs for update_irq function
  target/loongarch/cpu: Fix coverity errors about excp_names
  target/loongarch/tlb_helper: Fix coverity integer overflow error
  target/loongarch/op_helper: Fix coverity cond_at_most error

 hw/intc/loongarch_pch_pic.c   | 18 +++++++++++-------
 target/loongarch/cpu.c        | 15 ++++++++-------
 target/loongarch/op_helper.c  |  2 +-
 target/loongarch/tlb_helper.c |  4 ++--
 4 files changed, 22 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH v3 1/5] target/loongarch/cpu: Fix cpu_class_by_name function

[Xiaojuan Yang]

In loongarch_cpu_class_by_name(char *cpu_model) function,
the argument cpu_model already has the suffix '-loongarch-cpu',
so we should remove the LOONGARCH_CPU_TYPE_NAME(cpu_model) macro.
And add the assertion that 'cpu_model' resolves to a class of the
appropriate type.

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/loongarch/cpu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index e21715592a..ed26f9beed 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -571,11 +571,12 @@ static void loongarch_cpu_init(Object *obj)
 static ObjectClass *loongarch_cpu_class_by_name(const char *cpu_model)
 {
     ObjectClass *oc;
-    char *typename;
 
-    typename = g_strdup_printf(LOONGARCH_CPU_TYPE_NAME("%s"), cpu_model);
-    oc = object_class_by_name(typename);
-    g_free(typename);
+    oc = object_class_by_name(cpu_model);
+    if (!oc || !object_class_dynamic_cast(oc, TYPE_LOONGARCH_CPU) ||
+        object_class_is_abstract(oc)) {
+        return NULL;
+    }
     return oc;
 }
 
-- 
2.31.1

Re: [PATCH v3 1/5] target/loongarch/cpu: Fix cpu_class_by_name function

[Richard Henderson]

On 7/15/22 11:37, Xiaojuan Yang wrote:
> In loongarch_cpu_class_by_name(char *cpu_model) function,
> the argument cpu_model already has the suffix '-loongarch-cpu',
> so we should remove the LOONGARCH_CPU_TYPE_NAME(cpu_model) macro.
> And add the assertion that 'cpu_model' resolves to a class of the
> appropriate type.
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

This patch causes tests to fail, e.g.

   TEST    float_convd on loongarch64

qemu-loongarch64: unable to find CPU model 'la464'

make[1]: *** [/home/rth/qemu/src/tests/tcg/multiarch/Makefile.target:29: run-float_convd] 
Error 1


What caused you assume that all cpu models are already suffixed?


r~

Re: [PATCH v3 1/5] target/loongarch/cpu: Fix cpu_class_by_name function

[Richard Henderson]

On 7/19/22 12:16, Richard Henderson wrote:
> On 7/15/22 11:37, Xiaojuan Yang wrote:
>> In loongarch_cpu_class_by_name(char *cpu_model) function,
>> the argument cpu_model already has the suffix '-loongarch-cpu',
>> so we should remove the LOONGARCH_CPU_TYPE_NAME(cpu_model) macro.
>> And add the assertion that 'cpu_model' resolves to a class of the
>> appropriate type.
>>
>> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> This patch causes tests to fail, e.g.
> 
>    TEST    float_convd on loongarch64
> 
> qemu-loongarch64: unable to find CPU model 'la464'
> 
> make[1]: *** [/home/rth/qemu/src/tests/tcg/multiarch/Makefile.target:29: run-float_convd] 
> Error 1
> 
> 
> What caused you assume that all cpu models are already suffixed?

Mm.  I suppose the use over in hw/loongarch/loongson3.c.
I will make this function match target/alpha/cpu.c, which checks cpu_model as-is, and then 
tries again with the suffix.


r~

Re: [PATCH v3 1/5] target/loongarch/cpu: Fix cpu_class_by_name function

[Igor Mammedov]

On Tue, 19 Jul 2022 12:22:14 +0530
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 7/19/22 12:16, Richard Henderson wrote:
> > On 7/15/22 11:37, Xiaojuan Yang wrote:  
> >> In loongarch_cpu_class_by_name(char *cpu_model) function,
> >> the argument cpu_model already has the suffix '-loongarch-cpu',
> >> so we should remove the LOONGARCH_CPU_TYPE_NAME(cpu_model) macro.
> >> And add the assertion that 'cpu_model' resolves to a class of the
> >> appropriate type.
> >>
> >> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>  
> > 
> > This patch causes tests to fail, e.g.
> > 
> >    TEST    float_convd on loongarch64
> > 
> > qemu-loongarch64: unable to find CPU model 'la464'
> > 
> > make[1]: *** [/home/rth/qemu/src/tests/tcg/multiarch/Makefile.target:29: run-float_convd] 
> > Error 1
> > 
> > 
> > What caused you assume that all cpu models are already suffixed?  
> 
> Mm.  I suppose the use over in hw/loongarch/loongson3.c.
> I will make this function match target/alpha/cpu.c, which checks cpu_model as-is, and then 
> tries again with the suffix.

if we think about adding hotplug/qmp/-device support for CPUs in future,
those interface operate with type-names directly without any pre-processing whatsoever.

so I'd very much prefer just as-is (i.e. direct type-name matching)
(all other model parsing usually comes from legacy code)

> 
> 
> r~
> 

[PATCH v3 2/5] hw/intc/loongarch_pch_pic: Fix bugs for update_irq function

[Xiaojuan Yang]

Fix such errors:
1. We should not use 'unsigned long' type as argument when we use
find_first_bit(), and we use ctz64() to replace find_first_bit()
to fix this bug.
2. It is not standard to use '1ULL << irq' to generate a irq mask.
So, we replace it with 'MAKE_64BIT_MASK(irq, 1)'.

Fix coverity CID: 1489761 1489764 1489765

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 hw/intc/loongarch_pch_pic.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
index 3c9814a3b4..8fa64d2030 100644
--- a/hw/intc/loongarch_pch_pic.c
+++ b/hw/intc/loongarch_pch_pic.c
@@ -15,22 +15,26 @@
 
 static void pch_pic_update_irq(LoongArchPCHPIC *s, uint64_t mask, int level)
 {
-    unsigned long val;
+    uint64_t val;
     int irq;
 
     if (level) {
         val = mask & s->intirr & ~s->int_mask;
         if (val) {
-            irq = find_first_bit(&val, 64);
-            s->intisr |= 0x1ULL << irq;
-            qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 1);
+            irq = ctz64(val);
+            if (irq < 64) {
+                s->intisr |= MAKE_64BIT_MASK(irq, 1);
+                qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 1);
+            }
         }
     } else {
         val = mask & s->intisr;
         if (val) {
-            irq = find_first_bit(&val, 64);
-            s->intisr &= ~(0x1ULL << irq);
-            qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 0);
+            irq = ctz64(val);
+            if (irq < 64) {
+                s->intisr &= ~(MAKE_64BIT_MASK(irq, 1));
+                qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 0);
+            }
         }
     }
 }
-- 
2.31.1

Re: [PATCH v3 2/5] hw/intc/loongarch_pch_pic: Fix bugs for update_irq function

[Richard Henderson]

On 7/15/22 11:37, Xiaojuan Yang wrote:
> Fix such errors:
> 1. We should not use 'unsigned long' type as argument when we use
> find_first_bit(), and we use ctz64() to replace find_first_bit()
> to fix this bug.
> 2. It is not standard to use '1ULL << irq' to generate a irq mask.
> So, we replace it with 'MAKE_64BIT_MASK(irq, 1)'.
> 
> Fix coverity CID: 1489761 1489764 1489765
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   hw/intc/loongarch_pch_pic.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
> index 3c9814a3b4..8fa64d2030 100644
> --- a/hw/intc/loongarch_pch_pic.c
> +++ b/hw/intc/loongarch_pch_pic.c
> @@ -15,22 +15,26 @@
>   
>   static void pch_pic_update_irq(LoongArchPCHPIC *s, uint64_t mask, int level)
>   {
> -    unsigned long val;
> +    uint64_t val;
>       int irq;
>   
>       if (level) {
>           val = mask & s->intirr & ~s->int_mask;
>           if (val) {
> -            irq = find_first_bit(&val, 64);
> -            s->intisr |= 0x1ULL << irq;
> -            qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 1);
> +            irq = ctz64(val);
> +            if (irq < 64) {

This test is always true, provable by the val != 0 test just above.


r~

> +                s->intisr |= MAKE_64BIT_MASK(irq, 1);
> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 1);
> +            }
>           }
>       } else {
>           val = mask & s->intisr;
>           if (val) {
> -            irq = find_first_bit(&val, 64);
> -            s->intisr &= ~(0x1ULL << irq);
> -            qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 0);
> +            irq = ctz64(val);
> +            if (irq < 64) {
> +                s->intisr &= ~(MAKE_64BIT_MASK(irq, 1));
> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 0);
> +            }
>           }
>       }
>   }

[PATCH v3 3/5] target/loongarch/cpu: Fix coverity errors about excp_names

[Xiaojuan Yang]

Fix out-of-bounds errors when access excp_names[] array. the valid
boundary size of excp_names should be 0 to ARRAY_SIZE(excp_names)-1.
However, the general code do not consider the max boundary.

Fix coverity CID: 1489758

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/loongarch/cpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index ed26f9beed..89ea971cde 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -140,7 +140,7 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
 
     if (cs->exception_index != EXCCODE_INT) {
         if (cs->exception_index < 0 ||
-            cs->exception_index > ARRAY_SIZE(excp_names)) {
+            cs->exception_index >= ARRAY_SIZE(excp_names)) {
             name = "unknown";
         } else {
             name = excp_names[cs->exception_index];
@@ -190,8 +190,8 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
         cause = cs->exception_index;
         break;
     default:
-        qemu_log("Error: exception(%d) '%s' has not been supported\n",
-                 cs->exception_index, excp_names[cs->exception_index]);
+        qemu_log("Error: exception(%d) has not been supported\n",
+                 cs->exception_index);
         abort();
     }
 
-- 
2.31.1

[PATCH v3 4/5] target/loongarch/tlb_helper: Fix coverity integer overflow error

[Xiaojuan Yang]

Replace '1 << shift' with 'MAKE_64BIT_MASK(shift, 1)' to fix
unintentional integer overflow errors in tlb_helper file.

Fix coverity CID: 1489759 1489762

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/loongarch/tlb_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/loongarch/tlb_helper.c b/target/loongarch/tlb_helper.c
index bab19c7e05..610b6d123c 100644
--- a/target/loongarch/tlb_helper.c
+++ b/target/loongarch/tlb_helper.c
@@ -298,7 +298,7 @@ static void invalidate_tlb_entry(CPULoongArchState *env, int index)
     } else {
         tlb_ps = FIELD_EX64(env->CSR_STLBPS, CSR_STLBPS, PS);
     }
-    pagesize = 1 << tlb_ps;
+    pagesize = MAKE_64BIT_MASK(tlb_ps, 1);
     mask = MAKE_64BIT_MASK(0, tlb_ps + 1);
 
     if (tlb_v0) {
@@ -736,7 +736,7 @@ void helper_ldpte(CPULoongArchState *env, target_ulong base, target_ulong odd,
                 (tmp0 & (~(1 << R_TLBENTRY_G_SHIFT)));
         ps = ptbase + ptwidth - 1;
         if (odd) {
-            tmp0 += (1 << ps);
+            tmp0 += MAKE_64BIT_MASK(ps, 1);
         }
     } else {
         /* 0:64bit, 1:128bit, 2:192bit, 3:256bit */
-- 
2.31.1

[PATCH v3 5/5] target/loongarch/op_helper: Fix coverity cond_at_most error

[Xiaojuan Yang]

The boundary size of cpucfg array should be 0 to ARRAY_SIZE(cpucfg)-1.
So, using index bigger than max boundary to access cpucfg[] must be
forbidden.

Fix coverity CID: 1489760

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/loongarch/op_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/loongarch/op_helper.c b/target/loongarch/op_helper.c
index 4b429b6699..568c071601 100644
--- a/target/loongarch/op_helper.c
+++ b/target/loongarch/op_helper.c
@@ -81,7 +81,7 @@ target_ulong helper_crc32c(target_ulong val, target_ulong m, uint64_t sz)
 
 target_ulong helper_cpucfg(CPULoongArchState *env, target_ulong rj)
 {
-    return rj > 21 ? 0 : env->cpucfg[rj];
+    return rj >= ARRAY_SIZE(env->cpucfg) ? 0 : env->cpucfg[rj];
 }
 
 uint64_t helper_rdtime_d(CPULoongArchState *env)
-- 
2.31.1

Re: [PATCH v3 0/5] Fix LoongArch coverity error and cpu name bug

[Richard Henderson]

On 7/15/22 11:37, Xiaojuan Yang wrote:
> This series fix some coverity errors and loongarch_cpu_class_by_name function
> for LoongArch virt machine.
> 
> Only the loongarch_pch_pic patch(number 2/5) need to be reviewed in this v3
> verison, and other patches have been reviewed.
> 
> Changes for v3:
> 
> 1. In loongarch_pch_pic file, We should not use 'unsigned long'
>     type as argument when we use find_first_bit(), and we use
>     ctz64() to replace find_first_bit() to fix this bug.
> 2. It is not standard to use '1ULL << irq' to generate a irq mask.
>     So, we replace it with 'MAKE_64BIT_MASK(irq, 1)'.
> 3. Rewrite commit comments for op_helper patch(number 5/5).

Queued all of these, with fixes as described for patches 1 and 2.


r~

> 
> Changes for v2:
> 
> 1. Use MAKE_64BIT_MASK(shift, len) to replace 'xxx << shift'.
> 2. Use ARRAY_SIZE(arrqy) to get the array size.
> 3. Add the assertion that 'cpu_model' resolve to a class of the
>     appropriate type.
> 
> 
> Changes for v1:
> 
> 1. Fix coverity errors such as out-of-bounds, integer overflow,
>     cond_at_most, etc.
> 2. Fix loongarch_cpu_class_by_name function.
> 
> 
> Please help review
> Thanks.
> 
> Xiaojuan Yang (5):
>    target/loongarch/cpu: Fix cpu_class_by_name function
>    hw/intc/loongarch_pch_pic: Fix bugs for update_irq function
>    target/loongarch/cpu: Fix coverity errors about excp_names
>    target/loongarch/tlb_helper: Fix coverity integer overflow error
>    target/loongarch/op_helper: Fix coverity cond_at_most error
> 
>   hw/intc/loongarch_pch_pic.c   | 18 +++++++++++-------
>   target/loongarch/cpu.c        | 15 ++++++++-------
>   target/loongarch/op_helper.c  |  2 +-
>   target/loongarch/tlb_helper.c |  4 ++--
>   4 files changed, 22 insertions(+), 17 deletions(-)
>