Re: [PATCH v3 05/12] vfio-user: find and init PCI device

[Jag Raman <jag.raman@oracle.com>]

> On Oct 27, 2021, at 12:05 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Mon, Oct 11, 2021 at 01:31:10AM -0400, Jagannathan Raman wrote:
>> Find the PCI device with specified id. Initialize the device context
>> with the QEMU PCI device
>> 
>> 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>
>> ---
>> hw/remote/vfio-user-obj.c | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>> 
>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> index d26e5ec9e9..7ce4e5b256 100644
>> --- a/hw/remote/vfio-user-obj.c
>> +++ b/hw/remote/vfio-user-obj.c
>> @@ -44,6 +44,8 @@
>> #include "qemu/notify.h"
>> #include "sysemu/sysemu.h"
>> #include "libvfio-user.h"
>> +#include "hw/qdev-core.h"
>> +#include "hw/pci/pci.h"
>> 
>> #define TYPE_VFU_OBJECT "vfio-user-server"
>> OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>> @@ -68,6 +70,8 @@ struct VfuObject {
>>     Notifier machine_done;
>> 
>>     vfu_ctx_t *vfu_ctx;
>> +
>> +    PCIDevice *pci_dev;
>> };
>> 
>> static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
>> @@ -112,6 +116,9 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp)
>> static void vfu_object_machine_done(Notifier *notifier, void *data)
>> {
>>     VfuObject *o = container_of(notifier, VfuObject, machine_done);
>> +    DeviceState *dev = NULL;
>> +    vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL;
>> +    int ret;
>> 
>>     o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, 0,
>>                                 o, VFU_DEV_TYPE_PCI);
>> @@ -120,6 +127,31 @@ static void vfu_object_machine_done(Notifier *notifier, void *data)
>>                    strerror(errno));
>>         return;
>>     }
>> +
>> +    dev = qdev_find_recursive(sysbus_get_default(), o->device);
>> +    if (dev == NULL) {
>> +        error_setg(&error_abort, "vfu: Device %s not found", o->device);
>> +        return;
>> +    }
>> +
>> +    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> +        error_setg(&error_abort, "vfu: %s not a PCI device", o->device);
>> +        return;
>> +    }
>> +
>> +    o->pci_dev = PCI_DEVICE(dev);
>> +
>> +    if (pci_is_express(o->pci_dev)) {
>> +        pci_type = VFU_PCI_TYPE_EXPRESS;
>> +    }
>> +
>> +    ret = vfu_pci_init(o->vfu_ctx, pci_type, PCI_HEADER_TYPE_NORMAL, 0);
>> +    if (ret < 0) {
>> +        error_setg(&error_abort,
>> +                   "vfu: Failed to attach PCI device %s to context - %s",
>> +                   o->device, strerror(errno));
>> +        return;
>> +    }
> 
> It's unclear what happens when one of these error code paths is taken.
> o->vfu_ctx and o->pci_dev might both be initialized, so how does the
> code know not to service the vfio-user connection? It would be easy to
> tell that this is correct if o->pci_dev and o->vfu_ctx were destroyed
> when an error occurs.

Hey Stefan, if I understand your question correctly, you’re wondering
if the server would still service the connection if it takes the above
error path.

I don’t believe that would happen. When the above error path is taken,
“error_abort” is set. Setting error_abort immediately terminates the
QEMU process. Additionally, vfu_object_ctx_run() won’t be
attached to the unix socket as the function exits early - so the
connection wouldn’t be serviced.

—
Jag