[Qemu-devel] [PATCH for 2.11 0/1] Fix guest boot with vhost-scsi

[Qemu-devel] [PATCH for 2.11 0/1] Fix guest boot with vhost-scsi

[Eric Farman]

(Apologies for not noticing this earlier in the development cycle;
I have been on leave and am just getting back.)

A colleague of mine noticed an inability to start a guest with the
2.11 RC's, when a vhost-scsi device is defined in the guest.  The
symptoms on s390 are an exception loop in the guest, which is ugly:

[    1.006087] Kernel stack overflow.
[    1.006145] CPU: 1 PID: 114 Comm: systemd-udevd Not tainted 4.4.0-83-generic #106-Ubuntu
[    1.006237] task: 000000000165d7c0 ti: 0000000001474000 task.ti: 0000000001474000
[    1.006331] Krnl PSW : 0704d00180000000 00000000001f8bd0 (search_module_extables+0x68/0xc0)
[    1.006453]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 EA:3
Krnl GPRS: a42be0ffd90b3c69 0000000000000000 0000000000bd8ce0 0000000000000000
[    1.006627]            00000000001f8bd0 00000000001f0ff8 0000000000000000 0000000000000000
[    1.006693]            0704d00180000000 00000000001f8bd0 00000000000000e0 fffffffffffffff8
[    1.006786]            00000000001f8bd0 00000000007d8b38 00000000014740a8 0000000001474078
[    1.006886] Krnl Code: 00000000001f8bbe: c020004f0091        larl    %r2,bd8ce0
           00000000001f8bc4: ecb1fff800d9       aghik   %r11,%r1,-8
          #00000000001f8bca: ec1200268064       cgrj    %r1,%r2,8,1f8c16
          >00000000001f8bd0: d503d000b000       clc     0(4,%r13),0(%r11)
           00000000001f8bd6: a784fff1           brc     8,1f8bb8
           00000000001f8bda: e330b1740012       lt      %r3,372(%r11)
           00000000001f8be0: a784ffec           brc     8,1f8bb8
           00000000001f8be4: ec331dbc0355       risbg   %r3,%r3,29,188,3
[    1.007385] Call Trace:
[    1.007416] ([<00000000014740c8>] 0x14740c8)
[    1.007477]  [<000000000012205c>] do_no_context+0x34/0xf8
[    1.007528]  [<00000000007c2328>] pgm_check_handler+0x15c/0x1a4
[    1.007590]  [<00000000001f8bd0>] search_module_extables+0x68/0xc0
[    1.007661] ([<0000000001474270>] 0x1474270)
[    1.007725]  [<000000000012205c>] do_no_context+0x34/0xf8
[    1.007789]  [<00000000007c2328>] pgm_check_handler+0x15c/0x1a4
[    1.007865]  [<00000000001f8bd0>] search_module_extables+0x68/0xc0
[    1.007929] ([<0000000001474418>] 0x1474418)
...repeating...

Since this worked perfectly fine with 2.10, I was able to bisect the
problem to commit 5c0919d02066 ("virtio-scsi: Add virtqueue_size parameter
allowing virtqueue size to be set.").  When I attempted to revert this
commit from 2.11-rc3, I get a build error because commit 920036106044
("vhost-user-scsi: add missing virtqueue_size param") makes no sense.

This made the problem rather straightforward, because the change that
was put in for vhost-user-scsi needed to also be added for vhost-scsi.

I haven't looked at the vhost-user-scsi stuff to know how it works and/or
whether we could combine matters into VhostSCSICommon (or whatever).  If
there were more time before 2.11 I would, but thought this would be a
better solution in the short term.

Eric Farman (1):
  vhost-scsi: add missing virtqueue_size parameter

 hw/scsi/vhost-scsi.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.13.5

[Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Eric Farman]

Commit 5c0919d02066 ("virtio-scsi: Add virtqueue_size parameter allowing
virtqueue size to be set.") introduced a new parameter to virtio-scsi.
Later, commit 920036106044 ("vhost-user-scsi: add missing virtqueue_size
param") added that parameter to the new vhost-user-scsi interface but
neglected the existing vhost-scsi interface it was built on.

Apply the same change to vhost-scsi, so that we can boot a guest with
a device defined.  This also avoids crashing a guest when hotplugging
a vhost-scsi device.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 hw/scsi/vhost-scsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index cd4ab05233..9c1bea8ff3 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
     DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
     DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
     DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
+    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
+                       128),
     DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
                        0xFFFF),
     DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Paolo Bonzini]

On 01/12/2017 16:15, Eric Farman wrote:
> Commit 5c0919d02066 ("virtio-scsi: Add virtqueue_size parameter allowing
> virtqueue size to be set.") introduced a new parameter to virtio-scsi.
> Later, commit 920036106044 ("vhost-user-scsi: add missing virtqueue_size
> param") added that parameter to the new vhost-user-scsi interface but
> neglected the existing vhost-scsi interface it was built on.
> 
> Apply the same change to vhost-scsi, so that we can boot a guest with
> a device defined.  This also avoids crashing a guest when hotplugging
> a vhost-scsi device.
> 
> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> ---
>  hw/scsi/vhost-scsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index cd4ab05233..9c1bea8ff3 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
>      DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
>      DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
>      DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
> +    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
> +                       128),
>      DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
>                         0xFFFF),
>      DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Richard W.M. Jones]

On Fri, Dec 01, 2017 at 04:15:38PM +0100, Eric Farman wrote:
> Commit 5c0919d02066 ("virtio-scsi: Add virtqueue_size parameter allowing
> virtqueue size to be set.") introduced a new parameter to virtio-scsi.
> Later, commit 920036106044 ("vhost-user-scsi: add missing virtqueue_size
> param") added that parameter to the new vhost-user-scsi interface but
> neglected the existing vhost-scsi interface it was built on.
> 
> Apply the same change to vhost-scsi, so that we can boot a guest with
> a device defined.  This also avoids crashing a guest when hotplugging
> a vhost-scsi device.
> 
> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> ---
>  hw/scsi/vhost-scsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index cd4ab05233..9c1bea8ff3 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
>      DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
>      DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
>      DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
> +    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
> +                       128),
>      DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
>                         0xFFFF),
>      DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),

Yes, this fix is fine.  Sorry for breaking it :-/

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Eric Farman]

On 12/01/2017 10:16 AM, Paolo Bonzini wrote:
> On 01/12/2017 16:15, Eric Farman wrote:
>> Commit 5c0919d02066 ("virtio-scsi: Add virtqueue_size parameter allowing
>> virtqueue size to be set.") introduced a new parameter to virtio-scsi.
>> Later, commit 920036106044 ("vhost-user-scsi: add missing virtqueue_size
>> param") added that parameter to the new vhost-user-scsi interface but
>> neglected the existing vhost-scsi interface it was built on.
>>
>> Apply the same change to vhost-scsi, so that we can boot a guest with
>> a device defined.  This also avoids crashing a guest when hotplugging
>> a vhost-scsi device.
>>
>> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
>> ---
>>   hw/scsi/vhost-scsi.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index cd4ab05233..9c1bea8ff3 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
>>       DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
>>       DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0),
>>       DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
>> +    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
>> +                       128),
>>       DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
>>                          0xFFFF),
>>       DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),
>>
> 
> Queued, thanks.
> 
> Paolo
> 

Wow, super quick, thank you!  Seeing conversations today, is this queued 
for -rc4, or 2.12 and backport to qemu-stable?

  - Eric

Re: [Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Peter Maydell]

On 4 December 2017 at 17:52, Eric Farman <farman@linux.vnet.ibm.com> wrote:
> On 12/01/2017 10:16 AM, Paolo Bonzini wrote:
>>
>> On 01/12/2017 16:15, Eric Farman wrote:
>>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>> index cd4ab05233..9c1bea8ff3 100644
>>> --- a/hw/scsi/vhost-scsi.c
>>> +++ b/hw/scsi/vhost-scsi.c
>>> @@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
>>>       DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
>>>       DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt,
>>> 0),
>>>       DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
>>> 1),
>>> +    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon,
>>> conf.virtqueue_size,
>>> +                       128),
>>>       DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon,
>>> conf.max_sectors,
>>>                          0xFFFF),
>>>       DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon,
>>> conf.cmd_per_lun, 128),

> Wow, super quick, thank you!  Seeing conversations today, is this queued for
> -rc4, or 2.12 and backport to qemu-stable?

If people can come to a consensus on whether this is -rc4 material
by Tuesday 12:00 GMT that would be helpful.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Eric Farman]

On 12/04/2017 01:05 PM, Peter Maydell wrote:
> On 4 December 2017 at 17:52, Eric Farman <farman@linux.vnet.ibm.com> wrote:
>> On 12/01/2017 10:16 AM, Paolo Bonzini wrote:
>>>
>>> On 01/12/2017 16:15, Eric Farman wrote:
>>>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>>> index cd4ab05233..9c1bea8ff3 100644
>>>> --- a/hw/scsi/vhost-scsi.c
>>>> +++ b/hw/scsi/vhost-scsi.c
>>>> @@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
>>>>        DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
>>>>        DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt,
>>>> 0),
>>>>        DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
>>>> 1),
>>>> +    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon,
>>>> conf.virtqueue_size,
>>>> +                       128),
>>>>        DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon,
>>>> conf.max_sectors,
>>>>                           0xFFFF),
>>>>        DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon,
>>>> conf.cmd_per_lun, 128),
> 
>> Wow, super quick, thank you!  Seeing conversations today, is this queued for
>> -rc4, or 2.12 and backport to qemu-stable?
> 
> If people can come to a consensus on whether this is -rc4 material
> by Tuesday 12:00 GMT that would be helpful.

I'd vote for -rc4, since it's a new guest crash since 2.10.  But I also 
won't complain if it waits for 2.11.1 since I was out of the office 
during the whole freeze period and no one else noticed it.

  - Eric

Re: [Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Michael S. Tsirkin]

On Mon, Dec 04, 2017 at 03:00:45PM -0500, Eric Farman wrote:
> 
> 
> On 12/04/2017 01:05 PM, Peter Maydell wrote:
> > On 4 December 2017 at 17:52, Eric Farman <farman@linux.vnet.ibm.com> wrote:
> > > On 12/01/2017 10:16 AM, Paolo Bonzini wrote:
> > > > 
> > > > On 01/12/2017 16:15, Eric Farman wrote:
> > > > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> > > > > index cd4ab05233..9c1bea8ff3 100644
> > > > > --- a/hw/scsi/vhost-scsi.c
> > > > > +++ b/hw/scsi/vhost-scsi.c
> > > > > @@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
> > > > >        DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
> > > > >        DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt,
> > > > > 0),
> > > > >        DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
> > > > > 1),
> > > > > +    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon,
> > > > > conf.virtqueue_size,
> > > > > +                       128),
> > > > >        DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon,
> > > > > conf.max_sectors,
> > > > >                           0xFFFF),
> > > > >        DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon,
> > > > > conf.cmd_per_lun, 128),
> > 
> > > Wow, super quick, thank you!  Seeing conversations today, is this queued for
> > > -rc4, or 2.12 and backport to qemu-stable?
> > 
> > If people can come to a consensus on whether this is -rc4 material
> > by Tuesday 12:00 GMT that would be helpful.
> 
> I'd vote for -rc4, since it's a new guest crash since 2.10.

I think so too.

>  But I also
> won't complain if it waits for 2.11.1 since I was out of the office during
> the whole freeze period and no one else noticed it.
> 
>  - Eric

Re: [Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Peter Maydell]

On 5 December 2017 at 00:59, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Dec 04, 2017 at 03:00:45PM -0500, Eric Farman wrote:
>>
>>
>> On 12/04/2017 01:05 PM, Peter Maydell wrote:
>> > On 4 December 2017 at 17:52, Eric Farman <farman@linux.vnet.ibm.com> wrote:
>> > > On 12/01/2017 10:16 AM, Paolo Bonzini wrote:
>> > > >
>> > > > On 01/12/2017 16:15, Eric Farman wrote:
>> > > > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> > > > > index cd4ab05233..9c1bea8ff3 100644
>> > > > > --- a/hw/scsi/vhost-scsi.c
>> > > > > +++ b/hw/scsi/vhost-scsi.c
>> > > > > @@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
>> > > > >        DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
>> > > > >        DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt,
>> > > > > 0),
>> > > > >        DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
>> > > > > 1),
>> > > > > +    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon,
>> > > > > conf.virtqueue_size,
>> > > > > +                       128),
>> > > > >        DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon,
>> > > > > conf.max_sectors,
>> > > > >                           0xFFFF),
>> > > > >        DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon,
>> > > > > conf.cmd_per_lun, 128),
>> >
>> > > Wow, super quick, thank you!  Seeing conversations today, is this queued for
>> > > -rc4, or 2.12 and backport to qemu-stable?
>> >
>> > If people can come to a consensus on whether this is -rc4 material
>> > by Tuesday 12:00 GMT that would be helpful.
>>
>> I'd vote for -rc4, since it's a new guest crash since 2.10.
>
> I think so too.

Thanks; spoke also with Paolo on irc, and have pushed this to master
for -rc4.

-- PMM

Re: [Qemu-devel] [PATCH 1/1] vhost-scsi: add missing virtqueue_size parameter

[Eric Farman]

On 12/05/2017 07:40 AM, Peter Maydell wrote:
> On 5 December 2017 at 00:59, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Mon, Dec 04, 2017 at 03:00:45PM -0500, Eric Farman wrote:
>>>
>>>
>>> On 12/04/2017 01:05 PM, Peter Maydell wrote:
>>>> On 4 December 2017 at 17:52, Eric Farman <farman@linux.vnet.ibm.com> wrote:
>>>>> On 12/01/2017 10:16 AM, Paolo Bonzini wrote:
>>>>>>
>>>>>> On 01/12/2017 16:15, Eric Farman wrote:
>>>>>>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>>>>>> index cd4ab05233..9c1bea8ff3 100644
>>>>>>> --- a/hw/scsi/vhost-scsi.c
>>>>>>> +++ b/hw/scsi/vhost-scsi.c
>>>>>>> @@ -233,6 +233,8 @@ static Property vhost_scsi_properties[] = {
>>>>>>>         DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn),
>>>>>>>         DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt,
>>>>>>> 0),
>>>>>>>         DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
>>>>>>> 1),
>>>>>>> +    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon,
>>>>>>> conf.virtqueue_size,
>>>>>>> +                       128),
>>>>>>>         DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon,
>>>>>>> conf.max_sectors,
>>>>>>>                            0xFFFF),
>>>>>>>         DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon,
>>>>>>> conf.cmd_per_lun, 128),
>>>>
>>>>> Wow, super quick, thank you!  Seeing conversations today, is this queued for
>>>>> -rc4, or 2.12 and backport to qemu-stable?
>>>>
>>>> If people can come to a consensus on whether this is -rc4 material
>>>> by Tuesday 12:00 GMT that would be helpful.
>>>
>>> I'd vote for -rc4, since it's a new guest crash since 2.10.
>>
>> I think so too.
> 
> Thanks; spoke also with Paolo on irc, and have pushed this to master
> for -rc4.

Cool!  I just pulled master/rc4 and tested this scenario, and it works 
great.  Thanks everyone!

  - Eric