Hi
On Tue, Nov 7, 2023 at 1:37 PM Albert Esteve <aest...@redhat.com> wrote:
>
> Ensure that we cleanup all virtio shared
> resources when the vhost devices is cleaned
> up (after a hot unplug, or a crash).
>
> To track all owned uuids of a device, add
> a GSList to the vhost_dev struct. This way
> we can avoid traversing the full table
> for every cleanup, whether they actually
> own any shared resource or not.
>
> Signed-off-by: Albert Esteve <aest...@redhat.com>
> ---
> hw/virtio/vhost-user.c | 2 ++
> hw/virtio/vhost.c | 4 ++++
> include/hw/virtio/vhost.h | 6 ++++++
> 3 files changed, 12 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 5fdff0241f..04848d1fa0 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1598,6 +1598,7 @@ vhost_user_backend_handle_shared_object_add(struct
> vhost_dev *dev,
> QemuUUID uuid;
>
> memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> + dev->shared_uuids = g_slist_append(dev->shared_uuids, &uuid);
This will point to the stack variable.
> return virtio_add_vhost_device(&uuid, dev);
> }
>
> @@ -1623,6 +1624,7 @@ vhost_user_backend_handle_shared_object_remove(struct
> vhost_dev *dev,
> }
>
> memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> + dev->shared_uuids = g_slist_remove_all(dev->shared_uuids, &uuid);
> return virtio_remove_resource(&uuid);
> }
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9c9ae7109e..3aff94664b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -16,6 +16,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "hw/virtio/vhost.h"
> +#include "hw/virtio/virtio-dmabuf.h"
> #include "qemu/atomic.h"
> #include "qemu/range.h"
> #include "qemu/error-report.h"
> @@ -1599,6 +1600,9 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> migrate_del_blocker(&hdev->migration_blocker);
> g_free(hdev->mem);
> g_free(hdev->mem_sections);
> + /* free virtio shared objects */
> + g_slist_foreach(hdev->shared_uuids, (GFunc)virtio_remove_resource, NULL);
> + g_slist_free_full(g_steal_pointer(&hdev->shared_uuids), g_free);
(and will crash here)
Imho, you should just traverse the hashtable, instead of introducing
another list.
> if (hdev->vhost_ops) {
> hdev->vhost_ops->vhost_backend_cleanup(hdev);
> }
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 5e8183f64a..376bc8446d 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -118,6 +118,12 @@ struct vhost_dev {
> */
> uint64_t protocol_features;
>
> + /**
> + * @shared_uuids: contains the UUIDs of all the exported
> + * virtio objects owned by the vhost device.
> + */
> + GSList *shared_uuids;
> +
> uint64_t max_queues;
> uint64_t backend_cap;
> /* @started: is the vhost device started? */
> --
> 2.41.0
>
--
Marc-André Lureau
