[v3] virtio-gpio and various virtio cleanups

[PATCH v3 08/21] hw/virtio: handle un-configured shutdown in virtio-pci

Posted by Alex Bennée 3 years, 6 months ago

The assert() protecting against leakage is a little aggressive and
causes needless crashes if a device is shutdown without having been
configured. In this case no descriptors are lost because none have
been assigned.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/virtio/virtio-pci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 45327f0b31..5ce61f9b45 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -996,9 +996,14 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
 
     nvqs = MIN(nvqs, VIRTIO_QUEUE_MAX);
 
-    /* When deassigning, pass a consistent nvqs value
-     * to avoid leaking notifiers.
+    /*
+     * When deassigning, pass a consistent nvqs value to avoid leaking
+     * notifiers. But first check we've actually been configured, exit
+     * early if we haven't.
      */
+    if (!assign && !proxy->nvqs_with_notifiers) {
+        return 0;
+    }
     assert(assign || nvqs == proxy->nvqs_with_notifiers);
 
     proxy->nvqs_with_notifiers = nvqs;
-- 
2.30.2

Re: [PATCH v3 08/21] hw/virtio: handle un-configured shutdown in virtio-pci

Posted by Jason Wang 3 years, 6 months ago

在 2022/7/27 03:21, Alex Bennée 写道:
> The assert() protecting against leakage is a little aggressive and
> causes needless crashes if a device is shutdown without having been
> configured. In this case no descriptors are lost because none have
> been assigned.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/virtio/virtio-pci.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 45327f0b31..5ce61f9b45 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -996,9 +996,14 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
>   
>       nvqs = MIN(nvqs, VIRTIO_QUEUE_MAX);
>   
> -    /* When deassigning, pass a consistent nvqs value
> -     * to avoid leaking notifiers.
> +    /*
> +     * When deassigning, pass a consistent nvqs value to avoid leaking
> +     * notifiers. But first check we've actually been configured, exit
> +     * early if we haven't.
>        */
> +    if (!assign && !proxy->nvqs_with_notifiers) {
> +        return 0;
> +    }


I think the logic is the caller is in charge of tracking whether the 
vhost device is started so it can avoid calling this function?

Thanks


>       assert(assign || nvqs == proxy->nvqs_with_notifiers);
>   
>       proxy->nvqs_with_notifiers = nvqs;