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

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

[Allan Peramaki]

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>
---
 hw/audio/gusemu_hal.c   | 6 +++---
 hw/audio/gusemu_mixer.c | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

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
@@ -30,9 +30,9 @@
 #include "gustate.h"
 #include "gusemu.h"
 
-#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)))
 
 /* 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
@@ -26,11 +26,11 @@
 #include "gusemu.h"
 #include "gustate.h"
 
-#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)))
 
-#define GUSvoice(position) (*(uint16_t *)(voiceptr+(position)))
+#define GUSvoice(position) (*(uint16_t *)(voiceptr + (position)))
 
 /* samples are always 16bit stereo (4 bytes each, first right then left interleaved) */
 void gus_mixvoices(GUSEmuState * state, unsigned int playback_freq, unsigned int numsamples,
-- 
2.20.1

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

[Volker Rümelin]

> 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>
> ---
>  hw/audio/gusemu_hal.c   | 6 +++---
>  hw/audio/gusemu_mixer.c | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> 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
> @@ -30,9 +30,9 @@
>  #include "gustate.h"
>  #include "gusemu.h"
>  
> -#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)))
>  
>  /* 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
> @@ -26,11 +26,11 @@
>  #include "gusemu.h"
>  #include "gustate.h"
>  
> -#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)))
>  
> -#define GUSvoice(position) (*(uint16_t *)(voiceptr+(position)))
> +#define GUSvoice(position) (*(uint16_t *)(voiceptr + (position)))
>  
>  /* samples are always 16bit stereo (4 bytes each, first right then left interleaved) */
>  void gus_mixvoices(GUSEmuState * state, unsigned int playback_freq, unsigned int numsamples,

With this patch I can hear the sound effects of DOOM Shareware in a FreeDOS 1.2 guest with a GUS audio device.

Reviewed-by: Volker Rümelin <vr_qemu@t-online.de>

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

[Peter Maydell]

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

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

[Allan Peramaki]

On 17/06/2020 23:23, Peter Maydell wrote:
> 
> This patch is quite difficult to read because it mixes some
> whitespace only changes with some actual changes of
> behaviour.

Sorry about that. I had to put some whitespace in the two lines I 
modified because of checkpatch.pl, but then the nearby lines would have 
had inconsistent style if left unmodified. On the other hand, QEMU wiki 
said it is ok to fix style issues in the immediate area.

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

That's right. The only real change is uint16_t -> uint32_t in GUSregd. 
This reverses what seems to be an accident in commit 135f5ae1974c, where 
GUSdword (defined as "unsigned int" or "uint32_t" depending on compiler) 
was replaced with uint16_t. That change broke applications that 
read/write DRAM above 64k via I/O ports (in lieu of using DMA.)

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

Yes (assuming compiler aligns struct fields properly.) It is not obvious 
though (and easy to break), so I suppose refactoring much of the GUS 
code would be nice. For example, the few places where GUSregd(position) 
is used, position is 2 (mod 4). On the other hand, gusptr is also 2 (mod 
4) bytes (making gusptr+position = 0 (mod 4)), because we have the struct

typedef struct GUSState {
     ISADevice dev;
     GUSEmuState emu;
     QEMUSoundCard card;
     uint32_t freq;
     uint32_t port;
     int pos, left, shift, irqs;
     int16_t *mixbuf;
     uint8_t himem[1024 * 1024 + 32 + 4096];
     int samples;
     SWVoiceOut *voice;
     int64_t last_ticks;
     qemu_irq pic;
     IsaDma *isa_dma;
     PortioList portio_list1;
     PortioList portio_list2;
} GUSState;

where himem is 2 (mod 4). When GUSregd() is used, gusptr is (himem + 
1024 * 1024 + 32), so gusptr is also 2 (mod 4). Similar applies to 
GUSvoice(position).

Best regards,
Allan

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

[Allan Peramaki]

On 18/06/2020 01:25, Allan Peramaki wrote:
> On 17/06/2020 23:23, Peter Maydell wrote:
>> 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.
> 
> Yes (assuming compiler aligns struct fields properly.) It is not obvious 
> though (and easy to break), so I suppose refactoring much of the GUS 
> code would be nice. For example, the few places where GUSregd(position) 
> is used, position is 2 (mod 4). On the other hand, gusptr is also 2 (mod 
> 4) bytes (making gusptr+position = 0 (mod 4)), because we have the struct

It is aligned, but my reasoning had a mistake. Both gusptr and position 
(GUSDRAMPOS24bit and voicewavetableirq) are 0 (mod 4). (mixbuf is a 
pointer.) Sorry.

> 
> typedef struct GUSState {
>      ISADevice dev;
>      GUSEmuState emu;
>      QEMUSoundCard card;
>      uint32_t freq;
>      uint32_t port;
>      int pos, left, shift, irqs;
>      int16_t *mixbuf;
>      uint8_t himem[1024 * 1024 + 32 + 4096];
>      int samples;
>      SWVoiceOut *voice;
>      int64_t last_ticks;
>      qemu_irq pic;
>      IsaDma *isa_dma;
>      PortioList portio_list1;
>      PortioList portio_list2;
> } GUSState;
> 
> 

Best regards,
Allan

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

[Thomas Huth]

On 18/06/2020 00.25, Allan Peramaki wrote:
> On 17/06/2020 23:23, Peter Maydell wrote:
>>
>> This patch is quite difficult to read because it mixes some
>> whitespace only changes with some actual changes of
>> behaviour.
> 
> Sorry about that. I had to put some whitespace in the two lines I
> modified because of checkpatch.pl, but then the nearby lines would have
> had inconsistent style if left unmodified.

 Hi Allan!

Makes perfect sense, but for the review, it might have been helpful if
you'd put this information in the commit message.

Anyway, the change looks correct to me, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Peter Maydell]

On Wed, 17 Jun 2020 at 23:26, Allan Peramaki <aperamak@pp1.inet.fi> wrote:
>
> On 17/06/2020 23:23, Peter Maydell wrote:
> >
> > This patch is quite difficult to read because it mixes some
> > whitespace only changes with some actual changes of
> > behaviour.
>
> Sorry about that. I had to put some whitespace in the two lines I
> modified because of checkpatch.pl, but then the nearby lines would have
> had inconsistent style if left unmodified. On the other hand, QEMU wiki
> said it is ok to fix style issues in the immediate area.

Thanks for the explanation.

Yeah, we usually allow that (largely with the goal of "make
the changes that keep checkpatch from complaining about a patch").
I think the changes here just ended up a bit more lost in the
style fixes than they would for some cases (eg if the style
fixup is just "add braces to an if"). One approach if the
style-fixups swamp the actual content is to split into two,
so patch 1 is purely style fixups and patch 2 the bugfix
(or the other way round); but it's certainly not worth rerolling
this patch just for that.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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
> 

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

[Peter Maydell]

On Thu, 18 Jun 2020 at 11:22, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> 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?

Is it worth the effort of a respin at this point?

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 6/18/20 12:23 PM, Peter Maydell wrote:
> On Thu, 18 Jun 2020 at 11:22, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> 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?
> 
> Is it worth the effort of a respin at this point?
> 

I'll give it a try to see if it takes me less than 2min.

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

[Thomas Huth]

On 15/06/2020 22.17, Allan Peramaki 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>
> ---
>  hw/audio/gusemu_hal.c   | 6 +++---
>  hw/audio/gusemu_mixer.c | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> 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
> @@ -30,9 +30,9 @@
>  #include "gustate.h"
>  #include "gusemu.h"
>  
> -#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)))
>  
>  /* 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
> @@ -26,11 +26,11 @@
>  #include "gusemu.h"
>  #include "gustate.h"
>  
> -#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)))
>  
> -#define GUSvoice(position) (*(uint16_t *)(voiceptr+(position)))
> +#define GUSvoice(position) (*(uint16_t *)(voiceptr + (position)))
>  
>  /* samples are always 16bit stereo (4 bytes each, first right then left interleaved) */
>  void gus_mixvoices(GUSEmuState * state, unsigned int playback_freq, unsigned int numsamples,
> 

This might be a good candidate for the stable branches, too (now on the
CC: list).

 Thomas