Re: [PATCH] hw/audio/gus: Fix registers 32-bit access

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 15 Jun 2020 at 22:23, Allan Peramaki <aperamak@pp1.inet.fi> wrote:
>
> Fix audio on software that accesses DRAM above 64k via register peek/poke
> and some cases when more than 16 voices are used.
>
> Fixes: 135f5ae1974c ("audio: GUSsample is int16_t")
> Signed-off-by: Allan Peramaki <aperamak@pp1.inet.fi>

This patch is quite difficult to read because it mixes some
whitespace only changes with some actual changes of
behaviour.

> -#define GUSregb(position) (*            (gusptr+(position)))
> -#define GUSregw(position) (*(uint16_t *) (gusptr+(position)))
> -#define GUSregd(position) (*(uint16_t *)(gusptr+(position)))
> +#define GUSregb(position) (*(gusptr + (position)))
> +#define GUSregw(position) (*(uint16_t *)(gusptr + (position)))
> +#define GUSregd(position) (*(uint32_t *)(gusptr + (position)))

So, I think the actual bugfix change here is just the changing
of uint16_t to uint32_t in the GUSregd definition...

> -#define GUSregb(position)  (*            (gusptr+(position)))
> -#define GUSregw(position)  (*(uint16_t *) (gusptr+(position)))
> -#define GUSregd(position)  (*(uint16_t *)(gusptr+(position)))
> +#define GUSregb(position)  (*(gusptr + (position)))
> +#define GUSregw(position)  (*(uint16_t *)(gusptr + (position)))
> +#define GUSregd(position)  (*(uint32_t *)(gusptr + (position)))

...and similarly here, and all the other changes are purely
cleaning up the spaces. Is that right?

> -#define GUSvoice(position) (*(uint16_t *)(voiceptr+(position)))
> +#define GUSvoice(position) (*(uint16_t *)(voiceptr + (position)))

Are these accesses all guaranteed to be correctly aligned
to be 16 or 32 bit loads/stores ? Otherwise it would be
better to use the ldl_p/stl_p/ldw_p/stw_p/etc accessors,
which correctly handle possibly misaligned pointers.

thanks
-- PMM