On Mon, Apr 8, 2024 at 12:59 PM Jason Wang <jasow...@redhat.com> wrote:
>
> On Sun, Apr 7, 2024 at 3:00 PM Cindy Lu <l...@redhat.com> wrote:
> >
> > On Sun, Apr 7, 2024 at 12:20 PM Jason Wang <jasow...@redhat.com> wrote:
> > >
> > > On Tue, Apr 2, 2024 at 11:02 PM Cindy Lu <l...@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 <l...@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
> > > >
> > >
> >
>
