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

[John Snow]

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

This sends out a PIO Setup FIS for every PIO transfer and adjusts the
tests accordingly. Presumably any sane driver gets end of transfer
status from the D2H Register Update FIS and ... probably ignores the PIO
Setup FIS entirely. I *hope*.

(Certainly with how wrong we've gotten it, it seems very likely that
nobody uses this.)

It's probably still wrong for the reasons I've outlined in my initial
reply here, but it's probably less wrong.

I can't think of a reason you'd want an AHCI device to interrupt so
much, but it's my sincere hope that

(1) No sane AHCI driver uses PIO, and
(2) If it does, it does not do so with the PSFIS interrupt on ...

*shrug*

--js