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

[Jag Raman]

> On May 5, 2022, at 3:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jag Raman <jag.raman@oracle.com> writes:
> 
>>> 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.
> 
> Okay, let me try to clarify.
> 
> We have many, many ID namespaces, each associated with a certain kind of
> object: device IDs for TYPE_DEVICE, object IDs for TYPE_OBJECT
> implementing TYPE_USER_CREATABLE), block backend node names for
> BlockDriverState, ...
> 
> Aside: I believe a single namespace would have been a wiser design
> choice, but that ship sailed long ago.
> 
> To which of these namespaces do these two IDs belong, respectively?
> 
>> dev-id and vfu-id are the “id" sub-options of “-device” and “-object” 
>> command-line
>> options respectively.
> 
> This answers my question.
> 
>> "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.
> 
> Matter of taste, I guess.  I'd drop it simply to saves us the trouble of
> documenting it.
> 
> If we decide to keep it, then I think we should document it's always the
> last component of @vfu_path.
> 
>>>> + 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
> 
> vfu_object_set_device() is a QOM property setter.  It gets called if and
> only if the property is set.  If it's never set, ->device remains null.
> What ensures it's always set?

That’s a good question - it’s not obvious from this patch.

The code would not reach here if o->device is not set. If o->device is NULL,
vfu_object_init_ctx() would bail out early without setting up
vfu_object_attach_ctx() and vfu_object_ctx_run() (this function) handlers.

Also, device is a required parameter. QEMU would not initialize this object
without it. Please see the definition of VfioUserServerProperties in the
following patch - noting that optional parameters are prefixed with a ‘*’:
[PATCH v9 07/17] vfio-user: define vfio-user-server object.

May be we should add a comment here to explain why o->device
wouldn’t be NULL?

Thank you!

> 
>> There’s already an assert for o->pci_dev here, but we could add one
>> for o->device too?
> 
> I'll make up my mind when I'm convinced o->device can't be null here.
> 
>> Thank you!
> 
> You're welcome!
>