On Thu, 21 Oct 2010 21:55:53 -0500 Ryan Harper <ry...@us.ibm.com> wrote:
> Block hot unplug is racy since the guest is required to acknowlege the ACPI > unplug event; this may not happen synchronously with the device removal > command > > This series aims to close a gap where by mgmt applications that assume the > block resource has been removed without confirming that the guest has > acknowledged the removal may re-assign the underlying device to a second guest > leading to data leakage. > > This series introduces a new montor command to decouple asynchornous device > removal from restricting guest access to a block device. We do this by > creating > a new monitor command drive_unplug which maps to a bdrv_unplug() command which > does a qemu_aio_flush; bdrv_flush() and bdrv_close(). Once complete, > subsequent > IO is rejected from the device and the guest will get IO errors but continue > to > function. > > A subsequent device removal command can be issued to remove the device, to > which > the guest may or maynot respond, but as long as the unplugged bit is set, no > IO > will be sumbitted. > > Changes since v2: > - Added qmp command for drive_unplug > > Changes since v1: > - Added qemu_aio_flush() before bdrv_flush() to wait on pending io > > Signed-off-by: Ryan Harper <ry...@us.ibm.com> > --- > block.c | 7 +++++++ > block.h | 1 + > blockdev.c | 26 ++++++++++++++++++++++++++ > blockdev.h | 1 + > hmp-commands.hx | 15 +++++++++++++++ > qmp-commands.hx | 26 ++++++++++++++++++++++++++ > 6 files changed, 76 insertions(+), 0 deletions(-) > > diff --git a/block.c b/block.c > index a19374d..be47655 100644 > --- a/block.c > +++ b/block.c > @@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int > removable) > } > } > > +void bdrv_unplug(BlockDriverState *bs) > +{ > + qemu_aio_flush(); > + bdrv_flush(bs); > + bdrv_close(bs); > +} > + > int bdrv_is_removable(BlockDriverState *bs) > { > return bs->removable; > diff --git a/block.h b/block.h > index 5f64380..732f63e 100644 > --- a/block.h > +++ b/block.h > @@ -171,6 +171,7 @@ 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); > void bdrv_set_removable(BlockDriverState *bs, int removable); > +void bdrv_unplug(BlockDriverState *bs); > int bdrv_is_removable(BlockDriverState *bs); > int bdrv_is_read_only(BlockDriverState *bs); > int bdrv_is_sg(BlockDriverState *bs); > diff --git a/blockdev.c b/blockdev.c > index 5fc3b9b..68eb329 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -610,3 +610,29 @@ int do_change_block(Monitor *mon, const char *device, > } > return monitor_read_bdrv_key_start(mon, bs, NULL, NULL); > } > + > +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data) > +{ > + DriveInfo *dinfo; > + BlockDriverState *bs; > + const char *id; > + > + if (!qdict_haskey(qdict, "id")) { > + qerror_report(QERR_MISSING_PARAMETER, "id"); > + return -1; > + } You can drop this check, "id" is a required argument, the upper layer will perform the check for you. > + > + id = qdict_get_str(qdict, "id"); > + dinfo = drive_get_by_id(id); > + if (!dinfo) { > + qerror_report(QERR_DEVICE_NOT_FOUND, id); > + return -1; > + } > + > + /* mark block device unplugged */ > + bs = dinfo->bdrv; > + bdrv_unplug(bs); > + > + return 0; > +} > + > diff --git a/blockdev.h b/blockdev.h > index 19c6915..ecb9ac8 100644 > --- a/blockdev.h > +++ b/blockdev.h > @@ -52,5 +52,6 @@ int do_eject(Monitor *mon, const QDict *qdict, QObject > **ret_data); > int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject > **ret_data); > int do_change_block(Monitor *mon, const char *device, > const char *filename, const char *fmt); > +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data); > > #endif > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 81999aa..7a32a2e 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -68,6 +68,21 @@ Eject a removable medium (use -f to force it). > ETEXI > > { > + .name = "drive_unplug", > + .args_type = "id:s", > + .params = "device", > + .help = "unplug block device", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_drive_unplug, > + }, > + > +STEXI > +...@item unplug @var{device} > +...@findex unplug > +Unplug block device. > +ETEXI > + > + { > .name = "change", > .args_type = "device:B,target:F,arg:s?", > .params = "device filename [format]", > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 793cf1c..e8f3d4a 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -338,6 +338,32 @@ Example: > EQMP > > { > + .name = "drive_unplug", > + .args_type = "id:s", > + .params = "device", > + .help = "unplug block device", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_drive_unplug, > + }, > + > +SQMP > +drive unplug > +---------- > + > +Unplug a block device. > + > +Arguments: > + > +- "id": the device's ID (json-string) > + > +Example: > + > +-> { "execute": "drive_unplug", "arguments": { "id": "drive-virtio-blk1" } } > +<- { "return": {} } > + > +EQMP > + > + { > .name = "cpu", > .args_type = "index:i", > .params = "index",