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

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

[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

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

-- 
1.9.3

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

[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.

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 <jsnow@redhat.com>
---
 hw/ide/ahci.c | 48 +++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 8978643..0529d68 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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++) {
-- 
1.9.3

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

[Markus Armbruster]

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?

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

[John Snow]

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.

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

[Markus Armbruster]

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.

[Qemu-devel] [PATCH 2/6] ahci: Update byte count after DMA completion

[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 <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 41 +++++++++++++++++++++++++++++++----------
 hw/ide/core.c     | 11 +++++++----
 hw/ide/internal.h |  2 ++
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 0529d68..6966a94 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 190700a..f15b174 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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;
     }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 5c19f79..033329d 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -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;
-- 
1.9.3

[Qemu-devel] [PATCH 3/6] ide: repair PIO transfers for cases where nsector > 1

[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 <jsnow@redhat.com>
---
 hw/ide/ahci.c | 2 +-
 hw/ide/core.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 6966a94..db1d226 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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;
     }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f15b174..7c1929e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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);
 
-- 
1.9.3

[Qemu-devel] [PATCH 4/6] ahci: unify sglist preparation

[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 <jsnow@redhat.com>
---
 hw/ide/ahci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index db1d226..16cd248 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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);
-- 
1.9.3

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

[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 <jsnow@redhat.com>
---
 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(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 7f86e18..f51d6ee 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -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;
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 16cd248..67c1e36 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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;
 }
 
 /**
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7c1929e..82d01e8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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;
     }
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 2397f35..3f643c2 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -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);
-- 
1.9.3

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

[Paolo Bonzini]

On 10/02/2014 12:55 AM, John Snow wrote:
> +    /* 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

> +    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;
>  }

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

[John Snow]

On 10/27/2014 06:06 AM, Paolo Bonzini wrote:
>
>
> On 10/02/2014 12:55 AM, John Snow wrote:
>> +    /* 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
>
>> +    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;
>>   }

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

[Qemu-devel] [PATCH 6/6] ahci: Fix SDB FIS Construction

[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 <jsnow@redhat.com>
---
 hw/ide/ahci.c | 18 +++++++++---------
 hw/ide/ahci.h |  8 ++++++++
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 67c1e36..8002e9e 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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) |
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 1543df7..a58dd09 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -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);
 
-- 
1.9.3

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

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

[Michael S. Tsirkin]

On Wed, Oct 01, 2014 at 06:55:45PM -0400, 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.

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 <mst@redhat.com>

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

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

[Paolo Bonzini]

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.

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 <pbonzini@redhat.com>

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

On Wed, Oct 01, 2014 at 06:55:45PM -0400, 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
>   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(-)

Dropped the broken assert that Paolo spotted.

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

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[John Snow]

On 10/28/2014 09:51 AM, Stefan Hajnoczi wrote:
> On Wed, Oct 01, 2014 at 06:55:45PM -0400, 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
>>    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(-)
>
> 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.

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

[Paolo Bonzini]

On 10/29/2014 12:54 AM, John Snow wrote:
> 
> 
> On 10/28/2014 09:51 AM, Stefan Hajnoczi wrote:
>> On Wed, Oct 01, 2014 at 06:55:45PM -0400, 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
>>>    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(-)
>>
>> 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.

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

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

[John Snow]

On 10/28/2014 08:03 PM, Paolo Bonzini wrote:
>
>
> On 10/29/2014 12:54 AM, John Snow wrote:
>>
>>
>> On 10/28/2014 09:51 AM, Stefan Hajnoczi wrote:
>>> On Wed, Oct 01, 2014 at 06:55:45PM -0400, 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
>>>>     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(-)
>>>
>>> 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.
>
> 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.

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

[Paolo Bonzini]

>> 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.

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

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

[John Snow]

On 10/28/2014 08:27 PM, Paolo Bonzini wrote:
>
>>> 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.
>
>> 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
>> 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.

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

[Paolo Bonzini]

On 10/29/2014 02:28 AM, John Snow wrote:
> 
> (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.

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

Great!

Paolo

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

On Tue, Oct 28, 2014 at 09:28:26PM -0400, John Snow wrote:
> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]