Re: [Qemu-devel] [PATCH 0/6] AHCI Device Fixes

[John Snow]

Ping!

At KVM Forum I had a discussion with (someone, sorry!) that having some 
pointers to which specifications to look at here might be helpful, since 
some of the fixes were just spec-adherence fixes.

See below, in-line, for some additional notes on how to review these 
patches.

On 10/02/2014 12:55 AM, John Snow wrote:
> Based off of feedback from the RFC of the same name,
> this series batches together a group of fixes that
> improve the AHCI device to fix a number of bugs.
>
> A number of fixes included in the RFC that provide more
> radical changes are omitted for now in favor of a smaller,
> more easily reviewable set for QEMU 2.2.
>
> In summary:
>
> Patch #1 and #6 correct the format of FIS packet responses
> that are available to the guest operating system upon interrupt.
>
> Patch #2 corrects an oversight where we do not inform the
> guest how many bytes we've transferred. This is relied upon
> for non-NCQ modes and in some early bootup and shutdown code.
>
> Patch #5 corrects cases with malformed scatter-gather lists that
> may cause leaks, or cause QEMU to hang in an allocation loop.
>
> Patch #4 attempts to continue minimizing the divergence of the
> multiple pathways through the AHCI device by re-using existing
> callbacks.
>
> Taken together, these patches should allow non-ncq operation
> for Windows hosts, as well as enable hibernation for Windows 7.
>
> Hibernation for Windows 8 and AHCI remains non-functional.
>
> John Snow (6):
>    ahci: Correct PIO/D2H FIS responses

== 1/6 ==

The PIO and D2H FIS responses are straightforward fixes and are based 
off of the SATA specification, using 3.2 as a reference. "sata 3.2" is a 
good google query.

Section 10.5.11 covers the PIO FIS structure, and
Section 10.5.6 covers the Register Device to Host FIS.

This specification describes the fields of these structures and which 
ATA registers should be copied into them. The primary things here are:

(1) The reserved bytes that we now respect, and
(2) That these registers are the /post/ operation values and not the 
/pre/ operation values. Some commands, e.g. READ_NATIVE_MAX_ADDRESS 
return their information exclusively via the D2H FIS (See ATA8-ACS 
revision 6a) so it is improper to simply copy forward the user's values 
into the response. They should reflect the current state of the device.

>    ahci: Update byte count after DMA completion

== 2/6 ==

Byte count after DMA completion is covered under AHCI 1.3, which is 
freely available: 
http://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/serial-ata-ahci-spec-rev1_3.pdf

The field is first mentioned in section 4.2.2 (Command List Structure) 
on page 37 as field "PRDBC."

The rules on when this field is updated are described within section 
5.4.1 on page 64. Notably, it is mandatory for non-NCQ commands but 
optional for NCQ ones. Our current AHCI implementation does not use the 
hw/ide/core callbacks for non-NCQ transfer modes: we define an ncq_cb 
instead, so the changes in this patch only change non-NCQ operation.

This field *definitely* confuses windows in various ways if it is not 
set, including non-ncq operation and windows 7 hibernate/S4 operation.

>    ide: repair PIO transfers for cases where nsector > 1

== 3/6 ==

The specification deficit here is that PIO transfers, while not actually 
PIO under AHCI, must still work!

The commands are defined under ATA8-ACS revision 6a ("ata8 acs 6a" is a 
good google search term ...) and the relevant details are:

Section 7.35 "READ SECTOR(S)" command 0x20 (PIO Data-In)
This is the LBA28 command used by legacy devices to obtain (usually) a 
single sector at a time. Notably, it takes a count field, though, which 
can be 0x01 (one sector) up to 0xFF (255 sectors) or 0x00 (256 sectors.)

"This 28-bit command is *mandatory* for all devices implementing the 
General and PACKET feature sets." (i.e., hard drives and cdroms.)

Section 7.36 "READ SECTOR(S) EXT" command 0x24 (PIO Data-In)
This is the LBA48 version of the command above. It also defines a count 
field that can range from 0x0001 to 0x0000 -> 65536 sectors. This 
command is mandatory for any devices that implement the LBA48 feature set.

This patch corrects our ignorance of the "count" field for PIO 
transfers, before which we'd only transfer the first sector N times 
instead of N sectors. I have not observed this command be used in this 
way "in the wild" but it was trivial to fix and made writing a test grid 
in qtests for AHCI easier.

>    ahci: unify sglist preparation

== 4/6 ==

This is more mechanical and less spec-based, but I am trying to reduce 
the number of pathways in which we fiddle with the scatter-gather list.

>    ide: Correct handling of malformed/short PRDTs

== 5/6 ==

If an ATA command asks for too many bytes, it may cause problems in 
QEMU. In short, the scatter-gather list length must be 
equal-to-or-greater-than the byte count inferred by the sector count 
sent in the ATA command header.

E.g, ATA command 0xC8 READ SECTORS uses a field COUNT which, when 
multiplied by the sector size (512) should be less than or equal to the 
total number of bytes found within the scatter gather list.

The read command is defined in ATA8 ACS 6a, 0xC8 is a good 
representative example. Section 7.24 details READ DMA, 0xC8.

The scatter-gather list structure is defined in AHCI 1.3 section 4.2.3 
"Command Table" pp 39-40. This patch does not touch SATA relevant code.
FIS decomposition, the primary SATA portion of the AHCI code, is not 
impacted.

It's worth noting that this bug impacts the PCI IDE code as well, to a 
lesser degree. This patch cleans up these error handling pathways to 
appropriately detect short commands for both HBAs.

>    ahci: Fix SDB FIS Construction

== 6/6 ==

Another SATA 3.2 specification area.

The SDB (Set Device Bits) FIS structure is defined in section 10.5.7.
Bytes 0 through 3 are defined here. Word1 / Bytes 4-7 are defined 
elsewhere, because this is an unjust and uncaring universe.

The structure as a whole as it is used in NCQ operation (The only 
defined use that I know of) is in section 13.6.4.2 ("Success outputs") 
on page 638.

>   dma-helpers.c     |   3 ++
>   hw/ide/ahci.c     | 113 ++++++++++++++++++++++++++++++++----------------------
>   hw/ide/ahci.h     |   8 ++++
>   hw/ide/core.c     |  17 ++++++--
>   hw/ide/internal.h |   2 +
>   hw/ide/pci.c      |   5 ++-
>   6 files changed, 97 insertions(+), 51 deletions(-)