[Qemu-devel] [PATCH 0/8] misc fixes for coverity warnings in ARM devices

[Qemu-devel] [PATCH 0/8] misc fixes for coverity warnings in ARM devices

[Peter Maydell]

This patchset is a mixed bag of minor fixes for various
Coverity warnings in ARM devices.

Peter Maydell (8):
  hw/intc/allwinner-a10-pic: Add missing 'break'
  hw/net/cadence_gem: Remove dead code
  hw/display/pxa2xx_lcd: Fix 16bpp+alpha and 18bpp+alpha palette formats
  hw/arm/omap1: Avoid unintended sign extension writing omap_rtc
    YEARS_REG
  hw/dma/omap_dma: Add (uint32_t) casts when shifting uint16_t by 16
  hw/timer/exynos4210_mct: Avoid overflow in
    exynos4210_ltick_recalc_count
  hw/arm/stellaris: Correct handling of GPTM TAR register
  hw/arm/omap_gpmc: Avoid buffer overrun filling prefetch FIFO

 hw/arm/omap1.c              |  4 ++--
 hw/arm/stellaris.c          | 13 ++++++++++---
 hw/display/pxa2xx_lcd.c     | 14 +++++++-------
 hw/dma/omap_dma.c           | 12 ++++++------
 hw/intc/allwinner-a10-pic.c |  1 +
 hw/misc/omap_gpmc.c         |  4 ++++
 hw/net/cadence_gem.c        |  8 +-------
 hw/timer/exynos4210_mct.c   |  4 ++--
 8 files changed, 33 insertions(+), 27 deletions(-)

-- 
1.9.2

[Qemu-devel] [PATCH 1/8] hw/intc/allwinner-a10-pic: Add missing 'break'

[Peter Maydell]

Add missing 'break' after handling of AW_A10_PIC_BASE_ADDR write.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/allwinner-a10-pic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
index 0924d98..7b3e085 100644
--- a/hw/intc/allwinner-a10-pic.c
+++ b/hw/intc/allwinner-a10-pic.c
@@ -97,6 +97,7 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
     switch (offset) {
     case AW_A10_PIC_BASE_ADDR:
         s->base_addr = value & ~0x3;
+        break;
     case AW_A10_PIC_PROTECT:
         s->protect = value;
         break;
-- 
1.9.2

Re: [Qemu-devel] [PATCH 1/8] hw/intc/allwinner-a10-pic: Add missing 'break'

[Peter Crosthwaite]

On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Add missing 'break' after handling of AW_A10_PIC_BASE_ADDR write.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/intc/allwinner-a10-pic.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 0924d98..7b3e085 100644
> --- a/hw/intc/allwinner-a10-pic.c
> +++ b/hw/intc/allwinner-a10-pic.c
> @@ -97,6 +97,7 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
>      switch (offset) {
>      case AW_A10_PIC_BASE_ADDR:
>          s->base_addr = value & ~0x3;
> +        break;
>      case AW_A10_PIC_PROTECT:
>          s->protect = value;
>          break;
> --
> 1.9.2
>
>

[Qemu-devel] [PATCH 2/8] hw/net/cadence_gem: Remove dead code

[Peter Maydell]

Commit 191946c moved the code to handle padding to minimum
length from after the handling of the CRC to before it.
This means that the CRC code doesn't need to cope with the
possibility that the size is less than 60; remove this
dead code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/cadence_gem.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index cdb1825..afddc8a 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -717,7 +717,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         rxbuf_ptr = (void *)buf;
     } else {
         unsigned crc_val;
-        int      crc_offset;
 
         /* The application wants the FCS field, which QEMU does not provide.
          * We must try and caclculate one.
@@ -727,12 +726,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         memset(rxbuf + size, 0, sizeof(rxbuf) - size);
         rxbuf_ptr = rxbuf;
         crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
-        if (size < 60) {
-            crc_offset = 60;
-        } else {
-            crc_offset = size;
-        }
-        memcpy(rxbuf + crc_offset, &crc_val, sizeof(crc_val));
+        memcpy(rxbuf + size, &crc_val, sizeof(crc_val));
 
         bytes_to_copy += 4;
         size += 4;
-- 
1.9.2

Re: [Qemu-devel] [PATCH 2/8] hw/net/cadence_gem: Remove dead code

[Peter Crosthwaite]

On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Commit 191946c moved the code to handle padding to minimum
> length from after the handling of the CRC to before it.
> This means that the CRC code doesn't need to cope with the
> possibility that the size is less than 60; remove this
> dead code.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/net/cadence_gem.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index cdb1825..afddc8a 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -717,7 +717,6 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          rxbuf_ptr = (void *)buf;
>      } else {
>          unsigned crc_val;
> -        int      crc_offset;
>
>          /* The application wants the FCS field, which QEMU does not provide.
>           * We must try and caclculate one.
> @@ -727,12 +726,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          memset(rxbuf + size, 0, sizeof(rxbuf) - size);
>          rxbuf_ptr = rxbuf;
>          crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
> -        if (size < 60) {
> -            crc_offset = 60;
> -        } else {
> -            crc_offset = size;
> -        }
> -        memcpy(rxbuf + crc_offset, &crc_val, sizeof(crc_val));
> +        memcpy(rxbuf + size, &crc_val, sizeof(crc_val));
>
>          bytes_to_copy += 4;
>          size += 4;
> --
> 1.9.2
>
>

[Qemu-devel] [PATCH 3/8] hw/display/pxa2xx_lcd: Fix 16bpp+alpha and 18bpp+alpha palette formats

[Peter Maydell]

The pxa2xx palette entry "16bpp plus transparency" format is
xxxxxxxTRRRRR000GGGGGG00BBBBB000, and "18bpp plus transparency" is
xxxxxxxTRRRRRR00GGGGGG00BBBBBB00.

Correct errors in the code for reading these and converting
them to the internal format. In particular, the buggy code
was attempting to mask out bit 24 of a uint16_t, which
Coverity spotted as an error.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/pxa2xx_lcd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
index 09cdf17..fce013d 100644
--- a/hw/display/pxa2xx_lcd.c
+++ b/hw/display/pxa2xx_lcd.c
@@ -620,13 +620,13 @@ static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
             src += 2;
             break;
         case 1: /* 16 bpp plus transparency */
-            alpha = *(uint16_t *) src & (1 << 24);
+            alpha = *(uint32_t *) src & (1 << 24);
             if (s->control[0] & LCCR0_CMS)
-                r = g = b = *(uint16_t *) src & 0xff;
+                r = g = b = *(uint32_t *) src & 0xff;
             else {
-                r = (*(uint16_t *) src & 0xf800) >> 8;
-                g = (*(uint16_t *) src & 0x07e0) >> 3;
-                b = (*(uint16_t *) src & 0x001f) << 3;
+                r = (*(uint32_t *) src & 0x7c0000) >> 15;
+                g = (*(uint32_t *) src & 0x00fc00) >> 8;
+                b = (*(uint32_t *) src & 0x0000f8);
             }
             src += 2;
             break;
@@ -635,9 +635,9 @@ static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
             if (s->control[0] & LCCR0_CMS)
                 r = g = b = *(uint32_t *) src & 0xff;
             else {
-                r = (*(uint32_t *) src & 0xf80000) >> 16;
+                r = (*(uint32_t *) src & 0xfc0000) >> 16;
                 g = (*(uint32_t *) src & 0x00fc00) >> 8;
-                b = (*(uint32_t *) src & 0x0000f8);
+                b = (*(uint32_t *) src & 0x0000fc);
             }
             src += 4;
             break;
-- 
1.9.2

Re: [Qemu-devel] [PATCH 3/8] hw/display/pxa2xx_lcd: Fix 16bpp+alpha and 18bpp+alpha palette formats

[Peter Crosthwaite]

On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The pxa2xx palette entry "16bpp plus transparency" format is
> xxxxxxxTRRRRR000GGGGGG00BBBBB000, and "18bpp plus transparency" is
> xxxxxxxTRRRRRR00GGGGGG00BBBBBB00.
>
> Correct errors in the code for reading these and converting
> them to the internal format. In particular, the buggy code
> was attempting to mask out bit 24 of a uint16_t, which
> Coverity spotted as an error.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/display/pxa2xx_lcd.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
> index 09cdf17..fce013d 100644
> --- a/hw/display/pxa2xx_lcd.c
> +++ b/hw/display/pxa2xx_lcd.c
> @@ -620,13 +620,13 @@ static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
>              src += 2;
>              break;
>          case 1: /* 16 bpp plus transparency */
> -            alpha = *(uint16_t *) src & (1 << 24);
> +            alpha = *(uint32_t *) src & (1 << 24);
>              if (s->control[0] & LCCR0_CMS)
> -                r = g = b = *(uint16_t *) src & 0xff;
> +                r = g = b = *(uint32_t *) src & 0xff;
>              else {
> -                r = (*(uint16_t *) src & 0xf800) >> 8;
> -                g = (*(uint16_t *) src & 0x07e0) >> 3;
> -                b = (*(uint16_t *) src & 0x001f) << 3;
> +                r = (*(uint32_t *) src & 0x7c0000) >> 15;

16BPP format (pasted from above with byte spacing)
xxxxxxxT RRRRR000 GGGGGG00 BBBBB000

So shouldn't r be 0xf80000 >> 16?

Regards,
Peter

> +                g = (*(uint32_t *) src & 0x00fc00) >> 8;
> +                b = (*(uint32_t *) src & 0x0000f8);
>              }
>              src += 2;
>              break;
> @@ -635,9 +635,9 @@ static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
>              if (s->control[0] & LCCR0_CMS)
>                  r = g = b = *(uint32_t *) src & 0xff;
>              else {
> -                r = (*(uint32_t *) src & 0xf80000) >> 16;
> +                r = (*(uint32_t *) src & 0xfc0000) >> 16;
>                  g = (*(uint32_t *) src & 0x00fc00) >> 8;
> -                b = (*(uint32_t *) src & 0x0000f8);
> +                b = (*(uint32_t *) src & 0x0000fc);
>              }
>              src += 4;
>              break;
> --
> 1.9.2
>
>

Re: [Qemu-devel] [PATCH 3/8] hw/display/pxa2xx_lcd: Fix 16bpp+alpha and 18bpp+alpha palette formats

[Peter Maydell]

On 10 May 2014 00:35, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The pxa2xx palette entry "16bpp plus transparency" format is
>> xxxxxxxTRRRRR000GGGGGG00BBBBB000, and "18bpp plus transparency" is
>> xxxxxxxTRRRRRR00GGGGGG00BBBBBB00.
>>
>> Correct errors in the code for reading these and converting
>> them to the internal format. In particular, the buggy code
>> was attempting to mask out bit 24 of a uint16_t, which
>> Coverity spotted as an error.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/display/pxa2xx_lcd.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
>> index 09cdf17..fce013d 100644
>> --- a/hw/display/pxa2xx_lcd.c
>> +++ b/hw/display/pxa2xx_lcd.c
>> @@ -620,13 +620,13 @@ static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
>>              src += 2;
>>              break;
>>          case 1: /* 16 bpp plus transparency */
>> -            alpha = *(uint16_t *) src & (1 << 24);
>> +            alpha = *(uint32_t *) src & (1 << 24);
>>              if (s->control[0] & LCCR0_CMS)
>> -                r = g = b = *(uint16_t *) src & 0xff;
>> +                r = g = b = *(uint32_t *) src & 0xff;
>>              else {
>> -                r = (*(uint16_t *) src & 0xf800) >> 8;
>> -                g = (*(uint16_t *) src & 0x07e0) >> 3;
>> -                b = (*(uint16_t *) src & 0x001f) << 3;
>> +                r = (*(uint32_t *) src & 0x7c0000) >> 15;
>
> 16BPP format (pasted from above with byte spacing)
> xxxxxxxT RRRRR000 GGGGGG00 BBBBB000
>
> So shouldn't r be 0xf80000 >> 16?

Yep, you're right. My first attempt at this I read the wrong
diagram from the manual (the framebuffer pixel formats are
different from the palette formats) and failed to spot I
hadn't corrected the code completely.

Since this is the only patch in this set that needs a
respin, and they're all independent fixes, I'll put the
other 7 into target-arm.next and just resend this.

thanks
-- PMM

[Qemu-devel] [PATCH 4/8] hw/arm/omap1: Avoid unintended sign extension writing omap_rtc YEARS_REG

[Peter Maydell]

When writing to the YEARS_REG register, if the year value is
99 then the multiplication by 31536000 will overflow into
the sign bit of a 32 bit value and then be erroneously
sign-extended if time_t is 64 bits. Add a cast to avoid this.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/omap1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index b433748..b28e052 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -2709,8 +2709,8 @@ static void omap_rtc_write(void *opaque, hwaddr addr,
             s->ti += ti[1];
         } else {
             /* A less accurate version */
-            s->ti -= (s->current_tm.tm_year % 100) * 31536000;
-            s->ti += from_bcd(value) * 31536000;
+            s->ti -= (time_t)(s->current_tm.tm_year % 100) * 31536000;
+            s->ti += (time_t)from_bcd(value) * 31536000;
         }
         return;
 
-- 
1.9.2

Re: [Qemu-devel] [PATCH 4/8] hw/arm/omap1: Avoid unintended sign extension writing omap_rtc YEARS_REG

[Peter Crosthwaite]

On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> When writing to the YEARS_REG register, if the year value is
> 99 then the multiplication by 31536000 will overflow into
> the sign bit of a 32 bit value and then be erroneously
> sign-extended if time_t is 64 bits. Add a cast to avoid this.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/arm/omap1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
> index b433748..b28e052 100644
> --- a/hw/arm/omap1.c
> +++ b/hw/arm/omap1.c
> @@ -2709,8 +2709,8 @@ static void omap_rtc_write(void *opaque, hwaddr addr,
>              s->ti += ti[1];
>          } else {
>              /* A less accurate version */
> -            s->ti -= (s->current_tm.tm_year % 100) * 31536000;
> -            s->ti += from_bcd(value) * 31536000;
> +            s->ti -= (time_t)(s->current_tm.tm_year % 100) * 31536000;
> +            s->ti += (time_t)from_bcd(value) * 31536000;
>          }
>          return;
>
> --
> 1.9.2
>
>

[Qemu-devel] [PATCH 5/8] hw/dma/omap_dma: Add (uint32_t) casts when shifting uint16_t by 16

[Peter Maydell]

Add missing (uint32_t) casts in cases where we're trying to
put a uint16_t value into the top half of a 32-bit field.
These were already present in some but not all places.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
For new code or code I cared about I'd use deposit32(); but omap
is pretty ancient and unloved, so this is the minimal fix.
---
 hw/dma/omap_dma.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/dma/omap_dma.c b/hw/dma/omap_dma.c
index 0e8cccd..0f35c42 100644
--- a/hw/dma/omap_dma.c
+++ b/hw/dma/omap_dma.c
@@ -973,7 +973,7 @@ static int omap_dma_ch_reg_write(struct omap_dma_s *s,
 
     case 0x22:	/* DMA_COLOR_U */
         ch->color &= 0xffff;
-        ch->color |= value << 16;
+        ch->color |= (uint32_t)value << 16;
         break;
 
     case 0x24:	/* DMA_CCR2 */
@@ -1043,7 +1043,7 @@ static int omap_dma_3_2_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
 
     case 0xbca:	/* TOP_B1_U */
         s->src_f1_top &= 0x0000ffff;
-        s->src_f1_top |= value << 16;
+        s->src_f1_top |= (uint32_t)value << 16;
         break;
 
     case 0xbcc:	/* BOT_B1_L */
@@ -1265,7 +1265,7 @@ static int omap_dma_3_1_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
 
     case 0x304:	/* SYS_DMA_LCD_TOP_F1_U */
         s->src_f1_top &= 0x0000ffff;
-        s->src_f1_top |= value << 16;
+        s->src_f1_top |= (uint32_t)value << 16;
         break;
 
     case 0x306:	/* SYS_DMA_LCD_BOT_F1_L */
@@ -1275,7 +1275,7 @@ static int omap_dma_3_1_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
 
     case 0x308:	/* SYS_DMA_LCD_BOT_F1_U */
         s->src_f1_bottom &= 0x0000ffff;
-        s->src_f1_bottom |= value << 16;
+        s->src_f1_bottom |= (uint32_t)value << 16;
         break;
 
     case 0x30a:	/* SYS_DMA_LCD_TOP_F2_L */
@@ -1285,7 +1285,7 @@ static int omap_dma_3_1_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
 
     case 0x30c:	/* SYS_DMA_LCD_TOP_F2_U */
         s->src_f2_top &= 0x0000ffff;
-        s->src_f2_top |= value << 16;
+        s->src_f2_top |= (uint32_t)value << 16;
         break;
 
     case 0x30e:	/* SYS_DMA_LCD_BOT_F2_L */
@@ -1295,7 +1295,7 @@ static int omap_dma_3_1_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
 
     case 0x310:	/* SYS_DMA_LCD_BOT_F2_U */
         s->src_f2_bottom &= 0x0000ffff;
-        s->src_f2_bottom |= value << 16;
+        s->src_f2_bottom |= (uint32_t)value << 16;
         break;
 
     default:
-- 
1.9.2

Re: [Qemu-devel] [PATCH 5/8] hw/dma/omap_dma: Add (uint32_t) casts when shifting uint16_t by 16

[Peter Crosthwaite]

On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Add missing (uint32_t) casts in cases where we're trying to
> put a uint16_t value into the top half of a 32-bit field.
> These were already present in some but not all places.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
> For new code or code I cared about I'd use deposit32(); but omap
> is pretty ancient and unloved, so this is the minimal fix.
> ---
>  hw/dma/omap_dma.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/dma/omap_dma.c b/hw/dma/omap_dma.c
> index 0e8cccd..0f35c42 100644
> --- a/hw/dma/omap_dma.c
> +++ b/hw/dma/omap_dma.c
> @@ -973,7 +973,7 @@ static int omap_dma_ch_reg_write(struct omap_dma_s *s,
>
>      case 0x22: /* DMA_COLOR_U */
>          ch->color &= 0xffff;
> -        ch->color |= value << 16;
> +        ch->color |= (uint32_t)value << 16;
>          break;
>
>      case 0x24: /* DMA_CCR2 */
> @@ -1043,7 +1043,7 @@ static int omap_dma_3_2_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
>
>      case 0xbca:        /* TOP_B1_U */
>          s->src_f1_top &= 0x0000ffff;
> -        s->src_f1_top |= value << 16;
> +        s->src_f1_top |= (uint32_t)value << 16;
>          break;
>
>      case 0xbcc:        /* BOT_B1_L */
> @@ -1265,7 +1265,7 @@ static int omap_dma_3_1_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
>
>      case 0x304:        /* SYS_DMA_LCD_TOP_F1_U */
>          s->src_f1_top &= 0x0000ffff;
> -        s->src_f1_top |= value << 16;
> +        s->src_f1_top |= (uint32_t)value << 16;
>          break;
>
>      case 0x306:        /* SYS_DMA_LCD_BOT_F1_L */
> @@ -1275,7 +1275,7 @@ static int omap_dma_3_1_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
>
>      case 0x308:        /* SYS_DMA_LCD_BOT_F1_U */
>          s->src_f1_bottom &= 0x0000ffff;
> -        s->src_f1_bottom |= value << 16;
> +        s->src_f1_bottom |= (uint32_t)value << 16;
>          break;
>
>      case 0x30a:        /* SYS_DMA_LCD_TOP_F2_L */
> @@ -1285,7 +1285,7 @@ static int omap_dma_3_1_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
>
>      case 0x30c:        /* SYS_DMA_LCD_TOP_F2_U */
>          s->src_f2_top &= 0x0000ffff;
> -        s->src_f2_top |= value << 16;
> +        s->src_f2_top |= (uint32_t)value << 16;
>          break;
>
>      case 0x30e:        /* SYS_DMA_LCD_BOT_F2_L */
> @@ -1295,7 +1295,7 @@ static int omap_dma_3_1_lcd_write(struct omap_dma_lcd_channel_s *s, int offset,
>
>      case 0x310:        /* SYS_DMA_LCD_BOT_F2_U */
>          s->src_f2_bottom &= 0x0000ffff;
> -        s->src_f2_bottom |= value << 16;
> +        s->src_f2_bottom |= (uint32_t)value << 16;
>          break;
>
>      default:
> --
> 1.9.2
>
>

[Qemu-devel] [PATCH 6/8] hw/timer/exynos4210_mct: Avoid overflow in exynos4210_ltick_recalc_count

[Peter Maydell]

Add casts to avoid potentially overflowing the multiplications
of 32 bit quantities in exynos4210_ltick_recalc_count().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/exynos4210_mct.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 86f4fcd..69dbecd 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -824,14 +824,14 @@ static void exynos4210_ltick_recalc_count(struct tick_timer *s)
          */
 
         if (s->last_tcnto) {
-            to_count = s->last_tcnto * s->last_icnto;
+            to_count = (uint64_t)s->last_tcnto * s->last_icnto;
         } else {
             to_count = s->last_icnto;
         }
     } else {
         /* distance is passed, recalculate with tcnto * icnto */
         if (s->icntb) {
-            s->distance = s->tcntb * s->icntb;
+            s->distance = (uint64_t)s->tcntb * s->icntb;
         } else {
             s->distance = s->tcntb;
         }
-- 
1.9.2

Re: [Qemu-devel] [PATCH 6/8] hw/timer/exynos4210_mct: Avoid overflow in exynos4210_ltick_recalc_count

[Peter Crosthwaite]

On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Add casts to avoid potentially overflowing the multiplications
> of 32 bit quantities in exynos4210_ltick_recalc_count().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/timer/exynos4210_mct.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index 86f4fcd..69dbecd 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -824,14 +824,14 @@ static void exynos4210_ltick_recalc_count(struct tick_timer *s)
>           */
>
>          if (s->last_tcnto) {
> -            to_count = s->last_tcnto * s->last_icnto;
> +            to_count = (uint64_t)s->last_tcnto * s->last_icnto;
>          } else {
>              to_count = s->last_icnto;
>          }
>      } else {
>          /* distance is passed, recalculate with tcnto * icnto */
>          if (s->icntb) {
> -            s->distance = s->tcntb * s->icntb;
> +            s->distance = (uint64_t)s->tcntb * s->icntb;
>          } else {
>              s->distance = s->tcntb;
>          }
> --
> 1.9.2
>
>

[Qemu-devel] [PATCH 7/8] hw/arm/stellaris: Correct handling of GPTM TAR register

[Peter Maydell]

We don't implement very much of the GPTM TAR register, and what we
do is wrong. The "are we in RT mode?" field is in s->config, not
s->control. Correct this, use LOG_UNIMP rather than hw_error()
for the cases we don't support, and avoid an unlabelled fallthrough
that makes Coverity complain.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/stellaris.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index d6cc77b..487ee72 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -185,12 +185,19 @@ static uint64_t gptm_read(void *opaque, hwaddr offset,
     case 0x44: /* TBPMR */
         return s->match_prescale[1];
     case 0x48: /* TAR */
-        if (s->control == 1)
+        if (s->config == 1) {
             return s->rtc;
+        }
+        qemu_log_mask(LOG_UNIMP,
+                      "gptm_read of TAR but timer read not supported");
+        return 0;
     case 0x4c: /* TBR */
-        hw_error("TODO: Timer value read\n");
+        qemu_log_mask(LOG_UNIMP,
+                      "gptm_read of TBR but timer read not supported");
+        return 0;
     default:
-        hw_error("gptm_read: Bad offset 0x%x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "gptm_read: Bad offset 0x%x\n", (int)offset);
         return 0;
     }
 }
-- 
1.9.2

Re: [Qemu-devel] [PATCH 7/8] hw/arm/stellaris: Correct handling of GPTM TAR register

[Peter Crosthwaite]

On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> We don't implement very much of the GPTM TAR register, and what we
> do is wrong. The "are we in RT mode?" field is in s->config, not
> s->control. Correct this, use LOG_UNIMP rather than hw_error()
> for the cases we don't support, and avoid an unlabelled fallthrough
> that makes Coverity complain.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/stellaris.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index d6cc77b..487ee72 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -185,12 +185,19 @@ static uint64_t gptm_read(void *opaque, hwaddr offset,
>      case 0x44: /* TBPMR */
>          return s->match_prescale[1];
>      case 0x48: /* TAR */
> -        if (s->control == 1)
> +        if (s->config == 1) {
>              return s->rtc;
> +        }
> +        qemu_log_mask(LOG_UNIMP,
> +                      "gptm_read of TAR but timer read not supported");

Should it perhaps be "GPTM read" to be more human?

gptm_read is the qemu fn name and more developer centric, but if thats
what your really going for the perhaps it is better done with %s
__func__ ?

Otherwise:

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Regards,
Peter

> +        return 0;
>      case 0x4c: /* TBR */
> -        hw_error("TODO: Timer value read\n");
> +        qemu_log_mask(LOG_UNIMP,
> +                      "gptm_read of TBR but timer read not supported");
> +        return 0;
>      default:
> -        hw_error("gptm_read: Bad offset 0x%x\n", (int)offset);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gptm_read: Bad offset 0x%x\n", (int)offset);
>          return 0;
>      }
>  }
> --
> 1.9.2
>
>

Re: [Qemu-devel] [PATCH 7/8] hw/arm/stellaris: Correct handling of GPTM TAR register

[Peter Maydell]

On 10 May 2014 13:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> We don't implement very much of the GPTM TAR register, and what we
>> do is wrong. The "are we in RT mode?" field is in s->config, not
>> s->control. Correct this, use LOG_UNIMP rather than hw_error()
>> for the cases we don't support, and avoid an unlabelled fallthrough
>> that makes Coverity complain.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/arm/stellaris.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>> index d6cc77b..487ee72 100644
>> --- a/hw/arm/stellaris.c
>> +++ b/hw/arm/stellaris.c
>> @@ -185,12 +185,19 @@ static uint64_t gptm_read(void *opaque, hwaddr offset,
>>      case 0x44: /* TBPMR */
>>          return s->match_prescale[1];
>>      case 0x48: /* TAR */
>> -        if (s->control == 1)
>> +        if (s->config == 1) {
>>              return s->rtc;
>> +        }
>> +        qemu_log_mask(LOG_UNIMP,
>> +                      "gptm_read of TAR but timer read not supported");
>
> Should it perhaps be "GPTM read" to be more human?

Good idea.

thanks
-- PMM

[Qemu-devel] [PATCH 8/8] hw/arm/omap_gpmc: Avoid buffer overrun filling prefetch FIFO

[Peter Maydell]

In fill_prefetch_fifo(), if the device we are reading from is 16 bit,
then we must not try to transfer an odd number of bytes into the FIFO.
This could otherwise have resulted in our overrunning the prefetch.fifo
array by one byte.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Spotted by Coverity. I suspect Coverity is not smart enough
to figure out that this change really does prevent the overrun,
though :-(
---
 hw/misc/omap_gpmc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/misc/omap_gpmc.c b/hw/misc/omap_gpmc.c
index 2047274..cddea24 100644
--- a/hw/misc/omap_gpmc.c
+++ b/hw/misc/omap_gpmc.c
@@ -242,6 +242,10 @@ static void fill_prefetch_fifo(struct omap_gpmc_s *s)
     if (bytes > s->prefetch.count) {
         bytes = s->prefetch.count;
     }
+    if (is16bit) {
+        bytes &= ~1;
+    }
+
     s->prefetch.count -= bytes;
     s->prefetch.fifopointer += bytes;
     fptr = 64 - s->prefetch.fifopointer;
-- 
1.9.2

Re: [Qemu-devel] [PATCH 8/8] hw/arm/omap_gpmc: Avoid buffer overrun filling prefetch FIFO

[Peter Crosthwaite]

On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> In fill_prefetch_fifo(), if the device we are reading from is 16 bit,
> then we must not try to transfer an odd number of bytes into the FIFO.
> This could otherwise have resulted in our overrunning the prefetch.fifo
> array by one byte.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
> Spotted by Coverity. I suspect Coverity is not smart enough
> to figure out that this change really does prevent the overrun,
> though :-(
> ---
>  hw/misc/omap_gpmc.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/misc/omap_gpmc.c b/hw/misc/omap_gpmc.c
> index 2047274..cddea24 100644
> --- a/hw/misc/omap_gpmc.c
> +++ b/hw/misc/omap_gpmc.c
> @@ -242,6 +242,10 @@ static void fill_prefetch_fifo(struct omap_gpmc_s *s)
>      if (bytes > s->prefetch.count) {
>          bytes = s->prefetch.count;
>      }
> +    if (is16bit) {
> +        bytes &= ~1;
> +    }
> +
>      s->prefetch.count -= bytes;
>      s->prefetch.fifopointer += bytes;
>      fptr = 64 - s->prefetch.fifopointer;
> --
> 1.9.2
>
>