Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response

[John Snow <jsnow@redhat.com>]

On 06/29/2015 10:51 AM, Stefan Hajnoczi wrote:
> On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote:
>> On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote:
>>> On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote:
>>>> @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice
>>>> *ad, uint16_t len) pio_fis[9] = s->hob_lcyl; pio_fis[10] =
>>>> s->hob_hcyl; pio_fis[11] = 0; -    pio_fis[12] = cmd_fis[12]; -
>>>> pio_fis[13] = cmd_fis[13]; +    pio_fis[12] = s->nsector & 0xFF; 
>>>> +    pio_fis[13] = (s->nsector >> 8) & 0xFF;
>>>
>>> hw/ide/core.c decreases s->nsector until it reaches 0 and the
>>> request ends.
>>>
>>> Will the values reported back to the guest be correct if we use 
>>> s->nsector?
>>>
>>
>> See the commit message for justification of this one. Ultimately, it
>> doesn't matter what gets put in here (for data transfer commands) --
>> but getting RID of the cmd_fis mapping is a strong positive.
> 
> Getting rid of cmd_fis mapping is good.
> 
> Putting s->nsector into the undefined fields makes the code confusing.
> 
> It is clearer to zero the bytes with a comment saying the value does not
> matter according to the spec.
> 

Well, it's not that it doesn't matter /ever/, it's more that for
standard IO routines it doesn't matter. (See the normative output spec
in ATA8-AC3 -- for most cases it's N/A, but for a handful of cases it
carries a diagnostic signature.)

What's really the case is that the FIS always dutifully copies out what
the SATA registers are (or should be.)

There are still a handful of commands that, if we choose to support
them, copying the nsector register would be the "correct thing" to do,
so I decided to copy that field here to serve as documentation and
support future command additions.

I would argue that if this field ever does the /wrong thing/, it would
be a fix in the S/ATA layer, and not a change to the FIS generator here.

I am inclined to leave it as-is, since for the current cases, nsector is
going to empty to zero anyway. I believe the behavior presented here is
correct.

--js