| Message ID | 1364430430-968-1-git-send-email-nab@linux-iscsi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 28, 2013 at 12:27:10AM +0000, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger <nab@linux-iscsi.org> > > This patch adds a VHOST_TCM_FEATURES mask minus VIRTIO_RING_F_EVENT_IDX > so that vhost-scsi-pci userspace will strip this feature bit once > GET_FEATURES reports it as being unsupported on the host. > > This is to avoid a bug where ->handle_kicks() are missed when EVENT_IDX > is enabled by default in userspace code. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Asias He <asias@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> No chance we can debug this properly? > --- > drivers/vhost/tcm_vhost.c | 10 ++++++++-- > 1 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > index 0524267..b127edc 100644 > --- a/drivers/vhost/tcm_vhost.c > +++ b/drivers/vhost/tcm_vhost.c > @@ -60,6 +60,12 @@ enum { > VHOST_SCSI_VQ_IO = 2, > }; > > +enum { > + VHOST_TCM_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | > + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | > + (1ULL << VHOST_F_LOG_ALL) All the rest of the code uses VHOST_SCSI so rename this too? > +}; > + Please do something like +/* +* VIRTIO_RING_F_EVENT_IDX seems broken. Not sure the bug is in +* kernel but disabling it helps. +* TODO: debug and remove the workaround. +*/ +enum { + VHOST_SCSI_FEATURES = VHOST_FEATURES & (~VIRTIO_RING_F_EVENT_IDX) +} > #define VHOST_SCSI_MAX_TARGET 256 > #define VHOST_SCSI_MAX_VQ 128 > > @@ -981,7 +987,7 @@ static void vhost_scsi_flush(struct vhost_scsi *vs) > > static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features) > { > - if (features & ~VHOST_FEATURES) > + if (features & ~VHOST_TCM_FEATURES) > return -EOPNOTSUPP; > > mutex_lock(&vs->dev.mutex); > @@ -1027,7 +1033,7 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl, > return -EFAULT; > return 0; > case VHOST_GET_FEATURES: > - features = VHOST_FEATURES; > + features = VHOST_TCM_FEATURES; > if (copy_to_user(featurep, &features, sizeof features)) > return -EFAULT; > return 0; > -- > 1.7.2.5 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-03-28 at 08:04 +0200, Michael S. Tsirkin wrote: > On Thu, Mar 28, 2013 at 12:27:10AM +0000, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > > > This patch adds a VHOST_TCM_FEATURES mask minus VIRTIO_RING_F_EVENT_IDX > > so that vhost-scsi-pci userspace will strip this feature bit once > > GET_FEATURES reports it as being unsupported on the host. > > > > This is to avoid a bug where ->handle_kicks() are missed when EVENT_IDX > > is enabled by default in userspace code. > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: Asias He <asias@redhat.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > > No chance we can debug this properly? > Well, I assumed this was a low priority item ahead of tracking down the other blocking seabios related issue. I'm fine with dropping this patch and requiring the event_idx=off option for now for an initial QEMU merge, but thought this made things a little easier for CLI users to avoid confusion. > > --- > > drivers/vhost/tcm_vhost.c | 10 ++++++++-- > > 1 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > > index 0524267..b127edc 100644 > > --- a/drivers/vhost/tcm_vhost.c > > +++ b/drivers/vhost/tcm_vhost.c > > @@ -60,6 +60,12 @@ enum { > > VHOST_SCSI_VQ_IO = 2, > > }; > > > > +enum { > > + VHOST_TCM_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | > > + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | > > + (1ULL << VHOST_F_LOG_ALL) > > All the rest of the code uses VHOST_SCSI so rename this too? > > > +}; > > + > > Please do something like > > +/* > +* VIRTIO_RING_F_EVENT_IDX seems broken. Not sure the bug is in > +* kernel but disabling it helps. > +* TODO: debug and remove the workaround. > +*/ > +enum { > + VHOST_SCSI_FEATURES = VHOST_FEATURES & (~VIRTIO_RING_F_EVENT_IDX) > +} > Patch updated. Thanks, --nab > > #define VHOST_SCSI_MAX_TARGET 256 > > #define VHOST_SCSI_MAX_VQ 128 > > > > @@ -981,7 +987,7 @@ static void vhost_scsi_flush(struct vhost_scsi *vs) > > > > static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features) > > { > > - if (features & ~VHOST_FEATURES) > > + if (features & ~VHOST_TCM_FEATURES) > > return -EOPNOTSUPP; > > > > mutex_lock(&vs->dev.mutex); > > @@ -1027,7 +1033,7 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl, > > return -EFAULT; > > return 0; > > case VHOST_GET_FEATURES: > > - features = VHOST_FEATURES; > > + features = VHOST_TCM_FEATURES; > > if (copy_to_user(featurep, &features, sizeof features)) > > return -EFAULT; > > return 0; > > -- > > 1.7.2.5 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 27, 2013 at 11:25:02PM -0700, Nicholas A. Bellinger wrote: > On Thu, 2013-03-28 at 08:04 +0200, Michael S. Tsirkin wrote: > > On Thu, Mar 28, 2013 at 12:27:10AM +0000, Nicholas A. Bellinger wrote: > > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > > > > > This patch adds a VHOST_TCM_FEATURES mask minus VIRTIO_RING_F_EVENT_IDX > > > so that vhost-scsi-pci userspace will strip this feature bit once > > > GET_FEATURES reports it as being unsupported on the host. > > > > > > This is to avoid a bug where ->handle_kicks() are missed when EVENT_IDX > > > is enabled by default in userspace code. > > > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > Cc: Asias He <asias@redhat.com> > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > > > > No chance we can debug this properly? > > > > Well, I assumed this was a low priority item ahead of tracking down the > other blocking seabios related issue. > > I'm fine with dropping this patch and requiring the event_idx=off option > for now for an initial QEMU merge, but thought this made things a little > easier for CLI users to avoid confusion. Yea, we can't drink the ocean. > > > --- > > > drivers/vhost/tcm_vhost.c | 10 ++++++++-- > > > 1 files changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > > > index 0524267..b127edc 100644 > > > --- a/drivers/vhost/tcm_vhost.c > > > +++ b/drivers/vhost/tcm_vhost.c > > > @@ -60,6 +60,12 @@ enum { > > > VHOST_SCSI_VQ_IO = 2, > > > }; > > > > > > +enum { > > > + VHOST_TCM_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | > > > + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | > > > + (1ULL << VHOST_F_LOG_ALL) > > > > All the rest of the code uses VHOST_SCSI so rename this too? > > > > > +}; > > > + > > > > Please do something like > > > > +/* > > +* VIRTIO_RING_F_EVENT_IDX seems broken. Not sure the bug is in > > +* kernel but disabling it helps. > > +* TODO: debug and remove the workaround. > > +*/ > > +enum { > > + VHOST_SCSI_FEATURES = VHOST_FEATURES & (~VIRTIO_RING_F_EVENT_IDX) > > +} > > > > Patch updated. > > Thanks, > > --nab > > > > #define VHOST_SCSI_MAX_TARGET 256 > > > #define VHOST_SCSI_MAX_VQ 128 > > > > > > @@ -981,7 +987,7 @@ static void vhost_scsi_flush(struct vhost_scsi *vs) > > > > > > static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features) > > > { > > > - if (features & ~VHOST_FEATURES) > > > + if (features & ~VHOST_TCM_FEATURES) > > > return -EOPNOTSUPP; > > > > > > mutex_lock(&vs->dev.mutex); > > > @@ -1027,7 +1033,7 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl, > > > return -EFAULT; > > > return 0; > > > case VHOST_GET_FEATURES: > > > - features = VHOST_FEATURES; > > > + features = VHOST_TCM_FEATURES; > > > if (copy_to_user(featurep, &features, sizeof features)) > > > return -EFAULT; > > > return 0; > > > -- > > > 1.7.2.5 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 27, 2013 at 11:25:02PM -0700, Nicholas A. Bellinger wrote: > On Thu, 2013-03-28 at 08:04 +0200, Michael S. Tsirkin wrote: > > On Thu, Mar 28, 2013 at 12:27:10AM +0000, Nicholas A. Bellinger wrote: > > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > > > > > This patch adds a VHOST_TCM_FEATURES mask minus VIRTIO_RING_F_EVENT_IDX > > > so that vhost-scsi-pci userspace will strip this feature bit once > > > GET_FEATURES reports it as being unsupported on the host. > > > > > > This is to avoid a bug where ->handle_kicks() are missed when EVENT_IDX > > > is enabled by default in userspace code. > > > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > Cc: Asias He <asias@redhat.com> > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > > > > No chance we can debug this properly? > > > > Well, I assumed this was a low priority item ahead of tracking down the > other blocking seabios related issue. > > I'm fine with dropping this patch and requiring the event_idx=off option > for now for an initial QEMU merge, but thought this made things a little > easier for CLI users to avoid confusion. > > > > --- > > > drivers/vhost/tcm_vhost.c | 10 ++++++++-- > > > 1 files changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > > > index 0524267..b127edc 100644 > > > --- a/drivers/vhost/tcm_vhost.c > > > +++ b/drivers/vhost/tcm_vhost.c > > > @@ -60,6 +60,12 @@ enum { > > > VHOST_SCSI_VQ_IO = 2, > > > }; > > > > > > +enum { > > > + VHOST_TCM_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | > > > + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | > > > + (1ULL << VHOST_F_LOG_ALL) > > > > All the rest of the code uses VHOST_SCSI so rename this too? > > > > > +}; > > > + > > > > Please do something like > > > > +/* > > +* VIRTIO_RING_F_EVENT_IDX seems broken. Not sure the bug is in > > +* kernel but disabling it helps. > > +* TODO: debug and remove the workaround. > > +*/ > > +enum { > > + VHOST_SCSI_FEATURES = VHOST_FEATURES & (~VIRTIO_RING_F_EVENT_IDX) > > +} > > > > Patch updated. > > Thanks, > > --nab Still haven't seen it, anyway Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > #define VHOST_SCSI_MAX_TARGET 256 > > > #define VHOST_SCSI_MAX_VQ 128 > > > > > > @@ -981,7 +987,7 @@ static void vhost_scsi_flush(struct vhost_scsi *vs) > > > > > > static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features) > > > { > > > - if (features & ~VHOST_FEATURES) > > > + if (features & ~VHOST_TCM_FEATURES) > > > return -EOPNOTSUPP; > > > > > > mutex_lock(&vs->dev.mutex); > > > @@ -1027,7 +1033,7 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl, > > > return -EFAULT; > > > return 0; > > > case VHOST_GET_FEATURES: > > > - features = VHOST_FEATURES; > > > + features = VHOST_TCM_FEATURES; > > > if (copy_to_user(featurep, &features, sizeof features)) > > > return -EFAULT; > > > return 0; > > > -- > > > 1.7.2.5 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-03-28 at 18:44 +0200, Michael S. Tsirkin wrote: > On Wed, Mar 27, 2013 at 11:25:02PM -0700, Nicholas A. Bellinger wrote: > > On Thu, 2013-03-28 at 08:04 +0200, Michael S. Tsirkin wrote: > > > On Thu, Mar 28, 2013 at 12:27:10AM +0000, Nicholas A. Bellinger wrote: > > > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > > > > > > > This patch adds a VHOST_TCM_FEATURES mask minus VIRTIO_RING_F_EVENT_IDX > > > > so that vhost-scsi-pci userspace will strip this feature bit once > > > > GET_FEATURES reports it as being unsupported on the host. > > > > > > > > This is to avoid a bug where ->handle_kicks() are missed when EVENT_IDX > > > > is enabled by default in userspace code. > > > > > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > > Cc: Asias He <asias@redhat.com> > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > > > > > > No chance we can debug this properly? > > > > > > > Well, I assumed this was a low priority item ahead of tracking down the > > other blocking seabios related issue. > > > > I'm fine with dropping this patch and requiring the event_idx=off option > > for now for an initial QEMU merge, but thought this made things a little > > easier for CLI users to avoid confusion. > > > > > > --- > > > > drivers/vhost/tcm_vhost.c | 10 ++++++++-- > > > > 1 files changed, 8 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > > > > index 0524267..b127edc 100644 > > > > --- a/drivers/vhost/tcm_vhost.c > > > > +++ b/drivers/vhost/tcm_vhost.c > > > > @@ -60,6 +60,12 @@ enum { > > > > VHOST_SCSI_VQ_IO = 2, > > > > }; > > > > > > > > +enum { > > > > + VHOST_TCM_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | > > > > + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | > > > > + (1ULL << VHOST_F_LOG_ALL) > > > > > > All the rest of the code uses VHOST_SCSI so rename this too? > > > > > > > +}; > > > > + > > > > > > Please do something like > > > > > > +/* > > > +* VIRTIO_RING_F_EVENT_IDX seems broken. Not sure the bug is in > > > +* kernel but disabling it helps. > > > +* TODO: debug and remove the workaround. > > > +*/ > > > +enum { > > > + VHOST_SCSI_FEATURES = VHOST_FEATURES & (~VIRTIO_RING_F_EVENT_IDX) > > > +} > > > > > > > Patch updated. > > > > Thanks, > > > > --nab > > > Still haven't seen it, anyway Apologies, I had to attend to some other items this afternoon.. Sent this updated patch out a moment ago. > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > Thanks, including this now. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c index 0524267..b127edc 100644 --- a/drivers/vhost/tcm_vhost.c +++ b/drivers/vhost/tcm_vhost.c @@ -60,6 +60,12 @@ enum { VHOST_SCSI_VQ_IO = 2, }; +enum { + VHOST_TCM_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | + (1ULL << VHOST_F_LOG_ALL) +}; + #define VHOST_SCSI_MAX_TARGET 256 #define VHOST_SCSI_MAX_VQ 128 @@ -981,7 +987,7 @@ static void vhost_scsi_flush(struct vhost_scsi *vs) static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features) { - if (features & ~VHOST_FEATURES) + if (features & ~VHOST_TCM_FEATURES) return -EOPNOTSUPP; mutex_lock(&vs->dev.mutex); @@ -1027,7 +1033,7 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl, return -EFAULT; return 0; case VHOST_GET_FEATURES: - features = VHOST_FEATURES; + features = VHOST_TCM_FEATURES; if (copy_to_user(featurep, &features, sizeof features)) return -EFAULT; return 0;