Patchwork [04/10] ide: enable preallocated sg lists

login
register
mail settings
Submitter Alexander Graf
Date Nov. 17, 2010, 1:05 a.m.
Message ID <1289955937-24121-5-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/71492/
State New
Headers show

Comments

Alexander Graf - Nov. 17, 2010, 1:05 a.m.
The AHCI core does all the SG handling for us, so we need to allow it
to keep its own layouts.

This patch adds hooks into the IDE code to allow for preallocated SG lists.

Signed-off-by: Roland Elek <elek.roland@gmail.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/core.c     |   38 +++++++++++++++++++++++++++++++-------
 hw/ide/internal.h |    1 +
 2 files changed, 32 insertions(+), 7 deletions(-)
Gerd Hoffmann - Nov. 17, 2010, 8:59 a.m.
Hi,

> +            if (s->sg_third_party) {
> +                /* We've already parsed the guest RAM PRDT.
> +                 * This is essential for AHCI, where the PRDT is in a different
> +                 * format than in IDE BMDMA.
> +                 */
> +                memcpy((uint8_t *)&prd, s->sg.sg, sizeof(prd));
> +                s->sg.sg++;
> +            } else {
> +                cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
> +                bm->cur_addr += 8;
> +                prd.addr = le32_to_cpu(prd.addr);
> +                prd.size = le32_to_cpu(prd.size);
> +            }

Does it make sense to handle this via IDEBusOps too?

cheers,
   Gerd
Stefan Hajnoczi - Nov. 17, 2010, 9:21 a.m.
On Wed, Nov 17, 2010 at 1:05 AM, Alexander Graf <agraf@suse.de> wrote:
> @@ -535,12 +543,25 @@ static int dma_buf_rw(BMDMAState *bm, 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) >= IDE_PAGE_SIZE)
> -                return 0;
> -            cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
> -            bm->cur_addr += 8;
> -            prd.addr = le32_to_cpu(prd.addr);
> -            prd.size = le32_to_cpu(prd.size);
> +                (bm->cur_addr - bm->addr) >= IDE_PAGE_SIZE) {
> +                    s->sg.sg -= s->sg.nsg; //XXX

I don't understand why s->sg.sg needs to be modified at all.  Are we stashing
the pointer away accross dma_buf_rw() invocations?

> +                    return 0;
> +                }
> +            if (s->sg_third_party) {
> +                /* We've already parsed the guest RAM PRDT.
> +                 * This is essential for AHCI, where the PRDT is in a different
> +                 * format than in IDE BMDMA.
> +                 */
> +                memcpy((uint8_t *)&prd, s->sg.sg, sizeof(prd));

struct {
    uint32_t addr;
    uint32_t size;
} prd;

typedef struct {
    target_phys_addr_t base;
    target_phys_addr_t len;
} ScatterGatherEntry;

sizeof(target_phys_addr_t) != sizeof(uint32_t) for 64-bit targets

prd should use a proper struct instead of redefining it inline in dma_buf_rw()
and dma_buf_prepare().  That struct could either be a ScatterGatherEntry or
something else that we assign s->sg.sg fields to individually.

Stefan

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 276b853..4c15ea2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -444,8 +444,16 @@  static int dma_buf_prepare(BMDMAState *bm, int is_write)
         uint32_t addr;
         uint32_t size;
     } prd;
-    int l, len;
+    int l, len, i;
 
+    if (s->sg_third_party) {
+        /* We already have an sglist. */
+        s->io_buffer_size = 0;
+        for (i=0; i < s->sg.nsg; i++) {
+            s->io_buffer_size += s->sg.sg[i].len;
+        }
+        return s->io_buffer_size != 0;
+    }
     qemu_sglist_init(&s->sg, s->nsector / (IDE_PAGE_SIZE / 512) + 1);
     s->io_buffer_size = 0;
     for(;;) {
@@ -535,12 +543,25 @@  static int dma_buf_rw(BMDMAState *bm, 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) >= IDE_PAGE_SIZE)
-                return 0;
-            cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
-            bm->cur_addr += 8;
-            prd.addr = le32_to_cpu(prd.addr);
-            prd.size = le32_to_cpu(prd.size);
+                (bm->cur_addr - bm->addr) >= IDE_PAGE_SIZE) {
+                    s->sg.sg -= s->sg.nsg; //XXX
+                    return 0;
+                }
+            if (s->sg_third_party) {
+                /* We've already parsed the guest RAM PRDT.
+                 * This is essential for AHCI, where the PRDT is in a different
+                 * format than in IDE BMDMA.
+                 */
+                memcpy((uint8_t *)&prd, s->sg.sg, sizeof(prd));
+                s->sg.sg++;
+            } else {
+                cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
+                bm->cur_addr += 8;
+                prd.addr = le32_to_cpu(prd.addr);
+                prd.size = le32_to_cpu(prd.size);
+            }
+
+
             len = prd.size & 0xfffe;
             if (len == 0)
                 len = 0x10000;
@@ -2715,6 +2736,9 @@  int ide_init_drive(IDEState *s, BlockDriverState *bs,
     } else {
         pstrcpy(s->version, sizeof(s->version), QEMU_VERSION);
     }
+
+    s->sg_third_party = 0;
+
     ide_reset(s);
     bdrv_set_removable(bs, s->drive_kind == IDE_CD);
     return 0;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 19e5efb..005283c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -422,6 +422,7 @@  struct IDEState {
     /* ATA DMA state */
     int io_buffer_size;
     QEMUSGList sg;
+    int sg_third_party;
     /* PIO transfer handling */
     int req_nb_sectors; /* number of sectors per interrupt */
     EndTransferFunc *end_transfer_func;