| Message ID | 1403093631-7384-1-git-send-email-ming.lei@canonical.com |
|---|---|
| State | New |
| Headers | show |
Il 18/06/2014 14:13, Ming Lei ha scritto: > In case of vhost-scsi, the object type of VirtIODevice isn't > VirtIOSCSI, so use the cast trick to fix the problem like > in virtio_scsi_handle_cmd() > > Cc: qemu-stable@nongnu.org > Cc: Anthony Liguori <aliguori@amazon.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > hw/scsi/virtio-scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index b0d7517..13700f5 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -538,7 +538,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, > > static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq) > { > - VirtIOSCSI *s = VIRTIO_SCSI(vdev); > + VirtIOSCSI *s = (VirtIOSCSI *)vdev; > > if (s->events_dropped) { > virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0); > This should never be triggered by vhost-scsi. Perhaps a bug in the kernel? Paolo
On Wed, Jun 18, 2014 at 8:29 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 18/06/2014 14:13, Ming Lei ha scritto: > >> In case of vhost-scsi, the object type of VirtIODevice isn't >> VirtIOSCSI, so use the cast trick to fix the problem like >> in virtio_scsi_handle_cmd() >> >> Cc: qemu-stable@nongnu.org >> Cc: Anthony Liguori <aliguori@amazon.com> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Ming Lei <ming.lei@canonical.com> >> --- >> hw/scsi/virtio-scsi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c >> index b0d7517..13700f5 100644 >> --- a/hw/scsi/virtio-scsi.c >> +++ b/hw/scsi/virtio-scsi.c >> @@ -538,7 +538,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, >> SCSIDevice *dev, >> >> static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq) >> { >> - VirtIOSCSI *s = VIRTIO_SCSI(vdev); >> + VirtIOSCSI *s = (VirtIOSCSI *)vdev; >> >> if (s->events_dropped) { >> virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0); >> > > This should never be triggered by vhost-scsi. Perhaps a bug in the kernel? It can be triggered with rmmod, system suspend, reboot... Thanks, -- Ming Lei
Am 18.06.2014 14:13, schrieb Ming Lei: > In case of vhost-scsi, the object type of VirtIODevice isn't > VirtIOSCSI, so use the cast trick to fix the problem like > in virtio_scsi_handle_cmd() > > Cc: qemu-stable@nongnu.org > Cc: Anthony Liguori <aliguori@amazon.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > hw/scsi/virtio-scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index b0d7517..13700f5 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -538,7 +538,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, > > static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq) > { > - VirtIOSCSI *s = VIRTIO_SCSI(vdev); > + VirtIOSCSI *s = (VirtIOSCSI *)vdev; > > if (s->events_dropped) { If s is not of type VirtIOSCSI as you indicate, then you shouldn't be accessing its fields here either. You're basically disabling the safety mechanism to avoid just that. If you see a direct cast used elsewhere, it is most likely for performance reasons, not for correctness. Regards, Andreas > virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0); >
Il 18/06/2014 16:18, Ming Lei ha scritto: >> > This should never be triggered by vhost-scsi. Perhaps a bug in the kernel? > It can be triggered with rmmod, system suspend, reboot... Yes, but it should not. What happens if you change VHOST_SCSI_VQ_NUM_FIXED from 2 to 3? Paolo
Il 18/06/2014 17:02, Paolo Bonzini ha scritto: > Il 18/06/2014 16:18, Ming Lei ha scritto: >>> > This should never be triggered by vhost-scsi. Perhaps a bug in the >>> kernel? >> It can be triggered with rmmod, system suspend, reboot... > > Yes, but it should not. > > What happens if you change VHOST_SCSI_VQ_NUM_FIXED from 2 to 3? Sorry, brain fart here. The number of interrupts in virtio-scsi is queues+3 because of the configuration interrupt. This does not apply here. Anyway, vhost-scsi passes the number of virtqueues correctly to the kernel. QEMU might be triggering this because, when vhost is stopped, the queue is still not empty (as expected, since it's a receive queue). Can you check that this is the case? If so, patching QEMU is correct, but you need to change VIRTIO_SCSI(vdev) to VIRTIO_SCSI_COMMON(vdev), and move events_dropped from VirtIOSCSI to VirtIOSCSICommon. Paolo
On Wed, Jun 18, 2014 at 11:06 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 18/06/2014 17:02, Paolo Bonzini ha scritto: > >> Il 18/06/2014 16:18, Ming Lei ha scritto: >>>> >>>> > This should never be triggered by vhost-scsi. Perhaps a bug in the >>>> kernel? >>> >>> It can be triggered with rmmod, system suspend, reboot... >> >> >> Yes, but it should not. >> >> What happens if you change VHOST_SCSI_VQ_NUM_FIXED from 2 to 3? > > > Sorry, brain fart here. The number of interrupts in virtio-scsi is queues+3 > because of the configuration interrupt. This does not apply here. > > Anyway, vhost-scsi passes the number of virtqueues correctly to the kernel. > > QEMU might be triggering this because, when vhost is stopped, the queue is > still not empty (as expected, since it's a receive queue). > > Can you check that this is the case? Yes, that is the case. > > If so, patching QEMU is correct, but you need to change VIRTIO_SCSI(vdev) to > VIRTIO_SCSI_COMMON(vdev), and move events_dropped from VirtIOSCSI to > VirtIOSCSICommon. This approach should be better. Thanks, -- Ming Lei
Hi Ming & Paolo, On Wed, 2014-06-18 at 23:11 +0800, Ming Lei wrote: > On Wed, Jun 18, 2014 at 11:06 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 18/06/2014 17:02, Paolo Bonzini ha scritto: > > > >> Il 18/06/2014 16:18, Ming Lei ha scritto: > >>>> > >>>> > This should never be triggered by vhost-scsi. Perhaps a bug in the > >>>> kernel? > >>> > >>> It can be triggered with rmmod, system suspend, reboot... > >> > >> > >> Yes, but it should not. > >> > >> What happens if you change VHOST_SCSI_VQ_NUM_FIXED from 2 to 3? > > > > > > Sorry, brain fart here. The number of interrupts in virtio-scsi is queues+3 > > because of the configuration interrupt. This does not apply here. > > > > Anyway, vhost-scsi passes the number of virtqueues correctly to the kernel. > > > > QEMU might be triggering this because, when vhost is stopped, the queue is > > still not empty (as expected, since it's a receive queue). > > > > Can you check that this is the case? > > Yes, that is the case. > > > > > If so, patching QEMU is correct, but you need to change VIRTIO_SCSI(vdev) to > > VIRTIO_SCSI_COMMON(vdev), and move events_dropped from VirtIOSCSI to > > VirtIOSCSICommon. > > This approach should be better. Just curious if this patch was picked up by upstream yet, and if it needs to be CC'd to stable for vhost-scsi code in >= v1.5.y ..? Thanks, --nab
On Sat, Jul 26, 2014 at 7:10 AM, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote: > Hi Ming & Paolo, > > On Wed, 2014-06-18 at 23:11 +0800, Ming Lei wrote: >> On Wed, Jun 18, 2014 at 11:06 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> > Il 18/06/2014 17:02, Paolo Bonzini ha scritto: >> > >> >> Il 18/06/2014 16:18, Ming Lei ha scritto: >> >>>> >> >>>> > This should never be triggered by vhost-scsi. Perhaps a bug in the >> >>>> kernel? >> >>> >> >>> It can be triggered with rmmod, system suspend, reboot... >> >> >> >> >> >> Yes, but it should not. >> >> >> >> What happens if you change VHOST_SCSI_VQ_NUM_FIXED from 2 to 3? >> > >> > >> > Sorry, brain fart here. The number of interrupts in virtio-scsi is queues+3 >> > because of the configuration interrupt. This does not apply here. >> > >> > Anyway, vhost-scsi passes the number of virtqueues correctly to the kernel. >> > >> > QEMU might be triggering this because, when vhost is stopped, the queue is >> > still not empty (as expected, since it's a receive queue). >> > >> > Can you check that this is the case? >> >> Yes, that is the case. >> >> > >> > If so, patching QEMU is correct, but you need to change VIRTIO_SCSI(vdev) to >> > VIRTIO_SCSI_COMMON(vdev), and move events_dropped from VirtIOSCSI to >> > VirtIOSCSICommon. >> >> This approach should be better. > > Just curious if this patch was picked up by upstream yet, and if it > needs to be CC'd to stable for vhost-scsi code in >= v1.5.y ..? Below is the merged version which has been marked as stable: http://git.qemu.org/?p=qemu.git;a=commit;h=91d670fbf9945ca4ecbd123affb36889e7fe8a5d Thanks,
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index b0d7517..13700f5 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -538,7 +538,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq) { - VirtIOSCSI *s = VIRTIO_SCSI(vdev); + VirtIOSCSI *s = (VirtIOSCSI *)vdev; if (s->events_dropped) { virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
In case of vhost-scsi, the object type of VirtIODevice isn't VirtIOSCSI, so use the cast trick to fix the problem like in virtio_scsi_handle_cmd() Cc: qemu-stable@nongnu.org Cc: Anthony Liguori <aliguori@amazon.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- hw/scsi/virtio-scsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)