Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4821 bytes --]

On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +static void *qpci_get_driver(void *obj, const char *interface)
>  {
> -    QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +    QPCIBusPC *qpci = obj;
> +    if (!g_strcmp0(interface, "pci-bus")) {
> +        return &qpci->bus;
> +    }
> +    printf("%s not present in pci-bus-pc", interface);
> +    abort();
> +}

At this point I wonder if it makes sense to use the QEMU Object Model
(QOM), which has interfaces and inheritance.  qgraph duplicates part of
the object model.

> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
> +{
> +    if (!bus) {
> +        return;
> +    }

When does this happen and why?

> +    dev->bus = bus;
> +    dev->devfn = devfn;
> +
> +    if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
> +        printf("PCI Device not found\n");
> +        abort();
> +    }
> +    qpci_device_enable(dev);
> +}
> +
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)

It's not clear to me what the purpose of this function is - at least the
name is a bit cryptic since it seems more like an initialization
function than 'setting pc' on QPCIBusPC.  How about inlining this in
qpci_init_pc() instead of keeping a separate function?

> +{
>      assert(qts);
>  
>      ret->bus.pio_readb = qpci_pc_pio_readb;
> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>      ret->bus.mmio_alloc_ptr = 0xE0000000;
>      ret->bus.mmio_limit = 0x100000000ULL;
>  
> +    ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +{
> +    QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +    qpci_set_pc(ret, qts, alloc);
> +
>      return &ret->bus;
>  }
>  
>  void qpci_free_pc(QPCIBus *bus)
>  {
> +    if (!bus) {
> +        return;
> +    }

Why is this needed now?

> +
>      QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>  
>      g_free(s);
> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>  
>      qmp_eventwait("DEVICE_DELETED");
>  }
> +
> +static void qpci_pc(void)
> +{
> +    qos_node_create_driver("pci-bus-pc", NULL);
> +    qos_node_produces("pci-bus-pc", "pci-bus");

In QOM pci-bus-pc would be a class, pci-bus would be an interface.  From
a driver perspective it seems QOM can already do what is needed and the
qgraph infrastructure isn't necessary.

Obviously the depth-first search *is* unique and not in QOM, although
QOM does offer a tree namespace which can be used for looking up object
instances and I guess this could be used to configure tests at runtime.

I'll think about this more as I read the rest of the patches.

> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 491eeac756..ee381c5667 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,7 +15,15 @@
>  
>  #include "libqos/pci.h"
>  #include "libqos/malloc.h"
> +#include "qgraph.h"
>  
> +typedef struct QPCIBusPC {
> +    QOSGraphObject obj;
> +    QPCIBus bus;
> +} QPCIBusPC;

Why does this need to be public?

> +
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);

Why does this need to be public?

> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);

Why does this need to be public?

>  QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>  void     qpci_free_pc(QPCIBus *bus);
>  
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..c51c186867 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -15,6 +15,7 @@
>  
>  #include "hw/pci/pci_regs.h"
>  #include "qemu/host-utils.h"
> +#include "qgraph.h"
>  
>  void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
>                           void (*func)(QPCIDevice *dev, int devfn, void *data),
> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const char *id,
>      qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
>                           opts ? ", " : "", opts ? opts : "");
>  }
> +
> +static void qpci(void)
> +{
> +    qos_node_create_interface("pci-bus");
> +}
> +
> +libqos_init(qpci);

Why does an interface need to be created?  The drivers declare which
interfaces they support?

I don't think this can be used to detect typoes in the driver's
qos_node_produces() call since there is no explicit control over the
order in which libqos_init() functions are called.  So the driver may
call qos_node_produces() before the qos_node_create_interface() is
called?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]