AHCI Device Fixes

PATCH Applied John Snow 6 participants 23 messages Oct 1, 2014 – Oct 30, 2014

Share & tools

John Snow

author

2014-10-01 22:56 UTC #0 lore raw

cover AHCI Device Fixes

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
ahci: Update byte count after DMA completion
ide: repair PIO transfers for cases where nsector > 1
ahci: unify sglist preparation
ide: Correct handling of malformed/short PRDTs
ahci: Fix SDB FIS Construction

 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(-)

Signature · 1 line

1.9.3

John Snow

author

2014-10-01 22:56 UTC #1 lore raw

PATCH 1/6 ahci: Correct PIO/D2H FIS responses

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.

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c | 48 +++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

hw/ide/ahci.c +25 ~~-23~~

index 8978643..0529d68 100644
@@ -599,6 +599,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
     uint8_t *pio_fis, *cmd_fis;
     uint64_t tbl_addr;
     dma_addr_t cmd_len = 0x80;
+    IDEState *s = &ad->port.ifs[0];
 
     if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) {
         return;

@@ -628,21 +629,21 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
 
     pio_fis[0] = 0x5f;
     pio_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
-    pio_fis[2] = ad->port.ifs[0].status;
-    pio_fis[3] = ad->port.ifs[0].error;
-
-    pio_fis[4] = cmd_fis[4];
-    pio_fis[5] = cmd_fis[5];
-    pio_fis[6] = cmd_fis[6];
-    pio_fis[7] = cmd_fis[7];
-    pio_fis[8] = cmd_fis[8];
-    pio_fis[9] = cmd_fis[9];
-    pio_fis[10] = cmd_fis[10];
-    pio_fis[11] = cmd_fis[11];
+    pio_fis[2] = s->status;
+    pio_fis[3] = s->error;
+
+    pio_fis[4] = s->sector;
+    pio_fis[5] = s->lcyl;
+    pio_fis[6] = s->hcyl;
+    pio_fis[7] = s->select;
+    pio_fis[8] = s->hob_sector;
+    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[14] = 0;
-    pio_fis[15] = ad->port.ifs[0].status;
+    pio_fis[15] = s->status;
     pio_fis[16] = len & 255;
     pio_fis[17] = len >> 8;
     pio_fis[18] = 0;

@@ -669,6 +670,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
     int i;
     dma_addr_t cmd_len = 0x80;
     int cmd_mapped = 0;
+    IDEState *s = &ad->port.ifs[0];
 
     if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) {
         return;

@@ -686,17 +688,17 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
 
     d2h_fis[0] = 0x34;
     d2h_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
-    d2h_fis[2] = ad->port.ifs[0].status;
-    d2h_fis[3] = ad->port.ifs[0].error;
-
-    d2h_fis[4] = cmd_fis[4];
-    d2h_fis[5] = cmd_fis[5];
-    d2h_fis[6] = cmd_fis[6];
-    d2h_fis[7] = cmd_fis[7];
-    d2h_fis[8] = cmd_fis[8];
-    d2h_fis[9] = cmd_fis[9];
-    d2h_fis[10] = cmd_fis[10];
-    d2h_fis[11] = cmd_fis[11];
+    d2h_fis[2] = s->status;
+    d2h_fis[3] = s->error;
+
+    d2h_fis[4] = s->sector;
+    d2h_fis[5] = s->lcyl;
+    d2h_fis[6] = s->hcyl;
+    d2h_fis[7] = s->select;
+    d2h_fis[8] = s->hob_sector;
+    d2h_fis[9] = s->hob_lcyl;
+    d2h_fis[10] = s->hob_hcyl;
+    d2h_fis[11] = 0;
     d2h_fis[12] = cmd_fis[12];
     d2h_fis[13] = cmd_fis[13];
     for (i = 14; i < 20; i++) {

Signature · 1 line

1.9.3

Markus Armbruster

2014-10-27 09:32 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/6] ahci: Correct PIO/D2H FIS responses

Quoted 7 lines · John Snow

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.

Quoted 8 lines

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?

John Snow

author

2014-10-27 15:43 UTC #3 lore raw

Re: [Qemu-devel] [PATCH 1/6] ahci: Correct PIO/D2H FIS responses

Quoted 12 lines · Markus Armbruster

Quoted 10 lines · John Snow
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.

Quoted 9 lines

Quoted 5 lines
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.

Markus Armbruster

2014-10-27 15:59 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 1/6] ahci: Correct PIO/D2H FIS responses

Quoted 62 lines · John Snow

Quoted 12 lines · Markus Armbruster
Quoted 10 lines · John Snow
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.
Quoted 9 lines
Quoted 5 lines
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."

Quoted 5 lines

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.

John Snow

author

2014-10-01 22:56 UTC #5 lore raw

PATCH 2/6 ahci: Update byte count after DMA completion

In reply to John Snow

Currently, DMA read/write operations neglect to update the byte count after a successful transfer like ATAPI DMA read or PIO read/write operations do.

We correct this oversight by adding another callback into the IDEDMAOps structure. The commit callback is called whenever we are cleaning up a scatter-gather list. AHCI can register this callback in order to update post- transfer information such as byte count updates.

We use this callback in AHCI to consolidate where we delete the SGlist as generated from the PRDT, as well as update the byte count after the transfer is complete.

The QEMUSGList structure has an init flag added to it in order to make qemu_sglist_destroy a nop if it is called when there is no sglist, which simplifies cleanup and error paths.

This patch fixes several AHCI problems, notably Non-NCQ modes of operation for Windows 7 as well as Hibernate support for Windows 7.

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c     | 41 +++++++++++++++++++++++++++++++----------
 hw/ide/core.c     | 11 +++++++----
 hw/ide/internal.h |  2 ++
 3 files changed, 40 insertions(+), 14 deletions(-)

hw/ide/ahci.c +31 ~~-10~~

index 0529d68..6966a94 100644
@@ -48,6 +48,9 @@ static int handle_cmd(AHCIState *s,int port,int slot);
 static void ahci_reset_port(AHCIState *s, int port);
 static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
 static void ahci_init_d2h(AHCIDevice *ad);
+static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write);
+static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes);
+
 
 static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
 {

@@ -1104,16 +1107,12 @@ static void ahci_start_transfer(IDEDMA *dma)
         }
     }
 
-    /* update number of transferred bytes */
-    ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + size);
-
 out:
     /* declare that we processed everything */
     s->data_ptr = s->data_end;
 
-    if (has_sglist) {
-        qemu_sglist_destroy(&s->sg);
-    }
+    /* Update number of transferred bytes, destroy sglist */
+    ahci_commit_buf(dma, size);
 
     s->end_transfer_func(s);

@@ -1134,6 +1133,11 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
     dma_cb(s, 0);
 }
 
+/**
+ * Called in DMA R/W chains to read the PRDT, utilizing ahci_populate_sglist.
+ * Not currently invoked by PIO R/W chains,
+ * which invoke ahci_populate_sglist via ahci_start_transfer.
+ */
 static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);

@@ -1146,6 +1150,24 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
     return s->io_buffer_size != 0;
 }
 
+/**
+ * Destroys the scatter-gather list,
+ * and updates the command header with a bytes-read value.
+ * called explicitly via ahci_dma_rw_buf (ATAPI DMA),
+ * and ahci_start_transfer (PIO R/W),
+ * and called via callback from ide_dma_cb for DMA R/W paths.
+ */
+static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes)
+{
+    AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
+    IDEState *s = &ad->port.ifs[0];
+
+    tx_bytes += le32_to_cpu(ad->cur_cmd->status);
+    ad->cur_cmd->status = cpu_to_le32(tx_bytes);
+
+    qemu_sglist_destroy(&s->sg);
+}
+
 static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);

@@ -1163,11 +1185,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
         dma_buf_write(p, l, &s->sg);
     }
 
-    /* free sglist that was created in ahci_populate_sglist() */
-    qemu_sglist_destroy(&s->sg);
+    /* free sglist, update byte count */
+    ahci_commit_buf(dma, l);
 
-    /* update number of transferred bytes */
-    ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + l);
     s->io_buffer_index += l;
     s->io_buffer_offset += l;

@@ -1210,6 +1230,7 @@ static const IDEDMAOps ahci_dma_ops = {
     .start_dma = ahci_start_dma,
     .start_transfer = ahci_start_transfer,
     .prepare_buf = ahci_dma_prepare_buf,
+    .commit_buf = ahci_commit_buf,
     .rw_buf = ahci_dma_rw_buf,
     .set_unit = ahci_dma_set_unit,
     .cmd_done = ahci_cmd_done,

hw/ide/core.c +7 -4

index 190700a..f15b174 100644
@@ -631,8 +631,11 @@ void ide_sector_read(IDEState *s)
                                   ide_sector_read_cb, s);
 }
 
-static void dma_buf_commit(IDEState *s)
+static void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
 {
+    if (s->bus->dma->ops->commit_buf) {
+        s->bus->dma->ops->commit_buf(s->bus->dma, tx_bytes);
+    }
     qemu_sglist_destroy(&s->sg);
 }

@@ -647,6 +650,7 @@ void ide_set_inactive(IDEState *s, bool more)
 
 void ide_dma_error(IDEState *s)
 {
+    dma_buf_commit(s, 0);
     ide_abort_command(s);
     ide_set_inactive(s, false);
     ide_set_irq(s->bus);

@@ -662,7 +666,6 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         s->bus->error_status = op;
     } else if (action == BLOCK_ERROR_ACTION_REPORT) {
         if (op & IDE_RETRY_DMA) {
-            dma_buf_commit(s);
             ide_dma_error(s);
         } else {
             ide_rw_error(s);

@@ -706,7 +709,8 @@ void ide_dma_cb(void *opaque, int ret)
 
     sector_num = ide_get_sector(s);
     if (n > 0) {
-        dma_buf_commit(s);
+        assert(s->io_buffer_size == s->sg.size);
+        dma_buf_commit(s, s->io_buffer_size);
         sector_num += n;
         ide_set_sector(s, sector_num);
         s->nsector -= n;

@@ -737,7 +741,6 @@ void ide_dma_cb(void *opaque, int ret)
 
     if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&
         !ide_sect_range_ok(s, sector_num, n)) {
-        dma_buf_commit(s);
         ide_dma_error(s);
         return;
     }

hw/ide/internal.h +2 -0

index 5c19f79..033329d 100644
@@ -322,6 +322,7 @@ typedef void EndTransferFunc(IDEState *);
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
 typedef void DMAVoidFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
+typedef void DMAu32Func(IDEDMA *, uint32_t);
 typedef void DMAStopFunc(IDEDMA *, bool);
 typedef void DMARestartFunc(void *, int, RunState);

@@ -430,6 +431,7 @@ struct IDEDMAOps {
     DMAStartFunc *start_dma;
     DMAVoidFunc *start_transfer;
     DMAIntFunc *prepare_buf;
+    DMAu32Func *commit_buf;
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
     DMAStopFunc *set_inactive;

Signature · 1 line

1.9.3

John Snow

author

2014-10-01 22:56 UTC #6 lore raw

PATCH 3/6 ide: repair PIO transfers for cases where nsector > 1

In reply to John Snow

Currently, for emulated PIO transfers through the AHCI device, any attempt made to request more than a single sector's worth of data will result in the same sector being transferred over and over.

For example, if we request 8 sectors via PIO READ SECTORS, the AHCI device will give us the same sector eight times.

This patch adds offset tracking into the PIO pathways so that we can fulfill these requests appropriately.

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c | 2 +-
 hw/ide/core.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

hw/ide/ahci.c +1 -1

index 6966a94..db1d226 100644
@@ -1091,7 +1091,7 @@ static void ahci_start_transfer(IDEDMA *dma)
         goto out;
     }
 
-    if (!ahci_populate_sglist(ad, &s->sg, 0)) {
+    if (!ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) {
         has_sglist = 1;
     }

hw/ide/core.c +5 -0

index f15b174..7c1929e 100644
@@ -589,6 +589,7 @@ static void ide_sector_read_cb(void *opaque, int ret)
 
     ide_set_sector(s, ide_get_sector(s) + n);
     s->nsector -= n;
+    s->io_buffer_offset += 512 * n;
 }
 
 void ide_sector_read(IDEState *s)

@@ -829,6 +830,8 @@ static void ide_sector_write_cb(void *opaque, int ret)
         n = s->req_nb_sectors;
     }
     s->nsector -= n;
+    s->io_buffer_offset += 512 * n;
+
     if (s->nsector == 0) {
         /* no more sectors to write */
         ide_transfer_stop(s);

@@ -1230,6 +1233,7 @@ static bool cmd_read_pio(IDEState *s, uint8_t cmd)
 
     ide_cmd_lba48_transform(s, lba48);
     s->req_nb_sectors = 1;
+    s->io_buffer_offset = 0;
     ide_sector_read(s);
 
     return false;

@@ -1247,6 +1251,7 @@ static bool cmd_write_pio(IDEState *s, uint8_t cmd)
     ide_cmd_lba48_transform(s, lba48);
 
     s->req_nb_sectors = 1;
+    s->io_buffer_offset = 0;
     s->status = SEEK_STAT | READY_STAT;
     ide_transfer_start(s, s->io_buffer, 512, ide_sector_write);

Signature · 1 line

1.9.3

John Snow

author

2014-10-01 22:56 UTC #7 lore raw

PATCH 4/6 ahci: unify sglist preparation

In reply to John Snow

The intent of this patch is to further unify the creation and deletion of the sglist used for all AHCI transfers, including emulated PIO, ATAPI R/W, and native DMA R/W.

By replacing ahci_start_transfer's call to ahci_populate_sglist with ahci_dma_prepare_buf, we reduce the number of direct calls where we manipulate the scatter-gather list in the AHCI code.

To make this switch, the constant "0" passed as an offset in ahci_dma_prepare_buf is adjusted to use io_buffer_offset.

For DMA pathways, this has no effect: io_buffer_offset is always updated to 0 at the beginning of a DMA transfer loop regardless. DMA pathways through ide_dma_cb() update the io_buffer_offset accordingly, and for circumstances where we might make several trips through this loop, this may actually correct a design flaw.

For PIO pathways, the newly updated ahci_dma_prepare_buf will now prepare the sglist at the correct offset. It will also set io_buffer_size, but this is not used in the cmd_read_pio or cmd_write_pio pathways.

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/ide/ahci.c +2 -2

index db1d226..16cd248 100644
@@ -1091,7 +1091,7 @@ static void ahci_start_transfer(IDEDMA *dma)
         goto out;
     }
 
-    if (!ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) {
+    if (ahci_dma_prepare_buf(dma, is_write)) {
         has_sglist = 1;
     }

@@ -1143,7 +1143,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     IDEState *s = &ad->port.ifs[0];
 
-    ahci_populate_sglist(ad, &s->sg, 0);
+    ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset);
     s->io_buffer_size = s->sg.size;
 
     DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);

Signature · 1 line

1.9.3

John Snow

author

2014-10-01 22:56 UTC #8 lore raw

PATCH 5/6 ide: Correct handling of malformed/short PRDTs

In reply to John Snow

This impacts both BMDMA and AHCI HBA interfaces for IDE. Currently, we confuse the difference between a PRD having "0 bytes" and a PRD having "0 complete sectors."

This leads to, in the BMDMA case, leaked memory for short PRDTs, and infinite loops in the AHCI case.

the "prepare_buf" callback is reworked to return 0 if it could not allocate a full sector's worth of buffer space, instead of returning non-zero if it allocated any number of bytes.

ide_dma_cb adds a call to commit_buf in order to delete the short PRDT that it will not attempt to use to finish the DMA operation.

This patch corrects both occurrences and adds an assertion to prevent future regression. This assertion is tested in the existing ide-test, and is covered in a forthcoming AHCI test.

Signed-off-by John Snow <[email protected]>

---
 dma-helpers.c | 3 +++
 hw/ide/ahci.c | 2 +-
 hw/ide/core.c | 1 +
 hw/ide/pci.c  | 5 +++--
 4 files changed, 8 insertions(+), 3 deletions(-)

dma-helpers.c +3 -0

index 7f86e18..f51d6ee 100644
@@ -38,6 +38,9 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t len)
 void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
                       AddressSpace *as)
 {
+    /* If this is true, you're leaking memory. */
+    assert(qsg->sg == NULL);
+
     qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
     qsg->nsg = 0;
     qsg->nalloc = alloc_hint;

hw/ide/ahci.c +1 -1

index 16cd248..67c1e36 100644
@@ -1147,7 +1147,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
     s->io_buffer_size = s->sg.size;
 
     DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
-    return s->io_buffer_size != 0;
+    return s->io_buffer_size / 512 != 0;
 }
 
 /**

hw/ide/core.c +1 -0

index 7c1929e..82d01e8 100644
@@ -732,6 +732,7 @@ void ide_dma_cb(void *opaque, int ret)
         /* The PRDs were too short. Reset the Active bit, but don't raise an
          * interrupt. */
         s->status = READY_STAT | SEEK_STAT;
+        dma_buf_commit(s, false);
         goto eot;
     }

hw/ide/pci.c +3 -2

index 2397f35..3f643c2 100644
@@ -74,8 +74,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
         if (bm->cur_prd_len == 0) {
             /* end of table (with a fail safe of one page) */
             if (bm->cur_prd_last ||
-                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE)
-                return s->io_buffer_size != 0;
+                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
+                return (s->io_buffer_size / 512) != 0;
+            }
             pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
             bm->cur_addr += 8;
             prd.addr = le32_to_cpu(prd.addr);

Signature · 1 line

1.9.3

Paolo Bonzini

2014-10-27 10:07 UTC #9 lore raw

Re: [Qemu-devel] [PATCH 5/6] ide: Correct handling of malformed/short PRDTs

Quoted 1 line · John Snow

+ /* If this is true, you're leaking memory. */

... or qsg is uninitialized, which would work because qemu_sglist_init initializes all fields.

This is the only comment I have on the series. :)

Paolo

Quoted 12 lines

dma-helpers.c +3 -1

+    assert(qsg->sg == NULL);
+
     qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));

@@ -1147,7 +1147,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
     s->io_buffer_size = s->sg.size;
 
     DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
-    return s->io_buffer_size != 0;
+    return s->io_buffer_size / 512 != 0;
 }

John Snow

author

2014-10-27 18:30 UTC #10 lore raw

Re: [Qemu-devel] [PATCH 5/6] ide: Correct handling of malformed/short PRDTs

Quoted 17 lines · Paolo Bonzini

Quoted 1 line · John Snow
+ /* If this is true, you're leaking memory. */
... or qsg is uninitialized, which would work because qemu_sglist_init initializes all fields.
This is the only comment I have on the series. :)
Paolo
Quoted 5 lines
dma-helpers.c +2 -0
+    assert(qsg->sg == NULL);
+
      qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));

Quoted 7 lines

hw/ide/ahci.c static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write) · line 1151

Quoted 7 lines

hw/ide/ahci.c +1 -1

@@ -1147,7 +1147,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
      s->io_buffer_size = s->sg.size;
 
      DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
-    return s->io_buffer_size != 0;
+    return s->io_buffer_size / 512 != 0;
  }

oh, yeah :\

I really did want to guard against re-initialization, but if it's expected that this structure may have completely anything in it at init time, I don't really have a way to do that, do I.

I guess I'll just delete the assertion, unless you have a very simple idea to help guard against double-inits.

--j

John Snow

author

2014-10-01 22:56 UTC #11 lore raw

PATCH 6/6 ahci: Fix SDB FIS Construction

In reply to John Snow

The SDB FIS creation was mangled;
We were writing the error byte to byte 0, and omitting the SDB FIS magic byte.

Though the SDB packet layout states that:
byte 0: Must be 0xA1 to indicate SDB FIS. byte 1: Port multiplier select & other flags byte 2: status byte.
byte 3: error byte.

This patch adds an SDB FIS structure with human-readable names, and ensures that we are filling the structure appropriately.

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c | 18 +++++++++---------
 hw/ide/ahci.h |  8 ++++++++
 2 files changed, 17 insertions(+), 9 deletions(-)

hw/ide/ahci.c +9 -9

index 67c1e36..8002e9e 100644
@@ -569,24 +569,24 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
     AHCIDevice *ad = &s->dev[port];
     AHCIPortRegs *pr = &ad->port_regs;
     IDEState *ide_state;
-    uint8_t *sdb_fis;
+    SDBFIS *sdb_fis;
 
     if (!s->dev[port].res_fis ||
         !(pr->cmd & PORT_CMD_FIS_RX)) {
         return;
     }
 
-    sdb_fis = &ad->res_fis[RES_FIS_SDBFIS];
+    sdb_fis = (SDBFIS *)&ad->res_fis[RES_FIS_SDBFIS];
     ide_state = &ad->port.ifs[0];
 
-    /* clear memory */
-    *(uint32_t*)sdb_fis = 0;
-
-    /* write values */
-    sdb_fis[0] = ide_state->error;
-    sdb_fis[2] = ide_state->status & 0x77;
+    sdb_fis->type = 0xA1;
+    /* Interrupt pending & Notification bit */
+    sdb_fis->flags = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
+    sdb_fis->status = ide_state->status & 0x77;
+    sdb_fis->error = ide_state->error;
+    /* update SAct field in SDB_FIS */
     s->dev[port].finished |= finished;
-    *(uint32_t*)(sdb_fis + 4) = cpu_to_le32(ad->finished);
+    sdb_fis->payload = cpu_to_le32(ad->finished);
 
     /* Update shadow registers (except BSY 0x80 and DRQ 0x08) */
     pr->tfdata = (ad->port.ifs[0].error << 8) |

hw/ide/ahci.h +8 -0

index 1543df7..a58dd09 100644
@@ -327,6 +327,14 @@ typedef struct NCQFrame {
     uint8_t reserved10;
 } QEMU_PACKED NCQFrame;
 
+typedef struct SDBFIS {
+    uint8_t type;
+    uint8_t flags;
+    uint8_t status;
+    uint8_t error;
+    uint32_t payload;
+} QEMU_PACKED SDBFIS;
+
 void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports);
 void ahci_uninit(AHCIState *s);

Signature · 1 line

1.9.3

John Snow

author

2014-10-16 09:37 UTC #12 lore raw

In reply to 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.

Quoted 24 lines · John Snow

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.

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 9 lines

  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(-)

Michael S. Tsirkin

Tested-by on the v1 series

2014-10-25 20:00 UTC #13 lore raw

In reply to John Snow

Quoted 7 lines · John Snow

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.

I've been running with this for a while now,
and this patchset seems to have solved the
problems that I observed with random hangs of
windows on AHCI.

Tested-by Michael S. Tsirkin <[email protected]>

Quoted 39 lines

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
ahci: Update byte count after DMA completion
ide: repair PIO transfers for cases where nsector > 1
ahci: unify sglist preparation
ide: Correct handling of malformed/short PRDTs
ahci: Fix SDB FIS Construction
 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(-)
Signature · 1 line
1.9.3

Paolo Bonzini

Reviewed-by on the v1 series

2014-10-27 10:08 UTC #14 lore raw

In reply to John Snow

Quoted 13 lines · John Snow

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.

Quoted 6 lines

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.

I only had a comment on this patch. The assertion you added might work for IDE, but I'm afraid not in general. With the change suggested there,

Reviewed-by Paolo Bonzini <[email protected]>

Quoted 25 lines

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
ahci: Update byte count after DMA completion
ide: repair PIO transfers for cases where nsector > 1
ahci: unify sglist preparation
ide: Correct handling of malformed/short PRDTs
ahci: Fix SDB FIS Construction
 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(-)

Stefan Hajnoczi

Applied

2014-10-28 13:52 UTC #15 lore raw

In reply to John Snow

Quoted 37 lines · John Snow

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
ahci: Update byte count after DMA completion
ide: repair PIO transfers for cases where nsector > 1
ahci: unify sglist preparation
ide: Correct handling of malformed/short PRDTs
ahci: Fix SDB FIS Construction

Quoted 7 lines

 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(-)

Dropped the broken assert that Paolo spotted.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

John Snow

author

2014-10-28 23:54 UTC #16 lore raw

Quoted 46 lines · Stefan Hajnoczi

Quoted 37 lines · John Snow
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
ahci: Update byte count after DMA completion
ide: repair PIO transfers for cases where nsector > 1
ahci: unify sglist preparation
ide: Correct handling of malformed/short PRDTs
ahci: Fix SDB FIS Construction
Quoted 7 lines
  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(-)

Quoted 7 lines

Dropped the broken assert that Paolo spotted.
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan

Drop the PIO patch for now (#3), I ran into a regression with it with CDROM drives. I believe the others are still fine.

Paolo Bonzini

2014-10-29 00:04 UTC #17 lore raw

Quoted 52 lines · John Snow

Quoted 49 lines · Stefan Hajnoczi
Quoted 37 lines · John Snow
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
ahci: Update byte count after DMA completion
ide: repair PIO transfers for cases where nsector > 1
ahci: unify sglist preparation
ide: Correct handling of malformed/short PRDTs
ahci: Fix SDB FIS Construction
Quoted 8 lines
  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(-)
Dropped the broken assert that Paolo spotted.

Quoted 7 lines

Quoted 4 lines
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
Drop the PIO patch for now (#3), I ran into a regression with it with CDROM drives. I believe the others are still fine.

Yeah, I was wondering if any commands could have <512 bytes response...
I sort of convinced myself that the answer was no for ATA commands, but
stupidly forgot about packet (SCSI) commands. Their results are obviously shorter than 512 bytes.

Paolo

John Snow

author

2014-10-29 00:07 UTC #18 lore raw

Quoted 63 lines · Paolo Bonzini

Quoted 52 lines · John Snow
Quoted 49 lines · Stefan Hajnoczi
Quoted 37 lines · John Snow
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
ahci: Update byte count after DMA completion
ide: repair PIO transfers for cases where nsector > 1
ahci: unify sglist preparation
ide: Correct handling of malformed/short PRDTs
ahci: Fix SDB FIS Construction
Quoted 8 lines
   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(-)
Dropped the broken assert that Paolo spotted.
Quoted 7 lines
Quoted 4 lines
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
Drop the PIO patch for now (#3), I ran into a regression with it with CDROM drives. I believe the others are still fine.

Quoted 7 lines

Yeah, I was wondering if any commands could have <512 bytes response...
I sort of convinced myself that the answer was no for ATA commands, but
stupidly forgot about packet (SCSI) commands. Their results are obviously shorter than 512 bytes.
Paolo

Are you referencing the sglist underflow patch? (#5 instead of #3) There were cases in the code already where we /assumed/ that having any bytes implied we had at least a sector's worth. If there are valid cases for the sglist to have less than a sector's worth (SCSI) then I'll need to touch that again as well and update all the assumptions in the IDE code to look for numbytes instead of numsectors.

Paolo Bonzini

2014-10-29 00:27 UTC #19 lore raw

Quoted 7 lines

Quoted 5 lines
Yeah, I was wondering if any commands could have <512 bytes response...
I sort of convinced myself that the answer was no for ATA commands, but
stupidly forgot about packet (SCSI) commands. Their results are obviously shorter than 512 bytes.
Are you referencing the sglist underflow patch? (#5 instead of #3)

Hmm, yeah.

Quoted 2 lines

There were cases in the code already where we /assumed/ that having any bytes implied we had at least a sector's worth.

Even for ATAPI?

If there are valid cases for

Quoted 3 lines

the sglist to have less than a sector's worth (SCSI) then I'll need to touch that again as well and update all the assumptions in the IDE code to look for numbytes instead of numsectors.

cmd_inquiry can return 36 bytes. That can be both PIO and DMA. SeaBIOS only uses the DMA variant for AHCI.

Paolo

John Snow

author

2014-10-29 01:28 UTC #20 lore raw

Quoted 16 lines · Paolo Bonzini

Quoted 7 lines
Quoted 5 lines
Yeah, I was wondering if any commands could have <512 bytes response...
I sort of convinced myself that the answer was no for ATA commands, but
stupidly forgot about packet (SCSI) commands. Their results are obviously shorter than 512 bytes.
Are you referencing the sglist underflow patch? (#5 instead of #3)
Hmm, yeah.
Quoted 2 lines
There were cases in the code already where we /assumed/ that having any bytes implied we had at least a sector's worth.
Even for ATAPI?
If there are valid cases for

Quoted 9 lines

Quoted 3 lines
the sglist to have less than a sector's worth (SCSI) then I'll need to touch that again as well and update all the assumptions in the IDE code to look for numbytes instead of numsectors.
cmd_inquiry can return 36 bytes. That can be both PIO and DMA. SeaBIOS only uses the DMA variant for AHCI.
Paolo

OK. So the sglist underflow patch recognizes that functions in the DMA loop check to see if the "prepare buf" calls succeed or not by checking their return code, which was previously generated by something like:
return (size != 0);

Which I updated to be:
return (size / 512) != 0;

This was adjusted in two functions:
ahci_dma_prepare_buf
bmdma_prepare_buf

which are both usually accessed via the .prepare_buf member for IDE DMA OPS.

bmdma_prepare_buf is never accessed outside of this callback. AHCI's prepare buf is called only once, in ahci_start_transfer, which is the PIO pathway (so it includes the PACKET transfer mechanisms.) In ahci_start_transfer, we don't actually even check the return code, so we don't fail commands like cmd_inquiry, so this will succeed.

The only call to prepare_buf is otherwise in ide_dma_cb, which does not include PIO or PACKET pathways, so this command should always return full sectors.

So the code as-written does not actually prevent non-ATA commands from working as intended, however, it is a little messy because somebody might misunderstand what the return code from .prepare_buf really even means.

Here's what I propose:

(1) Update the prepare_buf callback (including the AHCI and BMDMA implementations) to return, simply, the number of bytes prepared. For AHCI, the largest this can ever be is something like

(2) Update uses of the callback or implementations to use this number directly to determine if the call succeeded, failed, or "succeeded enough" for our purposes.

(3) We can reserve the return code of -1 to imply catastrophic failure.

In the meantime:
Patches 1, 2, and 6 are fine and should be merged. I have also fixed patch 3, but I can submit that by itself a little later.

Patch 4 and 5 work best together but have the confusion documented above, and I can do better.

Paolo Bonzini

2014-10-29 08:59 UTC #21 lore raw

Quoted 10 lines · John Snow

(1) Update the prepare_buf callback (including the AHCI and BMDMA implementations) to return, simply, the number of bytes prepared. For AHCI, the largest this can ever be is something like
(2) Update uses of the callback or implementations to use this number directly to determine if the call succeeded, failed, or "succeeded enough" for our purposes.
(3) We can reserve the return code of -1 to imply catastrophic failure.

Sounds like a plan.

Quoted 2 lines

In the meantime:
Patches 1, 2, and 6 are fine and should be merged.

Great!

Paolo

Stefan Hajnoczi

2014-10-30 10:53 UTC #22 lore raw

In reply to John Snow

Quoted 4 lines · John Snow

On 10/28/2014 08:27 PM, Paolo Bonzini wrote:
In the meantime:
Patches 1, 2, and 6 are fine and should be merged. I have also fixed patch 3, but I can submit that by itself a little later.

In case I forgot to reply yesterday, I have dropped patches 3, 4, and 5 from my block branch.

Stefan