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

[Philippe Mathieu-Daudé]

On 6/17/20 10:23 PM, Peter Maydell wrote:
> 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.

In such cases the author might add a comment in the commit
description "this patch is easier to review with git-diff -w"
or other options.

Allan, can we get the patch with only the changed lines?

See:

---
diff --git a/hw/audio/gusemu_hal.c b/hw/audio/gusemu_hal.c
index ae40ca341c..e35e941926 100644
--- a/hw/audio/gusemu_hal.c
+++ b/hw/audio/gusemu_hal.c
@@ -32,7 +32,7 @@

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

 /* size given in bytes */
 unsigned int gus_read(GUSEmuState * state, int port, int size)
diff --git a/hw/audio/gusemu_mixer.c b/hw/audio/gusemu_mixer.c
index 00b9861b92..3b39254518 100644
--- a/hw/audio/gusemu_mixer.c
+++ b/hw/audio/gusemu_mixer.c
@@ -28,7 +28,7 @@

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

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

---

> 
>> -#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
>