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.mayd...@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 <pbonz...@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,
> };
>
