Re: [Qemu-devel] [PATCH 1/6] ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands

[Paolo Bonzini <pbonzini@redhat.com>]

On 05/06/2018 01:27, John Snow wrote:
> 
> 
> On 06/04/2018 11:50 AM, Paolo Bonzini wrote:
>> On 02/06/2018 03:22, John Snow wrote:
>>> - Status: Should be the status register after receiving the H2D Register
>>>   update FIS, but prior to the data transfer, I think. "New value of the
>>>   Status register of the Command Block for initiation of host data
>>>   transfer."
>>>   I think this is being set correctly after this patch.
>>>
>>> - Error: "Contains the new value of the Error register of the Command
>>>   Block at the conclusion of all subsequent Data to Device frames."
>>>
>>>   This is why we were sending out post-hoc PIO Setup FIS frames before,
>>>   how would I know what the error register *will* be...? What?
>>
>> You don't, I guess.  Zero?
>>
> 
> Yeah, I don't mean to hold it up based on other arcane stuff, I was just
> briefly hoping that maybe you actually understood it so I could fix it
> once and for all...
> 
>>> - LBA: Presumably unimportant for the purposes of receiving a command
>>>   PACKET, as we won't be writing it to disk, but a buffer. The values
>>>   can be reported dutifully.
>>>
>>> - Device: Just report the register value dutifully.
>>>
>>> - Count: Likely just relays 0, as the H2D REG FIS should have updated
>>>   that to zero as part of the PACKET command, as per ATA8 ACS3 7.21.3.
>>>   In any case, just report the register value dutifully.
>>>
>>> - E_Status: "Contains the new value of the Status register of the
>>>   Command Block at the conclusion of the subsequent Data FIS."
>>>
>>>   Again, how would I know...?
>>>
>>> - Transfer Count: Should be 12, as per what we specified in 0xA1
>>>   IDENTIFY PACKET DEVICE, see core.c line 234. Your patch gets this
>>>   correct, as we'll actually report the PIO FIS for the packet itself.
>>>
>>>
>>> What this patch does do, though, is change the context of the Status,
>>> Error and E_Status registers to something different. Everything else
>>> should be the same, but I'd feel better about taking this patch if I
>>> understood what exactly this FIS packet was supposed to convey, but I don't.
>>
>> At least Status and Transfer Count are correct after this patch. :/
>>
>> Paolo
>>
> 
> How about ...
> 
> https://github.com/jnsnow/qemu/commit/0657390a2639b7cb533ca8b514c49c0edd3f4483

Yes, it's sane.

Paolo