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

[Allan Peramaki <aperamak@pp1.inet.fi>]

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