All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] usb-msd: Add usb-storage, removable=on|off property
@ 2011-01-15  0:00 Stefan Hajnoczi
  2011-01-17  9:21 ` [Qemu-devel] " Christoph Hellwig
  0 siblings, 1 reply; 5+ messages in thread
From: Stefan Hajnoczi @ 2011-01-15  0:00 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Christoph Hellwig, Justin M. Forbes, Stefan Hajnoczi

USB Mass Storage Devices sometimes have the RMB (removable) bit set in
the SCSI INQUIRY response.  Thumbdrives tend to have the bit set whereas
hard disks do not.

Operating systems differentiate between removable devices and fixed
devices.  Under Linux, the anaconda installer looks for removable
devices.  Under Windows, only fixed devices may have more than one
partition and AutoRun is also affected by the removable bit.

For these reasons, allow USB Mass Storage Devices to override the
removable bit:

qemu -usb
     -drive if=none,file=test.img,cache=none,id=disk0
     -device usb-storage,drive=disk0,removable=on

The default is off.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/scsi-disk.c |    6 +++++-
 hw/usb-msd.c   |   19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 6cb317c..39c1279 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -548,7 +548,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
 
     if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
         outbuf[0] = 5;
-        outbuf[1] = 0x80;
         memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
     } else {
         outbuf[0] = 0;
@@ -557,6 +556,11 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
     memcpy(&outbuf[8], "QEMU    ", 8);
     memset(&outbuf[32], 0, 4);
     memcpy(&outbuf[32], s->version, MIN(4, strlen(s->version)));
+
+    if (bdrv_is_removable(s->bs)) {
+        outbuf[1] |= 0x80;
+    }
+
     /*
      * We claim conformance to SPC-3, which is required for guests
      * to ask for modern features like READ CAPACITY(16) or the
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 0a95d8d..2dce9df 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -50,6 +50,7 @@ typedef struct {
     SCSIBus bus;
     BlockConf conf;
     SCSIDevice *scsi_dev;
+    uint32_t removable;
     int result;
     /* For async completion.  */
     USBPacket *packet;
@@ -520,6 +521,13 @@ static void usb_msd_password_cb(void *opaque, int err)
         qdev_unplug(&s->dev.qdev);
 }
 
+static void usb_msd_change_cb(void *opaque)
+{
+    MSDState *s = opaque;
+
+    qdev_unplug(&s->dev.qdev);
+}
+
 static int usb_msd_initfn(USBDevice *dev)
 {
     MSDState *s = DO_UPCAST(MSDState, dev, dev);
@@ -548,6 +556,16 @@ static int usb_msd_initfn(USBDevice *dev)
     if (!s->scsi_dev) {
         return -1;
     }
+
+    /*
+     * Allow overriding the SCSI INQUIRY removable (RMB) bit for hard disks.
+     * USB thumbdrives tend report removable media but USB hard disks do not.
+     */
+    if (s->removable && bdrv_get_type_hint(bs) == BDRV_TYPE_HD) {
+        bdrv_set_change_cb(bs, usb_msd_change_cb, s);
+        bdrv_set_removable(bs, 1);
+    }
+
     s->bus.qbus.allow_hotplug = 0;
     usb_msd_handle_reset(dev);
 
@@ -634,6 +652,7 @@ static struct USBDeviceInfo msd_info = {
     .usbdevice_init = usb_msd_init,
     .qdev.props     = (Property[]) {
         DEFINE_BLOCK_PROPERTIES(MSDState, conf),
+        DEFINE_PROP_BIT("removable", MSDState, removable, 1, false),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [Qemu-devel] Re: [PATCH] usb-msd: Add usb-storage, removable=on|off property
  2011-01-15  0:00 [Qemu-devel] [PATCH] usb-msd: Add usb-storage, removable=on|off property Stefan Hajnoczi
@ 2011-01-17  9:21 ` Christoph Hellwig
  2011-01-17 10:04   ` Stefan Hajnoczi
  0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2011-01-17  9:21 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Kevin Wolf, qemu-devel, Justin M. Forbes, Christoph Hellwig

The actual removable bit looks fine, but I don't think the connection of
the change callback looks sane.  What's the rationale for it?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] usb-msd: Add usb-storage, removable=on|off property
  2011-01-17  9:21 ` [Qemu-devel] " Christoph Hellwig
@ 2011-01-17 10:04   ` Stefan Hajnoczi
  2011-01-17 16:01     ` Christoph Hellwig
  0 siblings, 1 reply; 5+ messages in thread
From: Stefan Hajnoczi @ 2011-01-17 10:04 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Kevin Wolf, Stefan Hajnoczi, Justin M. Forbes, qemu-devel

On Mon, Jan 17, 2011 at 10:21:26AM +0100, Christoph Hellwig wrote:
> The actual removable bit looks fine, but I don't think the connection of
> the change callback looks sane.  What's the rationale for it?

Since we're using bdrv_set_removable(), the user may try to eject the
block device from the QEMU monitor.  At that point we have a closed
BlockDriverState and all operations will (at best) error since there is
no medium.

The callback removes the USB MSD so that eject is equivalent to removing
the device.  It's a hack and we could remove it, but then we're left
with a weird guest-visible state that you can't get into with a physical
USB thumbdrive.

I was considering not using bdrv_set_removable() and instead adding a
hint to the BlockDriverState which gets checked when constructing the
SCSI INQUIRY response.  If we take that approach, then QEMU doesn't
consider the block device removable in the eject/change medium sense.

Stefan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] usb-msd: Add usb-storage, removable=on|off property
  2011-01-17 10:04   ` Stefan Hajnoczi
@ 2011-01-17 16:01     ` Christoph Hellwig
  2011-01-17 18:08       ` Stefan Hajnoczi
  0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2011-01-17 16:01 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Kevin Wolf, qemu-devel, Christoph Hellwig, Justin M. Forbes,
	Stefan Hajnoczi

On Mon, Jan 17, 2011 at 10:04:05AM +0000, Stefan Hajnoczi wrote:
> I was considering not using bdrv_set_removable() and instead adding a
> hint to the BlockDriverState which gets checked when constructing the
> SCSI INQUIRY response.  If we take that approach, then QEMU doesn't
> consider the block device removable in the eject/change medium sense.

I think that's the better approach.  What about moving the removable
qdev property up from the usb driver into the scsi-disk driver?  That's
where it logically belongs, and avoids the need to add another hint.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] usb-msd: Add usb-storage, removable=on|off property
  2011-01-17 16:01     ` Christoph Hellwig
@ 2011-01-17 18:08       ` Stefan Hajnoczi
  0 siblings, 0 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2011-01-17 18:08 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Kevin Wolf, Stefan Hajnoczi, Justin M. Forbes, qemu-devel

On Mon, Jan 17, 2011 at 4:01 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Jan 17, 2011 at 10:04:05AM +0000, Stefan Hajnoczi wrote:
>> I was considering not using bdrv_set_removable() and instead adding a
>> hint to the BlockDriverState which gets checked when constructing the
>> SCSI INQUIRY response.  If we take that approach, then QEMU doesn't
>> consider the block device removable in the eject/change medium sense.
>
> I think that's the better approach.  What about moving the removable
> qdev property up from the usb driver into the scsi-disk driver?  That's
> where it logically belongs, and avoids the need to add another hint.

That could work although the plumbing to get us to the point where a
scsi-disk qdev is created isn't very pretty either.  I'll try adding a
removable property and setting it in a sane way.

Stefan

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-01-17 18:09 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-15  0:00 [Qemu-devel] [PATCH] usb-msd: Add usb-storage, removable=on|off property Stefan Hajnoczi
2011-01-17  9:21 ` [Qemu-devel] " Christoph Hellwig
2011-01-17 10:04   ` Stefan Hajnoczi
2011-01-17 16:01     ` Christoph Hellwig
2011-01-17 18:08       ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.