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

[Jag Raman]

> On Oct 27, 2021, at 11:59 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> 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?

Yes,  “socket” is a required option. The server will not launch without that 
option.

I believe optional parameters are defined within ‘[‘ and ‘]’ braces in 
"./qapi/qom.json"

> 
>> +
>> +    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.

OK, will make the change for the error reporting to be synchronous.

> 
> 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.

Got it.

> 
>> +
>>     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.

We’ll check this scenario out. Thank you!

>> }
>> 
>> 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().

Got it.