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

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(-)

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
>

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
> >
>

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
> >

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
> > >
>

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
> > >
> >
>

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
> > > >
> > >
> >
>