ahci: Fix FIS decomposition

This patch introduces a few changes to how FIS packets are
deciphered in the AHCI virtual device. The summary of
changes can be grouped into two pieces:

[A] Changes to how we apply a preliminary sieve to FISes,
[B] Changes in how we internalize a decomposed FIS.

== Changes to how we apply a preliminary sieve to FISes ==

(1) Packets may now either update the Control register or
    the Command register, but not both. This is according
    to the SATA 3.2 specification which states:
    "...the device either initiates processing of the command
    indicated in the Command register or initiates processing
    of the control request indicated [...] depending on the
    state of the C bit in the FIS."

    See SATA 3.2 section 10.5.5.4, "Reception" in the 10.5.5
    "Register Host to Device FIS" section.

    This change accounts for the first two regions of change
    within the diff. All other changes belong to the following
    changes.

== Changes in how we internalize a decomposed FIS ==

(2) Instead of trying to extract the sector number out of the
    FIS from bytes 4-10 and setting it with ide_set_sector,
    we set the appropriate IDEState registers and trust that
    ide_get_sector can retrieve the correct sector later.

    By "constructing" the sector for use with ide_set_sector,
    we are duplicating the mechanisms of ide_get_sector.
    This change makes the FIS decomposition more obvious.

    SATA 3.2 as a specification does not make the legacy
    register mapping with respect to the D2H FIS obvious.
    However, SATA 3.2 section 10.5.5.1 "Register Host to
    Device FIS layout" describes all of the "cmd_fis"
    bytes:

    0 - FIS Type (0x27)
    1 - Port Multiplier Port and Command Update flag
    2 - ATA Command
    3 - Features_Low
    4 - LBA 7:0
    5 - LBA 15:8
    6 - LBA 23:16
    7 - Device, AKA "Drive Select."
    8 - LBA 31:24
    9 - LBA 39:32
    10 - LBA 47:40
    11 - Features_High
    12 - Count Low
    13 - Count High
    14 - ICC
    15 - Control
    16-19 - Auxiliary (for NCQ, defined per-command)

    Most of these registers map to existing IDEState registers
    in obvious ways, especially features, select, hob_features,
    and nsector (count). ICC is reserved in older specifications
    but is not supported in our implementation, and remains
    unused here. The Control register is not valid for a command
    that is trying to update the command register and is to be
    considered reserved at this point.

    What is not obvious is the LBA register mappings, but SATA 1.0
    can help inform of us legacy device support, see SATA 1.0 section
    8.5.2 "Register - Host to Device."

    LBA 7:0   - Sector Number    (sector)
    LBA 15:8  - Cyl Low          (lcyl)
    LBA 23:16 - Cyl High         (hcyl)
    LBA 31:24 - Sector Num Exp.  (hob_sector)
    LBA 39:32 - Cyl Low Exp.     (hob_lcyl)
    LBA 47:40 - Cyl High Exp.    (hob_hcyl)

    These mappings help guide which registers the FIS should be decomposed
    into/towards for CHS, LBA28 and LBA48 commands.

    As a note: The prior confusion that can be seen in the documentation
    arises from the fact that CHS and LBA28 commands use the low nybble
    of the drive select register to store LBA 27:24, whereas LNA48 commands
    use the hob_sector, hob_lcyl and hob_hcyl registers as explained above.

    The decomposition as it stands now will correctly decompose CHS, LBA28
    and LBA48 commands into their appropriate registers where the core
    IDE/ATAPI layers can deal with them correctly.

    See the below point for more information.

(3) We save cmd_fis[7] as ide_state->select, which informs
    decisions about if we are using LBA or CHS.
    This corrects a bug in AHCI wherein we attempt to set and/or
    retrieve the sector number by using ide_set_sector and
    ide_get_sector, which depend on the select register to
    determine if we are using LBA or CHS.

    Without this adjustment, LBA48 read/writes are currently
    broken. Thanks to Eniac Zheng @ HP for pointing this out.

(4) Save cmd_fis[11] as ide_state->hob_feature, as defined in SATA 3.2.

(5) For several ATA commands, the sector count register set to 0
    is a magic number that means 256 sectors. For LBA48 commands,
    this means 65,536 sectors. We drop the magic sector correction
    here, and trust the ide core layer to handle the conversion
    appropriately, in ide_cmd_lba48_transform(). As it stands,
    the current AHCI code is only compliant with LBA28 commands.
    By simply removing the magic, it will work with LBA28 and LBA48.

(6) We expand FIS decomposition to include both ATAPI and IDE devices.
    We leave the logic of determining if the fields are valid or not
    to the respective layers.

    This change intends to make it clearer that AHCI is only a
    composition mechanism for the FIS packets: the meanings of
    the registers is best left to the implementation layers for
    those devices.

(7) Forcefully setting the feature, hcyl and lcyl registers for ATAPI
    commands is removed.
    - The hcyl and lcyl magic present here is valid at boot only,
      and should not be overridden for every PACKET command.
    - The feature register is defined as valid for the PACKET command,
      so we should not suppress it. The ATAPI layer does not even
      currently depend on or require 0x01 as mandatory.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 1415058979-16604-3-git-send-email-jsnow@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

1 files changed, 26 insertions, 38 deletions

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index f2acddbea6..43da3637da 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1018,7 +1018,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
             break;
     }
 
-    switch (s->dev[port].port_state) {
+    if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
+        switch (s->dev[port].port_state) {
         case STATE_RUN:
             if (cmd_fis[15] & ATA_SRST) {
                 s->dev[port].port_state = STATE_RESET;
@@ -1029,9 +1030,10 @@ static int handle_cmd(AHCIState *s, int port, int slot)
                 ahci_reset_port(s, port);
             }
             break;
+        }
     }
 
-    if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
+    else if (cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
 
         /* Check for NCQ command */
         if (is_ncq(cmd_fis[2])) {
@@ -1039,50 +1041,36 @@ static int handle_cmd(AHCIState *s, int port, int slot)
             goto out;
         }
 
-        /* Decompose the FIS  */
-        ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
+        /* Decompose the FIS:
+         * AHCI does not interpret FIS packets, it only forwards them.
+         * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
+         * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
+         *
+         * ATA4 describes sector number for LBA28/CHS commands.
+         * ATA6 describes sector number for LBA48 commands.
+         * ATA8 deprecates CHS fully, describing only LBA28/48.
+         *
+         * We dutifully convert the FIS into IDE registers, and allow the
+         * core layer to interpret them as needed. */
         ide_state->feature = cmd_fis[3];
-        if (!ide_state->nsector) {
-            ide_state->nsector = 256;
-        }
-
-        if (ide_state->drive_kind != IDE_CD) {
-            /*
-             * We set the sector depending on the sector defined in the FIS.
-             * Unfortunately, the spec isn't exactly obvious on this one.
-             *
-             * Apparently LBA48 commands set fis bytes 10,9,8,6,5,4 to the
-             * 48 bit sector number. ATA_CMD_READ_DMA_EXT is an example for
-             * such a command.
-             *
-             * Non-LBA48 commands however use 7[lower 4 bits],6,5,4 to define a
-             * 28-bit sector number. ATA_CMD_READ_DMA is an example for such
-             * a command.
-             *
-             * Since the spec doesn't explicitly state what each field should
-             * do, I simply assume non-used fields as reserved and OR everything
-             * together, independent of the command.
-             */
-            ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40)
-                                    | ((uint64_t)cmd_fis[9] << 32)
-                                    /* This is used for LBA48 commands */
-                                    | ((uint64_t)cmd_fis[8] << 24)
-                                    /* This is used for non-LBA48 commands */
-                                    | ((uint64_t)(cmd_fis[7] & 0xf) << 24)
-                                    | ((uint64_t)cmd_fis[6] << 16)
-                                    | ((uint64_t)cmd_fis[5] << 8)
-                                    | cmd_fis[4]);
-        }
+        ide_state->sector = cmd_fis[4];     /* LBA 7:0 */
+        ide_state->lcyl = cmd_fis[5];       /* LBA 15:8  */
+        ide_state->hcyl = cmd_fis[6];       /* LBA 23:16 */
+        ide_state->select = cmd_fis[7];     /* LBA 27:24 (LBA28) */
+        ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
+        ide_state->hob_lcyl = cmd_fis[9];   /* LBA 39:32 */
+        ide_state->hob_hcyl = cmd_fis[10];  /* LBA 47:40 */
+        ide_state->hob_feature = cmd_fis[11];
+        ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
+        /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
+        /* 15: Only valid when UPDATE_COMMAND not set. */
 
         /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
          * table to ide_state->io_buffer
          */
         if (opts & AHCI_CMD_ATAPI) {
             memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
-            ide_state->lcyl = 0x14;
-            ide_state->hcyl = 0xeb;
             debug_print_fis(ide_state->io_buffer, 0x10);
-            ide_state->feature = IDE_FEATURE_DMA;
             s->dev[port].done_atapi_packet = false;
             /* XXX send PIO setup FIS */
         }