Re: [Xen-devel] [PATCH v2 14/18] xen: add implementations of xen-block connect and disconnect functions...
> -Original Message- > From: Anthony PERARD [mailto:anthony.per...@citrix.com] > Sent: 07 December 2018 18:21 > To: Paul Durrant > Cc: qemu-de...@nongnu.org; qemu-bl...@nongnu.org; xen- > de...@lists.xenproject.org; Stefano Stabellini ; > Kevin Wolf ; Max Reitz > Subject: Re: [PATCH v2 14/18] xen: add implementations of xen-block > connect and disconnect functions... > > On Thu, Dec 06, 2018 at 03:08:40PM +, Paul Durrant wrote: > > ...and wire in the dataplane. > > > > This patch adds the remaining code to make the xen-block XenDevice > > functional. The parameters that a block frontend expects to find are > > populated in the backend xenstore area, and the 'ring-ref' and > > 'event-channel' values specified in the frontend xenstore area are > > mapped/bound and used to set up the dataplane. > > > > Signed-off-by: Paul Durrant > > With this patch, we should be able to have QEMU instantiate a new > backend for a guest, right ? (via command line or QMP) > > I've tried, and that doesn't work, the xenstore path for the frontend is > wrong. In the qemu trace, I have: > xs_node_create /local/domain/0/backend/xen-disk/23/268572709 > Which is probably fine, even if not described in xenstore-paths.markdown. > xs_node_create /local/domain/23/device/xen-disk/268572709 > Which is not, instead of "xen-disk", we should have "vbd". > > I know that this is fixed in "xen: automatically create > XenBlockDevice-s", but at least the "vbd" type couldn't be added in this > patch, or maybe a previous one. > > > Another issue seems to be error handling. I've done a very simple test, > I've added '-device xen-disk,vdev=d536p37,id=mydisk' to the command > line (which is obvious wrong), and QEMU abort with: > qemu-system-i386: hw/block/xen-block.c:174: xen_block_realize: > Assertion `conf->blk' failed. > But I've pointed out the error in the code below. > > > And just for fun, adding then removing a xen-disk via QMP. Adding works > fine (once I've fixed the frontend name). I've run the following with > ./scripts/qmp/qmp-shell: > blockdev-add driver=file filename=/root/vm/disk/testing-disk.qcow2 node- > name=emptyfile > blockdev-add driver=qcow2 node-name=emptyqcow2 file=emptyfile > device_add driver=xen-disk vdev=xvdn id=fromqmp drive=emptyqcow2 > > But, then, remove doesn't work, running "device_del id=fromqmp" doesn't > do anything. I guess we can try to fix that later if you don't find > what's missing. The missing piece is a hotplug controller 'unplug' or 'unplug_request' method implementation. I think the 'right' thing to do is implement 'unplug_request' and mimic the behaviour of libxl... i.e. set 'online' to 0 and 'state' to closing (i.e. 5) under transaction lock. This means that any connected frontend should go through a clean disconnect and then the device will get removed. To that end I'm going to need to add transaction support to my helper functions (in patch #4) and then add in the unplug_request implementation in this patch. Paul ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 14/18] xen: add implementations of xen-block connect and disconnect functions...
> -Original Message- > From: Anthony PERARD [mailto:anthony.per...@citrix.com] > Sent: 07 December 2018 18:21 > To: Paul Durrant > Cc: qemu-de...@nongnu.org; qemu-bl...@nongnu.org; xen- > de...@lists.xenproject.org; Stefano Stabellini ; > Kevin Wolf ; Max Reitz > Subject: Re: [PATCH v2 14/18] xen: add implementations of xen-block > connect and disconnect functions... > > On Thu, Dec 06, 2018 at 03:08:40PM +, Paul Durrant wrote: > > ...and wire in the dataplane. > > > > This patch adds the remaining code to make the xen-block XenDevice > > functional. The parameters that a block frontend expects to find are > > populated in the backend xenstore area, and the 'ring-ref' and > > 'event-channel' values specified in the frontend xenstore area are > > mapped/bound and used to set up the dataplane. > > > > Signed-off-by: Paul Durrant > > With this patch, we should be able to have QEMU instantiate a new > backend for a guest, right ? (via command line or QMP) > > I've tried, and that doesn't work, the xenstore path for the frontend is > wrong. In the qemu trace, I have: > xs_node_create /local/domain/0/backend/xen-disk/23/268572709 > Which is probably fine, even if not described in xenstore-paths.markdown. > xs_node_create /local/domain/23/device/xen-disk/268572709 > Which is not, instead of "xen-disk", we should have "vbd". > > I know that this is fixed in "xen: automatically create > XenBlockDevice-s", but at least the "vbd" type couldn't be added in this > patch, or maybe a previous one. Yes, I guess I should move the name overrides into an earlier patch. > > > Another issue seems to be error handling. I've done a very simple test, > I've added '-device xen-disk,vdev=d536p37,id=mydisk' to the command > line (which is obvious wrong), and QEMU abort with: > qemu-system-i386: hw/block/xen-block.c:174: xen_block_realize: > Assertion `conf->blk' failed. > But I've pointed out the error in the code below. > > > And just for fun, adding then removing a xen-disk via QMP. Adding works > fine (once I've fixed the frontend name). I've run the following with > ./scripts/qmp/qmp-shell: > blockdev-add driver=file filename=/root/vm/disk/testing-disk.qcow2 node- > name=emptyfile > blockdev-add driver=qcow2 node-name=emptyqcow2 file=emptyfile > device_add driver=xen-disk vdev=xvdn id=fromqmp drive=emptyqcow2 > > But, then, remove doesn't work, running "device_del id=fromqmp" doesn't > do anything. I guess we can try to fix that later if you don't find > what's missing. Hmm, that's weird. I guess the name lookup must be failing somewhere. > > > @@ -76,6 +151,7 @@ static void xen_block_realize(XenDevice *xendev, > Error **errp) > > const char *type = object_get_typename(OBJECT(blockdev)); > > XenBlockVdev *vdev = >vdev; > > Error *local_err = NULL; > > +BlockConf *conf = >conf; > > > > if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) { > > error_setg(errp, "vdev property not set"); > > @@ -90,6 +166,59 @@ static void xen_block_realize(XenDevice *xendev, > Error **errp) > > error_propagate(errp, local_err); > > You probably want to add a return here, this is when > `blockdev_class->realize' fails. > Yes, I do. > > } > > } > > + > > +/* > > + * The blkif protocol does not deal with removable media, so it > must > > + * always be present, even for CDRom devices. > > + */ > > +assert(conf->blk); > > That assert should probably not be there, as a missing conf->blk isn't a > programming error, but a user error, I think. > > Actually, the issue is the missing return abrove, and the assert is > probably fine. > Yes, the assert is intentional and caught the programming error :-) Paul > -- > Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 14/18] xen: add implementations of xen-block connect and disconnect functions...
On Thu, Dec 06, 2018 at 03:08:40PM +, Paul Durrant wrote: > ...and wire in the dataplane. > > This patch adds the remaining code to make the xen-block XenDevice > functional. The parameters that a block frontend expects to find are > populated in the backend xenstore area, and the 'ring-ref' and > 'event-channel' values specified in the frontend xenstore area are > mapped/bound and used to set up the dataplane. > > Signed-off-by: Paul Durrant With this patch, we should be able to have QEMU instantiate a new backend for a guest, right ? (via command line or QMP) I've tried, and that doesn't work, the xenstore path for the frontend is wrong. In the qemu trace, I have: xs_node_create /local/domain/0/backend/xen-disk/23/268572709 Which is probably fine, even if not described in xenstore-paths.markdown. xs_node_create /local/domain/23/device/xen-disk/268572709 Which is not, instead of "xen-disk", we should have "vbd". I know that this is fixed in "xen: automatically create XenBlockDevice-s", but at least the "vbd" type couldn't be added in this patch, or maybe a previous one. Another issue seems to be error handling. I've done a very simple test, I've added '-device xen-disk,vdev=d536p37,id=mydisk' to the command line (which is obvious wrong), and QEMU abort with: qemu-system-i386: hw/block/xen-block.c:174: xen_block_realize: Assertion `conf->blk' failed. But I've pointed out the error in the code below. And just for fun, adding then removing a xen-disk via QMP. Adding works fine (once I've fixed the frontend name). I've run the following with ./scripts/qmp/qmp-shell: blockdev-add driver=file filename=/root/vm/disk/testing-disk.qcow2 node-name=emptyfile blockdev-add driver=qcow2 node-name=emptyqcow2 file=emptyfile device_add driver=xen-disk vdev=xvdn id=fromqmp drive=emptyqcow2 But, then, remove doesn't work, running "device_del id=fromqmp" doesn't do anything. I guess we can try to fix that later if you don't find what's missing. > @@ -76,6 +151,7 @@ static void xen_block_realize(XenDevice *xendev, Error > **errp) > const char *type = object_get_typename(OBJECT(blockdev)); > XenBlockVdev *vdev = >vdev; > Error *local_err = NULL; > +BlockConf *conf = >conf; > > if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) { > error_setg(errp, "vdev property not set"); > @@ -90,6 +166,59 @@ static void xen_block_realize(XenDevice *xendev, Error > **errp) > error_propagate(errp, local_err); You probably want to add a return here, this is when `blockdev_class->realize' fails. > } > } > + > +/* > + * The blkif protocol does not deal with removable media, so it must > + * always be present, even for CDRom devices. > + */ > +assert(conf->blk); That assert should probably not be there, as a missing conf->blk isn't a programming error, but a user error, I think. Actually, the issue is the missing return abrove, and the assert is probably fine. -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel