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

Commit Message

Jag Raman Oct. 11, 2021, 5:31 a.m. UTC

create a context with the vfio-user library to run a PCI device

Signed-off-by: Elena Ufimtseva <[e-mail]>
Signed-off-by: John G Johnson <[e-mail]>
Signed-off-by: Jagannathan Raman <[e-mail]>
---
 hw/remote/vfio-user-obj.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Stefan Hajnoczi Oct. 27, 2021, 3:59 p.m. UTC | #1

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

Jag Raman Oct. 29, 2021, 2:59 p.m. UTC | #2

> On Oct 27, 2021, at 11:59 AM, Stefan Hajnoczi <[e-mail]> 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.

Stefan Hajnoczi Nov. 1, 2021, 10:35 a.m. UTC | #3

On Fri, Oct 29, 2021 at 02:59:02PM +0000, Jag Raman wrote:
> 
> 
> > On Oct 27, 2021, at 11:59 AM, Stefan Hajnoczi <[e-mail]> 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.

Thanks for confirming!

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

Optional parameters have the asterisk ('*'). Block quotes are for lists.

Stefan

Patch

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index c2a300f0ff..d26e5ec9e9 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -41,6 +41,9 @@ 
 #include "hw/remote/machine.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-sockets.h"
+#include "qemu/notify.h"
+#include "sysemu/sysemu.h"
+#include "libvfio-user.h"
 
 #define TYPE_VFU_OBJECT "vfio-user-server"
 OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
@@ -61,6 +64,10 @@  struct VfuObject {
     SocketAddress *socket;
 
     char *device;
+
+    Notifier machine_done;
+
+    vfu_ctx_t *vfu_ctx;
 };
 
 static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
@@ -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);
+
+    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));
+        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;
+
     k->nr_devs++;
+
+    o->machine_done.notify = vfu_object_machine_done;
+    qemu_add_machine_init_done_notifier(&o->machine_done);
 }
 
 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) {