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

[Markus Armbruster]

Jag Raman <jag.raman@oracle.com> writes:

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

[...]

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

Yes:

    static void vfu_object_init_ctx(VfuObject *o, Error **errp)
    {
        ERRP_GUARD();
        DeviceState *dev = NULL;
        vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL;
        int ret;

        if (o->vfu_ctx || !o->socket || !o->device ||
                !phase_check(PHASE_MACHINE_READY)) {
            return;
        }

Bails out without setting an error.  Sure that's appropriate?

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

Perhaps assertion with a comment explaining why it holds.

> Thank you!

You're welcome!