Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()
On Wed, Apr 17, 2024 at 2:38 AM Michael S. Tsirkin wrote:
>
> On Tue, Apr 16, 2024 at 02:14:57PM +0100, Peter Maydell wrote:
> > On Tue, 16 Apr 2024 at 13:41, Cindy Lu wrote:
> > >
> > > On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell
> > > wrote:
> > > >
> > > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu 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-sta...@nongnu.org
> > > > > Signed-off-by: Cindy Lu
> > > > > ---
> > > > > 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
>
Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()
On Tue, Apr 16, 2024 at 02:14:57PM +0100, Peter Maydell wrote:
> On Tue, 16 Apr 2024 at 13:41, Cindy Lu wrote:
> >
> > On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell
> > wrote:
> > >
> > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu 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-sta...@nongnu.org
> > > > Signed-off-by: Cindy Lu
> > > > ---
> > > > 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
Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()
On Tue, 16 Apr 2024 at 13:41, Cindy Lu wrote:
>
> On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell
> wrote:
> >
> > On Tue, 16 Apr 2024 at 13:29, Cindy Lu 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-sta...@nongnu.org
> > > Signed-off-by: Cindy Lu
> > > ---
> > > 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
Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()
On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell wrote:
>
> On Tue, 16 Apr 2024 at 13:29, Cindy Lu 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-sta...@nongnu.org
> > Signed-off-by: Cindy Lu
> > ---
> > 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
Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()
On Tue, 16 Apr 2024 at 13:29, Cindy Lu 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-sta...@nongnu.org
> Signed-off-by: Cindy Lu
> ---
> 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
[PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()
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-sta...@nongnu.org
Signed-off-by: Cindy Lu
---
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);
+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, , );
-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)
{
--
2.43.0
