diff mbox series

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

Message ID 697ee91edc2af1aae07a01d49a27156d0e87c00c.1633929457.git.jag.raman@oracle.com
State New
Headers show
Series vfio-user server in QEMU | expand

Commit Message

Jag Raman Oct. 11, 2021, 5:31 a.m. UTC 
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(+)

Comments

Stefan Hajnoczi Oct. 27, 2021, 4:05 p.m. UTC | #1 
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.
Jag Raman Oct. 29, 2021, 3:58 p.m. UTC | #2 
> 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
Stefan Hajnoczi Nov. 1, 2021, 10:38 a.m. UTC | #3 
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
diff mbox series

Patch

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;
+    }
 }
 
 static void vfu_object_init(Object *obj)