Patchwork SCSI/BLOCK: Allow (scsi) disks to be REMOVABLE

login
register
mail settings
Submitter ronniesahlberg@gmail.com
Date July 30, 2012, 4:43 a.m.
Message ID <1343623393-3582-2-git-send-email-ronniesahlberg@gmail.com>
Download mbox | patch
Permalink /patch/173954/
State New
Headers show

Comments

ronniesahlberg@gmail.com - July 30, 2012, 4:43 a.m.
Add a new block device attribute : removable.
Make all cdrom devices removable by default just like today
but also allow (scsi) disk devices to become removable by a new
-drive keyword : removable.

Example
    -drive file=./scsi-disk.img,if=scsi,media=disk,removable

Removable disks are currently only supported for SCSI but should be possible
to extend to other bustypes that support removable media too.
StartStopUnit scsi command has been updated to honour the allow/prevent
medium removal flag for SBC devices in addition to MMC devices so that a
scsi disk that is mounted by the guest can not be "ejected".

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block.c         |   10 ++++++++++
 block.h         |    2 ++
 block_int.h     |    1 +
 blockdev.c      |   22 +++++++++++++++++-----
 hw/scsi-disk.c  |   18 ++++++------------
 qemu-config.c   |    4 ++++
 qemu-options.hx |    2 ++
 7 files changed, 42 insertions(+), 17 deletions(-)
Paolo Bonzini - July 30, 2012, 7:37 a.m.
Il 30/07/2012 06:43, Ronnie Sahlberg ha scritto:
> Add a new block device attribute : removable.
> Make all cdrom devices removable by default just like today
> but also allow (scsi) disk devices to become removable by a new
> -drive keyword : removable.
> 
> Example
>     -drive file=./scsi-disk.img,if=scsi,media=disk,removable
> 
> Removable disks are currently only supported for SCSI but should be possible
> to extend to other bustypes that support removable media too.
> StartStopUnit scsi command has been updated to honour the allow/prevent
> medium removal flag for SBC devices in addition to MMC devices so that a
> scsi disk that is mounted by the guest can not be "ejected".

Nack, just use the removable property of -device scsi-hd.  -drive should
only include host-visible properties.  This is not true in many cases,
but let's not make things worse.

Paolo

> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>  block.c         |   10 ++++++++++
>  block.h         |    2 ++
>  block_int.h     |    1 +
>  blockdev.c      |   22 +++++++++++++++++-----
>  hw/scsi-disk.c  |   18 ++++++------------
>  qemu-config.c   |    4 ++++
>  qemu-options.hx |    2 ++
>  7 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ce7eb8f..e9c0bfa 100644
> --- a/block.c
> +++ b/block.c
> @@ -2151,6 +2151,16 @@ int bdrv_is_read_only(BlockDriverState *bs)
>      return bs->read_only;
>  }
>  
> +int bdrv_is_removable(BlockDriverState *bs)
> +{
> +    return bs->removable;
> +}
> +
> +void bdrv_set_removable(BlockDriverState *bs, bool removable)
> +{
> +    bs->removable = removable;
> +}
> +
>  int bdrv_is_sg(BlockDriverState *bs)
>  {
>      return bs->sg;
> diff --git a/block.h b/block.h
> index c89590d..b6dd71f 100644
> --- a/block.h
> +++ b/block.h
> @@ -261,6 +261,8 @@ void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
>                         BlockErrorAction on_write_error);
>  BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
>  int bdrv_is_read_only(BlockDriverState *bs);
> +int bdrv_is_removable(BlockDriverState *bs);
> +void bdrv_set_removable(BlockDriverState *bs, bool removable);
>  int bdrv_is_sg(BlockDriverState *bs);
>  int bdrv_enable_write_cache(BlockDriverState *bs);
>  void bdrv_set_enable_write_cache(BlockDriverState *bs, bool wce);
> diff --git a/block_int.h b/block_int.h
> index d72317f..3f6c771 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -266,6 +266,7 @@ struct BlockDriverState {
>                                size in sectors */
>      int read_only; /* if true, the media is read only */
>      int keep_read_only; /* if true, the media was requested to stay read only */
> +    int removable; /* if true, the media is removable */
>      int open_flags; /* flags used to open the file, re-used for re-open */
>      int encrypted; /* if true, the media is encrypted */
>      int valid_key; /* if true, a valid encryption key has been set */
> diff --git a/blockdev.c b/blockdev.c
> index 3d75015..034112c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -288,6 +288,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>      int max_devs;
>      int index;
>      int ro = 0;
> +    int removable = 0;
>      int bdrv_flags = 0;
>      int on_read_error, on_write_error;
>      const char *devaddr;
> @@ -311,6 +312,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>  
>      snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
>      ro = qemu_opt_get_bool(opts, "readonly", 0);
> +    removable = qemu_opt_get_bool(opts, "removable", 0);
>      copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
>  
>      file = qemu_opt_get(opts, "file");
> @@ -591,15 +593,25 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>      if (media == MEDIA_CDROM) {
>          /* CDROM is fine for any interface, don't check.  */
>          ro = 1;
> -    } else if (ro == 1) {
> -        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
> -            type != IF_NONE && type != IF_PFLASH) {
> -            error_report("readonly not supported by this bus type");
> -            goto err;
> +        removable = 1;
> +    } else {
> +        if (ro == 1) {
> +            if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
> +                type != IF_NONE && type != IF_PFLASH) {
> +                error_report("readonly not supported by this bus type");
> +                goto err;
> +            }
> +        }
> +        if (removable == 1) {
> +            if (type != IF_SCSI) {
> +                error_report("removable not supported by this bus type");
> +                goto err;
> +            }
>          }
>      }
>  
>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
> +    bdrv_set_removable(dinfo->bdrv, removable);
>  
>      if (ro && copy_on_read) {
>          error_report("warning: disabling copy_on_read on readonly drive");
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 5426990..4c394bd 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -58,8 +58,7 @@ typedef struct SCSIDiskReq {
>      BlockAcctCookie acct;
>  } SCSIDiskReq;
>  
> -#define SCSI_DISK_F_REMOVABLE   0
> -#define SCSI_DISK_F_DPOFUA      1
> +#define SCSI_DISK_F_DPOFUA      0
>  
>  struct SCSIDiskState
>  {
> @@ -668,7 +667,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>      memset(outbuf, 0, buflen);
>  
>      outbuf[0] = s->qdev.type & 0x1f;
> -    outbuf[1] = (s->features & (1 << SCSI_DISK_F_REMOVABLE)) ? 0x80 : 0;
> +    outbuf[1] = bdrv_is_removable(s->qdev.conf.bs) ? 0x80 : 0;
>      if (s->qdev.type == TYPE_ROM) {
>          memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
>      } else {
> @@ -1251,7 +1250,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r)
>          return 0;
>      }
>  
> -    if (s->qdev.type == TYPE_ROM && loej) {
> +    if (bdrv_is_removable(s->qdev.conf.bs) && loej) {
>          if (!start && !s->tray_open && s->tray_locked) {
>              scsi_check_condition(r,
>                                   bdrv_is_inserted(s->qdev.conf.bs)
> @@ -1749,7 +1748,7 @@ static int scsi_initfn(SCSIDevice *dev)
>          return -1;
>      }
>  
> -    if (!(s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
> +    if (!bdrv_is_removable(s->qdev.conf.bs) &&
>          !bdrv_is_inserted(s->qdev.conf.bs)) {
>          error_report("Device needs media, but drive is empty");
>          return -1;
> @@ -1769,7 +1768,7 @@ static int scsi_initfn(SCSIDevice *dev)
>          return -1;
>      }
>  
> -    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
> +    if (bdrv_is_removable(s->qdev.conf.bs)) {
>          bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_cd_block_ops, s);
>      }
>      bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
> @@ -1792,7 +1791,6 @@ static int scsi_cd_initfn(SCSIDevice *dev)
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
>      s->qdev.blocksize = 2048;
>      s->qdev.type = TYPE_ROM;
> -    s->features |= 1 << SCSI_DISK_F_REMOVABLE;
>      return scsi_initfn(&s->qdev);
>  }
>  
> @@ -1866,7 +1864,7 @@ static int get_device_type(SCSIDiskState *s)
>      }
>      s->qdev.type = buf[0];
>      if (buf[1] & 0x80) {
> -        s->features |= 1 << SCSI_DISK_F_REMOVABLE;
> +        bdrv_set_removable(s->qdev.conf.bs, true);
>      }
>      return 0;
>  }
> @@ -1967,8 +1965,6 @@ static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
>  
>  static Property scsi_hd_properties[] = {
>      DEFINE_SCSI_DISK_PROPERTIES(),
> -    DEFINE_PROP_BIT("removable", SCSIDiskState, features,
> -                    SCSI_DISK_F_REMOVABLE, false),
>      DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
>                      SCSI_DISK_F_DPOFUA, false),
>      DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
> @@ -2075,8 +2071,6 @@ static TypeInfo scsi_block_info = {
>  
>  static Property scsi_disk_properties[] = {
>      DEFINE_SCSI_DISK_PROPERTIES(),
> -    DEFINE_PROP_BIT("removable", SCSIDiskState, features,
> -                    SCSI_DISK_F_REMOVABLE, false),
>      DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
>                      SCSI_DISK_F_DPOFUA, false),
>      DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
> diff --git a/qemu-config.c b/qemu-config.c
> index 5c3296b..ab12f5a 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -87,6 +87,10 @@ static QemuOptsList qemu_drive_opts = {
>              .type = QEMU_OPT_BOOL,
>              .help = "open drive file as read-only",
>          },{
> +            .name = "removable",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "mark the device as removable",
> +        },{
>              .name = "iops",
>              .type = QEMU_OPT_NUMBER,
>              .help = "limit total I/O operations per second",
> diff --git a/qemu-options.hx b/qemu-options.hx
> index dc68e15..3748e38 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -196,6 +196,8 @@ Open drive @option{file} as read-only. Guest write attempts will fail.
>  @item copy-on-read=@var{copy-on-read}
>  @var{copy-on-read} is "on" or "off" and enables whether to copy read backing
>  file sectors into the image file.
> +@item removable
> +Mark the drive @option{file} as removable. (CDROM devices are always removable).
>  @end table
>  
>  By default, writethrough caching is used for all block device.  This means that
>

Patch

diff --git a/block.c b/block.c
index ce7eb8f..e9c0bfa 100644
--- a/block.c
+++ b/block.c
@@ -2151,6 +2151,16 @@  int bdrv_is_read_only(BlockDriverState *bs)
     return bs->read_only;
 }
 
+int bdrv_is_removable(BlockDriverState *bs)
+{
+    return bs->removable;
+}
+
+void bdrv_set_removable(BlockDriverState *bs, bool removable)
+{
+    bs->removable = removable;
+}
+
 int bdrv_is_sg(BlockDriverState *bs)
 {
     return bs->sg;
diff --git a/block.h b/block.h
index c89590d..b6dd71f 100644
--- a/block.h
+++ b/block.h
@@ -261,6 +261,8 @@  void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
                        BlockErrorAction on_write_error);
 BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
 int bdrv_is_read_only(BlockDriverState *bs);
+int bdrv_is_removable(BlockDriverState *bs);
+void bdrv_set_removable(BlockDriverState *bs, bool removable);
 int bdrv_is_sg(BlockDriverState *bs);
 int bdrv_enable_write_cache(BlockDriverState *bs);
 void bdrv_set_enable_write_cache(BlockDriverState *bs, bool wce);
diff --git a/block_int.h b/block_int.h
index d72317f..3f6c771 100644
--- a/block_int.h
+++ b/block_int.h
@@ -266,6 +266,7 @@  struct BlockDriverState {
                               size in sectors */
     int read_only; /* if true, the media is read only */
     int keep_read_only; /* if true, the media was requested to stay read only */
+    int removable; /* if true, the media is removable */
     int open_flags; /* flags used to open the file, re-used for re-open */
     int encrypted; /* if true, the media is encrypted */
     int valid_key; /* if true, a valid encryption key has been set */
diff --git a/blockdev.c b/blockdev.c
index 3d75015..034112c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -288,6 +288,7 @@  DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     int max_devs;
     int index;
     int ro = 0;
+    int removable = 0;
     int bdrv_flags = 0;
     int on_read_error, on_write_error;
     const char *devaddr;
@@ -311,6 +312,7 @@  DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 
     snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
     ro = qemu_opt_get_bool(opts, "readonly", 0);
+    removable = qemu_opt_get_bool(opts, "removable", 0);
     copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
 
     file = qemu_opt_get(opts, "file");
@@ -591,15 +593,25 @@  DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     if (media == MEDIA_CDROM) {
         /* CDROM is fine for any interface, don't check.  */
         ro = 1;
-    } else if (ro == 1) {
-        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
-            type != IF_NONE && type != IF_PFLASH) {
-            error_report("readonly not supported by this bus type");
-            goto err;
+        removable = 1;
+    } else {
+        if (ro == 1) {
+            if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
+                type != IF_NONE && type != IF_PFLASH) {
+                error_report("readonly not supported by this bus type");
+                goto err;
+            }
+        }
+        if (removable == 1) {
+            if (type != IF_SCSI) {
+                error_report("removable not supported by this bus type");
+                goto err;
+            }
         }
     }
 
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
+    bdrv_set_removable(dinfo->bdrv, removable);
 
     if (ro && copy_on_read) {
         error_report("warning: disabling copy_on_read on readonly drive");
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 5426990..4c394bd 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -58,8 +58,7 @@  typedef struct SCSIDiskReq {
     BlockAcctCookie acct;
 } SCSIDiskReq;
 
-#define SCSI_DISK_F_REMOVABLE   0
-#define SCSI_DISK_F_DPOFUA      1
+#define SCSI_DISK_F_DPOFUA      0
 
 struct SCSIDiskState
 {
@@ -668,7 +667,7 @@  static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
     memset(outbuf, 0, buflen);
 
     outbuf[0] = s->qdev.type & 0x1f;
-    outbuf[1] = (s->features & (1 << SCSI_DISK_F_REMOVABLE)) ? 0x80 : 0;
+    outbuf[1] = bdrv_is_removable(s->qdev.conf.bs) ? 0x80 : 0;
     if (s->qdev.type == TYPE_ROM) {
         memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
     } else {
@@ -1251,7 +1250,7 @@  static int scsi_disk_emulate_start_stop(SCSIDiskReq *r)
         return 0;
     }
 
-    if (s->qdev.type == TYPE_ROM && loej) {
+    if (bdrv_is_removable(s->qdev.conf.bs) && loej) {
         if (!start && !s->tray_open && s->tray_locked) {
             scsi_check_condition(r,
                                  bdrv_is_inserted(s->qdev.conf.bs)
@@ -1749,7 +1748,7 @@  static int scsi_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (!(s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
+    if (!bdrv_is_removable(s->qdev.conf.bs) &&
         !bdrv_is_inserted(s->qdev.conf.bs)) {
         error_report("Device needs media, but drive is empty");
         return -1;
@@ -1769,7 +1768,7 @@  static int scsi_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
+    if (bdrv_is_removable(s->qdev.conf.bs)) {
         bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_cd_block_ops, s);
     }
     bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
@@ -1792,7 +1791,6 @@  static int scsi_cd_initfn(SCSIDevice *dev)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     s->qdev.blocksize = 2048;
     s->qdev.type = TYPE_ROM;
-    s->features |= 1 << SCSI_DISK_F_REMOVABLE;
     return scsi_initfn(&s->qdev);
 }
 
@@ -1866,7 +1864,7 @@  static int get_device_type(SCSIDiskState *s)
     }
     s->qdev.type = buf[0];
     if (buf[1] & 0x80) {
-        s->features |= 1 << SCSI_DISK_F_REMOVABLE;
+        bdrv_set_removable(s->qdev.conf.bs, true);
     }
     return 0;
 }
@@ -1967,8 +1965,6 @@  static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
 
 static Property scsi_hd_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
-    DEFINE_PROP_BIT("removable", SCSIDiskState, features,
-                    SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
                     SCSI_DISK_F_DPOFUA, false),
     DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
@@ -2075,8 +2071,6 @@  static TypeInfo scsi_block_info = {
 
 static Property scsi_disk_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
-    DEFINE_PROP_BIT("removable", SCSIDiskState, features,
-                    SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
                     SCSI_DISK_F_DPOFUA, false),
     DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
diff --git a/qemu-config.c b/qemu-config.c
index 5c3296b..ab12f5a 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -87,6 +87,10 @@  static QemuOptsList qemu_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
         },{
+            .name = "removable",
+            .type = QEMU_OPT_BOOL,
+            .help = "mark the device as removable",
+        },{
             .name = "iops",
             .type = QEMU_OPT_NUMBER,
             .help = "limit total I/O operations per second",
diff --git a/qemu-options.hx b/qemu-options.hx
index dc68e15..3748e38 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -196,6 +196,8 @@  Open drive @option{file} as read-only. Guest write attempts will fail.
 @item copy-on-read=@var{copy-on-read}
 @var{copy-on-read} is "on" or "off" and enables whether to copy read backing
 file sectors into the image file.
+@item removable
+Mark the drive @option{file} as removable. (CDROM devices are always removable).
 @end table
 
 By default, writethrough caching is used for all block device.  This means that