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

[Stefan Hajnoczi]

On Fri, Oct 29, 2021 at 03:58:28PM +0000, Jag Raman wrote:
> 
> 
> > 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.

Thanks. I'll revisit this next revision because error_abort cannot be
used for hotplug. It's okay to terminate the process on startup, but for
hotplug the expected behavior is to report an error and continue
running.

Stefan

signature.asc
Description: PGP signature