[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 10/17] vfio-user: run vfio-user context
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v9 10/17] vfio-user: run vfio-user context |
Date: |
Thu, 05 May 2022 16:42:33 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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!
- Re: [PATCH v9 06/17] vfio-user: build library, (continued)
[PATCH v9 09/17] vfio-user: find and init PCI device, Jagannathan Raman, 2022/05/03
[PATCH v9 10/17] vfio-user: run vfio-user context, Jagannathan Raman, 2022/05/03
- Re: [PATCH v9 10/17] vfio-user: run vfio-user context, Markus Armbruster, 2022/05/04
- Re: [PATCH v9 10/17] vfio-user: run vfio-user context, Jag Raman, 2022/05/04
- Re: [PATCH v9 10/17] vfio-user: run vfio-user context, Markus Armbruster, 2022/05/05
- Re: [PATCH v9 10/17] vfio-user: run vfio-user context, Jag Raman, 2022/05/05
- Re: [PATCH v9 10/17] vfio-user: run vfio-user context,
Markus Armbruster <=
- Re: [PATCH v9 10/17] vfio-user: run vfio-user context, Jag Raman, 2022/05/05
- Re: [PATCH v9 10/17] vfio-user: run vfio-user context, Markus Armbruster, 2022/05/06
- Re: [PATCH v9 10/17] vfio-user: run vfio-user context, Jag Raman, 2022/05/07
[PATCH v9 08/17] vfio-user: instantiate vfio-user context, Jagannathan Raman, 2022/05/03
[PATCH v9 11/17] vfio-user: handle PCI config space accesses, Jagannathan Raman, 2022/05/03
[PATCH v9 13/17] vfio-user: handle DMA mappings, Jagannathan Raman, 2022/05/03
[PATCH v9 12/17] vfio-user: IOMMU support for remote device, Jagannathan Raman, 2022/05/03
[PATCH v9 15/17] vfio-user: handle device interrupts, Jagannathan Raman, 2022/05/03
[PATCH v9 14/17] vfio-user: handle PCI BAR accesses, Jagannathan Raman, 2022/05/03