Skip to contentSkip to conversation

ahci: fix cdrom read corruption

PATCH Applied Jason Baron 3 participants 5 messages Aug 3, 2012 – Sep 18, 2012

lore
Share & tools
lore

Jason Baron

author
#0 lore raw

PATCH v2 1/1 ahci: fix cdrom read corruption

Hi,

While testing q35 I found data corruption on reads from the cdrom on the ahci controller. The first patch addresses this issue. I also noticed that there is a memory leak in the ahci code, which is addressed in the second patch.

Thanks,

-Jason

v2:
fprintf -> DPRINTF (so can't be triggered by guest) 0 sglist fields

Jason Baron (2):
ahci: Fix ahci cdrom read corruptions for reads > 128k
ahci: Fix sglist memleak in ahci_dma_rw_buf()

 dma-helpers.c     |    1 +
 hw/ide/ahci.c     |   44 +++++++++++++++++++++++++++++++++++++-------
 hw/ide/internal.h |    1 +
 3 files changed, 39 insertions(+), 7 deletions(-)
author
#1 lore raw

PATCH v2 1/2 ahci: Fix ahci cdrom read corruptions for reads > 128k

While testing q35, which has its cdrom attached to the ahci controller, I found that the Fedora 17 install would panic on boot. The panic occurs while squashfs is trying to read from the cdrom. The errors are:

[    8.622711] SQUASHFS error: xz_dec_run error, data probably corrupt
[    8.625180] SQUASHFS error: squashfs_read_data failed to read block

0x20be48a

I was also able to produce corrupt data reads using an installed piix based qemu machine, using 'dd'. I found that the corruptions were only occuring when then read size was greater than 128k. For example, the following command results in corrupted reads:

dd if=/dev/sr0 of=/tmp/blah bs=256k iflag=direct

The > 128k size reads exercise a different code path than 128k and below. In ide_atapi_cmd_read_dma_cb() s->io_buffer_size is capped at 128k. Thus, ide_atapi_cmd_read_dma_cb() is called a second time when the read is > 128k. However, ahci_dma_rw_buf() restart the read from offset 0, instead of at 128k. Thus, resulting in a corrupted read.

To fix this, I've introduced 'io_buffer_offset' field in IDEState to keep track of the offset. I've also modified ahci_populate_sglist() to take a new 3rd offset argument, so that the sglist is property initialized.

I've tested this patch using 'dd' testing, and Fedora 17 now correctly boots and installs on q35 with the cdrom ahci controller.

Signed-off-by Jason Baron <[email protected]>
Tested-by Andreas Färber <[email protected]>
---
 hw/ide/ahci.c     |   41 ++++++++++++++++++++++++++++++++++-------
 hw/ide/internal.h |    1 +
 2 files changed, 35 insertions(+), 7 deletions(-)
hw/ide/ahci.c +34 -7
index efea93f..de580a6 100644
@@ -636,7 +636,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
     }
 }
 
-static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist)
+static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
 {
     AHCICmdHdr *cmd = ad->cur_cmd;
     uint32_t opts = le32_to_cpu(cmd->opts);

@@ -647,6 +647,10 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist)
     uint8_t *prdt;
     int i;
     int r = 0;
+    int sum = 0;
+    int off_idx = -1;
+    int off_pos = -1;
+    int tbl_entry_size;
 
     if (!sglist_alloc_hint) {
         DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts);

@@ -669,10 +673,31 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist)
     /* Get entries in the PRDT, init a qemu sglist accordingly */
     if (sglist_alloc_hint > 0) {
         AHCI_SG *tbl = (AHCI_SG *)prdt;
-
-        qemu_sglist_init(sglist, sglist_alloc_hint, ad->hba->dma);
+        sum = 0;
         for (i = 0; i < sglist_alloc_hint; i++) {
             /* flags_size is zero-based */
+            tbl_entry_size = (le32_to_cpu(tbl[i].flags_size) + 1);
+            if (offset <= (sum + tbl_entry_size)) {
+                off_idx = i;
+                off_pos = offset - sum;
+                break;
+            }
+            sum += tbl_entry_size;
+        }
+        if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) {
+            DPRINTF(ad->port_no, "%s: Incorrect offset! "
+                            "off_idx: %d, off_pos: %d\n",
+                            __func__, off_idx, off_pos);
+            r = -1;
+            goto out;
+        }
+
+        qemu_sglist_init(sglist, (sglist_alloc_hint - off_idx), ad->hba->dma);
+        qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr + off_pos),
+                        le32_to_cpu(tbl[off_idx].flags_size) + 1 - off_pos);
+
+        for (i = off_idx + 1; i < sglist_alloc_hint; i++) {
+            /* flags_size is zero-based */
             qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
                             le32_to_cpu(tbl[i].flags_size) + 1);
         }

@@ -745,7 +770,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2,
             s->dev[port].port.ifs[0].nb_sectors - 1);
 
-    ahci_populate_sglist(&s->dev[port], &ncq_tfs->sglist);
+    ahci_populate_sglist(&s->dev[port], &ncq_tfs->sglist, 0);
     ncq_tfs->tag = tag;
 
     switch(ncq_fis->command) {

@@ -970,7 +995,7 @@ static int ahci_start_transfer(IDEDMA *dma)
         goto out;
     }
 
-    if (!ahci_populate_sglist(ad, &s->sg)) {
+    if (!ahci_populate_sglist(ad, &s->sg, 0)) {
         has_sglist = 1;
     }
 

@@ -1015,6 +1040,7 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
     DPRINTF(ad->port_no, "\n");
     ad->dma_cb = dma_cb;
     ad->dma_status |= BM_STATUS_DMAING;
+    s->io_buffer_offset = 0;
     dma_cb(s, 0);
 }
 

@@ -1023,7 +1049,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);
+    ahci_populate_sglist(ad, &s->sg, 0);
     s->io_buffer_size = s->sg.size;
 
     DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);

@@ -1037,7 +1063,7 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
     uint8_t *p = s->io_buffer + s->io_buffer_index;
     int l = s->io_buffer_size - s->io_buffer_index;
 
-    if (ahci_populate_sglist(ad, &s->sg)) {
+    if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) {
         return 0;
     }
 

@@ -1050,6 +1076,7 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
     /* 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;
 
     DPRINTF(ad->port_no, "len=%#x\n", l);
hw/ide/internal.h +1 -0
index 7170bd9..bf7d313 100644
@@ -393,6 +393,7 @@ struct IDEState {
     struct iovec iov;
     QEMUIOVector qiov;
     /* ATA DMA state */
+    int io_buffer_offset;
     int io_buffer_size;
     QEMUSGList sg;
     /* PIO transfer handling */
Signature · 1 line
1.7.1
author
#2 lore raw

PATCH v2 2/2 ahci: Fix sglist memleak in ahci_dma_rw_buf()

In reply to Jason Baron

I noticed that in hw/ide/ahci:ahci_dma_rw_buf() we do not free the sglist. Thus, I've added a call to qemu_sglist_destroy() to fix this memory leak.

In addition, I've adeed a call in qemu_sglist_destroy() to 0 all of the sglist fields, in case there is some other codepath that tries to free the sglist.

Signed-off-by Jason Baron <[email protected]>
---
 dma-helpers.c |    1 +
 hw/ide/ahci.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)
dma-helpers.c +1 -0
index 35cb500..13593d1 100644
@@ -65,6 +65,7 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
 void qemu_sglist_destroy(QEMUSGList *qsg)
 {
     g_free(qsg->sg);
+    memset(qsg, 0, sizeof(*qsg));
 }
 
 typedef struct {
hw/ide/ahci.c +3 -0
index de580a6..5ea3cad 100644
@@ -1073,6 +1073,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);
+
     /* 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;
Signature · 1 line
1.7.1
Applied
#3 lore raw
In reply to Jason Baron

Am 03.08.2012 21:57, schrieb Jason Baron:

Quoted 16 lines

Hi,

While testing q35 I found data corruption on reads from the cdrom on the ahci controller. The first patch addresses this issue. I also noticed that there is a memory leak in the ahci code, which is addressed in the second patch.

Thanks,

-Jason

v2:
fprintf -> DPRINTF (so can't be triggered by guest) 0 sglist fields

Quoted 9 lines

Jason Baron (2):
ahci: Fix ahci cdrom read corruptions for reads > 128k
ahci: Fix sglist memleak in ahci_dma_rw_buf()

 dma-helpers.c     |    1 +
 hw/ide/ahci.c     |   44 +++++++++++++++++++++++++++++++++++++-------
 hw/ide/internal.h |    1 +
 3 files changed, 39 insertions(+), 7 deletions(-)

Thanks, applied both to the block branch.

Kevin

#4 lore raw

Am 09.08.2012 11:59, schrieb Kevin Wolf:

Quoted 21 lines

Am 03.08.2012 21:57, schrieb Jason Baron:

Quoted 20 lines

Hi,

While testing q35 I found data corruption on reads from the cdrom on the ahci controller. The first patch addresses this issue. I also noticed that there is a memory leak in the ahci code, which is addressed in the second patch.

Thanks,

-Jason

v2:
fprintf -> DPRINTF (so can't be triggered by guest) 0 sglist fields

Jason Baron (2):
ahci: Fix ahci cdrom read corruptions for reads > 128k
ahci: Fix sglist memleak in ahci_dma_rw_buf()

Quoted 7 lines
Quoted 5 lines
 dma-helpers.c     |    1 +
 hw/ide/ahci.c     |   44 +++++++++++++++++++++++++++++++++++++-------
 hw/ide/internal.h |    1 +
 3 files changed, 39 insertions(+), 7 deletions(-)

Thanks, applied both to the block branch.

Thanks, backported both to stable-0.15 (avoiding the new DMA helpers and GLib).

Andreas

Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg