Re: [Qemu-devel] regression: sata cdrom boot broken

[Paolo Bonzini <pbonzini@redhat.com>]

On 20/06/2018 22:28, John Snow wrote:
> 
> ...Oh. So the PIO Setup FIS ... gets generated before the data is sent,
> but we don't copy it to the HBA memory buffers and notify the client
> until afterwards, but this is per-DRQ, I think, and not per-IDE command.

No, the copy happens before: "When a PIO setup FIS arrives from the
device, the HBA copies it to the PSFIS area of this structure".

However, the interrupt happens after the data is received too (and
whether it happens is decided per-FIS, i.e. per-DRQ).

>> Putting things together, there are two bugs in QEMU;
>>
>> - the PIO Setup interrupt must be generated at the end of data transfer
>>
> Oops.
> 
>> - the PIO Setup interrupt must not be generated for the ATAPI command
>> transfer
>>
> Oops again. I ought to have stopped you but I was long aware that the
> PIO Setup FIS ought to be generated "before" the transfer. I suppose
> what we were doing was more correct, though.

Not really, it's half and half and in fact the second of these two bugs
was very much preexisting.

Certainly it was wrong to generate the interrupt after the callback was
invoked, i.e. per command rather than per-DRQ.  This was the important
part of the original patch series because it is what made the recursion
not a tail recursion.

It's really hard to review this stuff since you have to find all the
tidbits across the AHCI and SATA (and sometimes ATA command set) specs.
If anything the blame goes to both of us for not testing the CD-ROM boot
case, rather than to you for missing it during the code review!

And on the bright side, it's still a win:

- it's possible to fix the bug without jeopardizing the whole READ CD series

- we've learnt more of the interactions between the AHCI and SATA specs

- the bug was discovered quickly enough

Paolo