[PATCH 0/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[PATCH 0/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[Cindy Lu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1344 bytes --]

There is a crash in the Non-standard guest image. The root cause of
the issue is that an IRQFD was used After it release 

During the booting process of the Vyatta image, the behavior of the
called function in qemu is as follows:

1. vhost_net_stop() was called. This will call the function
virtio_pci_set_guest_notifiers() with assgin= false, and
virtio_pci_set_guest_notifiers(） will release the irqfd for vector 0

2. virtio_reset() was called -->set configure vector to VIRTIO_NO_VECTOR

3.vhost_net_start() was called (at this time the configure vector is
still VIRTIO_NO_VECTOR) and call virtio_pci_set_guest_notifiers() with
assgin= true, so the irqfd for vector 0 was not "init" during this process

4. The system continues to boot, and msix_fire_vector_notifier() was
called unmask the vector 0 and then met the crash
[msix_fire_vector_notifier] 112 called vector 0 is_masked 1
[msix_fire_vector_notifier] 112 called vector 0 is_masked 0

To fix this, we need to call the function "kvm_virtio_pci_vector_use_one()"
when the vector changes back from VIRTIO_NO_VECTOR

Signed-off-by: Cindy Lu <lulu@redhat.com>

Cindy Lu (1):
  virtio-pci: Fix the crash when the vector changes back from
    VIRTIO_NO_VECTOR

 hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH 1/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[Cindy Lu]

When the guest calls virtio_stop and then virtio_reset, the vector will change
to VIRTIO_NO_VECTOR and the IRQFD for this vector will be released. After that
If you want to change the vector back, it will cause a crash.

To fix this, we need to call the function "kvm_virtio_pci_vector_use_one()"
when the vector changes back from VIRTIO_NO_VECTOR

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e433879542..45f3ab38c3 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -874,12 +874,14 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy *proxy, int queue_no,
     return 0;
 }
 
-static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
+static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no,
+                                         bool recovery)
 {
     unsigned int vector;
     int ret;
     EventNotifier *n;
     PCIDevice *dev = &proxy->pci_dev;
+    VirtIOIRQFD *irqfd;
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
 
@@ -890,10 +892,21 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
     if (vector >= msix_nr_vectors_allocated(dev)) {
         return 0;
     }
+    /*
+     * if this is recovery and irqfd still in use, means the irqfd was not
+     * release before and don't need to set up again
+     */
+    if (recovery) {
+        irqfd = &proxy->vector_irqfd[vector];
+        if (irqfd->users != 0) {
+            return 0;
+        }
+    }
     ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
     if (ret < 0) {
         goto undo;
     }
+
     /*
      * If guest supports masking, set up irqfd now.
      * Otherwise, delay until unmasked in the frontend.
@@ -932,14 +945,14 @@ static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs)
         if (!virtio_queue_get_num(vdev, queue_no)) {
             return -1;
         }
-        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no);
+        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no, false);
     }
     return ret;
 }
 
 static int kvm_virtio_pci_vector_config_use(VirtIOPCIProxy *proxy)
 {
-    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX);
+    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, false);
 }
 
 static void kvm_virtio_pci_vector_release_one(VirtIOPCIProxy *proxy,
@@ -1570,7 +1583,13 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
         } else {
             val = VIRTIO_NO_VECTOR;
         }
+        vector = vdev->config_vector;
         vdev->config_vector = val;
+        /*check if the vector need to recovery*/
+        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
+            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+            kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, true);
+        }
         break;
     case VIRTIO_PCI_COMMON_STATUS:
         if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
@@ -1611,6 +1630,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
             val = VIRTIO_NO_VECTOR;
         }
         virtio_queue_set_vector(vdev, vdev->queue_sel, val);
+
+        /*check if the vector need to recovery*/
+        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
+            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+            kvm_virtio_pci_vector_use_one(proxy, vdev->queue_sel, true);
+        }
         break;
     case VIRTIO_PCI_COMMON_Q_ENABLE:
         if (val == 1) {
-- 
2.43.0

Re: [PATCH 1/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[Jason Wang]

On Tue, Apr 2, 2024 at 11:02 PM Cindy Lu <lulu@redhat.com> wrote:
>
> When the guest calls virtio_stop and then virtio_reset,

Guests could not call those functions directly, it is triggered by for
example writing to some of the registers like reset or others.

> the vector will change
> to VIRTIO_NO_VECTOR and the IRQFD for this vector will be released. After that
> If you want to change the vector back,

What do you mean by "change the vector back"? Something like

assign VIRTIO_MSI_NO_VECTOR to vector 0
assign X to vector 0

And I guess what you meant is to configure the vector after DRIVER_OK.


> it will cause a crash.
>
> To fix this, we need to call the function "kvm_virtio_pci_vector_use_one()"
> when the vector changes back from VIRTIO_NO_VECTOR
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index e433879542..45f3ab38c3 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -874,12 +874,14 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy *proxy, int queue_no,
>      return 0;
>  }
>
> -static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> +static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no,
> +                                         bool recovery)
>  {
>      unsigned int vector;
>      int ret;
>      EventNotifier *n;
>      PCIDevice *dev = &proxy->pci_dev;
> +    VirtIOIRQFD *irqfd;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>
> @@ -890,10 +892,21 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
>      if (vector >= msix_nr_vectors_allocated(dev)) {
>          return 0;
>      }
> +    /*
> +     * if this is recovery and irqfd still in use, means the irqfd was not
> +     * release before and don't need to set up again
> +     */
> +    if (recovery) {
> +        irqfd = &proxy->vector_irqfd[vector];
> +        if (irqfd->users != 0) {
> +            return 0;
> +        }
> +    }
>      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
>      if (ret < 0) {
>          goto undo;
>      }
> +
>      /*
>       * If guest supports masking, set up irqfd now.
>       * Otherwise, delay until unmasked in the frontend.
> @@ -932,14 +945,14 @@ static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs)
>          if (!virtio_queue_get_num(vdev, queue_no)) {
>              return -1;
>          }
> -        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no);
> +        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no, false);
>      }
>      return ret;
>  }
>
>  static int kvm_virtio_pci_vector_config_use(VirtIOPCIProxy *proxy)
>  {
> -    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX);
> +    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, false);
>  }
>
>  static void kvm_virtio_pci_vector_release_one(VirtIOPCIProxy *proxy,
> @@ -1570,7 +1583,13 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>          } else {
>              val = VIRTIO_NO_VECTOR;
>          }
> +        vector = vdev->config_vector;
>          vdev->config_vector = val;
> +        /*check if the vector need to recovery*/
> +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +            kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, true);
> +        }

This looks too tricky.

Think hard of this. I think it's better to split this into two parts:

1) a series that disables config irqfd for vhost-net, this series
needs to be backported to -stable which needs to be conservative. It
looks more like your V1, but let's add a boolean for pci proxy.
2) a series that deal with the msix vector configuration after
driver_ok, we probably need some refactoring to do per vq use instead
of the current loop in DRIVER_OK

Does this make sense?

Thanks

>          break;
>      case VIRTIO_PCI_COMMON_STATUS:
>          if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> @@ -1611,6 +1630,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>              val = VIRTIO_NO_VECTOR;
>          }
>          virtio_queue_set_vector(vdev, vdev->queue_sel, val);
> +
> +        /*check if the vector need to recovery*/
> +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +            kvm_virtio_pci_vector_use_one(proxy, vdev->queue_sel, true);
> +        }
>          break;
>      case VIRTIO_PCI_COMMON_Q_ENABLE:
>          if (val == 1) {
> --
> 2.43.0
>

Re: [PATCH 1/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[Cindy Lu]

On Sun, Apr 7, 2024 at 12:20 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Apr 2, 2024 at 11:02 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > When the guest calls virtio_stop and then virtio_reset,
>
> Guests could not call those functions directly, it is triggered by for
> example writing to some of the registers like reset or others.
>
sure , Will fix this
> > the vector will change
> > to VIRTIO_NO_VECTOR and the IRQFD for this vector will be released. After that
> > If you want to change the vector back,
>
> What do you mean by "change the vector back"? Something like
>
> assign VIRTIO_MSI_NO_VECTOR to vector 0
> assign X to vector 0
>
yes, the process is something  like
....
set config_vector = VIRTIO_MSI_NO_VECTOR
...
set config_vector = 0
> And I guess what you meant is to configure the vector after DRIVER_OK.

>
>
> > it will cause a crash.
> >
> > To fix this, we need to call the function "kvm_virtio_pci_vector_use_one()"
> > when the vector changes back from VIRTIO_NO_VECTOR
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++++++---
> >  1 file changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index e433879542..45f3ab38c3 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -874,12 +874,14 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy *proxy, int queue_no,
> >      return 0;
> >  }
> >
> > -static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > +static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no,
> > +                                         bool recovery)
> >  {
> >      unsigned int vector;
> >      int ret;
> >      EventNotifier *n;
> >      PCIDevice *dev = &proxy->pci_dev;
> > +    VirtIOIRQFD *irqfd;
> >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >
> > @@ -890,10 +892,21 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> >      if (vector >= msix_nr_vectors_allocated(dev)) {
> >          return 0;
> >      }
> > +    /*
> > +     * if this is recovery and irqfd still in use, means the irqfd was not
> > +     * release before and don't need to set up again
> > +     */
> > +    if (recovery) {
> > +        irqfd = &proxy->vector_irqfd[vector];
> > +        if (irqfd->users != 0) {
> > +            return 0;
> > +        }
> > +    }
> >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> >      if (ret < 0) {
> >          goto undo;
> >      }
> > +
> >      /*
> >       * If guest supports masking, set up irqfd now.
> >       * Otherwise, delay until unmasked in the frontend.
> > @@ -932,14 +945,14 @@ static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs)
> >          if (!virtio_queue_get_num(vdev, queue_no)) {
> >              return -1;
> >          }
> > -        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no);
> > +        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no, false);
> >      }
> >      return ret;
> >  }
> >
> >  static int kvm_virtio_pci_vector_config_use(VirtIOPCIProxy *proxy)
> >  {
> > -    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX);
> > +    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, false);
> >  }
> >
> >  static void kvm_virtio_pci_vector_release_one(VirtIOPCIProxy *proxy,
> > @@ -1570,7 +1583,13 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> >          } else {
> >              val = VIRTIO_NO_VECTOR;
> >          }
> > +        vector = vdev->config_vector;
> >          vdev->config_vector = val;
> > +        /*check if the vector need to recovery*/
> > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > +            kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, true);
> > +        }
>
> This looks too tricky.
>
> Think hard of this. I think it's better to split this into two parts:
>
> 1) a series that disables config irqfd for vhost-net, this series
> needs to be backported to -stable which needs to be conservative. It
> looks more like your V1, but let's add a boolean for pci proxy.
sure, I wll try this

> 2) a series that deal with the msix vector configuration after
> driver_ok, we probably need some refactoring to do per vq use instead
> of the current loop in DRIVER_OK
>
Sorry I didn't get what you mean , Do you mean we need to move the check
to inside kvm_virtio_pci_vector_vq_use()？
Thanks
cindy
> Does this make sense?
>
> Thanks
>
> >          break;
> >      case VIRTIO_PCI_COMMON_STATUS:
> >          if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > @@ -1611,6 +1630,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> >              val = VIRTIO_NO_VECTOR;
> >          }
> >          virtio_queue_set_vector(vdev, vdev->queue_sel, val);
> > +
> > +        /*check if the vector need to recovery*/
> > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > +            kvm_virtio_pci_vector_use_one(proxy, vdev->queue_sel, true);
> > +        }
> >          break;
> >      case VIRTIO_PCI_COMMON_Q_ENABLE:
> >          if (val == 1) {
> > --
> > 2.43.0
> >
>

Re: [PATCH 1/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[Michael S. Tsirkin]

On Sun, Apr 07, 2024 at 12:19:57PM +0800, Jason Wang wrote:
> On Tue, Apr 2, 2024 at 11:02 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > When the guest calls virtio_stop and then virtio_reset,
> 
> Guests could not call those functions directly, it is triggered by for
> example writing to some of the registers like reset or others.
> 
> > the vector will change
> > to VIRTIO_NO_VECTOR and the IRQFD for this vector will be released. After that
> > If you want to change the vector back,
> 
> What do you mean by "change the vector back"? Something like
> 
> assign VIRTIO_MSI_NO_VECTOR to vector 0
> assign X to vector 0
> 
> And I guess what you meant is to configure the vector after DRIVER_OK.
> 
> 
> > it will cause a crash.
> >
> > To fix this, we need to call the function "kvm_virtio_pci_vector_use_one()"
> > when the vector changes back from VIRTIO_NO_VECTOR
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++++++---
> >  1 file changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index e433879542..45f3ab38c3 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -874,12 +874,14 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy *proxy, int queue_no,
> >      return 0;
> >  }
> >
> > -static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > +static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no,
> > +                                         bool recovery)
> >  {
> >      unsigned int vector;
> >      int ret;
> >      EventNotifier *n;
> >      PCIDevice *dev = &proxy->pci_dev;
> > +    VirtIOIRQFD *irqfd;
> >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >
> > @@ -890,10 +892,21 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> >      if (vector >= msix_nr_vectors_allocated(dev)) {
> >          return 0;
> >      }
> > +    /*
> > +     * if this is recovery and irqfd still in use, means the irqfd was not
> > +     * release before and don't need to set up again
> > +     */
> > +    if (recovery) {
> > +        irqfd = &proxy->vector_irqfd[vector];
> > +        if (irqfd->users != 0) {
> > +            return 0;
> > +        }
> > +    }
> >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> >      if (ret < 0) {
> >          goto undo;
> >      }
> > +
> >      /*
> >       * If guest supports masking, set up irqfd now.
> >       * Otherwise, delay until unmasked in the frontend.
> > @@ -932,14 +945,14 @@ static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs)
> >          if (!virtio_queue_get_num(vdev, queue_no)) {
> >              return -1;
> >          }
> > -        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no);
> > +        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no, false);
> >      }
> >      return ret;
> >  }
> >
> >  static int kvm_virtio_pci_vector_config_use(VirtIOPCIProxy *proxy)
> >  {
> > -    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX);
> > +    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, false);
> >  }
> >
> >  static void kvm_virtio_pci_vector_release_one(VirtIOPCIProxy *proxy,
> > @@ -1570,7 +1583,13 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> >          } else {
> >              val = VIRTIO_NO_VECTOR;
> >          }
> > +        vector = vdev->config_vector;
> >          vdev->config_vector = val;
> > +        /*check if the vector need to recovery*/
> > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > +            kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, true);
> > +        }
> 
> This looks too tricky.
> 
> Think hard of this. I think it's better to split this into two parts:
> 
> 1) a series that disables config irqfd for vhost-net, this series
> needs to be backported to -stable which needs to be conservative. It
> looks more like your V1, but let's add a boolean for pci proxy.

I don't get it. Looks like a huge change to do in stable.
All as a replacement to a small 20 line patch?

Generally I think irqfd is best used everywhere.


> 2) a series that deal with the msix vector configuration after
> driver_ok, we probably need some refactoring to do per vq use instead
> of the current loop in DRIVER_OK
> 
> Does this make sense?
> 
> Thanks


Not really let's fix the bug for starters, refactoring can be done later
as appropriate.

> >          break;
> >      case VIRTIO_PCI_COMMON_STATUS:
> >          if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > @@ -1611,6 +1630,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> >              val = VIRTIO_NO_VECTOR;
> >          }
> >          virtio_queue_set_vector(vdev, vdev->queue_sel, val);
> > +
> > +        /*check if the vector need to recovery*/
> > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > +            kvm_virtio_pci_vector_use_one(proxy, vdev->queue_sel, true);
> > +        }
> >          break;
> >      case VIRTIO_PCI_COMMON_Q_ENABLE:
> >          if (val == 1) {
> > --
> > 2.43.0
> >

Re: [PATCH 1/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[Jason Wang]

On Sun, Apr 7, 2024 at 7:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Apr 07, 2024 at 12:19:57PM +0800, Jason Wang wrote:
> > On Tue, Apr 2, 2024 at 11:02 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > When the guest calls virtio_stop and then virtio_reset,
> >
> > Guests could not call those functions directly, it is triggered by for
> > example writing to some of the registers like reset or others.
> >
> > > the vector will change
> > > to VIRTIO_NO_VECTOR and the IRQFD for this vector will be released. After that
> > > If you want to change the vector back,
> >
> > What do you mean by "change the vector back"? Something like
> >
> > assign VIRTIO_MSI_NO_VECTOR to vector 0
> > assign X to vector 0
> >
> > And I guess what you meant is to configure the vector after DRIVER_OK.
> >
> >
> > > it will cause a crash.
> > >
> > > To fix this, we need to call the function "kvm_virtio_pci_vector_use_one()"
> > > when the vector changes back from VIRTIO_NO_VECTOR
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++++++---
> > >  1 file changed, 28 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index e433879542..45f3ab38c3 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -874,12 +874,14 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy *proxy, int queue_no,
> > >      return 0;
> > >  }
> > >
> > > -static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > +static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no,
> > > +                                         bool recovery)
> > >  {
> > >      unsigned int vector;
> > >      int ret;
> > >      EventNotifier *n;
> > >      PCIDevice *dev = &proxy->pci_dev;
> > > +    VirtIOIRQFD *irqfd;
> > >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > >      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > >
> > > @@ -890,10 +892,21 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > >      if (vector >= msix_nr_vectors_allocated(dev)) {
> > >          return 0;
> > >      }
> > > +    /*
> > > +     * if this is recovery and irqfd still in use, means the irqfd was not
> > > +     * release before and don't need to set up again
> > > +     */
> > > +    if (recovery) {
> > > +        irqfd = &proxy->vector_irqfd[vector];
> > > +        if (irqfd->users != 0) {
> > > +            return 0;
> > > +        }
> > > +    }
> > >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> > >      if (ret < 0) {
> > >          goto undo;
> > >      }
> > > +
> > >      /*
> > >       * If guest supports masking, set up irqfd now.
> > >       * Otherwise, delay until unmasked in the frontend.
> > > @@ -932,14 +945,14 @@ static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs)
> > >          if (!virtio_queue_get_num(vdev, queue_no)) {
> > >              return -1;
> > >          }
> > > -        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no);
> > > +        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no, false);
> > >      }
> > >      return ret;
> > >  }
> > >
> > >  static int kvm_virtio_pci_vector_config_use(VirtIOPCIProxy *proxy)
> > >  {
> > > -    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX);
> > > +    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, false);
> > >  }
> > >
> > >  static void kvm_virtio_pci_vector_release_one(VirtIOPCIProxy *proxy,
> > > @@ -1570,7 +1583,13 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> > >          } else {
> > >              val = VIRTIO_NO_VECTOR;
> > >          }
> > > +        vector = vdev->config_vector;
> > >          vdev->config_vector = val;
> > > +        /*check if the vector need to recovery*/
> > > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > +            kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, true);
> > > +        }
> >
> > This looks too tricky.
> >
> > Think hard of this. I think it's better to split this into two parts:
> >
> > 1) a series that disables config irqfd for vhost-net, this series
> > needs to be backported to -stable which needs to be conservative. It
> > looks more like your V1, but let's add a boolean for pci proxy.
>
> I don't get it. Looks like a huge change to do in stable.
> All as a replacement to a small 20 line patch?

For example, this patch needs more tweak so it won't be that tiny:

1) needs to check if we can use guest notifiers (irqfd)
2) the switching from X to NO_VECTOR might need
kvm_virtio_pci_vq_vector_release()

>
> Generally I think irqfd is best used everywhere.

Only when msix and kvm are both enabled.

>
>
> > 2) a series that deal with the msix vector configuration after
> > driver_ok, we probably need some refactoring to do per vq use instead
> > of the current loop in DRIVER_OK
> >
> > Does this make sense?
> >
> > Thanks
>
>
> Not really let's fix the bug for starters, refactoring can be done later
> as appropriate.

This is exactly my point. Avoiding that for non vDPA device seems to
be easier or maybe I was wrong.

Thanks

>
> > >          break;
> > >      case VIRTIO_PCI_COMMON_STATUS:
> > >          if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > @@ -1611,6 +1630,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> > >              val = VIRTIO_NO_VECTOR;
> > >          }
> > >          virtio_queue_set_vector(vdev, vdev->queue_sel, val);
> > > +
> > > +        /*check if the vector need to recovery*/
> > > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > +            kvm_virtio_pci_vector_use_one(proxy, vdev->queue_sel, true);
> > > +        }
> > >          break;
> > >      case VIRTIO_PCI_COMMON_Q_ENABLE:
> > >          if (val == 1) {
> > > --
> > > 2.43.0
> > >
>

Re: [PATCH 1/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[Jason Wang]

On Sun, Apr 7, 2024 at 3:00 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Sun, Apr 7, 2024 at 12:20 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Apr 2, 2024 at 11:02 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > When the guest calls virtio_stop and then virtio_reset,
> >
> > Guests could not call those functions directly, it is triggered by for
> > example writing to some of the registers like reset or others.
> >
> sure , Will fix this
> > > the vector will change
> > > to VIRTIO_NO_VECTOR and the IRQFD for this vector will be released. After that
> > > If you want to change the vector back,
> >
> > What do you mean by "change the vector back"? Something like
> >
> > assign VIRTIO_MSI_NO_VECTOR to vector 0
> > assign X to vector 0
> >
> yes, the process is something  like
> ....
> set config_vector = VIRTIO_MSI_NO_VECTOR
> ...
> set config_vector = 0
> > And I guess what you meant is to configure the vector after DRIVER_OK.
>
> >
> >
> > > it will cause a crash.
> > >
> > > To fix this, we need to call the function "kvm_virtio_pci_vector_use_one()"
> > > when the vector changes back from VIRTIO_NO_VECTOR
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++++++---
> > >  1 file changed, 28 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index e433879542..45f3ab38c3 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -874,12 +874,14 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy *proxy, int queue_no,
> > >      return 0;
> > >  }
> > >
> > > -static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > +static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no,
> > > +                                         bool recovery)
> > >  {
> > >      unsigned int vector;
> > >      int ret;
> > >      EventNotifier *n;
> > >      PCIDevice *dev = &proxy->pci_dev;
> > > +    VirtIOIRQFD *irqfd;
> > >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > >      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > >
> > > @@ -890,10 +892,21 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > >      if (vector >= msix_nr_vectors_allocated(dev)) {
> > >          return 0;
> > >      }
> > > +    /*
> > > +     * if this is recovery and irqfd still in use, means the irqfd was not
> > > +     * release before and don't need to set up again
> > > +     */
> > > +    if (recovery) {
> > > +        irqfd = &proxy->vector_irqfd[vector];
> > > +        if (irqfd->users != 0) {
> > > +            return 0;
> > > +        }
> > > +    }
> > >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> > >      if (ret < 0) {
> > >          goto undo;
> > >      }
> > > +
> > >      /*
> > >       * If guest supports masking, set up irqfd now.
> > >       * Otherwise, delay until unmasked in the frontend.
> > > @@ -932,14 +945,14 @@ static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs)
> > >          if (!virtio_queue_get_num(vdev, queue_no)) {
> > >              return -1;
> > >          }
> > > -        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no);
> > > +        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no, false);
> > >      }
> > >      return ret;
> > >  }
> > >
> > >  static int kvm_virtio_pci_vector_config_use(VirtIOPCIProxy *proxy)
> > >  {
> > > -    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX);
> > > +    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, false);
> > >  }
> > >
> > >  static void kvm_virtio_pci_vector_release_one(VirtIOPCIProxy *proxy,
> > > @@ -1570,7 +1583,13 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> > >          } else {
> > >              val = VIRTIO_NO_VECTOR;
> > >          }
> > > +        vector = vdev->config_vector;
> > >          vdev->config_vector = val;
> > > +        /*check if the vector need to recovery*/
> > > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > +            kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, true);
> > > +        }
> >
> > This looks too tricky.
> >
> > Think hard of this. I think it's better to split this into two parts:
> >
> > 1) a series that disables config irqfd for vhost-net, this series
> > needs to be backported to -stable which needs to be conservative. It
> > looks more like your V1, but let's add a boolean for pci proxy.
> sure, I wll try this
>
> > 2) a series that deal with the msix vector configuration after
> > driver_ok, we probably need some refactoring to do per vq use instead
> > of the current loop in DRIVER_OK
> >
> Sorry I didn't get what you mean , Do you mean we need to move the check
> to inside kvm_virtio_pci_vector_vq_use()？
> Thanks
> cindy

I meant try to do use/release during mmio write instead of start.

Note that Michael seems to prefer the approach of this patch. Let's
fix the comment I gave in another thread and see.

Thanks

> > Does this make sense?
> >
> > Thanks
> >
> > >          break;
> > >      case VIRTIO_PCI_COMMON_STATUS:
> > >          if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > @@ -1611,6 +1630,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> > >              val = VIRTIO_NO_VECTOR;
> > >          }
> > >          virtio_queue_set_vector(vdev, vdev->queue_sel, val);
> > > +
> > > +        /*check if the vector need to recovery*/
> > > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > +            kvm_virtio_pci_vector_use_one(proxy, vdev->queue_sel, true);
> > > +        }
> > >          break;
> > >      case VIRTIO_PCI_COMMON_Q_ENABLE:
> > >          if (val == 1) {
> > > --
> > > 2.43.0
> > >
> >
>

Re: [PATCH 1/1] virtio-pci: Fix the crash when the vector changes back from VIRTIO_NO_VECTOR

[Cindy Lu]

On Mon, Apr 8, 2024 at 12:59 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sun, Apr 7, 2024 at 3:00 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > On Sun, Apr 7, 2024 at 12:20 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Tue, Apr 2, 2024 at 11:02 PM Cindy Lu <lulu@redhat.com> wrote:
> > > >
> > > > When the guest calls virtio_stop and then virtio_reset,
> > >
> > > Guests could not call those functions directly, it is triggered by for
> > > example writing to some of the registers like reset or others.
> > >
> > sure , Will fix this
> > > > the vector will change
> > > > to VIRTIO_NO_VECTOR and the IRQFD for this vector will be released. After that
> > > > If you want to change the vector back,
> > >
> > > What do you mean by "change the vector back"? Something like
> > >
> > > assign VIRTIO_MSI_NO_VECTOR to vector 0
> > > assign X to vector 0
> > >
> > yes, the process is something  like
> > ....
> > set config_vector = VIRTIO_MSI_NO_VECTOR
> > ...
> > set config_vector = 0
> > > And I guess what you meant is to configure the vector after DRIVER_OK.
> >
> > >
> > >
> > > > it will cause a crash.
> > > >
> > > > To fix this, we need to call the function "kvm_virtio_pci_vector_use_one()"
> > > > when the vector changes back from VIRTIO_NO_VECTOR
> > > >
> > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++++++---
> > > >  1 file changed, 28 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > index e433879542..45f3ab38c3 100644
> > > > --- a/hw/virtio/virtio-pci.c
> > > > +++ b/hw/virtio/virtio-pci.c
> > > > @@ -874,12 +874,14 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy *proxy, int queue_no,
> > > >      return 0;
> > > >  }
> > > >
> > > > -static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > > +static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no,
> > > > +                                         bool recovery)
> > > >  {
> > > >      unsigned int vector;
> > > >      int ret;
> > > >      EventNotifier *n;
> > > >      PCIDevice *dev = &proxy->pci_dev;
> > > > +    VirtIOIRQFD *irqfd;
> > > >      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > > >      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > > >
> > > > @@ -890,10 +892,21 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > >      if (vector >= msix_nr_vectors_allocated(dev)) {
> > > >          return 0;
> > > >      }
> > > > +    /*
> > > > +     * if this is recovery and irqfd still in use, means the irqfd was not
> > > > +     * release before and don't need to set up again
> > > > +     */
> > > > +    if (recovery) {
> > > > +        irqfd = &proxy->vector_irqfd[vector];
> > > > +        if (irqfd->users != 0) {
> > > > +            return 0;
> > > > +        }
> > > > +    }
> > > >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> > > >      if (ret < 0) {
> > > >          goto undo;
> > > >      }
> > > > +
> > > >      /*
> > > >       * If guest supports masking, set up irqfd now.
> > > >       * Otherwise, delay until unmasked in the frontend.
> > > > @@ -932,14 +945,14 @@ static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs)
> > > >          if (!virtio_queue_get_num(vdev, queue_no)) {
> > > >              return -1;
> > > >          }
> > > > -        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no);
> > > > +        ret = kvm_virtio_pci_vector_use_one(proxy, queue_no, false);
> > > >      }
> > > >      return ret;
> > > >  }
> > > >
> > > >  static int kvm_virtio_pci_vector_config_use(VirtIOPCIProxy *proxy)
> > > >  {
> > > > -    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX);
> > > > +    return kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, false);
> > > >  }
> > > >
> > > >  static void kvm_virtio_pci_vector_release_one(VirtIOPCIProxy *proxy,
> > > > @@ -1570,7 +1583,13 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> > > >          } else {
> > > >              val = VIRTIO_NO_VECTOR;
> > > >          }
> > > > +        vector = vdev->config_vector;
> > > >          vdev->config_vector = val;
> > > > +        /*check if the vector need to recovery*/
> > > > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > > > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > +            kvm_virtio_pci_vector_use_one(proxy, VIRTIO_CONFIG_IRQ_IDX, true);
> > > > +        }
> > >
> > > This looks too tricky.
> > >
> > > Think hard of this. I think it's better to split this into two parts:
> > >
> > > 1) a series that disables config irqfd for vhost-net, this series
> > > needs to be backported to -stable which needs to be conservative. It
> > > looks more like your V1, but let's add a boolean for pci proxy.
> > sure, I wll try this
> >
> > > 2) a series that deal with the msix vector configuration after
> > > driver_ok, we probably need some refactoring to do per vq use instead
> > > of the current loop in DRIVER_OK
> > >
> > Sorry I didn't get what you mean , Do you mean we need to move the check
> > to inside kvm_virtio_pci_vector_vq_use()？
> > Thanks
> > cindy
>
> I meant try to do use/release during mmio write instead of start.
>
> Note that Michael seems to prefer the approach of this patch. Let's
> fix the comment I gave in another thread and see.
>
> Thanks
>
sure, got ot. I will try this
thanks
cindy
> > > Does this make sense?
> > >
> > > Thanks
> > >
> > > >          break;
> > > >      case VIRTIO_PCI_COMMON_STATUS:
> > > >          if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > @@ -1611,6 +1630,12 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> > > >              val = VIRTIO_NO_VECTOR;
> > > >          }
> > > >          virtio_queue_set_vector(vdev, vdev->queue_sel, val);
> > > > +
> > > > +        /*check if the vector need to recovery*/
> > > > +        if ((val != VIRTIO_NO_VECTOR) && (vector == VIRTIO_NO_VECTOR) &&
> > > > +            (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > +            kvm_virtio_pci_vector_use_one(proxy, vdev->queue_sel, true);
> > > > +        }
> > > >          break;
> > > >      case VIRTIO_PCI_COMMON_Q_ENABLE:
> > > >          if (val == 1) {
> > > > --
> > > > 2.43.0
> > > >
> > >
> >
>