From: Markus Armbruster <armbru@redhat.com>
References: <1412204151-18117-1-git-send-email-jsnow@redhat.com>
	<1412204151-18117-2-git-send-email-jsnow@redhat.com>
	<87ioj5c0xz.fsf@blackfin.pond.sub.org> <544E6814.5030803@redhat.com>
Date: Mon, 27 Oct 2014 16:59:21 +0100
In-Reply-To: <544E6814.5030803@redhat.com> (John Snow's message of "Mon, 27
	Oct 2014 11:43:16 -0400")
Message-ID: <87ioj54i7a.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 1/6] ahci: Correct PIO/D2H FIS responses
To: John Snow <jsnow@redhat.com>
Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com

John Snow <jsnow@redhat.com> writes:

> On 10/27/2014 05:32 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Currently, the D2H FIS packets AHCI generates simply parrot back
>>> the LBA that the guest sent to us in the cmd_fis. However, some
>>> commands (like READ NATIVE MAX) modify the LBA registers as a
>>> return value, through which the AHCI D2H FIS is the only response
>>> mechanism. Thus, the D2H response should use the current register
>>> values, not the initial ones.
>>>
>>> This patch adjusts the LBA and drive select register responses for
>>> PIO Setup and D2H FIS response packets.
>>>
>>> Additionally, the PIO and D2H FIS responses copy too many bytes
>>> from the command FIS that it is being generated from. Specifically,
>>> byte 11 which is the Features(15:8) field for Register Host to
>>> Device FIS packets, is instead reserved for the PIO Setup FIS and
>>> should always be 0.
>>
>> Ignorant q: is this based on observation or some specification?  If
>> specification: where can I find a copy?
>>
>
> Not ignorant. I do all kinds of crazy things. It's based off an
> interpretation of the specification and an observation.
>
> Specification: The SATA specification defines the FIS responses to
> commands via the Register Device to Host FIS. See Sata 3.2 section
> 10.5.6 "Register Device to Host FIS"
>
> The fields of this packet are defined as things like:
> "LBA(7:0) - Contains the *new value* of the LBA low register of the
> Shadow Register Block." (emphasis mine.) Many other registers are
> defined similarly. All six LBA registers, Device, Error, Count, and so
> on.
>
> Implying that instead of returning back what the Host-To-Device FIS
> set, we are returning what it is currently (the "new value" after the
> command was run.)
>
> The observation: READ_NATIVE_MAX_ADDRESS is a non-data command, so it
> performs no PIO or DMA actions on the IDE device. It will only return,
> in the case of the AHCI controller, an FIS packet. If the FIS packet
> returns (for example) the LBA registers that the user sent, there is
> no mechanism by which the host can actually /receive/ the native max
> address.
>
> Therefore, I interpret the specification to imply that the response
> FIS after successful completion of a command should return the new,
> current values of the IDE registers, and not simply parrot back what
> the user sent. It's a synchronization mechanism between the device and
> the host.
>
> For further specification... READ_NATIVE_MAX_ADDRESS is deprecated in
> AC3, but we can look at ATA8-ACS revision 4a, which is pretty clear
> about the line response to this command:
>
> Section 7.33 READ NATIVE MAX ADDRESS - F8h, Non-data, subsection
> 7.33.4 "Normal Outputs":
> "See table 96. LBA contains the Native Max Address. Bits 47:28 of LBA
> shall be cleared to zero."
>
> Table 96 - "HPA Normal Output" Specifies the line output for this
> command. Error, Count, LBA, Device, Status. These line outputs are for
> sure the information that is bundled up into the D2H FIS.
>
> On this basis, then, is the reasoning behind this patch.

Makes sense to me, thanks.

Since you already got competent review, I won't review further, unless
you ask for it.