diff mbox

[V2,WIP,0/2] vhost-scsi: new device supporting the tcm_vhost Linux kernel module

Message ID 20130314094643.GD14977@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin March 14, 2013, 9:46 a.m. UTC 
On Thu, Mar 14, 2013 at 05:25:42PM +0800, Asias He wrote:
> On Thu, Mar 14, 2013 at 12:25:14PM +0800, Asias He wrote:
> > On Tue, Mar 12, 2013 at 02:29:40PM +0800, Asias He wrote:
> > > This is on top of Paolo and Nick's work.
> > > 
> > > Current status:
> > > Works now (guest boots fine, no hang any more) with seabios's virtio-scsi disabled.
> > > Rebased to latest qemu.org/master
> > > Change details are in commit log.
> > > 
> > > TODO:
> > > Make seabios happy.
> > 
> > Some updates:
> > 
> > In seabios, src/virtio-scsi.c:virtio_scsi_cmd()
> > 
> >     vring_kick(ioaddr, vq, 1);
> >     /* Wait for reply */
> >     while (!vring_more_used(vq))  ------------>>> we hang here
> >         usleep(5);
> > 
> > In tcm_vhost:
> > we got: vhost_get_vq_desc: head: -14, out: 0 in: 81216
> > 
> > The vring buffer provided by seabios is not correct?
> 
> More updates:
> 
> Basically, seabios + tcm_vhost works now, with the attached qemu and
> seabios patches.
> 
> We still have one more issue, vhost_verify_ring_mappings fails.
> 
> I am seeing this without the comment out hack.
> 
>    Unable to map ring buffer for ring 2
>    qemu-system-x86_64: /root/src/qemu/hw/vhost.c:426: vhost_set_memory:
>    Assertion `r >= 0' failed.
> 

Hmm it looks like a bug in cpu_physical_memory_map.
I think it can make length larger than what was passed in.
Could you try the following and see what happens?



> > > Paolo Bonzini (2):
> > >   virtio-scsi: create VirtIOSCSICommon
> > >   vhost-scsi: new device supporting the tcm_vhost Linux kernel module
> > > 
> > >  hw/Makefile.objs           |   5 +-
> > >  hw/s390x/s390-virtio-bus.c |  35 +++++++
> > >  hw/vhost-scsi.c            | 242 +++++++++++++++++++++++++++++++++++++++++++++
> > >  hw/vhost-scsi.h            |  64 ++++++++++++
> > >  hw/virtio-pci.c            |  59 +++++++++++
> > >  hw/virtio-scsi.c           | 199 +++++++++----------------------------
> > >  hw/virtio-scsi.h           | 129 ++++++++++++++++++++++++
> > >  include/qemu/osdep.h       |   4 +
> > >  8 files changed, 586 insertions(+), 151 deletions(-)
> > >  create mode 100644 hw/vhost-scsi.c
> > >  create mode 100644 hw/vhost-scsi.h
> > > 
> > > -- 
> > > 1.8.1.4
> > > 
> > 
> > -- 
> > Asias
> 
> --------------- 8 ---------------> qemu patch:
> diff --git a/hw/vhost.c b/hw/vhost.c
> index 4d6aee3..0c52ec4 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
>          return;
>      }
>  
> +#if 0
>      if (dev->started) {
>          r = vhost_verify_ring_mappings(dev, start_addr, size);
>          assert(r >= 0);
>      }
> +#endif
>  
>      if (!dev->log_enabled) {
>          r = ioctl(dev->control, VHOST_SET_MEM_TABLE, dev->mem);
> 
> --------------- 8 ---------------> seabios patch:
> diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
> index 879ddfb..4de1255 100644
> --- a/src/virtio-scsi.c
> +++ b/src/virtio-scsi.c
> @@ -147,6 +147,9 @@ init_virtio_scsi(struct pci_device *pci)
>          goto fail;
>      }
>  
> +    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> +                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
> +
>      int i, tot;
>      for (tot = 0, i = 0; i < 256; i++)
>          tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
> @@ -154,8 +157,6 @@ init_virtio_scsi(struct pci_device *pci)
>      if (!tot)
>          goto fail;
>  
> -    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> -                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
>      return;
>  
>  fail:
> diff --git a/src/virtio-scsi.h b/src/virtio-scsi.h
> index bbfbf30..96c3701 100644
> --- a/src/virtio-scsi.h
> +++ b/src/virtio-scsi.h
> @@ -26,7 +26,7 @@ struct virtio_scsi_req_cmd {
>      u8 prio;
>      u8 crn;
>      char cdb[VIRTIO_SCSI_CDB_SIZE];
> -};
> +} __attribute__((packed));
>  
>  /* This is the first element of the "in" scatter-gather list. */
>  struct virtio_scsi_resp_cmd {
> @@ -36,7 +36,7 @@ struct virtio_scsi_resp_cmd {
>      u8 status;
>      u8 response;
>      u8 sense[VIRTIO_SCSI_SENSE_SIZE];
> -};
> +} __attribute__((packed));
>  
>  #define VIRTIO_SCSI_S_OK            0

I see, the padding creates the problem?

> -- 
> Asias

Comments

Paolo Bonzini March 14, 2013, 10 a.m. UTC | #1 
>> --------------- 8 ---------------> seabios patch:
>> diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
>> index 879ddfb..4de1255 100644
>> --- a/src/virtio-scsi.c
>> +++ b/src/virtio-scsi.c
>> @@ -147,6 +147,9 @@ init_virtio_scsi(struct pci_device *pci)
>>          goto fail;
>>      }
>>  
>> +    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>> +                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
>> +
>>      int i, tot;
>>      for (tot = 0, i = 0; i < 256; i++)
>>          tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
>> @@ -154,8 +157,6 @@ init_virtio_scsi(struct pci_device *pci)
>>      if (!tot)
>>          goto fail;
>>  
>> -    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>> -                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
>>      return;
>>  
>>  fail:

ACK.

>> diff --git a/src/virtio-scsi.h b/src/virtio-scsi.h
>> index bbfbf30..96c3701 100644
>> --- a/src/virtio-scsi.h
>> +++ b/src/virtio-scsi.h
>> @@ -26,7 +26,7 @@ struct virtio_scsi_req_cmd {
>>      u8 prio;
>>      u8 crn;
>>      char cdb[VIRTIO_SCSI_CDB_SIZE];
>> -};
>> +} __attribute__((packed));
>>  
>>  /* This is the first element of the "in" scatter-gather list. */
>>  struct virtio_scsi_resp_cmd {
>> @@ -36,7 +36,7 @@ struct virtio_scsi_resp_cmd {
>>      u8 status;
>>      u8 response;
>>      u8 sense[VIRTIO_SCSI_SENSE_SIZE];
>> -};
>> +} __attribute__((packed));
>>  
>>  #define VIRTIO_SCSI_S_OK            0
> 
> I see, the padding creates the problem?

Looks like it does for req_cmd (which has length 51 and is padded to
56).  QEMU incorrectly relies on the framing.  Both of these are SeaBIOS
bugs, please do submit the patch.

Paolo
Michael S. Tsirkin March 14, 2013, 10:21 a.m. UTC | #2 
On Thu, Mar 14, 2013 at 11:00:24AM +0100, Paolo Bonzini wrote:
> 
> >> --------------- 8 ---------------> seabios patch:
> >> diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
> >> index 879ddfb..4de1255 100644
> >> --- a/src/virtio-scsi.c
> >> +++ b/src/virtio-scsi.c
> >> @@ -147,6 +147,9 @@ init_virtio_scsi(struct pci_device *pci)
> >>          goto fail;
> >>      }
> >>  
> >> +    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >> +                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
> >> +
> >>      int i, tot;
> >>      for (tot = 0, i = 0; i < 256; i++)
> >>          tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
> >> @@ -154,8 +157,6 @@ init_virtio_scsi(struct pci_device *pci)
> >>      if (!tot)
> >>          goto fail;
> >>  
> >> -    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >> -                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
> >>      return;
> >>  
> >>  fail:
> 
> ACK.
> 
> >> diff --git a/src/virtio-scsi.h b/src/virtio-scsi.h
> >> index bbfbf30..96c3701 100644
> >> --- a/src/virtio-scsi.h
> >> +++ b/src/virtio-scsi.h
> >> @@ -26,7 +26,7 @@ struct virtio_scsi_req_cmd {
> >>      u8 prio;
> >>      u8 crn;
> >>      char cdb[VIRTIO_SCSI_CDB_SIZE];
> >> -};
> >> +} __attribute__((packed));
> >>  
> >>  /* This is the first element of the "in" scatter-gather list. */
> >>  struct virtio_scsi_resp_cmd {
> >> @@ -36,7 +36,7 @@ struct virtio_scsi_resp_cmd {
> >>      u8 status;
> >>      u8 response;
> >>      u8 sense[VIRTIO_SCSI_SENSE_SIZE];
> >> -};
> >> +} __attribute__((packed));
> >>  
> >>  #define VIRTIO_SCSI_S_OK            0
> > 
> > I see, the padding creates the problem?
> 
> Looks like it does for req_cmd (which has length 51 and is padded to
> 56).  QEMU incorrectly relies on the framing.

That's bad though. Let's fix this in qemu.

> Both of these are SeaBIOS bugs, please do submit the patch.
> 
> Paolo

Right but in parallel we can use the old BIOS to
make sure qemu does not rely on framing.
Asias He March 15, 2013, 12:55 a.m. UTC | #3 
On Thu, Mar 14, 2013 at 11:46:43AM +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 14, 2013 at 05:25:42PM +0800, Asias He wrote:
> > On Thu, Mar 14, 2013 at 12:25:14PM +0800, Asias He wrote:
> > > On Tue, Mar 12, 2013 at 02:29:40PM +0800, Asias He wrote:
> > > > This is on top of Paolo and Nick's work.
> > > > 
> > > > Current status:
> > > > Works now (guest boots fine, no hang any more) with seabios's virtio-scsi disabled.
> > > > Rebased to latest qemu.org/master
> > > > Change details are in commit log.
> > > > 
> > > > TODO:
> > > > Make seabios happy.
> > > 
> > > Some updates:
> > > 
> > > In seabios, src/virtio-scsi.c:virtio_scsi_cmd()
> > > 
> > >     vring_kick(ioaddr, vq, 1);
> > >     /* Wait for reply */
> > >     while (!vring_more_used(vq))  ------------>>> we hang here
> > >         usleep(5);
> > > 
> > > In tcm_vhost:
> > > we got: vhost_get_vq_desc: head: -14, out: 0 in: 81216
> > > 
> > > The vring buffer provided by seabios is not correct?
> > 
> > More updates:
> > 
> > Basically, seabios + tcm_vhost works now, with the attached qemu and
> > seabios patches.
> > 
> > We still have one more issue, vhost_verify_ring_mappings fails.
> > 
> > I am seeing this without the comment out hack.
> > 
> >    Unable to map ring buffer for ring 2
> >    qemu-system-x86_64: /root/src/qemu/hw/vhost.c:426: vhost_set_memory:
> >    Assertion `r >= 0' failed.
> > 
> 
> Hmm it looks like a bug in cpu_physical_memory_map.
> I think it can make length larger than what was passed in.
> Could you try the following and see what happens?

I got:

   Unable to map ring buffer for ring 2 Debug data: 0x7f40d4005000 1000 1404
   
   qemu-system-x86_64: /root/src/qemu/hw/vhost.c:431: vhost_set_memory:
   Assertion `r >= 0' failed.

> diff --git a/hw/vhost.c b/hw/vhost.c
> index 37777c2..a30e551 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -320,7 +320,9 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
>          l = vq->ring_size;
>          p = cpu_physical_memory_map(vq->ring_phys, &l, 1);
>          if (!p || l != vq->ring_size) {
> -            fprintf(stderr, "Unable to map ring buffer for ring %d\n", i);
> +            fprintf(stderr, "Unable to map ring buffer for ring %d "
> +                    "Debug data: %p %llx %llx\n", i, p,
> +                    (unsigned long long)l, (unsigned long long)vq->ring_size);
>              return -ENOMEM;
>          }
>          if (p != vq->ring) {
> 
> 
> > > > Paolo Bonzini (2):
> > > >   virtio-scsi: create VirtIOSCSICommon
> > > >   vhost-scsi: new device supporting the tcm_vhost Linux kernel module
> > > > 
> > > >  hw/Makefile.objs           |   5 +-
> > > >  hw/s390x/s390-virtio-bus.c |  35 +++++++
> > > >  hw/vhost-scsi.c            | 242 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  hw/vhost-scsi.h            |  64 ++++++++++++
> > > >  hw/virtio-pci.c            |  59 +++++++++++
> > > >  hw/virtio-scsi.c           | 199 +++++++++----------------------------
> > > >  hw/virtio-scsi.h           | 129 ++++++++++++++++++++++++
> > > >  include/qemu/osdep.h       |   4 +
> > > >  8 files changed, 586 insertions(+), 151 deletions(-)
> > > >  create mode 100644 hw/vhost-scsi.c
> > > >  create mode 100644 hw/vhost-scsi.h
> > > > 
> > > > -- 
> > > > 1.8.1.4
> > > > 
> > > 
> > > -- 
> > > Asias
> > 
> > --------------- 8 ---------------> qemu patch:
> > diff --git a/hw/vhost.c b/hw/vhost.c
> > index 4d6aee3..0c52ec4 100644
> > --- a/hw/vhost.c
> > +++ b/hw/vhost.c
> > @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
> >          return;
> >      }
> >  
> > +#if 0
> >      if (dev->started) {
> >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> >          assert(r >= 0);
> >      }
> > +#endif
> >  
> >      if (!dev->log_enabled) {
> >          r = ioctl(dev->control, VHOST_SET_MEM_TABLE, dev->mem);
> > 
> > --------------- 8 ---------------> seabios patch:
> > diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
> > index 879ddfb..4de1255 100644
> > --- a/src/virtio-scsi.c
> > +++ b/src/virtio-scsi.c
> > @@ -147,6 +147,9 @@ init_virtio_scsi(struct pci_device *pci)
> >          goto fail;
> >      }
> >  
> > +    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > +                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
> > +
> >      int i, tot;
> >      for (tot = 0, i = 0; i < 256; i++)
> >          tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
> > @@ -154,8 +157,6 @@ init_virtio_scsi(struct pci_device *pci)
> >      if (!tot)
> >          goto fail;
> >  
> > -    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > -                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
> >      return;
> >  
> >  fail:
> > diff --git a/src/virtio-scsi.h b/src/virtio-scsi.h
> > index bbfbf30..96c3701 100644
> > --- a/src/virtio-scsi.h
> > +++ b/src/virtio-scsi.h
> > @@ -26,7 +26,7 @@ struct virtio_scsi_req_cmd {
> >      u8 prio;
> >      u8 crn;
> >      char cdb[VIRTIO_SCSI_CDB_SIZE];
> > -};
> > +} __attribute__((packed));
> >  
> >  /* This is the first element of the "in" scatter-gather list. */
> >  struct virtio_scsi_resp_cmd {
> > @@ -36,7 +36,7 @@ struct virtio_scsi_resp_cmd {
> >      u8 status;
> >      u8 response;
> >      u8 sense[VIRTIO_SCSI_SENSE_SIZE];
> > -};
> > +} __attribute__((packed));
> >  
> >  #define VIRTIO_SCSI_S_OK            0
> 
> I see, the padding creates the problem?
> 
> > -- 
> > Asias
Asias He March 15, 2013, 12:58 a.m. UTC | #4 
On Thu, Mar 14, 2013 at 11:00:24AM +0100, Paolo Bonzini wrote:
> 
> >> --------------- 8 ---------------> seabios patch:
> >> diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
> >> index 879ddfb..4de1255 100644
> >> --- a/src/virtio-scsi.c
> >> +++ b/src/virtio-scsi.c
> >> @@ -147,6 +147,9 @@ init_virtio_scsi(struct pci_device *pci)
> >>          goto fail;
> >>      }
> >>  
> >> +    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >> +                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
> >> +
> >>      int i, tot;
> >>      for (tot = 0, i = 0; i < 256; i++)
> >>          tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
> >> @@ -154,8 +157,6 @@ init_virtio_scsi(struct pci_device *pci)
> >>      if (!tot)
> >>          goto fail;
> >>  
> >> -    vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >> -                  VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
> >>      return;
> >>  
> >>  fail:
> 
> ACK.

I will include your ack.

> >> diff --git a/src/virtio-scsi.h b/src/virtio-scsi.h
> >> index bbfbf30..96c3701 100644
> >> --- a/src/virtio-scsi.h
> >> +++ b/src/virtio-scsi.h
> >> @@ -26,7 +26,7 @@ struct virtio_scsi_req_cmd {
> >>      u8 prio;
> >>      u8 crn;
> >>      char cdb[VIRTIO_SCSI_CDB_SIZE];
> >> -};
> >> +} __attribute__((packed));
> >>  
> >>  /* This is the first element of the "in" scatter-gather list. */
> >>  struct virtio_scsi_resp_cmd {
> >> @@ -36,7 +36,7 @@ struct virtio_scsi_resp_cmd {
> >>      u8 status;
> >>      u8 response;
> >>      u8 sense[VIRTIO_SCSI_SENSE_SIZE];
> >> -};
> >> +} __attribute__((packed));
> >>  
> >>  #define VIRTIO_SCSI_S_OK            0
> > 
> > I see, the padding creates the problem?
> 
> Looks like it does for req_cmd (which has length 51 and is padded to
> 56).  QEMU incorrectly relies on the framing.  Both of these are SeaBIOS
> bugs, please do submit the patch.

Will submit shortly.
diff mbox

Patch

diff --git a/hw/vhost.c b/hw/vhost.c
index 37777c2..a30e551 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -320,7 +320,9 @@  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         l = vq->ring_size;
         p = cpu_physical_memory_map(vq->ring_phys, &l, 1);
         if (!p || l != vq->ring_size) {
-            fprintf(stderr, "Unable to map ring buffer for ring %d\n", i);
+            fprintf(stderr, "Unable to map ring buffer for ring %d "
+                    "Debug data: %p %llx %llx\n", i, p,
+                    (unsigned long long)l, (unsigned long long)vq->ring_size);
             return -ENOMEM;
         }
         if (p != vq->ring) {