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?
signature.asc
Description: PGP signature
