Skip to contentSkip to conversation

hw/openrisc: fix masking in openrisc_pic_cpu_handler()

PATCH In review Xi Wang 2 participants 4 messages Aug 14, 2013

lore
Share & tools
lore

Xi Wang

author
#0 lore raw

PATCH v2 1/2 hw/openrisc: fix masking in openrisc_pic_cpu_handler()

Consider the masking of PICSR and PICMR:

    ((cpu->env.picsr && (1 << i)) && (cpu->env.picmr && (1 << i)))

To correctly mask bits, we should use the bitwise AND "&" rather than the logical AND "&&". Also, the loop is not necessary for masking. Simply use (cpu->env.picsr & cpu->env.picmr).

Cc Jia Liu <[email protected]>
Cc Paolo Bonzini <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 hw/openrisc/pic_cpu.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
hw/openrisc/pic_cpu.c +5 -8
index ca0b7c1..3fcee02 100644
@@ -26,7 +26,6 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
 {
     OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;
     CPUState *cs = CPU(cpu);
-    int i;
     uint32_t irq_bit = 1 << irq;
 
     if (irq > 31 || irq < 0) {

@@ -39,13 +38,11 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
         cpu->env.picsr &= ~irq_bit;
     }
 
-    for (i = 0; i < 32; i++) {
-        if ((cpu->env.picsr && (1 << i)) && (cpu->env.picmr && (1 << i))) {
-            cpu_interrupt(cs, CPU_INTERRUPT_HARD);
-        } else {
-            cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
-            cpu->env.picsr &= ~(1 << i);
-        }
+    if (cpu->env.picsr & cpu->env.picmr) {
+        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+    } else {
+        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
+        cpu->env.picsr = 0;
     }
 }
Signature · 1 line
1.8.1.2
author
#1 lore raw

PATCH v2 2/2 hw/openrisc: avoid undefined shift in openrisc_pic_cpu_handler()

In C99 signed shift (1 << 31) is undefined behavior, since the result exceeds INT_MAX. Use 1U instead and move the shift after the check.

Cc Jia Liu <[email protected]>
Cc Paolo Bonzini <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 hw/openrisc/pic_cpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
hw/openrisc/pic_cpu.c +3 -1
index 3fcee02..2af1d60 100644
@@ -26,12 +26,14 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
 {
     OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;
     CPUState *cs = CPU(cpu);
-    uint32_t irq_bit = 1 << irq;
+    uint32_t irq_bit;
 
     if (irq > 31 || irq < 0) {
         return;
     }
 
+    irq_bit = 1U << irq;
+
     if (level) {
         cpu->env.picsr |= irq_bit;
     } else {
Signature · 1 line
1.8.1.2
Acked-by on v2 2/2
#2 lore raw

Re: [Qemu-devel] [PATCH v2 2/2] hw/openrisc: avoid undefined shift in openrisc_pic_cpu_handler()

Hi Xi,

Quoted 17 lines · Xi Wang

In C99 signed shift (1 << 31) is undefined behavior, since the result exceeds INT_MAX. Use 1U instead and move the shift after the check.

Cc Jia Liu <[email protected]>
Cc Paolo Bonzini <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 hw/openrisc/pic_cpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
hw/openrisc/pic_cpu.c +0 -0
index 3fcee02..2af1d60 100644
@@ -26,12 +26,14 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
 {
     OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;

hw/openrisc/pic_cpu.c static void openrisc_pic_cpu_handler(void *opaque, int irq, int level) · line 36

Quoted 10 lines
hw/openrisc/pic_cpu.c +3 -1
     CPUState *cs = CPU(cpu);
-    uint32_t irq_bit = 1 << irq;
+    uint32_t irq_bit;
 
     if (irq > 31 || irq < 0) {
         return;
     }
 
+    irq_bit = 1U << irq;
+

Thanks for making the code better.

Acked-by Jia Liu <[email protected]>
Quoted 6 lines
hw/openrisc/pic_cpu.c +0 -1
     if (level) {
         cpu->env.picsr |= irq_bit;
     } else {
--

1.8.1.2

Regards,
Jia

Acked-by on v2 1/2
#3 lore raw
In reply to Xi Wang

Hi Xi,

You should use git format-patch with "-s" and "--cover-letter" next time, and manually edit cover letter for a brief description of your patch set. More info. at http://qemu-project.org/Contribute/SubmitAPatch .

Quoted 35 lines · Xi Wang

Consider the masking of PICSR and PICMR:

    ((cpu->env.picsr && (1 << i)) && (cpu->env.picmr && (1 << i)))

To correctly mask bits, we should use the bitwise AND "&" rather than the logical AND "&&". Also, the loop is not necessary for masking. Simply use (cpu->env.picsr & cpu->env.picmr).

Cc Jia Liu <[email protected]>
Cc Paolo Bonzini <[email protected]>
Signed-off-by Xi Wang <[email protected]>
---
 hw/openrisc/pic_cpu.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
hw/openrisc/pic_cpu.c +0 -5
index ca0b7c1..3fcee02 100644
@@ -26,7 +26,6 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
 {
     OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;
     CPUState *cs = CPU(cpu);
-    int i;
     uint32_t irq_bit = 1 << irq;
 
     if (irq > 31 || irq < 0) {

@@ -39,13 +38,11 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
         cpu->env.picsr &= ~irq_bit;
     }
 
-    for (i = 0; i < 32; i++) {
-        if ((cpu->env.picsr && (1 << i)) && (cpu->env.picmr && (1 << i))) {
-            cpu_interrupt(cs, CPU_INTERRUPT_HARD);
-        } else {

hw/openrisc/pic_cpu.c static void openrisc_pic_cpu_handler(void *opaque, int irq, int level) · line 47

Quoted 10 lines
hw/openrisc/pic_cpu.c +5 -3
-            cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
-            cpu->env.picsr &= ~(1 << i);
-        }
+    if (cpu->env.picsr & cpu->env.picmr) {
+        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+    } else {
+        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
+        cpu->env.picsr = 0;
     }
 }

Thank you very much for fixing this bug.

Acked-by Jia Liu <[email protected]>
Quoted 4 lines
hw/openrisc/pic_cpu.c +0 -1
--

1.8.1.2

Regards,
Jia.