diff mbox

[RFC-v2,1/6] msix: Work-around for vhost-scsi with KVM in-kernel MSI injection

Message ID 1344846917-7411-2-git-send-email-nab@linux-iscsi.org
State New
Headers show

Commit Message

Nicholas A. Bellinger Aug. 13, 2012, 8:35 a.m. UTC 
From: Nicholas Bellinger <nab@linux-iscsi.org>

This is required to get past the following assert with:

commit 1523ed9e1d46b0b54540049d491475ccac7e6421
Author: Jan Kiszka <jan.kiszka@siemens.com>
Date:   Thu May 17 10:32:39 2012 -0300

    virtio/vhost: Add support for KVM in-kernel MSI injection

Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 hw/msix.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Comments

Michael S. Tsirkin Aug. 13, 2012, 8:51 a.m. UTC | #1 
On Mon, Aug 13, 2012 at 08:35:12AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This is required to get past the following assert with:
> 
> commit 1523ed9e1d46b0b54540049d491475ccac7e6421
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date:   Thu May 17 10:32:39 2012 -0300
> 
>     virtio/vhost: Add support for KVM in-kernel MSI injection
> 
> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

Could you please add a bit more explanation why
this happens with virtio scsi and why this is valid?

> ---
>  hw/msix.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 800fc32..c1e6dc3 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -544,6 +544,9 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
>  {
>      int vector;
>  
> +    if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
> +        return;
> +
>      assert(dev->msix_vector_use_notifier &&
>             dev->msix_vector_release_notifier);
>  
> -- 
> 1.7.2.5
Jan Kiszka Aug. 13, 2012, 12:06 p.m. UTC | #2 
On 2012-08-13 10:35, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This is required to get past the following assert with:
> 
> commit 1523ed9e1d46b0b54540049d491475ccac7e6421
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date:   Thu May 17 10:32:39 2012 -0300
> 
>     virtio/vhost: Add support for KVM in-kernel MSI injection
> 
> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  hw/msix.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 800fc32..c1e6dc3 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -544,6 +544,9 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
>  {
>      int vector;
>  
> +    if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
> +        return;
> +
>      assert(dev->msix_vector_use_notifier &&
>             dev->msix_vector_release_notifier);
>  
> 

I think to remember pointing out that there is a bug somewhere in the
reset code which deactivates a non-active vhost instance, no?

Jan
Michael S. Tsirkin Aug. 13, 2012, 6:03 p.m. UTC | #3 
On Mon, Aug 13, 2012 at 02:06:10PM +0200, Jan Kiszka wrote:
> On 2012-08-13 10:35, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This is required to get past the following assert with:
> > 
> > commit 1523ed9e1d46b0b54540049d491475ccac7e6421
> > Author: Jan Kiszka <jan.kiszka@siemens.com>
> > Date:   Thu May 17 10:32:39 2012 -0300
> > 
> >     virtio/vhost: Add support for KVM in-kernel MSI injection
> > 
> > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >  hw/msix.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 800fc32..c1e6dc3 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -544,6 +544,9 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
> >  {
> >      int vector;
> >  
> > +    if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
> > +        return;
> > +
> >      assert(dev->msix_vector_use_notifier &&
> >             dev->msix_vector_release_notifier);
> >  
> > 
> 
> I think to remember pointing out that there is a bug somewhere in the
> reset code which deactivates a non-active vhost instance, no?
> 
> Jan

Could not find it. Could you dig it up pls?

> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
Jan Kiszka Aug. 13, 2012, 6:06 p.m. UTC | #4 
On 2012-08-13 20:03, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2012 at 02:06:10PM +0200, Jan Kiszka wrote:
>> On 2012-08-13 10:35, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>>
>>> This is required to get past the following assert with:
>>>
>>> commit 1523ed9e1d46b0b54540049d491475ccac7e6421
>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>> Date:   Thu May 17 10:32:39 2012 -0300
>>>
>>>     virtio/vhost: Add support for KVM in-kernel MSI injection
>>>
>>> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>>> ---
>>>  hw/msix.c |    3 +++
>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/msix.c b/hw/msix.c
>>> index 800fc32..c1e6dc3 100644
>>> --- a/hw/msix.c
>>> +++ b/hw/msix.c
>>> @@ -544,6 +544,9 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
>>>  {
>>>      int vector;
>>>  
>>> +    if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
>>> +        return;
>>> +
>>>      assert(dev->msix_vector_use_notifier &&
>>>             dev->msix_vector_release_notifier);
>>>  
>>>
>>
>> I think to remember pointing out that there is a bug somewhere in the
>> reset code which deactivates a non-active vhost instance, no?
>>
>> Jan
> 
> Could not find it. Could you dig it up pls?

http://thread.gmane.org/gmane.linux.scsi.target.devel/2277/focus=2309

Jan
Michael S. Tsirkin Aug. 13, 2012, 6:17 p.m. UTC | #5 
On Mon, Aug 13, 2012 at 08:06:17PM +0200, Jan Kiszka wrote:
> On 2012-08-13 20:03, Michael S. Tsirkin wrote:
> > On Mon, Aug 13, 2012 at 02:06:10PM +0200, Jan Kiszka wrote:
> >> On 2012-08-13 10:35, Nicholas A. Bellinger wrote:
> >>> From: Nicholas Bellinger <nab@linux-iscsi.org>
> >>>
> >>> This is required to get past the following assert with:
> >>>
> >>> commit 1523ed9e1d46b0b54540049d491475ccac7e6421
> >>> Author: Jan Kiszka <jan.kiszka@siemens.com>
> >>> Date:   Thu May 17 10:32:39 2012 -0300
> >>>
> >>>     virtio/vhost: Add support for KVM in-kernel MSI injection
> >>>
> >>> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> >>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Anthony Liguori <aliguori@us.ibm.com>
> >>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> >>> ---
> >>>  hw/msix.c |    3 +++
> >>>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/hw/msix.c b/hw/msix.c
> >>> index 800fc32..c1e6dc3 100644
> >>> --- a/hw/msix.c
> >>> +++ b/hw/msix.c
> >>> @@ -544,6 +544,9 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
> >>>  {
> >>>      int vector;
> >>>  
> >>> +    if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
> >>> +        return;
> >>> +
> >>>      assert(dev->msix_vector_use_notifier &&
> >>>             dev->msix_vector_release_notifier);
> >>>  
> >>>
> >>
> >> I think to remember pointing out that there is a bug somewhere in the
> >> reset code which deactivates a non-active vhost instance, no?
> >>
> >> Jan
> > 
> > Could not find it. Could you dig it up pls?
> 
> http://thread.gmane.org/gmane.linux.scsi.target.devel/2277/focus=2309
> 
> Jan

Ah yes. So let's not work around, need to get to the bottom of that.

> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
Blue Swirl Aug. 13, 2012, 7:39 p.m. UTC | #6 
On Mon, Aug 13, 2012 at 8:35 AM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This is required to get past the following assert with:
>
> commit 1523ed9e1d46b0b54540049d491475ccac7e6421
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date:   Thu May 17 10:32:39 2012 -0300
>
>     virtio/vhost: Add support for KVM in-kernel MSI injection
>
> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  hw/msix.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 800fc32..c1e6dc3 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -544,6 +544,9 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
>  {
>      int vector;
>
> +    if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
> +        return;

Missing braces, please read CODING_STYLE.

> +
>      assert(dev->msix_vector_use_notifier &&
>             dev->msix_vector_release_notifier);
>
> --
> 1.7.2.5
>
>
Nicholas A. Bellinger Aug. 14, 2012, 8:10 p.m. UTC | #7 
On Mon, 2012-08-13 at 21:17 +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2012 at 08:06:17PM +0200, Jan Kiszka wrote:
> > On 2012-08-13 20:03, Michael S. Tsirkin wrote:
> > > On Mon, Aug 13, 2012 at 02:06:10PM +0200, Jan Kiszka wrote:
> > >> On 2012-08-13 10:35, Nicholas A. Bellinger wrote:
> > >>> From: Nicholas Bellinger <nab@linux-iscsi.org>
> > >>>
> > >>> This is required to get past the following assert with:
> > >>>
> > >>> commit 1523ed9e1d46b0b54540049d491475ccac7e6421
> > >>> Author: Jan Kiszka <jan.kiszka@siemens.com>
> > >>> Date:   Thu May 17 10:32:39 2012 -0300
> > >>>
> > >>>     virtio/vhost: Add support for KVM in-kernel MSI injection
> > >>>
> > >>> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > >>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> > >>> Cc: Anthony Liguori <aliguori@us.ibm.com>
> > >>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > >>> ---
> > >>>  hw/msix.c |    3 +++
> > >>>  1 files changed, 3 insertions(+), 0 deletions(-)
> > >>>
> > >>> diff --git a/hw/msix.c b/hw/msix.c
> > >>> index 800fc32..c1e6dc3 100644
> > >>> --- a/hw/msix.c
> > >>> +++ b/hw/msix.c
> > >>> @@ -544,6 +544,9 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
> > >>>  {
> > >>>      int vector;
> > >>>  
> > >>> +    if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
> > >>> +        return;
> > >>> +
> > >>>      assert(dev->msix_vector_use_notifier &&
> > >>>             dev->msix_vector_release_notifier);
> > >>>  
> > >>>
> > >>
> > >> I think to remember pointing out that there is a bug somewhere in the
> > >> reset code which deactivates a non-active vhost instance, no?
> > >>
> > >> Jan
> > > 
> > > Could not find it. Could you dig it up pls?
> > 
> > http://thread.gmane.org/gmane.linux.scsi.target.devel/2277/focus=2309
> > 
> > Jan
> 
> Ah yes. So let's not work around, need to get to the bottom of that.
> 

Ok, so the assert being triggered in msix_unset_vector_notifiers()
appears to have been a side effect of the memory corruption bug in
virtio-scsi fixed in Patch #6, and is no longer required to start
vhost-scsi with the bugfix in place.

That said, dropping this patch for RFC-v3..
diff mbox

Patch

diff --git a/hw/msix.c b/hw/msix.c
index 800fc32..c1e6dc3 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -544,6 +544,9 @@  void msix_unset_vector_notifiers(PCIDevice *dev)
 {
     int vector;
 
+    if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
+        return;
+
     assert(dev->msix_vector_use_notifier &&
            dev->msix_vector_release_notifier);