[Qemu-devel] [PATCH v3 0/4] usb-msd: Add usb-storage, removable=on|off property

[Qemu-devel] [PATCH v3 0/4] usb-msd: Add usb-storage, removable=on|off property

[Stefan Hajnoczi]

Allow overriding the SCSI INQUIRY removable (RMB) bit for scsi-disk and usb-msd
devices.  In particular this addresses the problem that some usb-msd devices
have the bit set while other do not have it set.  Now the user can choose and
get desired guest behavior.

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

The default is off.

v3:
 * Document removable property in qdev-device-use.txt
 * Use bit number 0 instead of bit 1 for the qdev property

v2:
 * Rewritten to override the bit at the scsi-disk level

 docs/qdev-device-use.txt |   13 +++++++++++--
 hw/pci-hotplug.c         |    2 +-
 hw/scsi-bus.c            |    8 ++++++--
 hw/scsi-disk.c           |    3 +++
 hw/scsi.h                |    3 ++-
 hw/usb-msd.c             |    4 +++-
 6 files changed, 26 insertions(+), 7 deletions(-)

[Qemu-devel] [PATCH v3 1/4] scsi-disk: Allow overriding SCSI INQUIRY removable bit

[Stefan Hajnoczi]

Provide the "removable" qdev property bit to override the SCSI INQUIRY
removable (RMB) bit for non-CDROM devices.  This will be used by USB
Mass Storage Devices, which sometimes have this guest-visible bit set
and sometimes do not.  They therefore requires a means for user
configuration.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/scsi-disk.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 6cb317c..488eedd 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -72,6 +72,7 @@ struct SCSIDiskState
     /* The qemu block layer uses a fixed 512 byte sector size.
        This is the number of 512 byte blocks in a single scsi sector.  */
     int cluster_size;
+    uint32_t removable;
     uint64_t max_lba;
     QEMUBH *bh;
     char *version;
@@ -552,6 +553,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
         memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
     } else {
         outbuf[0] = 0;
+        outbuf[1] = s->removable ? 0x80 : 0;
         memcpy(&outbuf[16], "QEMU HARDDISK   ", 16);
     }
     memcpy(&outbuf[8], "QEMU    ", 8);
@@ -1295,6 +1297,7 @@ static SCSIDeviceInfo scsi_disk_info = {
         DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
         DEFINE_PROP_STRING("ver",  SCSIDiskState, version),
         DEFINE_PROP_STRING("serial",  SCSIDiskState, serial),
+        DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
-- 
1.7.2.3

[Qemu-devel] [PATCH v3 2/4] scsi: Allow SCSI devices to override the removable bit

[Stefan Hajnoczi]

Some SCSI devices may wish to override the removable bit.  Add support
for a qdev property on the SCSI device.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/pci-hotplug.c |    2 +-
 hw/scsi-bus.c    |    8 ++++++--
 hw/scsi.h        |    3 ++-
 hw/usb-msd.c     |    2 +-
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 716133c..270a982 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -90,7 +90,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
      * specified).
      */
     dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1);
-    scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit);
+    scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit, false);
     if (!scsidev) {
         return -1;
     }
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 7febb86..ceeb4ec 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -87,7 +87,8 @@ void scsi_qdev_register(SCSIDeviceInfo *info)
 }
 
 /* handle legacy '-drive if=scsi,...' cmd line args */
-SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit)
+SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
+                                      int unit, bool removable)
 {
     const char *driver;
     DeviceState *dev;
@@ -95,6 +96,9 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int
     driver = bdrv_is_sg(bdrv) ? "scsi-generic" : "scsi-disk";
     dev = qdev_create(&bus->qbus, driver);
     qdev_prop_set_uint32(dev, "scsi-id", unit);
+    if (qdev_prop_exists(dev, "removable")) {
+        qdev_prop_set_bit(dev, "removable", removable);
+    }
     if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) {
         qdev_free(dev);
         return NULL;
@@ -117,7 +121,7 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
             continue;
         }
         qemu_opts_loc_restore(dinfo->opts);
-        if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit)) {
+        if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit, false)) {
             res = -1;
             break;
         }
diff --git a/hw/scsi.h b/hw/scsi.h
index bf02adf..846fbba 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -94,7 +94,8 @@ static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
     return DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
 }
 
-SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit);
+SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
+                                      int unit, bool removable);
 int scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
 
 SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun);
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 0a95d8d..ee897b6 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -544,7 +544,7 @@ static int usb_msd_initfn(USBDevice *dev)
 
     s->dev.speed = USB_SPEED_FULL;
     scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete);
-    s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0);
+    s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, false);
     if (!s->scsi_dev) {
         return -1;
     }
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH v3 2/4] scsi: Allow SCSI devices to override the removable bit

[Markus Armbruster]

Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes:

> Some SCSI devices may wish to override the removable bit.  Add support
> for a qdev property on the SCSI device.

I find this description a bit misleading.  The qdev property is added in
1/4.  Here, you merely extend scsi_bus_legacy_add_drive() to provide
access to it.  Its primary users (-drive if=scsi & friends) don't use
that access.  But there's another user: usb_msd_initfn()[*], and 3/4
will make that one use the access.

I guess I'd squash 2+3 together, but that's strictly a matter of taste.

>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  hw/pci-hotplug.c |    2 +-
>  hw/scsi-bus.c    |    8 ++++++--
>  hw/scsi.h        |    3 ++-
>  hw/usb-msd.c     |    2 +-
>  4 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> index 716133c..270a982 100644
> --- a/hw/pci-hotplug.c
> +++ b/hw/pci-hotplug.c
> @@ -90,7 +90,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
>       * specified).
>       */
>      dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1);
> -    scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit);
> +    scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit, false);
>      if (!scsidev) {
>          return -1;
>      }
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 7febb86..ceeb4ec 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -87,7 +87,8 @@ void scsi_qdev_register(SCSIDeviceInfo *info)
>  }
>  
>  /* handle legacy '-drive if=scsi,...' cmd line args */
> -SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit)
> +SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
> +                                      int unit, bool removable)
>  {
>      const char *driver;
>      DeviceState *dev;
> @@ -95,6 +96,9 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int
>      driver = bdrv_is_sg(bdrv) ? "scsi-generic" : "scsi-disk";
>      dev = qdev_create(&bus->qbus, driver);
>      qdev_prop_set_uint32(dev, "scsi-id", unit);
> +    if (qdev_prop_exists(dev, "removable")) {

Isn't this just a funky way to check for "scsi-disk"?

Removable gets silently ignored for -device usb-storage with a scsi
generic drive.  Worth nothing in 4/4.

> +        qdev_prop_set_bit(dev, "removable", removable);
> +    }
>      if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) {
>          qdev_free(dev);
>          return NULL;
> @@ -117,7 +121,7 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
>              continue;
>          }
>          qemu_opts_loc_restore(dinfo->opts);
> -        if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit)) {
> +        if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit, false)) {
>              res = -1;
>              break;
>          }
[...]


[*] I hate that usb_msd_initfn() uses scsi_bus_legacy_add_drive().
Actually, I hate pretty much everything about usb-msd.c.  But it's not
your fault.

Re: [Qemu-devel] [PATCH v3 2/4] scsi: Allow SCSI devices to override the removable bit

[Stefan Hajnoczi]

On Fri, Jan 21, 2011 at 06:26:14PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes:
> 
> > Some SCSI devices may wish to override the removable bit.  Add support
> > for a qdev property on the SCSI device.
> 
> I find this description a bit misleading.  The qdev property is added in
> 1/4.  Here, you merely extend scsi_bus_legacy_add_drive() to provide
> access to it.  Its primary users (-drive if=scsi & friends) don't use
> that access.  But there's another user: usb_msd_initfn()[*], and 3/4
> will make that one use the access.
> 
> I guess I'd squash 2+3 together, but that's strictly a matter of taste.

You're right, the description is poor.  Sorry about that.

> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > ---
> >  hw/pci-hotplug.c |    2 +-
> >  hw/scsi-bus.c    |    8 ++++++--
> >  hw/scsi.h        |    3 ++-
> >  hw/usb-msd.c     |    2 +-
> >  4 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> > index 716133c..270a982 100644
> > --- a/hw/pci-hotplug.c
> > +++ b/hw/pci-hotplug.c
> > @@ -90,7 +90,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
> >       * specified).
> >       */
> >      dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1);
> > -    scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit);
> > +    scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit, false);
> >      if (!scsidev) {
> >          return -1;
> >      }
> > diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> > index 7febb86..ceeb4ec 100644
> > --- a/hw/scsi-bus.c
> > +++ b/hw/scsi-bus.c
> > @@ -87,7 +87,8 @@ void scsi_qdev_register(SCSIDeviceInfo *info)
> >  }
> >  
> >  /* handle legacy '-drive if=scsi,...' cmd line args */
> > -SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit)
> > +SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
> > +                                      int unit, bool removable)
> >  {
> >      const char *driver;
> >      DeviceState *dev;
> > @@ -95,6 +96,9 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int
> >      driver = bdrv_is_sg(bdrv) ? "scsi-generic" : "scsi-disk";
> >      dev = qdev_create(&bus->qbus, driver);
> >      qdev_prop_set_uint32(dev, "scsi-id", unit);
> > +    if (qdev_prop_exists(dev, "removable")) {
> 
> Isn't this just a funky way to check for "scsi-disk"?
> 
> Removable gets silently ignored for -device usb-storage with a scsi
> generic drive.  Worth nothing in 4/4.

Yes, it only applies for scsi-disk and I'll note that in 4/4.

Stefan

[Qemu-devel] [PATCH v3 3/4] usb-msd: Propagate removable bit to SCSI device

[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/usb-msd.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index ee897b6..f1f150b 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;
@@ -544,7 +545,7 @@ static int usb_msd_initfn(USBDevice *dev)
 
     s->dev.speed = USB_SPEED_FULL;
     scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete);
-    s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, false);
+    s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable);
     if (!s->scsi_dev) {
         return -1;
     }
@@ -634,6 +635,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, 0, false),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
-- 
1.7.2.3

[Qemu-devel] [PATCH v3 4/4] docs: Document scsi-disk and usb-storage removable parameter

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 docs/qdev-device-use.txt |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index f2f9b75..e888843 100644
--- a/docs/qdev-device-use.txt
+++ b/docs/qdev-device-use.txt
@@ -80,7 +80,11 @@ The -device argument differs in detail for each kind of drive:
   This SCSI controller a single SCSI bus, named ID.0.  Put a disk on
   it:
 
-  -device scsi-disk,drive=DRIVE-ID,bus=ID.0,scsi-id=SCSI-ID
+  -device scsi-disk,drive=DRIVE-ID,bus=ID.0,scsi-id=SCSI-ID,removable=RMB
+
+  The (optional) removable parameter lets you override the SCSI INQUIRY
+  removable (RMB) bit for non CD-ROM devices.  It is ignored for CD-ROM devices
+  which are always removable.  RMB is "on" or "off".
 
 * if=floppy
 
@@ -116,7 +120,12 @@ For USB devices, the old way is actually different:
 Provides much less control than -drive's HOST-OPTS...  The new way
 fixes that:
 
-    -device usb-storage,drive=DRIVE-ID
+    -device usb-storage,drive=DRIVE-ID,removable=RMB
+
+The removable parameter gives control over the SCSI INQUIRY removable (RMB)
+bit.  USB thumbdrives usually set removable=on, while USB hard disks set
+removable=off.  See the if=scsi description above for details on the removable
+parameter.
 
 === Character Devices ===
 
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH v3 0/4] usb-msd: Add usb-storage, removable=on|off property

[Kevin Wolf]

Am 21.01.2011 12:00, schrieb Stefan Hajnoczi:
> Allow overriding the SCSI INQUIRY removable (RMB) bit for scsi-disk and usb-msd
> devices.  In particular this addresses the problem that some usb-msd devices
> have the bit set while other do not have it set.  Now the user can choose and
> get desired guest behavior.
> 
> qemu -usb
>      -drive if=none,file=test.img,cache=none,id=disk0
>      -device usb-storage,drive=disk0,removable=on
> 
> The default is off.
> 
> v3:
>  * Document removable property in qdev-device-use.txt
>  * Use bit number 0 instead of bit 1 for the qdev property
> 
> v2:
>  * Rewritten to override the bit at the scsi-disk level
> 
>  docs/qdev-device-use.txt |   13 +++++++++++--
>  hw/pci-hotplug.c         |    2 +-
>  hw/scsi-bus.c            |    8 ++++++--
>  hw/scsi-disk.c           |    3 +++
>  hw/scsi.h                |    3 ++-
>  hw/usb-msd.c             |    4 +++-
>  6 files changed, 26 insertions(+), 7 deletions(-)

Thanks, applied all to the block branch.

Kevin