Re: [PATCH v3 04/12] vfio-user: instantiate vfio-user context

[Stefan Hajnoczi]

On Mon, Oct 11, 2021 at 01:31:09AM -0400, Jagannathan Raman wrote:
> @@ -94,9 +101,31 @@ static void vfu_object_set_device(Object *obj, const char 
> *str, Error **errp)
>      trace_vfu_prop("device", str);
>  }
>  
> +/*
> + * vfio-user-server depends on the availability of the 'socket' and 'device'
> + * properties. It also depends on devices instantiated in QEMU. These
> + * dependencies are not available during the instance_init phase of this
> + * object's life-cycle. As such, the server is initialized after the
> + * machine is setup. machine_init_done_notifier notifies vfio-user-server
> + * when the machine is setup, and the dependencies are available.
> + */
> +static void vfu_object_machine_done(Notifier *notifier, void *data)
> +{
> +    VfuObject *o = container_of(notifier, VfuObject, machine_done);

Was there a check for non-NULL o->socket before this? Maybe it's not
needed because QAPI treats 'socket' as a required field and refuses to
create the SocketAddress if it's missing?

> +
> +    o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, 0,
> +                                o, VFU_DEV_TYPE_PCI);
> +    if (o->vfu_ctx == NULL) {
> +        error_setg(&error_abort, "vfu: Failed to create context - %s",
> +                   strerror(errno));

The error reporting needs to be synchronous so that hotplugging with
object-add fails instead of succeeding and leaving a failed object.

In the startup case (not hotplug) it's okay to abort.

> +        return;
> +    }
> +}
> +
>  static void vfu_object_init(Object *obj)
>  {
>      VfuObjectClass *k = VFU_OBJECT_GET_CLASS(obj);
> +    VfuObject *o = VFU_OBJECT(obj);
>  
>      if (!object_dynamic_cast(OBJECT(current_machine), TYPE_REMOTE_MACHINE)) {
>          error_setg(&error_abort, "vfu: %s only compatible with %s machine",
> @@ -111,7 +140,12 @@ static void vfu_object_init(Object *obj)
>          return;
>      }
>  
> +    o->vfu_ctx = NULL;

The object's fields are initialized to 0 so this isn't necessary.

> +
>      k->nr_devs++;
> +
> +    o->machine_done.notify = vfu_object_machine_done;
> +    qemu_add_machine_init_done_notifier(&o->machine_done);

The notifier is invoked immediately if the machine has already been
initialized. That means vfu_object_machine_done() is called before the
properties ('socket' and 'device') have been set when object-add hotplug
is used. I think this needs to be moved elsewhere.
>  }
>  
>  static void vfu_object_finalize(Object *obj)
> @@ -123,6 +157,10 @@ static void vfu_object_finalize(Object *obj)
>  
>      g_free(o->socket);
>  
> +    if (o->vfu_ctx) {
> +        vfu_destroy_ctx(o->vfu_ctx);
> +    }
> +
>      g_free(o->device);
>  
>      if (k->nr_devs == 0) {

Missing qemu_remove_machine_init_done_notifier().

signature.asc
Description: PGP signature