| Message ID | 20240416122919.597819-1-lulu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one() | expand |
On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote: > > In function kvm_virtio_pci_vector_use_one(), in the undo label, > the function will get the vector incorrectly while using > VIRTIO_CONFIG_IRQ_IDX > To fix this, we remove this label and simplify the failure process > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") > Cc: qemu-stable@nongnu.org > Signed-off-by: Cindy Lu <lulu@redhat.com> > --- > hw/virtio/virtio-pci.c | 19 +++---------------- > 1 file changed, 3 insertions(+), 16 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index b138fa127a..565bdb0897 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > } > ret = kvm_virtio_pci_vq_vector_use(proxy, vector); > if (ret < 0) { > - goto undo; > + return ret; > } > /* > * If guest supports masking, set up irqfd now. > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > ret = kvm_virtio_pci_irqfd_use(proxy, n, vector); > if (ret < 0) { > kvm_virtio_pci_vq_vector_release(proxy, vector); > - goto undo; > + kvm_virtio_pci_irqfd_release(proxy, n, vector); Are you sure this is right? The kvm_virtio_pci_irqfd_use() just failed, so why do we need to call kvm_virtio_pci_irqfd_release() ? thanks -- PMM
On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote: > > > > In function kvm_virtio_pci_vector_use_one(), in the undo label, > > the function will get the vector incorrectly while using > > VIRTIO_CONFIG_IRQ_IDX > > To fix this, we remove this label and simplify the failure process > > > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > --- > > hw/virtio/virtio-pci.c | 19 +++---------------- > > 1 file changed, 3 insertions(+), 16 deletions(-) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index b138fa127a..565bdb0897 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > > } > > ret = kvm_virtio_pci_vq_vector_use(proxy, vector); > > if (ret < 0) { > > - goto undo; > > + return ret; > > } > > /* > > * If guest supports masking, set up irqfd now. > > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > > ret = kvm_virtio_pci_irqfd_use(proxy, n, vector); > > if (ret < 0) { > > kvm_virtio_pci_vq_vector_release(proxy, vector); > > - goto undo; > > + kvm_virtio_pci_irqfd_release(proxy, n, vector); > > Are you sure this is right? The kvm_virtio_pci_irqfd_use() > just failed, so why do we need to call > kvm_virtio_pci_irqfd_release() ? > > thanks > -- PMM > This version should be correct. when kvm_virtio_pci_irqfd_use() fail we need to call kvm_virtio_pci_vq_vector_release() and kvm_virtio_pci_irqfd_release() but for kvm_virtio_pci_vq_vector_use fail we can simple return, in old version there is a error in failure process. while the kvm_virtio_pci_vq_vector_use fail it call the kvm_virtio_pci_irqfd_release,but at this time this is irqfd is not using now I have do the qtest and sanity test for this patch Thanks cindy
On Tue, 16 Apr 2024 at 13:41, Cindy Lu <lulu@redhat.com> wrote: > > On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote: > > > > > > In function kvm_virtio_pci_vector_use_one(), in the undo label, > > > the function will get the vector incorrectly while using > > > VIRTIO_CONFIG_IRQ_IDX > > > To fix this, we remove this label and simplify the failure process > > > > > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") > > > Cc: qemu-stable@nongnu.org > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > > --- > > > hw/virtio/virtio-pci.c | 19 +++---------------- > > > 1 file changed, 3 insertions(+), 16 deletions(-) > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > index b138fa127a..565bdb0897 100644 > > > --- a/hw/virtio/virtio-pci.c > > > +++ b/hw/virtio/virtio-pci.c > > > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > > > } > > > ret = kvm_virtio_pci_vq_vector_use(proxy, vector); > > > if (ret < 0) { > > > - goto undo; > > > + return ret; > > > } > > > /* > > > * If guest supports masking, set up irqfd now. > > > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > > > ret = kvm_virtio_pci_irqfd_use(proxy, n, vector); > > > if (ret < 0) { > > > kvm_virtio_pci_vq_vector_release(proxy, vector); > > > - goto undo; > > > + kvm_virtio_pci_irqfd_release(proxy, n, vector); > > > > Are you sure this is right? The kvm_virtio_pci_irqfd_use() > > just failed, so why do we need to call > > kvm_virtio_pci_irqfd_release() ? > This version should be correct. when kvm_virtio_pci_irqfd_use() fail > we need to call kvm_virtio_pci_vq_vector_release() and > kvm_virtio_pci_irqfd_release() > but for kvm_virtio_pci_vq_vector_use fail we can simple return, But *why* do we need to call kvm_virtio_pci_irqfd_release()? In most API designs, this kind of pairing of "get/use/allocate something" and "free/release something" function only requires you to do the "release" if the "get" succeeded, because if the "get" fails it's supposed to fail in way that means "I didn't do anything". Is this API not following that standard pattern ? > in old version there is a error in failure process. > while the kvm_virtio_pci_vq_vector_use fail it call the > kvm_virtio_pci_irqfd_release,but at this time this is irqfd > is not using now thanks -- PMM
On Tue, Apr 16, 2024 at 02:14:57PM +0100, Peter Maydell wrote: > On Tue, 16 Apr 2024 at 13:41, Cindy Lu <lulu@redhat.com> wrote: > > > > On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote: > > > > > > > > In function kvm_virtio_pci_vector_use_one(), in the undo label, > > > > the function will get the vector incorrectly while using > > > > VIRTIO_CONFIG_IRQ_IDX > > > > To fix this, we remove this label and simplify the failure process And then what happens? It's unclear whether it's a real or theoretical issue. > > > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") > > > > Cc: qemu-stable@nongnu.org > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > > > --- > > > > hw/virtio/virtio-pci.c | 19 +++---------------- > > > > 1 file changed, 3 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > > index b138fa127a..565bdb0897 100644 > > > > --- a/hw/virtio/virtio-pci.c > > > > +++ b/hw/virtio/virtio-pci.c > > > > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > > > > } > > > > ret = kvm_virtio_pci_vq_vector_use(proxy, vector); > > > > if (ret < 0) { > > > > - goto undo; > > > > + return ret; > > > > } > > > > /* > > > > * If guest supports masking, set up irqfd now. > > > > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > > > > ret = kvm_virtio_pci_irqfd_use(proxy, n, vector); > > > > if (ret < 0) { > > > > kvm_virtio_pci_vq_vector_release(proxy, vector); > > > > - goto undo; > > > > + kvm_virtio_pci_irqfd_release(proxy, n, vector); > > > > > > Are you sure this is right? The kvm_virtio_pci_irqfd_use() > > > just failed, so why do we need to call > > > kvm_virtio_pci_irqfd_release() ? > > > This version should be correct. when kvm_virtio_pci_irqfd_use() fail > > we need to call kvm_virtio_pci_vq_vector_release() and > > kvm_virtio_pci_irqfd_release() > > but for kvm_virtio_pci_vq_vector_use fail we can simple return, > > But *why* do we need to call kvm_virtio_pci_irqfd_release()? > > In most API designs, this kind of pairing of "get/use/allocate > something" and "free/release something" function only > requires you to do the "release" if the "get" succeeded, > because if the "get" fails it's supposed to fail in way that > means "I didn't do anything". Is this API not following that > standard pattern ? I am just as puzzled. > > in old version there is a error in failure process. > > while the kvm_virtio_pci_vq_vector_use fail it call the > > kvm_virtio_pci_irqfd_release,but at this time this is irqfd > > is not using now > > thanks > -- PMM
On Wed, Apr 17, 2024 at 2:38 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Apr 16, 2024 at 02:14:57PM +0100, Peter Maydell wrote: > > On Tue, 16 Apr 2024 at 13:41, Cindy Lu <lulu@redhat.com> wrote: > > > > > > On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote: > > > > > > > > > > In function kvm_virtio_pci_vector_use_one(), in the undo label, > > > > > the function will get the vector incorrectly while using > > > > > VIRTIO_CONFIG_IRQ_IDX > > > > > To fix this, we remove this label and simplify the failure process > > And then what happens? It's unclear whether it's a real or > theoretical issue. > > > > > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") > > > > > Cc: qemu-stable@nongnu.org > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > > > > --- > > > > > hw/virtio/virtio-pci.c | 19 +++---------------- > > > > > 1 file changed, 3 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > > > index b138fa127a..565bdb0897 100644 > > > > > --- a/hw/virtio/virtio-pci.c > > > > > +++ b/hw/virtio/virtio-pci.c > > > > > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > > > > > } > > > > > ret = kvm_virtio_pci_vq_vector_use(proxy, vector); > > > > > if (ret < 0) { > > > > > - goto undo; > > > > > + return ret; > > > > > } > > > > > /* > > > > > * If guest supports masking, set up irqfd now. > > > > > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) > > > > > ret = kvm_virtio_pci_irqfd_use(proxy, n, vector); > > > > > if (ret < 0) { > > > > > kvm_virtio_pci_vq_vector_release(proxy, vector); > > > > > - goto undo; > > > > > + kvm_virtio_pci_irqfd_release(proxy, n, vector); > > > > > > > > Are you sure this is right? The kvm_virtio_pci_irqfd_use() > > > > just failed, so why do we need to call > > > > kvm_virtio_pci_irqfd_release() ? > > > > > This version should be correct. when kvm_virtio_pci_irqfd_use() fail > > > we need to call kvm_virtio_pci_vq_vector_release() and > > > kvm_virtio_pci_irqfd_release() > > > but for kvm_virtio_pci_vq_vector_use fail we can simple return, > > > > But *why* do we need to call kvm_virtio_pci_irqfd_release()? > > > > In most API designs, this kind of pairing of "get/use/allocate > > something" and "free/release something" function only > > requires you to do the "release" if the "get" succeeded, > > because if the "get" fails it's supposed to fail in way that > > means "I didn't do anything". Is this API not following that > > standard pattern ? > > > I am just as puzzled. > got it. I made a mistake here, I will send the new version Thanks cindy > > > in old version there is a error in failure process. > > > while the kvm_virtio_pci_vq_vector_use fail it call the > > > kvm_virtio_pci_irqfd_release,but at this time this is irqfd > > > is not using now > > > > thanks > > -- PMM >
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index b138fa127a..565bdb0897 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) } ret = kvm_virtio_pci_vq_vector_use(proxy, vector); if (ret < 0) { - goto undo; + return ret; } /* * If guest supports masking, set up irqfd now. @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no) ret = kvm_virtio_pci_irqfd_use(proxy, n, vector); if (ret < 0) { kvm_virtio_pci_vq_vector_release(proxy, vector); - goto undo; + kvm_virtio_pci_irqfd_release(proxy, n, vector); + return ret; } } return 0; -undo: - - vector = virtio_queue_vector(vdev, queue_no); - if (vector >= msix_nr_vectors_allocated(dev)) { - return ret; - } - if (vdev->use_guest_notifier_mask && k->guest_notifier_mask) { - ret = virtio_pci_get_notifier(proxy, queue_no, &n, &vector); - if (ret < 0) { - return ret; - } - kvm_virtio_pci_irqfd_release(proxy, n, vector); - } - return ret; } static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs) {
In function kvm_virtio_pci_vector_use_one(), in the undo label, the function will get the vector incorrectly while using VIRTIO_CONFIG_IRQ_IDX To fix this, we remove this label and simplify the failure process Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") Cc: qemu-stable@nongnu.org Signed-off-by: Cindy Lu <lulu@redhat.com> --- hw/virtio/virtio-pci.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-)