Re: [Qemu-devel] Possibility of unaligned DMA accesses via the QEMU DMA API?

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 18/07/13 14:44, Alexander Graf wrote:

>> What I would do, however, is to complete even the INPUT/OUTPUT_MORE
>> commands only at the end of the whole request. This is definitely
>> allowed behaviour, and it ensures that a memory region isn't already
>> reused by the OS while e.g. a write request is still running and taking
>> data from this memory. We should only complete the DMA command as
>> soon as we don't touch the memory any more.
>
> Yes, that's the version that I described as "throw away almost all of today's code and rewrite it" :).
> Keep in mind that the same DMA controller can be used for Ethernet, so coupling it very tightly with
 > IDE doesn't sound overly appealing to me either.

I had a go at implementing this over yesterday afternoon and I managed 
to come up with something that boots both Linux and Darwin. I've 
uploaded it to github in order for you to take a look at here: 
https://github.com/mcayland/qemu/tree/mca-macio.

On the plus side, the code seems a lot more readable; but on the 
downside my heisenbug is *still* present, even in this version of the 
code (see more below). Does the revised version look good enough to you 
both? If so, I'll tidy it up a bit further and post it to the qemu-devel 
list.

In order to ensure that things aren't too tightly coupled to IDE, I've 
introduced a new ready() function pointer which is called from the 
modified DBDMA code. Even though it's currently not used for anything 
other than IDE in QEMU, I think this would enable the same controller to 
be used with Ethernet if required.

A couple of stylistic points for the patch:

1) Can I swap m->aiocb for s->bus->dma->aiocb? This seems to be the 
field used by the internal IDE core.

2) Should the bdrv_acct_start() calls be removed from 
pmac_ide_transfer()? It looks as if they are already handled in 
ide_sector_start_dma() and ide_atapi_cmd_read_dma().

Now back to the heisenbug: for Kevin's benefit, the reason I started 
looking at this is because I have a bug with Alex's existing patches in 
that Aurenlien's test Linux PPC images fails to boot with DMA timeout 
errors (see http://www.ilande.co.uk/tmp/bad-ppc-linux.png). Sadly if I 
try and debug it (by compiling with -O0 -g or adding debug printf() 
statements) then everything works as normal :/

On the plus side, with my revised version I can trigger the bug fairly 
easily by commenting out DBDMA_kick in ide_dbdma_start() and enabling 
DEBUG_IDE_ATAPI in hw/ide/internal.h. Note that while the timeout occurs 
on the CDROM for the majority of time, I've still seen it very 
occasionally on the HD device too.

With DEBUG_IDE_ATAPI enabled the tail of the debug output I can get 
without the bug disappearing looks like this:

reply: tx_size=36 elem_tx_size=0 index=0
byte_count_limit=36
status=0x58
reply: tx_size=0 elem_tx_size=0 index=36
status=0x50
ATAPI limit=0x8 packet: 46 00 00 00 00 00 00 00 08 00 00 00
reply: tx_size=8 elem_tx_size=0 index=0
byte_count_limit=8
status=0x58
reply: tx_size=0 elem_tx_size=0 index=8
status=0x50
ATAPI limit=0x14 packet: 46 01 00 00 00 00 00 00 14 00 00 00
reply: tx_size=20 elem_tx_size=0 index=0
byte_count_limit=20
status=0x58
reply: tx_size=0 elem_tx_size=0 index=20
status=0x50
ATAPI limit=0xc packet: 43 00 00 00 00 00 01 00 0c 00 00 00
reply: tx_size=12 elem_tx_size=0 index=0
byte_count_limit=12
status=0x58
reply: tx_size=0 elem_tx_size=0 index=12
status=0x50
ATAPI limit=0x14 packet: 43 00 00 00 00 00 01 00 14 00 00 00
reply: tx_size=20 elem_tx_size=0 index=0
byte_count_limit=20
status=0x58
reply: tx_size=0 elem_tx_size=0 index=20
status=0x50
ATAPI limit=0xc packet: 43 00 01 00 00 00 00 00 0c 00 00 00
reply: tx_size=12 elem_tx_size=0 index=0
byte_count_limit=12
status=0x58
reply: tx_size=0 elem_tx_size=0 index=12
status=0x50
ATAPI limit=0x20 packet: 51 00 00 00 00 00 00 00 20 00 00 00
Data ready for CD device: 20 bytes

When the bug appears, the guest doesn't bother to program the DMA 
controller to finish the transfer which makes me wonder if one of the 
IDE status flags is not being cleared correctly? Sadly if I enable 
DEBUG_IDE then of the course the delay introduced makes everything work 
again :/

Interestingly enough, the final 0x51 command packet above has s->lba set 
to -1. Given that the code tries to use s->lba as the DMA sector number, 
this is definitely a bug - what should we actually be doing in this case?


ATB,

Mark.