Re: [PATCH v9 10/17] vfio-user: run vfio-user context

[Jag Raman]

> On May 4, 2022, at 7:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jagannathan Raman <jag.raman@oracle.com> writes:
> 
>> Setup a handler to run vfio-user context. The context is driven by
>> messages to the file descriptor associated with it - get the fd for
>> the context and hook up the handler with it
>> 
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>> qapi/misc.json | 30 +++++++++++
>> hw/remote/vfio-user-obj.c | 102 +++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 131 insertions(+), 1 deletion(-)
>> 
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index b83cc39029..fa49f2876a 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -553,3 +553,33 @@
>> ##
>> { 'event': 'RTC_CHANGE',
>> 'data': { 'offset': 'int', 'qom-path': 'str' } }
>> +
>> +##
>> +# @VFU_CLIENT_HANGUP:
>> +#
>> +# Emitted when the client of a TYPE_VFIO_USER_SERVER closes the
>> +# communication channel
>> +#
>> +# @vfu-id: ID of the TYPE_VFIO_USER_SERVER object
>> +#
>> +# @vfu-qom-path: path to the TYPE_VFIO_USER_SERVER object in the QOM tree
>> +#
>> +# @dev-id: ID of attached PCI device
>> +#
>> +# @dev-qom-path: path to attached PCI device in the QOM tree
> 
> I'm still unsure what kind(s) of ID @vfu-id and @dev-id are. See below.

I’m not sure what you mean by kind of ID - I thought of ID as a
unique string. I’ll try my best to explain.

dev-id and vfu-id are the “id" sub-options of “-device” and “-object” 
command-line
options respectively.

"dev-id” is the “id” member of “DeviceState” which QEMU sets using
qdev_set_id() when the device is added. 

The Object ID (vfu-id in this case) is a bit tricky. It’s also the “id”
command-line sub-option, but QEMU stores it as a child property
of the parent object.

> 
>> +#
>> +# Since: 7.1
>> +#
>> +# Example:
>> +#
>> +# <- { "event": "VFU_CLIENT_HANGUP",
>> +# "data": { "vfu-id": "vfu1",
>> +# "vfu-qom-path": "/objects/vfu1",
>> +# "dev-id": "sas1",
>> +# "dev-qom-path": "/machine/peripheral/sas1" },
>> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> +#
>> +##
>> +{ 'event': 'VFU_CLIENT_HANGUP',
>> + 'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
>> + 'dev-id': 'str', 'dev-qom-path': 'str' } }
>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> index 3ca6aa2b45..3a4c6a9fa0 100644
>> --- a/hw/remote/vfio-user-obj.c
>> +++ b/hw/remote/vfio-user-obj.c
>> @@ -27,6 +27,9 @@
>> *
>> * device - id of a device on the server, a required option. PCI devices
>> * alone are supported presently.
>> + *
>> + * notes - x-vfio-user-server could block IO and monitor during the
>> + * initialization phase.
>> */
>> 
>> #include "qemu/osdep.h"
>> @@ -40,11 +43,14 @@
>> #include "hw/remote/machine.h"
>> #include "qapi/error.h"
>> #include "qapi/qapi-visit-sockets.h"
>> +#include "qapi/qapi-events-misc.h"
>> #include "qemu/notify.h"
>> +#include "qemu/thread.h"
>> #include "sysemu/sysemu.h"
>> #include "libvfio-user.h"
>> #include "hw/qdev-core.h"
>> #include "hw/pci/pci.h"
>> +#include "qemu/timer.h"
>> 
>> #define TYPE_VFU_OBJECT "x-vfio-user-server"
>> OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>> @@ -86,6 +92,8 @@ struct VfuObject {
>> PCIDevice *pci_dev;
>> 
>> Error *unplug_blocker;
>> +
>> + int vfu_poll_fd;
>> };
>> 
>> static void vfu_object_init_ctx(VfuObject *o, Error **errp);
>> @@ -164,6 +172,76 @@ static void vfu_object_set_device(Object *obj, const 
>> char *str, Error **errp)
>> vfu_object_init_ctx(o, errp);
>> }
>> 
>> +static void vfu_object_ctx_run(void *opaque)
>> +{
>> + VfuObject *o = opaque;
>> + const char *vfu_id;
>> + char *vfu_path, *pci_dev_path;
>> + int ret = -1;
>> +
>> + while (ret != 0) {
>> + ret = vfu_run_ctx(o->vfu_ctx);
>> + if (ret < 0) {
>> + if (errno == EINTR) {
>> + continue;
>> + } else if (errno == ENOTCONN) {
>> + vfu_id = object_get_canonical_path_component(OBJECT(o));
>> + vfu_path = object_get_canonical_path(OBJECT(o));
> 
> Hmm. @vfu_id is always the last component of @vfu_path. Why do we need
> to send both?

vfu_id is the ID that the user/Orchestrator passed as a command-line option
during addition/creation. So it made sense to report back with the same ID
that they used. But I’m OK with dropping this if that’s what you prefer.

> 
>> + g_assert(o->pci_dev);
>> + pci_dev_path = object_get_canonical_path(OBJECT(o->pci_dev));
>> + qapi_event_send_vfu_client_hangup(vfu_id, vfu_path,
>> + o->device, pci_dev_path);
> 
> Where is o->device set? I'm asking because I it must not be null here,
> and that's not locally obvious.

Yeah, it’s not obvious from this patch that o->device is guaranteed to be
non-NULL. It’s set by vfu_object_set_device(). Please see the following
patches in the series:
vfio-user: define vfio-user-server object
vfio-user: instantiate vfio-user context

There’s already an assert for o->pci_dev here, but we could add one
for o->device too?

Thank you!
—
Jag

> 
>> + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL);
>> + o->vfu_poll_fd = -1;
>> + object_unparent(OBJECT(o));
>> + g_free(vfu_path);
>> + g_free(pci_dev_path);
>> + break;
>> + } else {
>> + VFU_OBJECT_ERROR(o, "vfu: Failed to run device %s - %s",
>> + o->device, strerror(errno));
>> + break;
>> + }
>> + }
>> + }
>> +}
> 
> [...]