Message ID | 1344846917-7411-5-git-send-email-nab@linux-iscsi.org |
---|---|
State | New |
Headers | show |
Il 13/08/2012 10:35, Nicholas A. Bellinger ha scritto: > From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > This patch starts and stops vhost as the virtio device transitions > through its status phases. Vhost can only be started once the guest > reports its driver has successfully initialized, which means the > virtqueues have been set up by the guest. > > v2: - Squash virtio-scsi: use the vhost-scsi host device from stefan (nab) > - Fix up virtio_scsi_properties[] conflict w/ upstream qemu (nab) > - Drop usage of to_virtio_scsi() in virtio_scsi_set_status() > (reported by paolo) > - Use modern VirtIOSCSIConf define in virtio-scsi.h (reported by paolo) > - Use s->conf->vhost_scsi instead of proxyconf->vhost_scsi in > virtio_scsi_init() (reported by paolo) > - Only register QEMU SCSI bus is vhost-scsi is not active (reported > by paolo) How much of the functionality of virtio-scsi.[ch] is still in use at this point? Would it make more sense to use a separate vhost-scsi-pci device instead? Especially since advertising VIRTIO_SCSI_F_HOTPLUG and VIRTIO_SCSI_F_CHANGE is probably wrong for vhost-scsi... Paolo > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > --- > hw/virtio-pci.c | 1 + > hw/virtio-scsi.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > hw/virtio-scsi.h | 1 + > 3 files changed, 50 insertions(+), 0 deletions(-) > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 125eded..b29fc3b 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -1036,6 +1036,7 @@ static void virtio_scsi_exit_pci(PCIDevice *pci_dev) > } > > static Property virtio_scsi_properties[] = { > + DEFINE_PROP_VHOST_SCSI("vhost-scsi", VirtIOPCIProxy, scsi.vhost_scsi), > DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), > DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED), > DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), > diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c > index 5f737ac..8130956 100644 > --- a/hw/virtio-scsi.c > +++ b/hw/virtio-scsi.c > @@ -13,9 +13,13 @@ > * > */ > > +#include "qemu-common.h" > +#include "qemu-error.h" > +#include "vhost-scsi.h" > #include "virtio-scsi.h" > #include <hw/scsi.h> > #include <hw/scsi-defs.h> > +#include "vhost.h" > > #define VIRTIO_SCSI_VQ_SIZE 128 > #define VIRTIO_SCSI_CDB_SIZE 32 > @@ -147,6 +151,9 @@ typedef struct { > VirtQueue *ctrl_vq; > VirtQueue *event_vq; > VirtQueue *cmd_vqs[0]; > + > + bool vhost_started; > + VHostSCSI *vhost_scsi; > } VirtIOSCSI; > > typedef struct VirtIOSCSIReq { > @@ -699,6 +706,38 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = { > .load_request = virtio_scsi_load_request, > }; > > +static bool virtio_scsi_started(VirtIOSCSI *s, uint8_t val) > +{ > + return (val & VIRTIO_CONFIG_S_DRIVER_OK) && s->vdev.vm_running; > +} > + > +static void virtio_scsi_set_status(VirtIODevice *vdev, uint8_t val) > +{ > + VirtIOSCSI *s = (VirtIOSCSI *)vdev; > + bool start = virtio_scsi_started(s, val); > + > + if (s->vhost_started == start) { > + return; > + } > + > + if (start) { > + int ret; > + > + ret = vhost_scsi_start(s->vhost_scsi, vdev); > + if (ret < 0) { > + error_report("virtio-scsi: unable to start vhost: %s\n", > + strerror(-ret)); > + > + /* There is no userspace virtio-scsi fallback so exit */ > + exit(1); > + } > + } else { > + vhost_scsi_stop(s->vhost_scsi, vdev); > + } > + > + s->vhost_started = start; > +} > + > VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf) > { > VirtIOSCSI *s; > @@ -712,12 +751,17 @@ VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf) > > s->qdev = dev; > s->conf = proxyconf; > + s->vhost_started = false; > + s->vhost_scsi = s->conf->vhost_scsi; > > /* TODO set up vdev function pointers */ > s->vdev.get_config = virtio_scsi_get_config; > s->vdev.set_config = virtio_scsi_set_config; > s->vdev.get_features = virtio_scsi_get_features; > s->vdev.reset = virtio_scsi_reset; > + if (s->vhost_scsi) { > + s->vdev.set_status = virtio_scsi_set_status; > + } > > s->ctrl_vq = virtio_add_queue(&s->vdev, VIRTIO_SCSI_VQ_SIZE, > virtio_scsi_handle_ctrl); > @@ -743,5 +787,9 @@ void virtio_scsi_exit(VirtIODevice *vdev) > { > VirtIOSCSI *s = (VirtIOSCSI *)vdev; > unregister_savevm(s->qdev, "virtio-scsi", s); > + > + /* This will stop vhost backend if appropriate. */ > + virtio_scsi_set_status(vdev, 0); > + > virtio_cleanup(vdev); > } > diff --git a/hw/virtio-scsi.h b/hw/virtio-scsi.h > index 4bc889d..74e9422 100644 > --- a/hw/virtio-scsi.h > +++ b/hw/virtio-scsi.h > @@ -22,6 +22,7 @@ > #define VIRTIO_ID_SCSI 8 > > struct VirtIOSCSIConf { > + VHostSCSI *vhost_scsi; > uint32_t num_queues; > uint32_t max_sectors; > uint32_t cmd_per_lun; >
On Mon, Aug 20, 2012 at 10:04 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 13/08/2012 10:35, Nicholas A. Bellinger ha scritto: >> From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> >> >> This patch starts and stops vhost as the virtio device transitions >> through its status phases. Vhost can only be started once the guest >> reports its driver has successfully initialized, which means the >> virtqueues have been set up by the guest. >> >> v2: - Squash virtio-scsi: use the vhost-scsi host device from stefan (nab) >> - Fix up virtio_scsi_properties[] conflict w/ upstream qemu (nab) >> - Drop usage of to_virtio_scsi() in virtio_scsi_set_status() >> (reported by paolo) >> - Use modern VirtIOSCSIConf define in virtio-scsi.h (reported by paolo) >> - Use s->conf->vhost_scsi instead of proxyconf->vhost_scsi in >> virtio_scsi_init() (reported by paolo) >> - Only register QEMU SCSI bus is vhost-scsi is not active (reported >> by paolo) > > How much of the functionality of virtio-scsi.[ch] is still in use at > this point? Would it make more sense to use a separate vhost-scsi-pci > device instead? Since the SCSI target lives in the kernel, almost everything is driven from tcm_vhost.ko. tcm_vhost.ko basically implements the full device so I see the argument for -device vhost-scsi-pci. > Especially since advertising VIRTIO_SCSI_F_HOTPLUG and > VIRTIO_SCSI_F_CHANGE is probably wrong for vhost-scsi... vhost participates in feature bit negotiation, see VHOST_GET_FEATURES/VHOST_SET_FEATURES ioctls. Stefan
On Mon, Aug 20, 2012 at 12:31:01PM +0100, Stefan Hajnoczi wrote: > On Mon, Aug 20, 2012 at 10:04 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 13/08/2012 10:35, Nicholas A. Bellinger ha scritto: > >> From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > >> > >> This patch starts and stops vhost as the virtio device transitions > >> through its status phases. Vhost can only be started once the guest > >> reports its driver has successfully initialized, which means the > >> virtqueues have been set up by the guest. > >> > >> v2: - Squash virtio-scsi: use the vhost-scsi host device from stefan (nab) > >> - Fix up virtio_scsi_properties[] conflict w/ upstream qemu (nab) > >> - Drop usage of to_virtio_scsi() in virtio_scsi_set_status() > >> (reported by paolo) > >> - Use modern VirtIOSCSIConf define in virtio-scsi.h (reported by paolo) > >> - Use s->conf->vhost_scsi instead of proxyconf->vhost_scsi in > >> virtio_scsi_init() (reported by paolo) > >> - Only register QEMU SCSI bus is vhost-scsi is not active (reported > >> by paolo) > > > > How much of the functionality of virtio-scsi.[ch] is still in use at > > this point? Would it make more sense to use a separate vhost-scsi-pci > > device instead? > > Since the SCSI target lives in the kernel, almost everything is driven > from tcm_vhost.ko. tcm_vhost.ko basically implements the full device > so I see the argument for -device vhost-scsi-pci. A bit unhappy that there is no virtio in the name since it is implementing virtio protocol. > > Especially since advertising VIRTIO_SCSI_F_HOTPLUG and > > VIRTIO_SCSI_F_CHANGE is probably wrong for vhost-scsi... > > vhost participates in feature bit negotiation, see > VHOST_GET_FEATURES/VHOST_SET_FEATURES ioctls. > > Stefan
Il 20/08/2012 13:57, Michael S. Tsirkin ha scritto: >>> > > How much of the functionality of virtio-scsi.[ch] is still in use at >>> > > this point? Would it make more sense to use a separate vhost-scsi-pci >>> > > device instead? >> > >> > Since the SCSI target lives in the kernel, almost everything is driven >> > from tcm_vhost.ko. tcm_vhost.ko basically implements the full device >> > so I see the argument for -device vhost-scsi-pci. > A bit unhappy that there is no virtio in the name since it is > implementing virtio protocol. > Yeah, but vhost \subseteq virtio... Paolo
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 125eded..b29fc3b 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -1036,6 +1036,7 @@ static void virtio_scsi_exit_pci(PCIDevice *pci_dev) } static Property virtio_scsi_properties[] = { + DEFINE_PROP_VHOST_SCSI("vhost-scsi", VirtIOPCIProxy, scsi.vhost_scsi), DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED), DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c index 5f737ac..8130956 100644 --- a/hw/virtio-scsi.c +++ b/hw/virtio-scsi.c @@ -13,9 +13,13 @@ * */ +#include "qemu-common.h" +#include "qemu-error.h" +#include "vhost-scsi.h" #include "virtio-scsi.h" #include <hw/scsi.h> #include <hw/scsi-defs.h> +#include "vhost.h" #define VIRTIO_SCSI_VQ_SIZE 128 #define VIRTIO_SCSI_CDB_SIZE 32 @@ -147,6 +151,9 @@ typedef struct { VirtQueue *ctrl_vq; VirtQueue *event_vq; VirtQueue *cmd_vqs[0]; + + bool vhost_started; + VHostSCSI *vhost_scsi; } VirtIOSCSI; typedef struct VirtIOSCSIReq { @@ -699,6 +706,38 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = { .load_request = virtio_scsi_load_request, }; +static bool virtio_scsi_started(VirtIOSCSI *s, uint8_t val) +{ + return (val & VIRTIO_CONFIG_S_DRIVER_OK) && s->vdev.vm_running; +} + +static void virtio_scsi_set_status(VirtIODevice *vdev, uint8_t val) +{ + VirtIOSCSI *s = (VirtIOSCSI *)vdev; + bool start = virtio_scsi_started(s, val); + + if (s->vhost_started == start) { + return; + } + + if (start) { + int ret; + + ret = vhost_scsi_start(s->vhost_scsi, vdev); + if (ret < 0) { + error_report("virtio-scsi: unable to start vhost: %s\n", + strerror(-ret)); + + /* There is no userspace virtio-scsi fallback so exit */ + exit(1); + } + } else { + vhost_scsi_stop(s->vhost_scsi, vdev); + } + + s->vhost_started = start; +} + VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf) { VirtIOSCSI *s; @@ -712,12 +751,17 @@ VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf) s->qdev = dev; s->conf = proxyconf; + s->vhost_started = false; + s->vhost_scsi = s->conf->vhost_scsi; /* TODO set up vdev function pointers */ s->vdev.get_config = virtio_scsi_get_config; s->vdev.set_config = virtio_scsi_set_config; s->vdev.get_features = virtio_scsi_get_features; s->vdev.reset = virtio_scsi_reset; + if (s->vhost_scsi) { + s->vdev.set_status = virtio_scsi_set_status; + } s->ctrl_vq = virtio_add_queue(&s->vdev, VIRTIO_SCSI_VQ_SIZE, virtio_scsi_handle_ctrl); @@ -743,5 +787,9 @@ void virtio_scsi_exit(VirtIODevice *vdev) { VirtIOSCSI *s = (VirtIOSCSI *)vdev; unregister_savevm(s->qdev, "virtio-scsi", s); + + /* This will stop vhost backend if appropriate. */ + virtio_scsi_set_status(vdev, 0); + virtio_cleanup(vdev); } diff --git a/hw/virtio-scsi.h b/hw/virtio-scsi.h index 4bc889d..74e9422 100644 --- a/hw/virtio-scsi.h +++ b/hw/virtio-scsi.h @@ -22,6 +22,7 @@ #define VIRTIO_ID_SCSI 8 struct VirtIOSCSIConf { + VHostSCSI *vhost_scsi; uint32_t num_queues; uint32_t max_sectors; uint32_t cmd_per_lun;