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

[Jag Raman]

> On May 5, 2022, at 10:42 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> 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?

It’s not an error per se - vfu_object_init_ctx() doesn’t proceed
further if device/socket is not set or if the machine is not ready.

Both socket and device are required properties, so they would
eventually be set by vfu_object_set_socket() /
vfu_object_set_device() - and these setters eventually kick
vfu_object_init_ctx().

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

OK, will do.

--
Jag

> 
>> Thank you!
> 
> You're welcome!
>