[Qemu-devel] [PATCH for-2.5?] hw/ppc/ppc405_boards: Fix infinite recursion by converting taihu_cpld from old_mmio

[Qemu-devel] [PATCH for-2.5?] hw/ppc/ppc405_boards: Fix infinite recursion by converting taihu_cpld from old_mmio

[Peter Maydell]

The taihu_cpld_writel() function had an obvious typo that meant that
if it was ever called it would go into an infinite recursion. Newer
versions of clang will detect and warn about this:
  hw/ppc/ppc405_boards.c:481:1: warning: all paths through this function will call itself [-Winfinite-recursion]

Fix this by converting taihu_cpld from the legacy old_mmio accessors
to new-style ones, with an impl {} declaration to cause the core
memory code to do the splitting of 16 bit and 32 bit accesses into
multiple 8-bit accesses.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Marked 'for-2.5?' because of the infinite recursion (though the bug
has been present since the board support was first committed in 2007).
NB that I don't have a Taihu image that would exercise the device.
There would obviously be a smaller fix that just dealt with the recursion
problem, but old_mmio is an obsolete interface we should be switching
away from anyhow.

(The equivalent bug in ref405ep_fpga_writel was fixed in commit 8de2410635
in 2008.)

 hw/ppc/ppc405_boards.c | 52 ++++++++------------------------------------------
 1 file changed, 8 insertions(+), 44 deletions(-)

diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index ec87587..31bc186 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -408,7 +408,7 @@ struct taihu_cpld_t {
     uint8_t reg1;
 };
 
-static uint32_t taihu_cpld_readb (void *opaque, hwaddr addr)
+static uint64_t taihu_cpld_read(void *opaque, hwaddr addr, unsigned size)
 {
     taihu_cpld_t *cpld;
     uint32_t ret;
@@ -429,8 +429,8 @@ static uint32_t taihu_cpld_readb (void *opaque, hwaddr addr)
     return ret;
 }
 
-static void taihu_cpld_writeb (void *opaque,
-                               hwaddr addr, uint32_t value)
+static void taihu_cpld_write(void *opaque, hwaddr addr,
+                             uint64_t value, unsigned size)
 {
     taihu_cpld_t *cpld;
 
@@ -447,48 +447,12 @@ static void taihu_cpld_writeb (void *opaque,
     }
 }
 
-static uint32_t taihu_cpld_readw (void *opaque, hwaddr addr)
-{
-    uint32_t ret;
-
-    ret = taihu_cpld_readb(opaque, addr) << 8;
-    ret |= taihu_cpld_readb(opaque, addr + 1);
-
-    return ret;
-}
-
-static void taihu_cpld_writew (void *opaque,
-                               hwaddr addr, uint32_t value)
-{
-    taihu_cpld_writeb(opaque, addr, (value >> 8) & 0xFF);
-    taihu_cpld_writeb(opaque, addr + 1, value & 0xFF);
-}
-
-static uint32_t taihu_cpld_readl (void *opaque, hwaddr addr)
-{
-    uint32_t ret;
-
-    ret = taihu_cpld_readb(opaque, addr) << 24;
-    ret |= taihu_cpld_readb(opaque, addr + 1) << 16;
-    ret |= taihu_cpld_readb(opaque, addr + 2) << 8;
-    ret |= taihu_cpld_readb(opaque, addr + 3);
-
-    return ret;
-}
-
-static void taihu_cpld_writel (void *opaque,
-                               hwaddr addr, uint32_t value)
-{
-    taihu_cpld_writel(opaque, addr, (value >> 24) & 0xFF);
-    taihu_cpld_writel(opaque, addr + 1, (value >> 16) & 0xFF);
-    taihu_cpld_writel(opaque, addr + 2, (value >> 8) & 0xFF);
-    taihu_cpld_writeb(opaque, addr + 3, value & 0xFF);
-}
-
 static const MemoryRegionOps taihu_cpld_ops = {
-    .old_mmio = {
-        .read = { taihu_cpld_readb, taihu_cpld_readw, taihu_cpld_readl, },
-        .write = { taihu_cpld_writeb, taihu_cpld_writew, taihu_cpld_writel, },
+    .read = taihu_cpld_read,
+    .write = taihu_cpld_write,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
     },
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
-- 
1.9.1

Re: [Qemu-devel] [PATCH for-2.5?] hw/ppc/ppc405_boards: Fix infinite recursion by converting taihu_cpld from old_mmio

[Paolo Bonzini]

On 16/11/2015 15:57, Peter Maydell wrote:
> The taihu_cpld_writel() function had an obvious typo that meant that
> if it was ever called it would go into an infinite recursion. Newer
> versions of clang will detect and warn about this:
>   hw/ppc/ppc405_boards.c:481:1: warning: all paths through this function will call itself [-Winfinite-recursion]
> 
> Fix this by converting taihu_cpld from the legacy old_mmio accessors
> to new-style ones, with an impl {} declaration to cause the core
> memory code to do the splitting of 16 bit and 32 bit accesses into
> multiple 8-bit accesses.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Marked 'for-2.5?' because of the infinite recursion (though the bug
> has been present since the board support was first committed in 2007).
> NB that I don't have a Taihu image that would exercise the device.
> There would obviously be a smaller fix that just dealt with the recursion
> problem, but old_mmio is an obsolete interface we should be switching
> away from anyhow.

Yes, it makes sense and the diffstat is nice too. :)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

(The equivalent bug in ref405ep_fpga_writel was fixed in commit 8de2410635
> in 2008.)
> 
>  hw/ppc/ppc405_boards.c | 52 ++++++++------------------------------------------
>  1 file changed, 8 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index ec87587..31bc186 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -408,7 +408,7 @@ struct taihu_cpld_t {
>      uint8_t reg1;
>  };
>  
> -static uint32_t taihu_cpld_readb (void *opaque, hwaddr addr)
> +static uint64_t taihu_cpld_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      taihu_cpld_t *cpld;
>      uint32_t ret;
> @@ -429,8 +429,8 @@ static uint32_t taihu_cpld_readb (void *opaque, hwaddr addr)
>      return ret;
>  }
>  
> -static void taihu_cpld_writeb (void *opaque,
> -                               hwaddr addr, uint32_t value)
> +static void taihu_cpld_write(void *opaque, hwaddr addr,
> +                             uint64_t value, unsigned size)
>  {
>      taihu_cpld_t *cpld;
>  
> @@ -447,48 +447,12 @@ static void taihu_cpld_writeb (void *opaque,
>      }
>  }
>  
> -static uint32_t taihu_cpld_readw (void *opaque, hwaddr addr)
> -{
> -    uint32_t ret;
> -
> -    ret = taihu_cpld_readb(opaque, addr) << 8;
> -    ret |= taihu_cpld_readb(opaque, addr + 1);
> -
> -    return ret;
> -}
> -
> -static void taihu_cpld_writew (void *opaque,
> -                               hwaddr addr, uint32_t value)
> -{
> -    taihu_cpld_writeb(opaque, addr, (value >> 8) & 0xFF);
> -    taihu_cpld_writeb(opaque, addr + 1, value & 0xFF);
> -}
> -
> -static uint32_t taihu_cpld_readl (void *opaque, hwaddr addr)
> -{
> -    uint32_t ret;
> -
> -    ret = taihu_cpld_readb(opaque, addr) << 24;
> -    ret |= taihu_cpld_readb(opaque, addr + 1) << 16;
> -    ret |= taihu_cpld_readb(opaque, addr + 2) << 8;
> -    ret |= taihu_cpld_readb(opaque, addr + 3);
> -
> -    return ret;
> -}
> -
> -static void taihu_cpld_writel (void *opaque,
> -                               hwaddr addr, uint32_t value)
> -{
> -    taihu_cpld_writel(opaque, addr, (value >> 24) & 0xFF);
> -    taihu_cpld_writel(opaque, addr + 1, (value >> 16) & 0xFF);
> -    taihu_cpld_writel(opaque, addr + 2, (value >> 8) & 0xFF);
> -    taihu_cpld_writeb(opaque, addr + 3, value & 0xFF);
> -}
> -
>  static const MemoryRegionOps taihu_cpld_ops = {
> -    .old_mmio = {
> -        .read = { taihu_cpld_readb, taihu_cpld_readw, taihu_cpld_readl, },
> -        .write = { taihu_cpld_writeb, taihu_cpld_writew, taihu_cpld_writel, },
> +    .read = taihu_cpld_read,
> +    .write = taihu_cpld_write,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
>      },
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5?] hw/ppc/ppc405_boards: Fix infinite recursion by converting taihu_cpld from old_mmio

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Tue, Nov 17, 2015 at 10:43:36AM +0100, Paolo Bonzini wrote:
> 
> 
> On 16/11/2015 15:57, Peter Maydell wrote:
> > The taihu_cpld_writel() function had an obvious typo that meant that
> > if it was ever called it would go into an infinite recursion. Newer
> > versions of clang will detect and warn about this:
> >   hw/ppc/ppc405_boards.c:481:1: warning: all paths through this function will call itself [-Winfinite-recursion]
> > 
> > Fix this by converting taihu_cpld from the legacy old_mmio accessors
> > to new-style ones, with an impl {} declaration to cause the core
> > memory code to do the splitting of 16 bit and 32 bit accesses into
> > multiple 8-bit accesses.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Marked 'for-2.5?' because of the infinite recursion (though the bug
> > has been present since the board support was first committed in 2007).
> > NB that I don't have a Taihu image that would exercise the device.
> > There would obviously be a smaller fix that just dealt with the recursion
> > problem, but old_mmio is an obsolete interface we should be switching
> > away from anyhow.
> 
> Yes, it makes sense and the diffstat is nice too. :)
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Applied to my for-2.5 tree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]