* [Qemu-devel] regression: sata cdrom boot broken
@ 2018-06-20 11:07 Gerd Hoffmann
2018-06-20 12:18 ` Paolo Bonzini
2018-06-20 18:15 ` Thomas Huth
0 siblings, 2 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2018-06-20 11:07 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, pbonzini, f4bug
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 <jsnow@redhat.com>
Date: Wed Jun 6 15:09:50 2018 -0400
ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands
cheers,
Gerd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] regression: sata cdrom boot broken
2018-06-20 11:07 [Qemu-devel] regression: sata cdrom boot broken Gerd Hoffmann
@ 2018-06-20 12:18 ` Paolo Bonzini
2018-06-20 20:28 ` John Snow
2018-06-20 18:15 ` Thomas Huth
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2018-06-20 12:18 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel; +Cc: jsnow, f4bug
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 <jsnow@redhat.com>
> 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".
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
Putting things together, there are two bugs in QEMU;
- the PIO Setup interrupt must be generated at the end of data transfer
- the PIO Setup interrupt must not be generated for the ATAPI command
transfer
*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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] regression: sata cdrom boot broken
2018-06-20 11:07 [Qemu-devel] regression: sata cdrom boot broken Gerd Hoffmann
2018-06-20 12:18 ` Paolo Bonzini
@ 2018-06-20 18:15 ` Thomas Huth
2018-06-21 11:22 ` Paolo Bonzini
1 sibling, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2018-06-20 18:15 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel; +Cc: pbonzini, jsnow, f4bug
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
Once this has been fixed, please add a regression test to tests/cdrom-test.c
for this, so that it can not happen so easily again. A line like this
should do the job:
qtest_add_data_func("cdrom/boot/q35", "-M q35 -cdrom ", test_cdboot);
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] regression: sata cdrom boot broken
2018-06-20 12:18 ` Paolo Bonzini
@ 2018-06-20 20:28 ` John Snow
2018-06-21 10:50 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: John Snow @ 2018-06-20 20:28 UTC (permalink / raw)
To: Paolo Bonzini, Gerd Hoffmann, qemu-devel; +Cc: f4bug
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 <jsnow@redhat.com>
>> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] regression: sata cdrom boot broken
2018-06-20 20:28 ` John Snow
@ 2018-06-21 10:50 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2018-06-21 10:50 UTC (permalink / raw)
To: John Snow, Gerd Hoffmann, qemu-devel; +Cc: f4bug
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] regression: sata cdrom boot broken
2018-06-20 18:15 ` Thomas Huth
@ 2018-06-21 11:22 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2018-06-21 11:22 UTC (permalink / raw)
To: Thomas Huth, Gerd Hoffmann, qemu-devel; +Cc: jsnow, f4bug
On 20/06/2018 20:15, Thomas Huth 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
>
> Once this has been fixed, please add a regression test to tests/cdrom-test.c
> for this, so that it can not happen so easily again. A line like this
> should do the job:
>
> qtest_add_data_func("cdrom/boot/q35", "-M q35 -cdrom ", test_cdboot);
Nice! The patch I've sent fixes tests/ahci-test.c to match what the
code now does, but cdrom-test.c is also good.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-21 11:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-20 11:07 [Qemu-devel] regression: sata cdrom boot broken Gerd Hoffmann
2018-06-20 12:18 ` Paolo Bonzini
2018-06-20 20:28 ` John Snow
2018-06-21 10:50 ` Paolo Bonzini
2018-06-20 18:15 ` Thomas Huth
2018-06-21 11:22 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).