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

[John Snow]

On 06/20/2018 08:18 AM, Paolo Bonzini wrote:
> On 20/06/2018 13:07, Gerd Hoffmann wrote:
>>   Hi,
>>
>> $subject says all.  Noticed while testing the upcoming seabios update.
>> Reproducer:
>>
>>   qemu-system-x86_64 -M q35 -m 4G -cdrom 
>> Fedora-Workstation-Live-x86_64-28-1.1.iso
>>
>> bisected to:
>>
>>   commit 956556e131e35f387ac482ad7b41151576fef057
>>   Author: John Snow <address@hidden>
>>   Date:   Wed Jun 6 15:09:50 2018 -0400
>>
>>     ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands
>>
>> cheers,
>>   Gerd
>>
> 
> If you know where to look at, the spec is actually pretty clear with
> respect to when the interrupt is generated: "A PIO Setup FIS has been
> received with the ‘I’ bit set, it has been copied into system memory,
> and the data related to that FIS has been transferred".
> 

You're quoting AHCI 1.3.1 section 3.3.5 here, the documentation for the
PxIS register.

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

> However, after reading the SATA specification I believe that the PIO
> Setup FIS should never generate an interrupt in SeaBIOS, because:
> 
>   If this is the first DRQ data block for this command, the Interrupt
>   bit shall be cleared to zero. If this is not the first DRQ data block
>   for this command, the Interrupt bit shall be set to one
> 

...and this gem is from SATA 3.2 section 11.9, "PIO data-out command
protocol." I have long wondered what controlled that 'I' bit...

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

> *But* because SeaBIOS always uses DMA for ATAPI commands, there should
> never be more than one DRQ data block for each command, and it should be
> possible to remove the fishy PIO Setup FIS handling in SeaBIOS.
> 
> Paolo
>