[Qemu-devel] [PATCH 0/5] My remaining block/SCSI patches for 1.0

[Qemu-devel] [PATCH 0/5] My remaining block/SCSI patches for 1.0

[Paolo Bonzini]

The first three replace patches 20/21 and are basically rewritten with
input from Kevin.

The fourth is new.  I had it queued for 1.1, but it turns out it is
needed now or scsi-block might access some requests incorrectly when
restarting after an error.

The fifth is basically the same as patch 35 from the first submission.

The last three patches had been submitted Sep 20 and were lost at sea;
support for eject requests is required by udev 173.

Paolo Bonzini (8):
  scsi: do not call transfer_data after canceling a request
  scsi-disk: bump SCSIRequest reference count until aio completion runs
  scsi-generic: bump SCSIRequest reference count until aio completion runs
  scsi: push request restart to SCSIDevice
  scsi-disk: add scsi-block for device passthrough
  block: add eject request callback
  atapi: implement eject requests
  scsi-disk: implement eject requests

 block.c           |    7 ++
 block.h           |    7 ++
 blockdev.c        |    8 +-
 hw/ide/atapi.c    |   11 ++-
 hw/ide/core.c     |   13 +++
 hw/scsi-bus.c     |   79 ++++++++++++++++-
 hw/scsi-disk.c    |  254 ++++++++++++++++++++++++++++++++++++++--------------
 hw/scsi-generic.c |   18 ++++
 hw/scsi.h         |    6 ++
 trace-events      |    1 +
 10 files changed, 325 insertions(+), 79 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/8] scsi: do not call transfer_data after canceling a request

[Paolo Bonzini]

Otherwise, if cancellation is "faked" by the AIO layer and goes
through qemu_aio_flush, the whole request is completed synchronously
during scsi_req_cancel.

Using the enqueued flag would work here, but not in the next patches,
so I'm introducing a new io_canceled flag.  That's because scsi_req_data
is a synchronous callback and the enqueued flag might be reset by the
time it returns.  scsi-disk cannot unref the request until after calling
scsi_req_data.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |   23 +++++++++++++++++++----
 hw/scsi.h     |    1 +
 trace-events  |    1 +
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 72c0dd2..cd0c2cd 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1107,8 +1107,12 @@ void scsi_req_continue(SCSIRequest *req)
    Once it completes, calling scsi_req_continue will restart I/O.  */
 void scsi_req_data(SCSIRequest *req, int len)
 {
-    trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
-    req->bus->info->transfer_data(req, len);
+    if (req->io_canceled) {
+        trace_scsi_req_data_canceled(req->dev->id, req->lun, req->tag, len);
+    } else {
+        trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
+        req->bus->info->transfer_data(req, len);
+    }
 }
 
 void scsi_req_print(SCSIRequest *req)
@@ -1173,11 +1177,15 @@ void scsi_req_complete(SCSIRequest *req, int status)
 
 void scsi_req_cancel(SCSIRequest *req)
 {
-    if (req->ops->cancel_io) {
-        req->ops->cancel_io(req);
+    if (!req->enqueued) {
+        return;
     }
     scsi_req_ref(req);
     scsi_req_dequeue(req);
+    req->io_canceled = true;
+    if (req->ops->cancel_io) {
+        req->ops->cancel_io(req);
+    }
     if (req->bus->info->cancel) {
         req->bus->info->cancel(req);
     }
@@ -1186,10 +1194,17 @@ void scsi_req_cancel(SCSIRequest *req)
 
 void scsi_req_abort(SCSIRequest *req, int status)
 {
+    if (!req->enqueued) {
+        return;
+    }
+    scsi_req_ref(req);
+    scsi_req_dequeue(req);
+    req->io_canceled = true;
     if (req->ops->cancel_io) {
         req->ops->cancel_io(req);
     }
     scsi_req_complete(req, status);
+    scsi_req_unref(req);
 }
 
 void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense)
diff --git a/hw/scsi.h b/hw/scsi.h
index 8ea744a..fcc3455 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -51,6 +51,7 @@ struct SCSIRequest {
     uint8_t sense[SCSI_SENSE_BUF_SIZE];
     uint32_t sense_len;
     bool enqueued;
+    bool io_canceled;
     void *hba_private;
     QTAILQ_ENTRY(SCSIRequest) next;
 };
diff --git a/trace-events b/trace-events
index 7f9cec4..148084e 100644
--- a/trace-events
+++ b/trace-events
@@ -277,6 +277,7 @@ usb_host_claim_port(int bus, int hub, int port) "bus %d, hub addr %d, port %d"
 # hw/scsi-bus.c
 scsi_req_alloc(int target, int lun, int tag) "target %d lun %d tag %d"
 scsi_req_data(int target, int lun, int tag, int len) "target %d lun %d tag %d len %d"
+scsi_req_data_canceled(int target, int lun, int tag, int len) "target %d lun %d tag %d len %d"
 scsi_req_dequeue(int target, int lun, int tag) "target %d lun %d tag %d"
 scsi_req_continue(int target, int lun, int tag) "target %d lun %d tag %d"
 scsi_req_parsed(int target, int lun, int tag, int cmd, int mode, int xfer) "target %d lun %d tag %d command %d dir %d length %d"
-- 
1.7.6

[Qemu-devel] [PATCH 2/8] scsi-disk: bump SCSIRequest reference count until aio completion runs

[Paolo Bonzini]

In some cases a request may be canceled before the completion callback
runs.  Keep a reference to the request between starting an AIO operation
and the corresponding scsi_req_cancel or scsi_*_complete.

When a request has to be retried, the request can be dropped because
scsi_dma_restart_bh only looks at requests that are enqueued.  As such,
they always have at least a reference.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   48 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index d12db92..bf5686a 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -102,8 +102,14 @@ static void scsi_cancel_io(SCSIRequest *req)
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
 
     DPRINTF("Cancel tag=0x%x\n", req->tag);
+    r->status &= ~(SCSI_REQ_STATUS_RETRY | SCSI_REQ_STATUS_RETRY_TYPE_MASK);
     if (r->req.aiocb) {
         bdrv_aio_cancel(r->req.aiocb);
+
+        /* This reference was left in by scsi_*_data.  We take ownership of
+         * it the moment scsi_req_cancel is called, independent of whether
+         * bdrv_aio_cancel completes the request or not.  */
+        scsi_req_unref(&r->req);
     }
     r->req.aiocb = NULL;
 }
@@ -134,7 +140,7 @@ static void scsi_read_complete(void * opaque, int ret)
 
     if (ret) {
         if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
-            return;
+            goto done;
         }
     }
 
@@ -144,6 +150,11 @@ static void scsi_read_complete(void * opaque, int ret)
     r->sector += n;
     r->sector_count -= n;
     scsi_req_data(&r->req, r->qiov.size);
+
+done:
+    if (!r->req.io_canceled) {
+        scsi_req_unref(&r->req);
+    }
 }
 
 static void scsi_flush_complete(void * opaque, int ret)
@@ -158,11 +169,16 @@ static void scsi_flush_complete(void * opaque, int ret)
 
     if (ret < 0) {
         if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_FLUSH)) {
-            return;
+            goto done;
         }
     }
 
     scsi_req_complete(&r->req, GOOD);
+
+done:
+    if (!r->req.io_canceled) {
+        scsi_req_unref(&r->req);
+    }
 }
 
 /* Read more data from scsi device into buffer.  */
@@ -188,6 +204,8 @@ static void scsi_read_data(SCSIRequest *req)
     /* No data transfer may already be in progress */
     assert(r->req.aiocb == NULL);
 
+    /* The request is used as the AIO opaque value, so add a ref.  */
+    scsi_req_ref(&r->req);
     if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
         DPRINTF("Data transfer direction invalid\n");
         scsi_read_complete(r, -EINVAL);
@@ -196,7 +214,9 @@ static void scsi_read_data(SCSIRequest *req)
 
     if (s->tray_open) {
         scsi_read_complete(r, -ENOMEDIUM);
+        return;
     }
+
     n = scsi_init_iovec(r);
     bdrv_acct_start(s->qdev.conf.bs, &r->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
     r->req.aiocb = bdrv_aio_readv(s->qdev.conf.bs, r->sector, &r->qiov, n,
@@ -206,6 +226,13 @@ static void scsi_read_data(SCSIRequest *req)
     }
 }
 
+/*
+ * scsi_handle_rw_error has two return values.  0 means that the error
+ * must be ignored, 1 means that the error has been processed and the
+ * caller should not do anything else for this request.  Note that
+ * scsi_handle_rw_error always manages its reference counts, independent
+ * of the return value.
+ */
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
 {
     int is_read = (type == SCSI_REQ_STATUS_RETRY_READ);
@@ -226,6 +253,11 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         bdrv_mon_event(s->qdev.conf.bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RUN_STATE_IO_ERROR);
         bdrv_iostatus_set_err(s->qdev.conf.bs, error);
+
+        /* No need to save a reference, because scsi_dma_restart_bh just
+         * looks at the request list.  If a request is canceled, the
+         * retry request is just dropped.
+         */
     } else {
         switch (error) {
         case ENOMEDIUM:
@@ -259,7 +291,7 @@ static void scsi_write_complete(void * opaque, int ret)
 
     if (ret) {
         if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_WRITE)) {
-            return;
+            goto done;
         }
     }
 
@@ -273,6 +305,11 @@ static void scsi_write_complete(void * opaque, int ret)
         DPRINTF("Write complete tag=0x%x more=%d\n", r->req.tag, r->qiov.size);
         scsi_req_data(&r->req, r->qiov.size);
     }
+
+done:
+    if (!r->req.io_canceled) {
+        scsi_req_unref(&r->req);
+    }
 }
 
 static void scsi_write_data(SCSIRequest *req)
@@ -284,6 +321,8 @@ static void scsi_write_data(SCSIRequest *req)
     /* No data transfer may already be in progress */
     assert(r->req.aiocb == NULL);
 
+    /* The request is used as the AIO opaque value, so add a ref.  */
+    scsi_req_ref(&r->req);
     if (r->req.cmd.mode != SCSI_XFER_TO_DEV) {
         DPRINTF("Data transfer direction invalid\n");
         scsi_write_complete(r, -EINVAL);
@@ -294,6 +333,7 @@ static void scsi_write_data(SCSIRequest *req)
     if (n) {
         if (s->tray_open) {
             scsi_write_complete(r, -ENOMEDIUM);
+            return;
         }
         bdrv_acct_start(s->qdev.conf.bs, &r->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_WRITE);
         r->req.aiocb = bdrv_aio_writev(s->qdev.conf.bs, r->sector, &r->qiov, n,
@@ -1332,6 +1372,8 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
         r->iov.iov_len = rc;
         break;
     case SYNCHRONIZE_CACHE:
+        /* The request is used as the AIO opaque value, so add a ref.  */
+        scsi_req_ref(&r->req);
         bdrv_acct_start(s->qdev.conf.bs, &r->acct, 0, BDRV_ACCT_FLUSH);
         r->req.aiocb = bdrv_aio_flush(s->qdev.conf.bs, scsi_flush_complete, r);
         if (r->req.aiocb == NULL) {
-- 
1.7.6

[Qemu-devel] [PATCH 3/8] scsi-generic: bump SCSIRequest reference count until aio completion runs

[Paolo Bonzini]

Same as before, but for scsi-generic.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-generic.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index b30cf09..ff738fd 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -113,6 +113,9 @@ static void scsi_command_complete(void *opaque, int ret)
             r, r->req.tag, status);
 
     scsi_req_complete(&r->req, status);
+    if (!r->req.io_canceled) {
+        scsi_req_unref(&r->req);
+    }
 }
 
 /* Cancel a pending data transfer.  */
@@ -123,6 +126,11 @@ static void scsi_cancel_io(SCSIRequest *req)
     DPRINTF("Cancel tag=0x%x\n", req->tag);
     if (r->req.aiocb) {
         bdrv_aio_cancel(r->req.aiocb);
+
+        /* This reference was left in by scsi_*_data.  We take ownership of
+         * it independent of whether bdrv_aio_cancel completes the request
+         * or not.  */
+        scsi_req_unref(&r->req);
     }
     r->req.aiocb = NULL;
 }
@@ -183,6 +191,9 @@ static void scsi_read_complete(void * opaque, int ret)
         bdrv_set_buffer_alignment(s->conf.bs, s->blocksize);
 
         scsi_req_data(&r->req, len);
+        if (!r->req.io_canceled) {
+            scsi_req_unref(&r->req);
+        }
     }
 }
 
@@ -194,6 +205,9 @@ static void scsi_read_data(SCSIRequest *req)
     int ret;
 
     DPRINTF("scsi_read_data 0x%x\n", req->tag);
+
+    /* The request is used as the AIO opaque value, so add a ref.  */
+    scsi_req_ref(&r->req);
     if (r->len == -1) {
         scsi_command_complete(r, 0);
         return;
@@ -242,6 +256,8 @@ static void scsi_write_data(SCSIRequest *req)
         return;
     }
 
+    /* The request is used as the AIO opaque value, so add a ref.  */
+    scsi_req_ref(&r->req);
     ret = execute_command(s->conf.bs, r, SG_DXFER_TO_DEV, scsi_write_complete);
     if (ret < 0) {
         scsi_command_complete(r, ret);
@@ -285,6 +301,8 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
             g_free(r->buf);
         r->buflen = 0;
         r->buf = NULL;
+        /* The request is used as the AIO opaque value, so add a ref.  */
+        scsi_req_ref(&r->req);
         ret = execute_command(s->conf.bs, r, SG_DXFER_NONE, scsi_command_complete);
         if (ret < 0) {
             scsi_command_complete(r, ret);
-- 
1.7.6

[Qemu-devel] [PATCH 4/8] scsi: push request restart to SCSIDevice

[Paolo Bonzini]

The request restart mechanism is generic and could be reused for
scsi-generic.  In the meanwhile, pushing it to SCSIDevice avoids
that scsi_dma_restart_bh looks at SCSIGenericReqs when working on
a scsi-block device.

The code is the same that is already in hw/scsi-disk.c, with
the type flags replaced by req->cmd.mode and a more generic way to
requeue SCSI_XFER_NONE commands.

I also added a missing call to qemu_del_vm_change_state_handler.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c  |   56 +++++++++++++++++++++++++++++++++++++++++
 hw/scsi-disk.c |   76 +++++--------------------------------------------------
 hw/scsi.h      |    5 +++
 3 files changed, 68 insertions(+), 69 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index cd0c2cd..243d4aa 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -8,6 +8,7 @@
 
 static char *scsibus_get_fw_dev_path(DeviceState *dev);
 static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf);
+static void scsi_req_dequeue(SCSIRequest *req);
 static int scsi_build_sense(uint8_t *in_buf, int in_len,
                             uint8_t *buf, int len, bool fixed);
 
@@ -33,6 +34,53 @@ void scsi_bus_new(SCSIBus *bus, DeviceState *host, const SCSIBusInfo *info)
     bus->qbus.allow_hotplug = 1;
 }
 
+static void scsi_dma_restart_bh(void *opaque)
+{
+    SCSIDevice *s = opaque;
+    SCSIRequest *req, *next;
+
+    qemu_bh_delete(s->bh);
+    s->bh = NULL;
+
+    QTAILQ_FOREACH_SAFE(req, &s->requests, next, next) {
+        scsi_req_ref(req);
+        if (req->retry) {
+            req->retry = false;
+            switch (req->cmd.mode) {
+            case SCSI_XFER_FROM_DEV:
+            case SCSI_XFER_TO_DEV:
+                scsi_req_continue(req);
+                break;
+            case SCSI_XFER_NONE:
+                scsi_req_dequeue(req);
+                scsi_req_enqueue(req);
+                break;
+            }
+        }
+        scsi_req_unref(req);
+    }
+}
+
+void scsi_req_retry(SCSIRequest *req)
+{
+    /* No need to save a reference, because scsi_dma_restart_bh just
+     * looks at the request list.  */
+    req->retry = true;
+}
+
+static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
+{
+    SCSIDevice *s = opaque;
+
+    if (!running) {
+        return;
+    }
+    if (!s->bh) {
+        s->bh = qemu_bh_new(scsi_dma_restart_bh, s);
+        qemu_bh_schedule(s->bh);
+    }
+}
+
 static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
@@ -83,6 +131,10 @@ static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
     dev->info = info;
     QTAILQ_INIT(&dev->requests);
     rc = dev->info->init(dev);
+    if (rc == 0) {
+        dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
+                                                         dev);
+    }
 
 err:
     return rc;
@@ -92,6 +144,9 @@ static int scsi_qdev_exit(DeviceState *qdev)
 {
     SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
 
+    if (dev->vmsentry) {
+        qemu_del_vm_change_state_handler(dev->vmsentry);
+    }
     if (dev->info->destroy) {
         dev->info->destroy(dev);
     }
@@ -586,6 +641,7 @@ int32_t scsi_req_enqueue(SCSIRequest *req)
 static void scsi_req_dequeue(SCSIRequest *req)
 {
     trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag);
+    req->retry = false;
     if (req->enqueued) {
         QTAILQ_REMOVE(&req->dev->requests, req, next);
         req->enqueued = false;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index bf5686a..f1f9335 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -42,12 +42,6 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #define SCSI_DMA_BUF_SIZE    131072
 #define SCSI_MAX_INQUIRY_LEN 256
 
-#define SCSI_REQ_STATUS_RETRY           0x01
-#define SCSI_REQ_STATUS_RETRY_TYPE_MASK 0x06
-#define SCSI_REQ_STATUS_RETRY_READ      0x00
-#define SCSI_REQ_STATUS_RETRY_WRITE     0x02
-#define SCSI_REQ_STATUS_RETRY_FLUSH     0x04
-
 typedef struct SCSIDiskState SCSIDiskState;
 
 typedef struct SCSIDiskReq {
@@ -58,7 +52,6 @@ typedef struct SCSIDiskReq {
     uint32_t buflen;
     struct iovec iov;
     QEMUIOVector qiov;
-    uint32_t status;
     BlockAcctCookie acct;
 } SCSIDiskReq;
 
@@ -75,8 +68,7 @@ struct SCSIDiskState
     bool tray_locked;
 };
 
-static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
-static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf);
+static int scsi_handle_rw_error(SCSIDiskReq *r, int error);
 
 static void scsi_free_request(SCSIRequest *req)
 {
@@ -102,7 +94,6 @@ static void scsi_cancel_io(SCSIRequest *req)
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
 
     DPRINTF("Cancel tag=0x%x\n", req->tag);
-    r->status &= ~(SCSI_REQ_STATUS_RETRY | SCSI_REQ_STATUS_RETRY_TYPE_MASK);
     if (r->req.aiocb) {
         bdrv_aio_cancel(r->req.aiocb);
 
@@ -139,7 +130,7 @@ static void scsi_read_complete(void * opaque, int ret)
     }
 
     if (ret) {
-        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
+        if (scsi_handle_rw_error(r, -ret)) {
             goto done;
         }
     }
@@ -168,7 +159,7 @@ static void scsi_flush_complete(void * opaque, int ret)
     }
 
     if (ret < 0) {
-        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_FLUSH)) {
+        if (scsi_handle_rw_error(r, -ret)) {
             goto done;
         }
     }
@@ -233,9 +224,9 @@ static void scsi_read_data(SCSIRequest *req)
  * scsi_handle_rw_error always manages its reference counts, independent
  * of the return value.
  */
-static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
+static int scsi_handle_rw_error(SCSIDiskReq *r, int error)
 {
-    int is_read = (type == SCSI_REQ_STATUS_RETRY_READ);
+    int is_read = (r->req.cmd.xfer == SCSI_XFER_FROM_DEV);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     BlockErrorAction action = bdrv_get_on_error(s->qdev.conf.bs, is_read);
 
@@ -247,17 +238,10 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
 
-        type &= SCSI_REQ_STATUS_RETRY_TYPE_MASK;
-        r->status |= SCSI_REQ_STATUS_RETRY | type;
-
         bdrv_mon_event(s->qdev.conf.bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RUN_STATE_IO_ERROR);
         bdrv_iostatus_set_err(s->qdev.conf.bs, error);
-
-        /* No need to save a reference, because scsi_dma_restart_bh just
-         * looks at the request list.  If a request is canceled, the
-         * retry request is just dropped.
-         */
+        scsi_req_retry(&r->req);
     } else {
         switch (error) {
         case ENOMEDIUM:
@@ -290,7 +274,7 @@ static void scsi_write_complete(void * opaque, int ret)
     }
 
     if (ret) {
-        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_WRITE)) {
+        if (scsi_handle_rw_error(r, -ret)) {
             goto done;
         }
     }
@@ -347,51 +331,6 @@ static void scsi_write_data(SCSIRequest *req)
     }
 }
 
-static void scsi_dma_restart_bh(void *opaque)
-{
-    SCSIDiskState *s = opaque;
-    SCSIRequest *req;
-    SCSIDiskReq *r;
-
-    qemu_bh_delete(s->bh);
-    s->bh = NULL;
-
-    QTAILQ_FOREACH(req, &s->qdev.requests, next) {
-        r = DO_UPCAST(SCSIDiskReq, req, req);
-        if (r->status & SCSI_REQ_STATUS_RETRY) {
-            int status = r->status;
-
-            r->status &=
-                ~(SCSI_REQ_STATUS_RETRY | SCSI_REQ_STATUS_RETRY_TYPE_MASK);
-
-            switch (status & SCSI_REQ_STATUS_RETRY_TYPE_MASK) {
-            case SCSI_REQ_STATUS_RETRY_READ:
-                scsi_read_data(&r->req);
-                break;
-            case SCSI_REQ_STATUS_RETRY_WRITE:
-                scsi_write_data(&r->req);
-                break;
-            case SCSI_REQ_STATUS_RETRY_FLUSH:
-                scsi_send_command(&r->req, r->req.cmd.buf);
-                break;
-            }
-        }
-    }
-}
-
-static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
-{
-    SCSIDiskState *s = opaque;
-
-    if (!running) {
-        return;
-    }
-    if (!s->bh) {
-        s->bh = qemu_bh_new(scsi_dma_restart_bh, s);
-        qemu_bh_schedule(s->bh);
-    }
-}
-
 /* Return a pointer to the data buffer.  */
 static uint8_t *scsi_get_buf(SCSIRequest *req)
 {
@@ -1591,7 +1530,6 @@ static int scsi_initfn(SCSIDevice *dev)
     }
     bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
 
-    qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
     bdrv_iostatus_enable(s->qdev.conf.bs);
     add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
     return 0;
diff --git a/hw/scsi.h b/hw/scsi.h
index fcc3455..ff8fdd0 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -3,6 +3,7 @@
 
 #include "qdev.h"
 #include "block.h"
+#include "sysemu.h"
 
 #define MAX_SCSI_DEVS	255
 
@@ -52,6 +53,7 @@ struct SCSIRequest {
     uint32_t sense_len;
     bool enqueued;
     bool io_canceled;
+    bool retry;
     void *hba_private;
     QTAILQ_ENTRY(SCSIRequest) next;
 };
@@ -59,6 +61,8 @@ struct SCSIRequest {
 struct SCSIDevice
 {
     DeviceState qdev;
+    VMChangeStateEntry *vmsentry;
+    QEMUBH *bh;
     uint32_t id;
     BlockConf conf;
     SCSIDeviceInfo *info;
@@ -194,6 +198,7 @@ uint8_t *scsi_req_get_buf(SCSIRequest *req);
 int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
 void scsi_req_abort(SCSIRequest *req, int status);
 void scsi_req_cancel(SCSIRequest *req);
+void scsi_req_retry(SCSIRequest *req);
 void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
 int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed);
 SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun);
-- 
1.7.6

[Qemu-devel] [PATCH 5/8] scsi-disk: add scsi-block for device passthrough

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 118 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index f1f9335..f407e2b 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -39,6 +39,10 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #include "blockdev.h"
 #include "block_int.h"
 
+#ifdef __linux
+#include <scsi/sg.h>
+#endif
+
 #define SCSI_DMA_BUF_SIZE    131072
 #define SCSI_MAX_INQUIRY_LEN 256
 
@@ -1588,6 +1592,105 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
     return req;
 }
 
+#ifdef __linux__
+static int get_device_type(SCSIDiskState *s)
+{
+    BlockDriverState *bdrv = s->qdev.conf.bs;
+    uint8_t cmd[16];
+    uint8_t buf[36];
+    uint8_t sensebuf[8];
+    sg_io_hdr_t io_header;
+    int ret;
+
+    memset(cmd, 0, sizeof(cmd));
+    memset(buf, 0, sizeof(buf));
+    cmd[0] = INQUIRY;
+    cmd[4] = sizeof(buf);
+
+    memset(&io_header, 0, sizeof(io_header));
+    io_header.interface_id = 'S';
+    io_header.dxfer_direction = SG_DXFER_FROM_DEV;
+    io_header.dxfer_len = sizeof(buf);
+    io_header.dxferp = buf;
+    io_header.cmdp = cmd;
+    io_header.cmd_len = sizeof(cmd);
+    io_header.mx_sb_len = sizeof(sensebuf);
+    io_header.sbp = sensebuf;
+    io_header.timeout = 6000; /* XXX */
+
+    ret = bdrv_ioctl(bdrv, SG_IO, &io_header);
+    if (ret < 0 || io_header.driver_status || io_header.host_status) {
+        return -1;
+    }
+    s->qdev.type = buf[0];
+    s->removable = (buf[1] & 0x80) != 0;
+    return 0;
+}
+
+static int scsi_block_initfn(SCSIDevice *dev)
+{
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    int sg_version;
+    int rc;
+
+    if (!s->qdev.conf.bs) {
+        error_report("scsi-block: drive property not set");
+        return -1;
+    }
+
+    /* check we are using a driver managing SG_IO (version 3 and after) */
+    if (bdrv_ioctl(s->qdev.conf.bs, SG_GET_VERSION_NUM, &sg_version) < 0 ||
+        sg_version < 30000) {
+        error_report("scsi-block: scsi generic interface too old");
+        return -1;
+    }
+
+    /* get device type from INQUIRY data */
+    rc = get_device_type(s);
+    if (rc < 0) {
+        error_report("scsi-block: INQUIRY failed");
+        return -1;
+    }
+
+    /* Make a guess for the block size, we'll fix it when the guest sends.
+     * READ CAPACITY.  If they don't, they likely would assume these sizes
+     * anyway. (TODO: check in /sys).
+     */
+    if (s->qdev.type == TYPE_ROM || s->qdev.type == TYPE_WORM) {
+        s->qdev.blocksize = 2048;
+    } else {
+        s->qdev.blocksize = 512;
+    }
+    return scsi_initfn(&s->qdev);
+}
+
+static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
+                                           uint32_t lun, uint8_t *buf,
+                                           void *hba_private)
+{
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
+
+    switch (buf[0]) {
+    case READ_6:
+    case READ_10:
+    case READ_12:
+    case READ_16:
+    case WRITE_6:
+    case WRITE_10:
+    case WRITE_12:
+    case WRITE_16:
+    case WRITE_VERIFY_10:
+    case WRITE_VERIFY_12:
+    case WRITE_VERIFY_16:
+        return scsi_req_alloc(&scsi_disk_reqops, &s->qdev, tag, lun,
+                              hba_private);
+    }
+
+    return scsi_req_alloc(&scsi_generic_req_ops, &s->qdev, tag, lun,
+                          hba_private);
+}
+#endif
+
 #define DEFINE_SCSI_DISK_PROPERTIES()                           \
     DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),          \
     DEFINE_PROP_STRING("ver",  SCSIDiskState, version),         \
@@ -1623,6 +1726,21 @@ static SCSIDeviceInfo scsi_disk_info[] = {
             DEFINE_SCSI_DISK_PROPERTIES(),
             DEFINE_PROP_END_OF_LIST(),
         },
+#ifdef __linux__
+    },{
+        .qdev.name    = "scsi-block",
+        .qdev.fw_name = "disk",
+        .qdev.desc    = "SCSI block device passthrough",
+        .qdev.size    = sizeof(SCSIDiskState),
+        .qdev.reset   = scsi_disk_reset,
+        .init         = scsi_block_initfn,
+        .destroy      = scsi_destroy,
+        .alloc_req    = scsi_block_new_request,
+        .qdev.props   = (Property[]) {
+            DEFINE_SCSI_DISK_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        },
+#endif
     },{
         .qdev.name    = "scsi-disk", /* legacy -device scsi-disk */
         .qdev.fw_name = "disk",
-- 
1.7.6

[Qemu-devel] [PATCH 6/8] block: add eject request callback

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c    |    7 +++++++
 block.h    |    7 +++++++
 blockdev.c |    8 +++++---
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 9873b57..53e21ba 100644
--- a/block.c
+++ b/block.c
@@ -821,6 +821,13 @@ bool bdrv_dev_has_removable_media(BlockDriverState *bs)
     return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb);
 }
 
+void bdrv_dev_eject_request(BlockDriverState *bs, bool force)
+{
+    if (bs->dev_ops && bs->dev_ops->eject_request_cb) {
+        bs->dev_ops->eject_request_cb(bs->dev_opaque, force);
+    }
+}
+
 bool bdrv_dev_is_tray_open(BlockDriverState *bs)
 {
     if (bs->dev_ops && bs->dev_ops->is_tray_open) {
diff --git a/block.h b/block.h
index e77988e..d3c3d62 100644
--- a/block.h
+++ b/block.h
@@ -39,6 +39,12 @@ typedef struct BlockDevOps {
      */
     void (*change_media_cb)(void *opaque, bool load);
     /*
+     * Runs when an eject request is issued from the monitor, the tray
+     * is closed, and the medium is locked.
+     * Device models with removable media must implement this callback.
+     */
+    void (*eject_request_cb)(void *opaque, bool force);
+    /*
      * Is the virtual tray open?
      * Device models implement this only when the device has a tray.
      */
@@ -116,6 +122,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev);
 void *bdrv_get_attached_dev(BlockDriverState *bs);
 void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
                       void *opaque);
+void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
 bool bdrv_dev_has_removable_media(BlockDriverState *bs);
 bool bdrv_dev_is_tray_open(BlockDriverState *bs);
 bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
diff --git a/blockdev.c b/blockdev.c
index 0827bf7..4cf333a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -635,9 +635,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
         qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
         return -1;
     }
-    if (!force && !bdrv_dev_is_tray_open(bs)
-        && bdrv_dev_is_medium_locked(bs)) {
-        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
+    if (bdrv_dev_is_medium_locked(bs) && !bdrv_dev_is_tray_open(bs)) {
+        bdrv_dev_eject_request(bs, force);
+        if (!force) {
+            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
+        }
         return -1;
     }
     bdrv_close(bs);
-- 
1.7.6

[Qemu-devel] [PATCH 7/8] atapi: implement eject requests

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/atapi.c |   11 ++++++++---
 hw/ide/core.c  |   13 +++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 90b6729..1fed359 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -516,9 +516,14 @@ static unsigned int event_status_media(IDEState *s,
 
     /* Event notification descriptor */
     event_code = MEC_NO_CHANGE;
-    if (media_status != MS_TRAY_OPEN && s->events.new_media) {
-        event_code = MEC_NEW_MEDIA;
-        s->events.new_media = false;
+    if (media_status != MS_TRAY_OPEN) {
+        if (s->events.new_media) {
+            event_code = MEC_NEW_MEDIA;
+            s->events.new_media = false;
+        } else if (s->events.eject_request) {
+            event_code = MEC_EJECT_REQUESTED;
+            s->events.eject_request = false;
+        }
     }
 
     buf[4] = event_code;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1f366e7..237d17c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -804,6 +804,18 @@ static void ide_cd_change_cb(void *opaque, bool load)
      */
     s->cdrom_changed = 1;
     s->events.new_media = true;
+    s->events.eject_request = false;
+    ide_set_irq(s->bus);
+}
+
+static void ide_cd_eject_request_cb(void *opaque, bool force)
+{
+    IDEState *s = opaque;
+
+    s->events.eject_request = true;
+    if (force) {
+        s->tray_locked = false;
+    }
     ide_set_irq(s->bus);
 }
 
@@ -1811,6 +1823,7 @@ static bool ide_cd_is_medium_locked(void *opaque)
 
 static const BlockDevOps ide_cd_block_ops = {
     .change_media_cb = ide_cd_change_cb,
+    .eject_request_cb = ide_cd_eject_request_cb,
     .is_tray_open = ide_cd_is_tray_open,
     .is_medium_locked = ide_cd_is_medium_locked,
 };
-- 
1.7.6

[Qemu-devel] [PATCH 8/8] scsi-disk: implement eject requests

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index f407e2b..241a676 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -65,6 +65,7 @@ struct SCSIDiskState
     uint32_t removable;
     bool media_changed;
     bool media_event;
+    bool eject_request;
     QEMUBH *bh;
     char *version;
     char *serial;
@@ -671,9 +672,14 @@ static int scsi_event_status_media(SCSIDiskState *s, uint8_t *outbuf)
 
     /* Event notification descriptor */
     event_code = MEC_NO_CHANGE;
-    if (media_status != MS_TRAY_OPEN && s->media_event) {
-        event_code = MEC_NEW_MEDIA;
-        s->media_event = false;
+    if (media_status != MS_TRAY_OPEN) {
+        if (s->media_event) {
+            event_code = MEC_NEW_MEDIA;
+            s->media_event = false;
+        } else if (s->eject_request) {
+            event_code = MEC_EJECT_REQUESTED;
+            s->eject_request = false;
+        }
     }
 
     outbuf[0] = event_code;
@@ -1470,6 +1476,17 @@ static void scsi_cd_change_media_cb(void *opaque, bool load)
     s->tray_open = !load;
     s->qdev.unit_attention = SENSE_CODE(UNIT_ATTENTION_NO_MEDIUM);
     s->media_event = true;
+    s->eject_request = false;
+}
+
+static void scsi_cd_eject_request_cb(void *opaque, bool force)
+{
+    SCSIDiskState *s = opaque;
+
+    s->eject_request = true;
+    if (force) {
+        s->tray_locked = false;
+    }
 }
 
 static bool scsi_cd_is_tray_open(void *opaque)
@@ -1484,6 +1501,7 @@ static bool scsi_cd_is_medium_locked(void *opaque)
 
 static const BlockDevOps scsi_cd_block_ops = {
     .change_media_cb = scsi_cd_change_media_cb,
+    .eject_request_cb = scsi_cd_eject_request_cb,
     .is_tray_open = scsi_cd_is_tray_open,
     .is_medium_locked = scsi_cd_is_medium_locked,
 };
-- 
1.7.6

[Qemu-devel] ping Re: [PATCH 0/5] My remaining block/SCSI patches for 1.0

[Paolo Bonzini]

On 10/25/2011 12:53 PM, Paolo Bonzini wrote:
> The first three replace patches 20/21 and are basically rewritten with
> input from Kevin.
>
> The fourth is new.  I had it queued for 1.1, but it turns out it is
> needed now or scsi-block might access some requests incorrectly when
> restarting after an error.
>
> The fifth is basically the same as patch 35 from the first submission.
>
> The last three patches had been submitted Sep 20 and were lost at sea;
> support for eject requests is required by udev 173.
>
> Paolo Bonzini (8):
>    scsi: do not call transfer_data after canceling a request
>    scsi-disk: bump SCSIRequest reference count until aio completion runs
>    scsi-generic: bump SCSIRequest reference count until aio completion runs
>    scsi: push request restart to SCSIDevice
>    scsi-disk: add scsi-block for device passthrough
>    block: add eject request callback
>    atapi: implement eject requests
>    scsi-disk: implement eject requests
>
>   block.c           |    7 ++
>   block.h           |    7 ++
>   blockdev.c        |    8 +-
>   hw/ide/atapi.c    |   11 ++-
>   hw/ide/core.c     |   13 +++
>   hw/scsi-bus.c     |   79 ++++++++++++++++-
>   hw/scsi-disk.c    |  254 ++++++++++++++++++++++++++++++++++++++--------------
>   hw/scsi-generic.c |   18 ++++
>   hw/scsi.h         |    6 ++
>   trace-events      |    1 +
>   10 files changed, 325 insertions(+), 79 deletions(-)
>

Ping?

Paolo

Re: [Qemu-devel] [PATCH 5/8] scsi-disk: add scsi-block for device passthrough

[Kevin Wolf]

Am 25.10.2011 12:53, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi-disk.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 118 insertions(+), 0 deletions(-)

This has lost its commit message since the last version. I'll pick it
from there:

scsi-block is a new device that supports device passthrough of Linux
block devices (i.e. /dev/sda, not /dev/sg0).  It uses SG_IO for commands
other than I/O commands, and regular AIO read/writes for I/O commands.
Besides being simpler to configure (no mapping required to scsi-generic
device names), this removes the need for a large bounce buffer and,
in the future, will get scatter/gather support for free from scsi-disk.

Kevin

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Kevin Wolf]

Am 25.10.2011 12:53, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c    |    7 +++++++
>  block.h    |    7 +++++++
>  blockdev.c |    8 +++++---
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9873b57..53e21ba 100644
> --- a/block.c
> +++ b/block.c
> @@ -821,6 +821,13 @@ bool bdrv_dev_has_removable_media(BlockDriverState *bs)
>      return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb);
>  }
>  
> +void bdrv_dev_eject_request(BlockDriverState *bs, bool force)
> +{
> +    if (bs->dev_ops && bs->dev_ops->eject_request_cb) {
> +        bs->dev_ops->eject_request_cb(bs->dev_opaque, force);
> +    }
> +}
> +
>  bool bdrv_dev_is_tray_open(BlockDriverState *bs)
>  {
>      if (bs->dev_ops && bs->dev_ops->is_tray_open) {
> diff --git a/block.h b/block.h
> index e77988e..d3c3d62 100644
> --- a/block.h
> +++ b/block.h
> @@ -39,6 +39,12 @@ typedef struct BlockDevOps {
>       */
>      void (*change_media_cb)(void *opaque, bool load);
>      /*
> +     * Runs when an eject request is issued from the monitor, the tray
> +     * is closed, and the medium is locked.
> +     * Device models with removable media must implement this callback.
> +     */
> +    void (*eject_request_cb)(void *opaque, bool force);
> +    /*
>       * Is the virtual tray open?
>       * Device models implement this only when the device has a tray.
>       */
> @@ -116,6 +122,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev);
>  void *bdrv_get_attached_dev(BlockDriverState *bs);
>  void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>                        void *opaque);
> +void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
>  bool bdrv_dev_has_removable_media(BlockDriverState *bs);
>  bool bdrv_dev_is_tray_open(BlockDriverState *bs);
>  bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
> diff --git a/blockdev.c b/blockdev.c
> index 0827bf7..4cf333a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -635,9 +635,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>          qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>          return -1;
>      }
> -    if (!force && !bdrv_dev_is_tray_open(bs)
> -        && bdrv_dev_is_medium_locked(bs)) {
> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +    if (bdrv_dev_is_medium_locked(bs) && !bdrv_dev_is_tray_open(bs)) {
> +        bdrv_dev_eject_request(bs, force);
> +        if (!force) {
> +            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +        }
>          return -1;
>      }
>      bdrv_close(bs);

Now force doesn't force any more. It avoids the error message, but
doesn't forcefully close the BlockDriverState any more. Intentional? If
so, why is it a good idea?

Kevin

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Paolo Bonzini]

On 10/28/2011 07:21 PM, Kevin Wolf wrote:
>> >  -    if (!force&&  !bdrv_dev_is_tray_open(bs)
>> >  -&&  bdrv_dev_is_medium_locked(bs)) {
>> >  -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>> >  +    if (bdrv_dev_is_medium_locked(bs)&&  !bdrv_dev_is_tray_open(bs)) {
>> >  +        bdrv_dev_eject_request(bs, force);
>> >  +        if (!force) {
>> >  +            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
>> >  +        }
>> >            return -1;
>> >        }
>> >        bdrv_close(bs);
> Now force doesn't force any more. It avoids the error message, but
> doesn't forcefully close the BlockDriverState any more. Intentional? If
> so, why is it a good idea?

In theory the guest OS should eject the disk itself.  However, force 
does unlock the disk so that: 1) two ejects will have the desired 
effect; 2) force eject followed by change will work even with the tray 
locked, unlike before this series.

Paolo

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Markus Armbruster]

I apologize for the lateness of this review.

Paolo Bonzini <pbonzini@redhat.com> writes:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

The commit message should explain why we need this callback.  The cover
letter says "support for eject requests is required by udev 173."
Please elaborate on that.

> ---
>  block.c    |    7 +++++++
>  block.h    |    7 +++++++
>  blockdev.c |    8 +++++---
>  3 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9873b57..53e21ba 100644
> --- a/block.c
> +++ b/block.c
> @@ -821,6 +821,13 @@ bool bdrv_dev_has_removable_media(BlockDriverState *bs)
>      return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb);
>  }
>  
> +void bdrv_dev_eject_request(BlockDriverState *bs, bool force)
> +{
> +    if (bs->dev_ops && bs->dev_ops->eject_request_cb) {
> +        bs->dev_ops->eject_request_cb(bs->dev_opaque, force);
> +    }
> +}
> +
>  bool bdrv_dev_is_tray_open(BlockDriverState *bs)
>  {
>      if (bs->dev_ops && bs->dev_ops->is_tray_open) {
> diff --git a/block.h b/block.h
> index e77988e..d3c3d62 100644
> --- a/block.h
> +++ b/block.h
> @@ -39,6 +39,12 @@ typedef struct BlockDevOps {
>       */
>      void (*change_media_cb)(void *opaque, bool load);
>      /*
> +     * Runs when an eject request is issued from the monitor, the tray
> +     * is closed, and the medium is locked.
> +     * Device models with removable media must implement this callback.
> +     */
> +    void (*eject_request_cb)(void *opaque, bool force);
> +    /*

You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8.  You don't
implement it for floppy, despite the comment.  That's okay; floppy has
no use for it.  It's the comment that needs fixing.  Devices that
implement is_medium_locked() must implement this one as well.

>       * Is the virtual tray open?
>       * Device models implement this only when the device has a tray.
>       */
> @@ -116,6 +122,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev);
>  void *bdrv_get_attached_dev(BlockDriverState *bs);
>  void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>                        void *opaque);
> +void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
>  bool bdrv_dev_has_removable_media(BlockDriverState *bs);
>  bool bdrv_dev_is_tray_open(BlockDriverState *bs);
>  bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
> diff --git a/blockdev.c b/blockdev.c
> index 0827bf7..4cf333a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -635,9 +635,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>          qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>          return -1;
>      }
> -    if (!force && !bdrv_dev_is_tray_open(bs)
> -        && bdrv_dev_is_medium_locked(bs)) {
> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +    if (bdrv_dev_is_medium_locked(bs) && !bdrv_dev_is_tray_open(bs)) {
> +        bdrv_dev_eject_request(bs, force);
> +        if (!force) {
> +            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +        }
>          return -1;
>      }
>      bdrv_close(bs);

Like Kevin, I'm not entirely comfortable with changing the meaning of
"-f".

Here's my mental model of monitor command eject:

1. eject without -f behaves like the physical tray button.  It has
immediate effect, unless the tray is locked closed.  Then, the drive
just notifies the OS of the button push, so the OS can react to it.  The
latter isn't implemented in QEMU.

2. eject with -f behaves like whatever physical way there is to pry the
tray open, locked or not.  CD-ROM drives commonly have a little button
hidden in some hope you can reach with a bent paperclip.

Could you explain your mental model?

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Paolo Bonzini]

On 11/07/2011 02:21 PM, Markus Armbruster wrote:
> The commit message should explain why we need this callback.  The cover
> letter says "support for eject requests is required by udev 173."
> Please elaborate on that.

Well, first and foremost eject requests are in the spec.  :)  The fact 
that recent guests need it is more a justification for pulling this in 1.0.

> You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8.  You don't
> implement it for floppy, despite the comment.  That's okay; floppy has
> no use for it.  It's the comment that needs fixing.  Devices that
> implement is_medium_locked() must implement this one as well.

Right.

> 1. eject without -f behaves like the physical tray button.  It has
> immediate effect, unless the tray is locked closed.  Then, the drive
> just notifies the OS of the button push, so the OS can react to it.  The
> latter isn't implemented in QEMU.
>
> 2. eject with -f behaves like whatever physical way there is to pry the
> tray open, locked or not.  CD-ROM drives commonly have a little button
> hidden in some hope you can reach with a bent paperclip.
>
> Could you explain your mental model?

1. eject without -f is as you mentioned.

2. eject with -f should really never be needed, but it does whatever is 
needed to be able to follow up with a "change" command.  It turns out it 
is really "unlock" and "ask the guest to eject" combined, but that's the 
implementation, not the model.

The difference from the paperclip model is that it gives a chance for 
the OS to clean up and eject safely.  It wouldn't be hard to convince me 
otherwise though, especially if it can help getting the patch in 1.0. 
The "eject -f"+"change" can be replaced by "eject", possibly followed by 
"eject -f" after a timeout, and then followed again by "change".

Paolo

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Kevin Wolf]

Am 07.11.2011 14:36, schrieb Paolo Bonzini:
> On 11/07/2011 02:21 PM, Markus Armbruster wrote:
>> The commit message should explain why we need this callback.  The cover
>> letter says "support for eject requests is required by udev 173."
>> Please elaborate on that.
> 
> Well, first and foremost eject requests are in the spec.  :)  The fact 
> that recent guests need it is more a justification for pulling this in 1.0.
> 
>> You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8.  You don't
>> implement it for floppy, despite the comment.  That's okay; floppy has
>> no use for it.  It's the comment that needs fixing.  Devices that
>> implement is_medium_locked() must implement this one as well.
> 
> Right.
> 
>> 1. eject without -f behaves like the physical tray button.  It has
>> immediate effect, unless the tray is locked closed.  Then, the drive
>> just notifies the OS of the button push, so the OS can react to it.  The
>> latter isn't implemented in QEMU.
>>
>> 2. eject with -f behaves like whatever physical way there is to pry the
>> tray open, locked or not.  CD-ROM drives commonly have a little button
>> hidden in some hope you can reach with a bent paperclip.
>>
>> Could you explain your mental model?
> 
> 1. eject without -f is as you mentioned.
> 
> 2. eject with -f should really never be needed, but it does whatever is 
> needed to be able to follow up with a "change" command.  It turns out it 
> is really "unlock" and "ask the guest to eject" combined, but that's the 
> implementation, not the model.

Does this give different results than just asking the guest to eject
without forcefully unlocking? I would expect that a guest that responds
to the eject request would also unlock the drive. In which case I think
eject without -f should be enough?

> The difference from the paperclip model is that it gives a chance for 
> the OS to clean up and eject safely.  It wouldn't be hard to convince me 
> otherwise though, especially if it can help getting the patch in 1.0. 
> The "eject -f"+"change" can be replaced by "eject", possibly followed by 
> "eject -f" after a timeout, and then followed again by "change".

Kevin

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Paolo Bonzini]

On 11/07/2011 02:49 PM, Kevin Wolf wrote:
> >  2. eject with -f should really never be needed, but it does whatever is
> >  needed to be able to follow up with a "change" command.  It turns out it
> >  is really "unlock" and "ask the guest to eject" combined, but that's the
> >  implementation, not the model.
>
> Does this give different results than just asking the guest to eject
> without forcefully unlocking? I would expect that a guest that responds
> to the eject request would also unlock the drive. In which case I think
> eject without -f should be enough?

Only if the guest is not buggy (e.g. locks the tray but stops polling 
for eject requests) and has not crashed.

Paolo

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Kevin Wolf]

Am 07.11.2011 14:56, schrieb Paolo Bonzini:
> On 11/07/2011 02:49 PM, Kevin Wolf wrote:
>>>  2. eject with -f should really never be needed, but it does whatever is
>>>  needed to be able to follow up with a "change" command.  It turns out it
>>>  is really "unlock" and "ask the guest to eject" combined, but that's the
>>>  implementation, not the model.
>>
>> Does this give different results than just asking the guest to eject
>> without forcefully unlocking? I would expect that a guest that responds
>> to the eject request would also unlock the drive. In which case I think
>> eject without -f should be enough?
> 
> Only if the guest is not buggy (e.g. locks the tray but stops polling 
> for eject requests) and has not crashed.

If the guest is broken, forcefully unlocking and doing an eject request
won't help either (the drive will be unlocked, but not ejected) and the
only way to do anything useful with the drive is sending another eject
command. This doesn't help with cleanly unmounting the medium, of course.

So I think this is actually a point for the eject -f = paperclip model.

Kevin

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/07/2011 02:21 PM, Markus Armbruster wrote:
>> The commit message should explain why we need this callback.  The cover
>> letter says "support for eject requests is required by udev 173."
>> Please elaborate on that.
>
> Well, first and foremost eject requests are in the spec.  :)  The fact
> that recent guests need it is more a justification for pulling this in
> 1.0.

I agree we should try to solve the problem for 1.0.

>> You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8.  You don't
>> implement it for floppy, despite the comment.  That's okay; floppy has
>> no use for it.  It's the comment that needs fixing.  Devices that
>> implement is_medium_locked() must implement this one as well.
>
> Right.
>
>> 1. eject without -f behaves like the physical tray button.  It has
>> immediate effect, unless the tray is locked closed.  Then, the drive
>> just notifies the OS of the button push, so the OS can react to it.  The
>> latter isn't implemented in QEMU.
>>
>> 2. eject with -f behaves like whatever physical way there is to pry the
>> tray open, locked or not.  CD-ROM drives commonly have a little button
>> hidden in some hope you can reach with a bent paperclip.
>>
>> Could you explain your mental model?
>
> 1. eject without -f is as you mentioned.

Would implementing the missing part help with the problem at hand?

> 2. eject with -f should really never be needed, but it does whatever
> is needed to be able to follow up with a "change" command.  It turns
> out it is really "unlock" and "ask the guest to eject" combined, but
> that's the implementation, not the model.

Physical hardware doesn't work that way (unless I misunderstand it).
Always a warning sign.

> The difference from the paperclip model is that it gives a chance for
> the OS to clean up and eject safely.  It wouldn't be hard to convince
> me otherwise though, especially if it can help getting the patch in
> 1.0. The "eject -f"+"change" can be replaced by "eject", possibly
> followed by "eject -f" after a timeout, and then followed again by
> "change".

On bare metal, the pressing the tray button accomplishes that: the drive
notifies the OS the button was pressed, the well-behaved OS cleans up
and ejects.  With a misbehaving OS, you're reduced to the paperclip.

Can't we replicate that?

1. Tray button / eject without -f

   A. when the tray is not locked: tray opens immediately.

   B. when the tray is locked: OS gets notified.  We expect it to clean
      up, unlock and open the tray at some point in the near future.

2. Paperclip / eject with -f

   Tray opens immediately.  Guest OS may be unhappy about it.

Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Paolo Bonzini]

On 11/07/2011 04:23 PM, Markus Armbruster wrote:
>> 1. eject without -f is as you mentioned.
>
> Would implementing the missing part help with the problem at hand?

It would help for non-buggy guests.  Buggy means even "echo -1 > 
/sys/block/sr0/events_poll_msecs".

However, a full solution would require a change in management, and 
adding a TRAY_STATUS_CHANGED event to QEMU.  Not sure this is required 
for 1.0, as it can even be added later to stable.

>> 2. eject with -f should really never be needed, but it does whatever
>> is needed to be able to follow up with a "change" command.  It turns
>> out it is really "unlock" and "ask the guest to eject" combined, but
>> that's the implementation, not the model.
>
> Physical hardware doesn't work that way (unless I misunderstand it).
> Always a warning sign.

True.

>> The difference from the paperclip model is that it gives a chance for
>> the OS to clean up and eject safely.  It wouldn't be hard to convince
>> me otherwise though, especially if it can help getting the patch in
>> 1.0. The "eject -f"+"change" can be replaced by "eject", possibly
>> followed by "eject -f" after a timeout, and then followed again by
>> "change".
>
> On bare metal, the pressing the tray button accomplishes that: the drive
> notifies the OS the button was pressed, the well-behaved OS cleans up
> and ejects.  With a misbehaving OS, you're reduced to the paperclip.
>
> Can't we replicate that?
>
> 1. Tray button / eject without -f
>
>     A. when the tray is not locked: tray opens immediately.
>
>     B. when the tray is locked: OS gets notified.  We expect it to clean
>        up, unlock and open the tray at some point in the near future.
>
> 2. Paperclip / eject with -f
>
>     Tray opens immediately.  Guest OS may be unhappy about it.

I now redid my tests with the paperclip behavior (it's really one line 
of different code) and everything seems to work.  Will submit v2.

Paolo

[Qemu-devel] [PATCH 6/8 v2] block: add eject request callback

[Paolo Bonzini]

Recent versions of udev always keep the tray locked so that the kernel
can observe "eject request" events (aka tray button presses) even on
discs that aren't mounted.  Add support for these events in the ATAPI
and SCSI cd drive device models.

To let management cope with the behavior of udev, an event should also
be added for "tray opened/closed".  This way, after issuing an "eject"
command, management can poll until the guests actually reacts to the
command.  They can then issue the "change" command after the tray has been
opened, or try with "eject -f" after a (configurable?) timeout.  However,
with this patch and the corresponding support in the device models,
at least it is possible to do a manual two-step eject+change sequence.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2: do not change the behavior of eject -f.

 block.c    |    7 +++++++
 block.h    |    8 ++++++++
 blockdev.c |   10 ++++++----
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 0568df2..e847738 100644
--- a/block.c
+++ b/block.c
@@ -816,6 +816,13 @@ bool bdrv_dev_has_removable_media(BlockDriverState *bs)
     return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb);
 }
 
+void bdrv_dev_eject_request(BlockDriverState *bs, bool force)
+{
+    if (bs->dev_ops && bs->dev_ops->eject_request_cb) {
+        bs->dev_ops->eject_request_cb(bs->dev_opaque, force);
+    }
+}
+
 bool bdrv_dev_is_tray_open(BlockDriverState *bs)
 {
     if (bs->dev_ops && bs->dev_ops->is_tray_open) {
diff --git a/block.h b/block.h
index 38cd748..38fd0f6 100644
--- a/block.h
+++ b/block.h
@@ -39,6 +39,15 @@ typedef struct BlockDevOps {
      */
     void (*change_media_cb)(void *opaque, bool load);
     /*
+     * Runs when an eject request is issued from the monitor, the tray
+     * is closed, and the medium is locked.
+     * Device models that do not implement is_medium_locked will not need
+     * this callback.  Device models that can lock the medium or tray might
+     * want to implement the callback and unlock the tray when "force" is
+     * true, even if they do not support eject requests.
+     */
+    void (*eject_request_cb)(void *opaque, bool force);
+    /*
      * Is the virtual tray open?
      * Device models implement this only when the device has a tray.
      */
@@ -111,6 +118,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev);
 void *bdrv_get_attached_dev(BlockDriverState *bs);
 void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
                       void *opaque);
+void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
 bool bdrv_dev_has_removable_media(BlockDriverState *bs);
 bool bdrv_dev_is_tray_open(BlockDriverState *bs);
 bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
diff --git a/blockdev.c b/blockdev.c
index 0827bf7..2228186 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -635,10 +635,12 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
         qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
         return -1;
     }
-    if (!force && !bdrv_dev_is_tray_open(bs)
-        && bdrv_dev_is_medium_locked(bs)) {
-        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
-        return -1;
+    if (bdrv_dev_is_medium_locked(bs) && !bdrv_dev_is_tray_open(bs)) {
+        bdrv_dev_eject_request(bs, force);
+        if (!force) {
+            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
+            return -1;
+        }
     }
     bdrv_close(bs);
     return 0;
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH 6/8 v2] block: add eject request callback

[Kevin Wolf]

Am 07.11.2011 17:50, schrieb Paolo Bonzini:
> Recent versions of udev always keep the tray locked so that the kernel
> can observe "eject request" events (aka tray button presses) even on
> discs that aren't mounted.  Add support for these events in the ATAPI
> and SCSI cd drive device models.
> 
> To let management cope with the behavior of udev, an event should also
> be added for "tray opened/closed".  This way, after issuing an "eject"
> command, management can poll until the guests actually reacts to the
> command.  They can then issue the "change" command after the tray has been
> opened, or try with "eject -f" after a (configurable?) timeout.  However,
> with this patch and the corresponding support in the device models,
> at least it is possible to do a manual two-step eject+change sequence.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v1->v2: do not change the behavior of eject -f.

Thanks, applied all three eject patches to the block-stable branch (for 1.0)

Kevin