Re: [Qemu-devel] [PATCH] target-or32: fix masking in openrisc_pic_cpu_handler()

Re: [Qemu-devel] [PATCH] target-or32: fix masking in openrisc_pic_cpu_handler()

[Paolo Bonzini]

Il 22/01/2013 16:57, Xi Wang ha scritto:
> A correct mask should be `x & (1 << i)', rather than `x && (1 << i)'.
> 
> Also, in C99 signed shift (1 << 31) is undefined behavior, since the
> result exceeds INT_MAX; use 1U instead.
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  hw/openrisc_pic.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/openrisc_pic.c b/hw/openrisc_pic.c
> index aaeb9a9..4f6d5a0 100644
> --- a/hw/openrisc_pic.c
> +++ b/hw/openrisc_pic.c
> @@ -26,12 +26,14 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
>  {
>      OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;
>      int i;
> -    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 {
> @@ -39,11 +41,11 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
>      }
>  
>      for (i = 0; i < 32; i++) {
> -        if ((cpu->env.picsr && (1 << i)) && (cpu->env.picmr && (1 << i))) {
> +        if ((cpu->env.picsr & (1U << i)) && (cpu->env.picmr & (1U << i))) {
>              cpu_interrupt(&cpu->env, CPU_INTERRUPT_HARD);
>          } else {
>              cpu_reset_interrupt(&cpu->env, CPU_INTERRUPT_HARD);
> -            cpu->env.picsr &= ~(1 << i);
> +            cpu->env.picsr &= ~(1U << i);
>          }
>      }
>  }
> 

Ping.

Paolo

Re: [Qemu-devel] [PATCH] target-or32: fix masking in openrisc_pic_cpu_handler()

[Jia Liu]

Hi Xi,

On Tue, Jan 22, 2013 at 11:57 PM, Xi Wang <xi.wang@gmail.com> wrote:
> A correct mask should be `x & (1 << i)', rather than `x && (1 << i)'.
>
> Also, in C99 signed shift (1 << 31) is undefined behavior, since the
> result exceeds INT_MAX; use 1U instead.
>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  hw/openrisc_pic.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/openrisc_pic.c b/hw/openrisc_pic.c
> index aaeb9a9..4f6d5a0 100644
> --- a/hw/openrisc_pic.c
> +++ b/hw/openrisc_pic.c
> @@ -26,12 +26,14 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
>  {
>      OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;
>      int i;
> -    uint32_t irq_bit = 1 << irq;
> +    uint32_t irq_bit;
>
>      if (irq > 31 || irq < 0) {
>          return;
>      }
>
> +    irq_bit = 1U << irq;
> +

Thanks, this part is OK.

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

May you please test it? Your change here make qemu-system-or32 can not
boot Linux.
You can download pre-compiled Linux form here:
https://docs.google.com/file/d/0BxeTrz3x0CBLbTNmU0lrV1Y0V0U/edit?usp=sharing
or build one yourself.

>          }
>      }
>  }
> --
> 1.7.10.4
>

At last, openrisc_pic.c is renamed into pic_cpu.c, please update your patch.

Regards,
Jia.