Albert Esteve <aest...@redhat.com> writes:
> On Tue, Feb 6, 2024 at 12:11 AM Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Albert Esteve <aest...@redhat.com> writes:
>
> > Ensure that we cleanup all virtio shared
> > resources when the vhost devices is cleaned
> > up (after a hot unplug, or a crash).
> >
> > To do so, we add a new function to the virtio_dmabuf
> > API called `virtio_dmabuf_vhost_cleanup`, which
> > loop through the table and removes all
> > resources owned by the vhost device parameter.
> >
> > Also, add a test to verify that the new
> > function in the API behaves as expected.
> >
> > Signed-off-by: Albert Esteve <aest...@redhat.com>
> > Acked-by: Stefan Hajnoczi <stefa...@redhat.com>
> > ---
> > hw/display/virtio-dmabuf.c | 22 +++++++++++++++++++++
> > hw/virtio/vhost.c | 3 +++
> > include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
> > tests/unit/test-virtio-dmabuf.c | 33 +++++++++++++++++++++++++++++++
> > 4 files changed, 68 insertions(+)
> >
> > diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> > index 3dba4577ca..6688809777 100644
> > --- a/hw/display/virtio-dmabuf.c
> > +++ b/hw/display/virtio-dmabuf.c
> > @@ -136,6 +136,28 @@ SharedObjectType virtio_object_type(const QemuUUID
> *uuid)
> > return vso->type;
> > }
> >
> > +static bool virtio_dmabuf_resource_is_owned(gpointer key,
> > + gpointer value,
> > + gpointer dev)
> > +{
> > + VirtioSharedObject *vso;
> > +
> > + vso = (VirtioSharedObject *) value;
> > + return vso->type == TYPE_VHOST_DEV && vso->value == dev;
>
> It's a bit surprising to see vso->value being an anonymous gpointer
> rather than the proper type and a bit confusing between value and
> vso->value.
>
> It is the signature required for this to be used with
> `g_hash_table_foreach_remove`.
> For the naming, the HashMap stores gpointers, that point to
> `VirtioSharedObject`, and
> these point to the underlying type (stored at `vso->value`). It may sound a
> bit confusing,
> but is a byproduct of the VirtioSharedObject indirection. Not sure which
> names could be
> more fit for this, but I'm open to change them.
This is the problem without overloading value and vso->value. I
appreciate that virtio_dmabuf_resource_is_owned() has to follow the
signature for g_hash_table_foreach_remove but usually the compare
function then casts gpointer to the underlying type for any comparison.
So something like:
typedef struct VirtioSharedObject {
SharedObjectType type;
union {
vhost_dev *dev; /* TYPE_VHOST_DEV */
int udma_buf; /* TYPE_DMABUF */
} value;
} VirtioSharedObject;
and then you would have:
VirtioSharedObject *vso = value;
if (vso->type == TYPE_VHOST_DEV) {
vhost_dev *dev = dev;
return vso->value.dev == dev;
}
In fact I think you can skip the cast so have:
VirtioSharedObject *vso = value;
return vso->type == TYPE_VHOST_DEV && vso->value.dev == dev;
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
