ide: add TRIM support

PATCH In review Christoph Hellwig 2 participants 9 messages May 19, 2011 – Jun 14, 2011

Share & tools

Christoph Hellwig

author

2011-05-19 08:57 UTC #0 lore raw

cover ide: add TRIM support

This patchset resurrects my older patches to add TRIM support. The old approach to pass an action handler through the IDE subsystem doesn't work any more after the refactoring for the AHCI driver, so not we simple switch on the type of command passed in.

Christoph Hellwig

author

2011-05-19 08:58 UTC #1 lore raw

PATCH 1/3 make dma_bdrv_io available to drivers

Make dma_bdrv_io available for drivers, and pass an explicit I/O function instead of hardcoding bdrv_aio_readv/bdrv_aio_writev. This is required to implement non-READ/WRITE dma commands in the ide driver, e.g. the upcoming TRIM support.

Signed-off-by Christoph Hellwig <[email protected]>

dma-helpers.c +10 ~~-13~~

Index: qemu/dma-helpers.c
===================================================================
@@ -47,6 +47,7 @@ typedef struct {
     target_phys_addr_t sg_cur_byte;
     QEMUIOVector iov;
     QEMUBH *bh;
+    DMAIOFunc *io_func;
 } DMAAIOCB;
 
 static void dma_bdrv_cb(void *opaque, int ret);

@@ -116,13 +117,8 @@ static void dma_bdrv_cb(void *opaque, in
         return;
     }
 
-    if (dbs->is_write) {
-        dbs->acb = bdrv_aio_writev(dbs->bs, dbs->sector_num, &dbs->iov,
-                                   dbs->iov.size / 512, dma_bdrv_cb, dbs);
-    } else {
-        dbs->acb = bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
-                                  dbs->iov.size / 512, dma_bdrv_cb, dbs);
-    }
+    dbs->acb = dbs->io_func(dbs->bs, dbs->sector_num, &dbs->iov,
+                            dbs->iov.size / 512, dma_bdrv_cb, dbs);
     if (!dbs->acb) {
         dma_bdrv_unmap(dbs);
         qemu_iovec_destroy(&dbs->iov);

@@ -144,12 +140,12 @@ static AIOPool dma_aio_pool = {
     .cancel             = dma_aio_cancel,
 };
 
-static BlockDriverAIOCB *dma_bdrv_io(
+BlockDriverAIOCB *dma_bdrv_io(
     BlockDriverState *bs, QEMUSGList *sg, uint64_t sector_num,
-    BlockDriverCompletionFunc *cb, void *opaque,
-    int is_write)
+    DMAIOFunc *io_func, BlockDriverCompletionFunc *cb,
+    void *opaque, int is_write)
 {
-    DMAAIOCB *dbs =  qemu_aio_get(&dma_aio_pool, bs, cb, opaque);
+    DMAAIOCB *dbs = qemu_aio_get(&dma_aio_pool, bs, cb, opaque);
 
     dbs->acb = NULL;
     dbs->bs = bs;

@@ -158,6 +154,7 @@ static BlockDriverAIOCB *dma_bdrv_io(
     dbs->sg_cur_index = 0;
     dbs->sg_cur_byte = 0;
     dbs->is_write = is_write;
+    dbs->io_func = io_func;
     dbs->bh = NULL;
     qemu_iovec_init(&dbs->iov, sg->nsg);
     dma_bdrv_cb(dbs, 0);

@@ -173,12 +170,12 @@ BlockDriverAIOCB *dma_bdrv_read(BlockDri
                                 QEMUSGList *sg, uint64_t sector,
                                 void (*cb)(void *opaque, int ret), void *opaque)
 {
-    return dma_bdrv_io(bs, sg, sector, cb, opaque, 0);
+    return dma_bdrv_io(bs, sg, sector, bdrv_aio_readv, cb, opaque, 0);
 }
 
 BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
                                  QEMUSGList *sg, uint64_t sector,
                                  void (*cb)(void *opaque, int ret), void *opaque)
 {
-    return dma_bdrv_io(bs, sg, sector, cb, opaque, 1);
+    return dma_bdrv_io(bs, sg, sector, bdrv_aio_writev, cb, opaque, 1);
 }

dma.h +8 -0

Index: qemu/dma.h
===================================================================
@@ -32,6 +32,14 @@ void qemu_sglist_add(QEMUSGList *qsg, ta
                      target_phys_addr_t len);
 void qemu_sglist_destroy(QEMUSGList *qsg);
 
+typedef BlockDriverAIOCB *DMAIOFunc(BlockDriverState *bs, int64_t sector_num,
+                                 QEMUIOVector *iov, int nb_sectors,
+                                 BlockDriverCompletionFunc *cb, void *opaque);
+
+BlockDriverAIOCB *dma_bdrv_io(BlockDriverState *bs,
+                              QEMUSGList *sg, uint64_t sector_num,
+                              DMAIOFunc *io_func, BlockDriverCompletionFunc *cb,
+                              void *opaque, int is_write);
 BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
                                 QEMUSGList *sg, uint64_t sector,
                                 BlockDriverCompletionFunc *cb, void *opaque);

Christoph Hellwig

author

2011-05-19 08:58 UTC #2 lore raw

PATCH 2/3 ide: allow other dma comands than read and write

In reply to Christoph Hellwig

Replace the is_read flag with a dma_cmd flag to allow the dma and restart logic to handler other commands like TRIM.

Signed-off-by Christoph Hellwig <[email protected]>

hw/ide/core.c +14 ~~-11~~

Index: qemu/hw/ide/core.c
===================================================================
@@ -473,7 +473,7 @@ handle_rw_error:
     if (ret < 0) {
         int op = BM_STATUS_DMA_RETRY;
 
-        if (s->is_read)
+        if (s->dma_cmd == IDE_DMA_READ)
             op |= BM_STATUS_RETRY_READ;
         if (ide_handle_rw_error(s, -ret, op)) {
             return;

@@ -483,7 +483,7 @@ handle_rw_error:
     n = s->io_buffer_size >> 9;
     sector_num = ide_get_sector(s);
     if (n > 0) {
-        dma_buf_commit(s, s->is_read);
+        dma_buf_commit(s, ide_cmd_is_read(s));
         sector_num += n;
         ide_set_sector(s, sector_num);
         s->nsector -= n;

@@ -500,20 +500,23 @@ handle_rw_error:
     n = s->nsector;
     s->io_buffer_index = 0;
     s->io_buffer_size = n * 512;
-    if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->is_read) == 0)
+    if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) == 0)
         goto eot;
 
 #ifdef DEBUG_AIO
-    printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, is_read=%d\n",
-           sector_num, n, s->is_read);
+    printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
+           sector_num, n, s->dma_cmd);
 #endif
 
-    if (s->is_read) {
+    switch (s->dma_cmd) {
+    case IDE_DMA_READ:
         s->bus->dma->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
                                            ide_dma_cb, s);
-    } else {
+        break;
+    case IDE_DMA_WRITE:
         s->bus->dma->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
                                             ide_dma_cb, s);
+        break;
     }
 
     if (!s->bus->dma->aiocb) {

@@ -527,12 +530,12 @@ eot:
    ide_set_inactive(s);
 }
 
-static void ide_sector_start_dma(IDEState *s, int is_read)
+static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 {
     s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
     s->io_buffer_index = 0;
     s->io_buffer_size = 0;
-    s->is_read = is_read;
+    s->dma_cmd = dma_cmd;
     s->bus->dma->ops->start_dma(s->bus->dma, s, ide_dma_cb);
 }

@@ -915,7 +918,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t
         if (!s->bs)
             goto abort_cmd;
 	ide_cmd_lba48_transform(s, lba48);
-        ide_sector_start_dma(s, 1);
+        ide_sector_start_dma(s, IDE_DMA_READ);
         break;
 	case WIN_WRITEDMA_EXT:
 	lba48 = 1;

@@ -924,7 +927,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t
         if (!s->bs)
             goto abort_cmd;
 	ide_cmd_lba48_transform(s, lba48);
-        ide_sector_start_dma(s, 0);
+        ide_sector_start_dma(s, IDE_DMA_WRITE);
         s->media_changed = 1;
         break;
     case WIN_READ_NATIVE_MAX_EXT:

hw/ide/internal.h +9 -1

Index: qemu/hw/ide/internal.h
===================================================================
@@ -379,6 +379,14 @@ struct unreported_events {
     bool new_media;
 };
 
+enum ide_dma_cmd {
+    IDE_DMA_READ,
+    IDE_DMA_WRITE,
+};
+
+#define ide_cmd_is_read(s) \
+	((s)->dma_cmd == IDE_DMA_READ)
+
 /* NOTE: IDEState represents in fact one drive */
 struct IDEState {
     IDEBus *bus;

@@ -446,7 +454,7 @@ struct IDEState {
     uint32_t mdata_size;
     uint8_t *mdata_storage;
     int media_changed;
-    int is_read;
+    enum ide_dma_cmd dma_cmd;
     /* SMART */
     uint8_t smart_enabled;
     uint8_t smart_autosave;

hw/ide/macio.c +7 -2

Index: qemu/hw/ide/macio.c
===================================================================
@@ -145,12 +145,17 @@ static void pmac_ide_transfer_cb(void *o
     io->addr += io->len;
     io->len = 0;
 
-    if (s->is_read)
+    switch (s->dma_cmd) {
+    case IDE_DMA_READ:
         m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
 		                 pmac_ide_transfer_cb, io);
-    else
+        break;
+    case IDE_DMA_WRITE:
         m->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
 		                  pmac_ide_transfer_cb, io);
+        break;
+    }
+
     if (!m->aiocb)
         pmac_ide_transfer_cb(io, -1);
 }

hw/ide/pci.c +3 -3

Index: qemu/hw/ide/pci.c
===================================================================
@@ -169,7 +169,7 @@ static int bmdma_set_inactive(IDEDMA *dm
     return 0;
 }
 
-static void bmdma_restart_dma(BMDMAState *bm, int is_read)
+static void bmdma_restart_dma(BMDMAState *bm, enum ide_dma_cmd dma_cmd)
 {
     IDEState *s = bmdma_active_if(bm);

@@ -177,7 +177,7 @@ static void bmdma_restart_dma(BMDMAState
     s->io_buffer_index = 0;
     s->io_buffer_size = 0;
     s->nsector = bm->nsector;
-    s->is_read = is_read;
+    s->dma_cmd = dma_cmd;
     bm->cur_addr = bm->addr;
     bm->dma_cb = ide_dma_cb;
     bmdma_start_dma(&bm->dma, s, bm->dma_cb);

@@ -195,7 +195,7 @@ static void bmdma_restart_bh(void *opaqu
 
     if (bm->status & BM_STATUS_DMA_RETRY) {
         bm->status &= ~(BM_STATUS_DMA_RETRY | BM_STATUS_RETRY_READ);
-        bmdma_restart_dma(bm, is_read);
+        bmdma_restart_dma(bm, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
     } else if (bm->status & BM_STATUS_PIO_RETRY) {
         bm->status &= ~(BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ);
         if (is_read) {

Christoph Hellwig

author

2011-05-19 08:58 UTC #3 lore raw

PATCH 3/3 ide: add TRIM support

In reply to Christoph Hellwig

Add support for TRIM sub function of the data set management command, and wire it up to the qemu discard infrastructure.

Signed-off-by Christoph Hellwig <[email protected]>

hw/ide/core.c +90 -0

Index: qemu/hw/ide/core.c
===================================================================
@@ -124,6 +124,9 @@ static void ide_identify(IDEState *s)
     put_le16(p + 66, 120);
     put_le16(p + 67, 120);
     put_le16(p + 68, 120);
+    if (dev && dev->conf.discard_granularity) {
+        put_le16(p + 69, (1 << 14)); /* determinate TRIM behavior */
+    }
 
     if (s->ncq_queues) {
         put_le16(p + 75, s->ncq_queues - 1);

@@ -157,6 +160,9 @@ static void ide_identify(IDEState *s)
     dev = s->unit ? s->bus->slave : s->bus->master;
     if (dev && dev->conf.physical_block_size)
         put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
+    if (dev && dev->conf.discard_granularity) {
+        put_le16(p + 169, 1); /* TRIM support */
+    }
 
     memcpy(s->identify_data, p, sizeof(s->identify_data));
     s->identify_set = 1;

@@ -299,6 +305,72 @@ static void ide_set_signature(IDEState *
     }
 }
 
+typedef struct TrimAIOCB {
+    BlockDriverAIOCB common;
+    QEMUBH *bh;
+    int ret;
+} TrimAIOCB;
+
+static void trim_aio_cancel(BlockDriverAIOCB *acb)
+{
+    TrimAIOCB *iocb = container_of(acb, TrimAIOCB, common);
+
+    qemu_bh_delete(iocb->bh);
+    iocb->bh = NULL;
+    qemu_aio_release(iocb);
+}
+
+static AIOPool trim_aio_pool = {
+    .aiocb_size         = sizeof(TrimAIOCB),
+    .cancel             = trim_aio_cancel,
+};
+
+static void ide_trim_bh_cb(void *opaque)
+{
+    TrimAIOCB *iocb = opaque;
+
+    iocb->common.cb(iocb->common.opaque, iocb->ret);
+
+    qemu_bh_delete(iocb->bh);
+    iocb->bh = NULL;
+
+    qemu_aio_release(iocb);
+}
+
+BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    TrimAIOCB *iocb;
+    int i, j, ret;
+
+    iocb = qemu_aio_get(&trim_aio_pool, bs, cb, opaque);
+    iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
+    iocb->ret = 0;
+
+    for (j = 0; j < qiov->niov; j++) {
+        uint64_t *buffer = qiov->iov[j].iov_base;
+
+        for (i = 0; i < qiov->iov[j].iov_len / 8; i++) {
+            /* 6-byte LBA + 2-byte range per entry */
+            uint64_t entry = le64_to_cpu(buffer[i]);
+            uint64_t sector = entry & 0x0000ffffffffffffULL;
+            uint16_t count = entry >> 48;
+
+            if (count == 0)
+                break;
+
+            ret = bdrv_discard(bs, sector * 512, count * 512);
+            if (!iocb->ret)
+                iocb->ret = ret;
+        }
+    }
+
+    qemu_bh_schedule(iocb->bh);
+
+    return &iocb->common;
+}
+
 static inline void ide_abort_command(IDEState *s)
 {
     s->status = READY_STAT | ERR_STAT;

@@ -475,6 +547,9 @@ handle_rw_error:
 
         if (s->dma_cmd == IDE_DMA_READ)
             op |= BM_STATUS_RETRY_READ;
+        else if (s->dma_cmd == IDE_DMA_TRIM)
+            op |= BM_STATUS_RETRY_TRIM;
+
         if (ide_handle_rw_error(s, -ret, op)) {
             return;
         }

@@ -517,6 +592,10 @@ handle_rw_error:
         s->bus->dma->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
                                             ide_dma_cb, s);
         break;
+    case IDE_DMA_TRIM:
+        s->bus->dma->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
+                                         ide_issue_trim, ide_dma_cb, s, 1);
+        break;
     }
 
     if (!s->bus->dma->aiocb) {

@@ -817,6 +896,17 @@ void ide_exec_cmd(IDEBus *bus, uint32_t
         return;
 
     switch(val) {
+    case WIN_DSM:
+        switch (s->feature) {
+        case DSM_TRIM:
+            if (!s->bs)
+                goto abort_cmd;
+            ide_sector_start_dma(s, IDE_DMA_TRIM);
+            break;
+        default:
+            goto abort_cmd;
+        }
+        break;
     case WIN_IDENTIFY:
         if (s->bs && s->drive_kind != IDE_CD) {
             if (s->drive_kind != IDE_CFATA)

hw/ide/internal.h +13 -1

Index: qemu/hw/ide/internal.h
===================================================================
@@ -62,7 +62,11 @@ typedef struct IDEDMAOps IDEDMAOps;
  */
 #define CFA_REQ_EXT_ERROR_CODE		0x03 /* CFA Request Extended Error Code */
 /*
- *	0x04->0x07 Reserved
+ *      0x04->0x05 Reserved
+ */
+#define WIN_DSM                         0x06
+/*
+ *      0x07 Reserved
  */
 #define WIN_SRST			0x08 /* ATAPI soft reset command */
 #define WIN_DEVICE_RESET		0x08

@@ -190,6 +194,9 @@ typedef struct IDEDMAOps IDEDMAOps;
 
 #define IDE_DMA_BUF_SECTORS 256
 
+/* feature values for Data Set Management */
+#define DSM_TRIM                        0x01
+
 #if (IDE_DMA_BUF_SECTORS < MAX_MULT_SECTORS)
 #error "IDE_DMA_BUF_SECTORS must be bigger or equal to MAX_MULT_SECTORS"
 #endif

@@ -382,6 +389,7 @@ struct unreported_events {
 enum ide_dma_cmd {
     IDE_DMA_READ,
     IDE_DMA_WRITE,
+    IDE_DMA_TRIM,
 };
 
 #define ide_cmd_is_read(s) \

@@ -517,6 +525,7 @@ struct IDEDeviceInfo {
 #define BM_STATUS_PIO_RETRY  0x10
 #define BM_STATUS_RETRY_READ  0x20
 #define BM_STATUS_RETRY_FLUSH 0x40
+#define BM_STATUS_RETRY_TRIM 0x80
 
 #define BM_CMD_START     0x01
 #define BM_CMD_READ      0x08

@@ -583,6 +592,9 @@ void ide_transfer_start(IDEState *s, uin
                         EndTransferFunc *end_transfer_func);
 void ide_transfer_stop(IDEState *s);
 void ide_set_inactive(IDEState *s);
+BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque);
 
 /* hw/ide/atapi.c */
 void ide_atapi_cmd(IDEState *s);

hw/ide/qdev.c +5 -0

Index: qemu/hw/ide/qdev.c
===================================================================
@@ -125,6 +125,11 @@ static int ide_drive_initfn(IDEDevice *d
     const char *serial;
     DriveInfo *dinfo;
 
+    if (dev->conf.discard_granularity && dev->conf.discard_granularity != 512) {
+        error_report("discard_granularity must be 512 for ide");
+        return -1;
+    }
+
     serial = dev->serial;
     if (!serial) {
         /* try to fall back to value set with legacy -drive serial=... */

hw/ide/macio.c +4 -0

Index: qemu/hw/ide/macio.c
===================================================================
@@ -154,6 +154,10 @@ static void pmac_ide_transfer_cb(void *o
         m->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
 		                  pmac_ide_transfer_cb, io);
         break;
+    case IDE_DMA_TRIM:
+        m->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
+                               ide_issue_trim, pmac_ide_transfer_cb, s, 1);
+        break;
     }
 
     if (!m->aiocb)

hw/ide/pci.c +3 -0

Index: qemu/hw/ide/pci.c
===================================================================
@@ -205,6 +205,9 @@ static void bmdma_restart_bh(void *opaqu
         }
     } else if (bm->status & BM_STATUS_RETRY_FLUSH) {
         ide_flush_cache(bmdma_active_if(bm));
+    } else if (bm->status & BM_STATUS_RETRY_TRIM) {
+        bm->status &= ~BM_STATUS_RETRY_TRIM;
+        bmdma_restart_dma(bm, IDE_DMA_TRIM);
     }
 }

Kevin Wolf

2011-06-10 11:44 UTC #4 lore raw

Am 19.05.2011 10:58, schrieb Christoph Hellwig:

Quoted 79 lines

Add support for TRIM sub function of the data set management command, and wire it up to the qemu discard infrastructure.

Signed-off-by Christoph Hellwig <[email protected]>

Index: qemu/hw/ide/core.c
===================================================================

hw/ide/core.c +52 -0

@@ -124,6 +124,9 @@ static void ide_identify(IDEState *s)
     put_le16(p + 66, 120);
     put_le16(p + 67, 120);
     put_le16(p + 68, 120);
+    if (dev && dev->conf.discard_granularity) {
+        put_le16(p + 69, (1 << 14)); /* determinate TRIM behavior */
+    }
 
     if (s->ncq_queues) {
         put_le16(p + 75, s->ncq_queues - 1);

@@ -157,6 +160,9 @@ static void ide_identify(IDEState *s)
     dev = s->unit ? s->bus->slave : s->bus->master;
     if (dev && dev->conf.physical_block_size)
         put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
+    if (dev && dev->conf.discard_granularity) {
+        put_le16(p + 169, 1); /* TRIM support */
+    }
 
     memcpy(s->identify_data, p, sizeof(s->identify_data));
     s->identify_set = 1;

@@ -299,6 +305,72 @@ static void ide_set_signature(IDEState *
     }
 }
 
+typedef struct TrimAIOCB {
+    BlockDriverAIOCB common;
+    QEMUBH *bh;
+    int ret;
+} TrimAIOCB;
+
+static void trim_aio_cancel(BlockDriverAIOCB *acb)
+{
+    TrimAIOCB *iocb = container_of(acb, TrimAIOCB, common);
+
+    qemu_bh_delete(iocb->bh);
+    iocb->bh = NULL;
+    qemu_aio_release(iocb);
+}
+
+static AIOPool trim_aio_pool = {
+    .aiocb_size         = sizeof(TrimAIOCB),
+    .cancel             = trim_aio_cancel,
+};
+
+static void ide_trim_bh_cb(void *opaque)
+{
+    TrimAIOCB *iocb = opaque;
+
+    iocb->common.cb(iocb->common.opaque, iocb->ret);
+
+    qemu_bh_delete(iocb->bh);
+    iocb->bh = NULL;
+
+    qemu_aio_release(iocb);
+}
+
+BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    TrimAIOCB *iocb;
+    int i, j, ret;
+
+    iocb = qemu_aio_get(&trim_aio_pool, bs, cb, opaque);
+    iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
+    iocb->ret = 0;
+
+    for (j = 0; j < qiov->niov; j++) {
+        uint64_t *buffer = qiov->iov[j].iov_base;
+

hw/ide/core.c static void ide_set_signature(IDEState * · line 363

Quoted 10 lines

hw/ide/core.c +10 -0

+        for (i = 0; i < qiov->iov[j].iov_len / 8; i++) {
+            /* 6-byte LBA + 2-byte range per entry */
+            uint64_t entry = le64_to_cpu(buffer[i]);
+            uint64_t sector = entry & 0x0000ffffffffffffULL;
+            uint16_t count = entry >> 48;
+
+            if (count == 0)
+                break;
+
+            ret = bdrv_discard(bs, sector * 512, count * 512);

Hm... bdrv_discard wants sector numbers instead of bytes, doesn't it?

If you agree, I'll send out and apply a fixed and rebased version.

Kevin

Kevin Wolf

2011-06-10 14:02 UTC #5 lore raw

In reply to Christoph Hellwig

Am 19.05.2011 10:58, schrieb Christoph Hellwig:

Quoted 10 lines

Add support for TRIM sub function of the data set management command, and wire it up to the qemu discard infrastructure.
Signed-off-by Christoph Hellwig <[email protected]>
Index: qemu/hw/ide/pci.c
===================================================================

hw/ide/pci.c static void bmdma_restart_bh(void *opaqu · line 212

Quoted 8 lines

hw/ide/pci.c +3 -0

@@ -205,6 +205,9 @@ static void bmdma_restart_bh(void *opaqu
         }
     } else if (bm->status & BM_STATUS_RETRY_FLUSH) {
         ide_flush_cache(bmdma_active_if(bm));
+    } else if (bm->status & BM_STATUS_RETRY_TRIM) {
+        bm->status &= ~BM_STATUS_RETRY_TRIM;
+        bmdma_restart_dma(bm, IDE_DMA_TRIM);
     }
 }

Just noticed that this is wrong. BM_STATUS_DMA_RETRY is always set at the same time, so this is dead code.

Kevin

Kevin Wolf

2011-06-14 15:52 UTC #6 lore raw

PATCH v2 3/3 ide: add TRIM support

In reply to Christoph Hellwig

From: Christoph Hellwig <[email protected]>

Add support for TRIM sub function of the data set management command, and wire it up to the qemu discard infrastructure.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
v2:
- Fixed bdrv_discard call (takes sectors, not bytes)
- Fixed TRIM restart after failure
- Fixed use of uninitialised variable
- Fixed coding style

---
 hw/ide/core.c     |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ide/internal.h |   14 +++++++-
 hw/ide/macio.c    |    4 ++
 hw/ide/pci.c      |    9 ++++-
 hw/ide/qdev.c     |    5 +++
 5 files changed, 124 insertions(+), 5 deletions(-)

hw/ide/core.c +95 -2

index 14bda82..ca17a43 100644
@@ -78,7 +78,7 @@ static void ide_identify(IDEState *s)
 {
     uint16_t *p;
     unsigned int oldsize;
-    IDEDevice *dev;
+    IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
 
     if (s->identify_set) {
 	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));

@@ -124,6 +124,9 @@ static void ide_identify(IDEState *s)
     put_le16(p + 66, 120);
     put_le16(p + 67, 120);
     put_le16(p + 68, 120);
+    if (dev && dev->conf.discard_granularity) {
+        put_le16(p + 69, (1 << 14)); /* determinate TRIM behavior */
+    }
 
     if (s->ncq_queues) {
         put_le16(p + 75, s->ncq_queues - 1);

@@ -154,9 +157,12 @@ static void ide_identify(IDEState *s)
     put_le16(p + 101, s->nb_sectors >> 16);
     put_le16(p + 102, s->nb_sectors >> 32);
     put_le16(p + 103, s->nb_sectors >> 48);
-    dev = s->unit ? s->bus->slave : s->bus->master;
+
     if (dev && dev->conf.physical_block_size)
         put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
+    if (dev && dev->conf.discard_granularity) {
+        put_le16(p + 169, 1); /* TRIM support */
+    }
 
     memcpy(s->identify_data, p, sizeof(s->identify_data));
     s->identify_set = 1;

@@ -299,6 +305,74 @@ static void ide_set_signature(IDEState *s)
     }
 }
 
+typedef struct TrimAIOCB {
+    BlockDriverAIOCB common;
+    QEMUBH *bh;
+    int ret;
+} TrimAIOCB;
+
+static void trim_aio_cancel(BlockDriverAIOCB *acb)
+{
+    TrimAIOCB *iocb = container_of(acb, TrimAIOCB, common);
+
+    qemu_bh_delete(iocb->bh);
+    iocb->bh = NULL;
+    qemu_aio_release(iocb);
+}
+
+static AIOPool trim_aio_pool = {
+    .aiocb_size         = sizeof(TrimAIOCB),
+    .cancel             = trim_aio_cancel,
+};
+
+static void ide_trim_bh_cb(void *opaque)
+{
+    TrimAIOCB *iocb = opaque;
+
+    iocb->common.cb(iocb->common.opaque, iocb->ret);
+
+    qemu_bh_delete(iocb->bh);
+    iocb->bh = NULL;
+
+    qemu_aio_release(iocb);
+}
+
+BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    TrimAIOCB *iocb;
+    int i, j, ret;
+
+    iocb = qemu_aio_get(&trim_aio_pool, bs, cb, opaque);
+    iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
+    iocb->ret = 0;
+
+    for (j = 0; j < qiov->niov; j++) {
+        uint64_t *buffer = qiov->iov[j].iov_base;
+
+        for (i = 0; i < qiov->iov[j].iov_len / 8; i++) {
+            /* 6-byte LBA + 2-byte range per entry */
+            uint64_t entry = le64_to_cpu(buffer[i]);
+            uint64_t sector = entry & 0x0000ffffffffffffULL;
+            uint16_t count = entry >> 48;
+
+            if (count == 0) {
+                break;
+            }
+
+            ret = bdrv_discard(bs, sector, count);
+            if (!iocb->ret) {
+                iocb->ret = ret;
+            }
+        }
+    }
+
+    qemu_bh_schedule(iocb->bh);
+
+    return &iocb->common;
+}
+
 static inline void ide_abort_command(IDEState *s)
 {
     s->status = READY_STAT | ERR_STAT;

@@ -474,6 +548,9 @@ handle_rw_error:
 
         if (s->dma_cmd == IDE_DMA_READ)
             op |= BM_STATUS_RETRY_READ;
+        else if (s->dma_cmd == IDE_DMA_TRIM)
+            op |= BM_STATUS_RETRY_TRIM;
+
         if (ide_handle_rw_error(s, -ret, op)) {
             return;
         }

@@ -519,6 +596,10 @@ handle_rw_error:
         s->bus->dma->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
                                             ide_dma_cb, s);
         break;
+    case IDE_DMA_TRIM:
+        s->bus->dma->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
+                                         ide_issue_trim, ide_dma_cb, s, 1);
+        break;
     }
 
     if (!s->bus->dma->aiocb) {

@@ -818,6 +899,18 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
         return;
 
     switch(val) {
+    case WIN_DSM:
+        switch (s->feature) {
+        case DSM_TRIM:
+            if (!s->bs) {
+                goto abort_cmd;
+            }
+            ide_sector_start_dma(s, IDE_DMA_TRIM);
+            break;
+        default:
+            goto abort_cmd;
+        }
+        break;
     case WIN_IDENTIFY:
         if (s->bs && s->drive_kind != IDE_CD) {
             if (s->drive_kind != IDE_CFATA)

hw/ide/internal.h +13 -1

index ea3edf5..02e805f 100644
@@ -62,7 +62,11 @@ typedef struct IDEDMAOps IDEDMAOps;
  */
 #define CFA_REQ_EXT_ERROR_CODE		0x03 /* CFA Request Extended Error Code */
 /*
- *	0x04->0x07 Reserved
+ *      0x04->0x05 Reserved
+ */
+#define WIN_DSM                         0x06
+/*
+ *      0x07 Reserved
  */
 #define WIN_SRST			0x08 /* ATAPI soft reset command */
 #define WIN_DEVICE_RESET		0x08

@@ -190,6 +194,9 @@ typedef struct IDEDMAOps IDEDMAOps;
 
 #define IDE_DMA_BUF_SECTORS 256
 
+/* feature values for Data Set Management */
+#define DSM_TRIM                        0x01
+
 #if (IDE_DMA_BUF_SECTORS < MAX_MULT_SECTORS)
 #error "IDE_DMA_BUF_SECTORS must be bigger or equal to MAX_MULT_SECTORS"
 #endif

@@ -382,6 +389,7 @@ struct unreported_events {
 enum ide_dma_cmd {
     IDE_DMA_READ,
     IDE_DMA_WRITE,
+    IDE_DMA_TRIM,
 };
 
 #define ide_cmd_is_read(s) \

@@ -521,6 +529,7 @@ struct IDEDeviceInfo {
 #define BM_STATUS_PIO_RETRY  0x10
 #define BM_STATUS_RETRY_READ  0x20
 #define BM_STATUS_RETRY_FLUSH 0x40
+#define BM_STATUS_RETRY_TRIM 0x80
 
 #define BM_MIGRATION_COMPAT_STATUS_BITS \
         (BM_STATUS_DMA_RETRY | BM_STATUS_PIO_RETRY | \

@@ -591,6 +600,9 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
                         EndTransferFunc *end_transfer_func);
 void ide_transfer_stop(IDEState *s);
 void ide_set_inactive(IDEState *s);
+BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque);
 
 /* hw/ide/atapi.c */
 void ide_atapi_cmd(IDEState *s);

hw/ide/macio.c +4 -0

index 099b8cb..7daeb31 100644
@@ -154,6 +154,10 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         m->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
 		                  pmac_ide_transfer_cb, io);
         break;
+    case IDE_DMA_TRIM:
+        m->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
+                               ide_issue_trim, pmac_ide_transfer_cb, s, 1);
+        break;
     }
 
     if (!m->aiocb)

hw/ide/pci.c +7 -2

index 80c5794..c940375 100644
@@ -200,8 +200,13 @@ static void bmdma_restart_bh(void *opaque)
     is_read = !!(bus->error_status & BM_STATUS_RETRY_READ);
 
     if (bus->error_status & BM_STATUS_DMA_RETRY) {
-        bus->error_status &= ~(BM_STATUS_DMA_RETRY | BM_STATUS_RETRY_READ);
-        bmdma_restart_dma(bm, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
+        if (bus->error_status & BM_STATUS_RETRY_TRIM) {
+            bus->error_status &= ~BM_STATUS_RETRY_TRIM;
+            bmdma_restart_dma(bm, IDE_DMA_TRIM);
+        } else {
+            bus->error_status &= ~(BM_STATUS_DMA_RETRY | BM_STATUS_RETRY_READ);
+            bmdma_restart_dma(bm, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
+        }
     } else if (bus->error_status & BM_STATUS_PIO_RETRY) {
         bus->error_status &= ~(BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ);
         if (is_read) {

hw/ide/qdev.c +5 -0

index 3f9dc89..d9b8f24 100644
@@ -125,6 +125,11 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     const char *serial;
     DriveInfo *dinfo;
 
+    if (dev->conf.discard_granularity && dev->conf.discard_granularity != 512) {
+        error_report("discard_granularity must be 512 for ide");
+        return -1;
+    }
+
     serial = dev->serial;
     if (!serial) {
         /* try to fall back to value set with legacy -drive serial=... */

Signature · 1 line

1.7.5.2

Christoph Hellwig

author

2011-06-07 11:13 UTC #7 lore raw

In reply to Christoph Hellwig

ping?

Quoted 4 lines · Christoph Hellwig

This patchset resurrects my older patches to add TRIM support. The old approach to pass an action handler through the IDE subsystem doesn't work any more after the refactoring for the AHCI driver, so not we simple switch on the type of command passed in.

---end quoted text---

Kevin Wolf

2011-06-07 14:09 UTC #8 lore raw

Am 07.06.2011 13:13, schrieb Christoph Hellwig:

Quoted 1 line

ping?

Sorry, forgot to answer... The problem that I had with this was that you add to the BM status register abuse with BM_STATUS_RETRY_TRIM. I intended to look into this and then review the rest of it.

We'll need to have a fix for all the other abused bits anyway, so I guess we can as well temporarily abuse one more bit (it happens to be reserved/hardwired to 0 in my specs) and fix it only on top.

Kevin