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

[Paolo Bonzini <pbonzini@redhat.com>]

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,
>  };
>