[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 03/34] tests/qgraph: pci-pc driver and interf
From: |
Laurent Vivier |
Subject: |
Re: [Qemu-devel] [PATCH v2 03/34] tests/qgraph: pci-pc driver and interface nodes |
Date: |
Wed, 8 Aug 2018 19:39:51 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 06/08/2018 16:33, Emanuele Giuseppe Esposito wrote:
> Add pci-bus-pc node, move QPCIBusPC struct declaration in its header
> (since it will be needed by other drivers) and introduce a setter method
> for drivers that do not need to allocate but have to initialize QPCIBusPC.
>
> Signed-off-by: Emanuele Giuseppe Esposito <address@hidden>
> ---
> tests/Makefile.include | 4 +++-
> tests/libqos/pci-pc.c | 41 +++++++++++++++++++++++++++++-------
> tests/libqos/pci-pc.h | 15 ++++++++++++-
> tests/libqos/pci.c | 48 +++++++++++++++++++++++++++++++++++++++---
> tests/libqos/pci.h | 15 +++++++++++++
> 5 files changed, 110 insertions(+), 13 deletions(-)
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index eabf9ed8b4..f04f9fbc3a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -771,11 +771,13 @@ libqos-imx-obj-y = $(libqos-obj-y)
> tests/libqos/i2c-imx.o
> libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y)
> tests/libqos/usb.o
> libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y)
> tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o
> tests/libqos/malloc-generic.o
>
> +libqgraph-pci-obj-y = $(libqos-pc-obj-y)
> +
> check-unit-y += tests/test-qgraph$(EXESUF)
> tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
>
> check-qtest-pci-y += tests/qos-test$(EXESUF)
> -tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-obj-y)
> +tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-pci-obj-y)
>
> tests/qmp-test$(EXESUF): tests/qmp-test.o
> tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 83a3a32129..f5fb94eabc 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -18,15 +18,9 @@
>
> #include "qemu-common.h"
>
> -
> #define ACPI_PCIHP_ADDR 0xae00
> #define PCI_EJ_BASE 0x0008
>
> -typedef struct QPCIBusPC
> -{
> - QPCIBus bus;
> -} QPCIBusPC;
> -
> static uint8_t qpci_pc_pio_readb(QPCIBus *bus, uint32_t addr)
> {
> return inb(addr);
> @@ -115,12 +109,23 @@ static void qpci_pc_config_writel(QPCIBus *bus, int
> devfn, uint8_t offset, uint3
> outl(0xcfc, value);
> }
>
> -QPCIBus *qpci_pc_new(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\n", interface);
fprintf(stderr, ...) ?
> + abort();
You should use g_assert_not_reached().
> +}
>
> +void qpci_init_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> +{
> assert(qts);
>
> + /* tests can use pci-bus */
> + ret->bus.has_buggy_msi = FALSE;
I think you should introduce the field has_buggy_msi when it will be
really needed: with patch 09/34 adn pci-spapr.
> +
> ret->bus.pio_readb = qpci_pc_pio_readb;
> ret->bus.pio_readw = qpci_pc_pio_readw;
> ret->bus.pio_readl = qpci_pc_pio_readl;
> @@ -147,11 +152,23 @@ QPCIBus *qpci_pc_new(QTestState *qts, QGuestAllocator
> *alloc)
> ret->bus.mmio_alloc_ptr = 0xE0000000;
> ret->bus.mmio_limit = 0x100000000ULL;
>
> + ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_pc_new(QTestState *qts, QGuestAllocator *alloc)
As I said for 02/34, I think qpci_new_pc() should be a better name to
like the one we have for qpci_free_pc().
> +{
> + QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
perhaps you can rename "ret" to "qpci"?
> + qpci_init_pc(ret, qts, alloc);
> +
> return &ret->bus;
> }
>
> void qpci_free_pc(QPCIBus *bus)
> {
> + if (!bus) {
> + return;
> + }
> +
> QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
Generally, gcc doesn't like to mix declarations and instructions. I
think something like this would be nicer:
QPCIBusPC *s;
if (!bus) {
return;
}
s = container_of(bus, QPCIBusPC, bus);
>
> g_free(s);
> @@ -176,3 +193,11 @@ void qpci_unplug_acpi_device_test(const char *id,
> uint8_t slot)
>
> qmp_eventwait("DEVICE_DELETED");
> }
> +
> +static void qpci_pc(void)
This name is not very clear, for the qemu part, type_init() is generally
used with XXX_register_types name.
Perhaps you can use something like "qpci_pc_register_nodes" name?
> +{
> + qos_node_create_driver("pci-bus-pc", NULL);
> + qos_node_produces("pci-bus-pc", "pci-bus");
> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 88be29eaf3..a3754c1c86 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,9 +15,22 @@
>
> #include "libqos/pci.h"
> #include "libqos/malloc.h"
> +#include "libqos/qgraph.h"
>
> +typedef struct QPCIBusPC {
> + QOSGraphObject obj;
> + QPCIBus bus;
> +} QPCIBusPC;
> +
> +/* qpci_init_pc():
> + * this function initialize an already allocated
> + * QPCIBusPC object.
> + *
> + * @ret must be a valid QPCIBusPC * pointer.
> + */
> +void qpci_init_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
> /* qpci_pc_new():
> -* this function creates a new QPCIBusPC object,
> + * this function creates a new QPCIBusPC object,
> * and properly initialize its fields.
> *
> * returns the QPCIBus *bus field of a newly
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..91440ece5c 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 "libqos/qgraph.h"
>
> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> void (*func)(QPCIDevice *dev, int devfn, void
> *data),
> @@ -50,15 +51,31 @@ void qpci_device_foreach(QPCIBus *bus, int vendor_id, int
> device_id,
> }
> }
>
> -QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
> +bool qpci_has_buggy_msi(QPCIDevice *dev)
> {
> - QPCIDevice *dev;
> + return dev->bus->has_buggy_msi;
> +}
As above, introduce this when you need it.
> - dev = g_malloc0(sizeof(*dev));
> +/* returns TRUE if everything goes fine, FALSE if not */
> +static bool qpci_device_set(QPCIDevice *dev, QPCIBus *bus, int devfn)
> +{
> dev->bus = bus;
> dev->devfn = devfn;
>
> if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
> + return FALSE;
> + }
I think what we check here is to see if the PCI device is available.
0xFFFF means there is a problem.
I think you should add a function "qpci_device_available(devn, bus)"
> +
> + return TRUE;
> +}
> +
> +QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
> +{
> + QPCIDevice *dev;
> +
> + dev = g_malloc0(sizeof(*dev));
> +
> + if (!qpci_device_set(dev, bus, devfn)) {
> g_free(dev);
> return NULL;
> }
> @@ -66,6 +83,21 @@ QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
> return dev;
> }
>
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr)
> +{
> + uint16_t vendor_id, device_id;
> +
> + if (!qpci_device_set(dev, bus, addr->devfn)) {
> + printf("PCI Device not found\n");
> + abort();
> + }
so, here, you should use qpci_device_set() and qpci_device_available()...
> +
> + vendor_id = qpci_config_readw(dev, PCI_VENDOR_ID);
or you can only check vendor_id to see it is 0xffff or not...
> + device_id = qpci_config_readw(dev, PCI_DEVICE_ID);
> + g_assert(vendor_id == addr->vendor_id);
that should be in fact detected by this g_assert().
> + g_assert(device_id == addr->device_id);
> +}
> +
> void qpci_device_enable(QPCIDevice *dev)
> {
> uint16_t cmd;
> @@ -402,3 +434,13 @@ 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 : "");
> }
> +
> +void add_qpci_address(QOSGraphEdgeOptions *opts, QPCIAddress *addr)
> +{
> + if (!addr || !opts) {
> + return;
> + }
Should it be an error case and fails (g_assert())?
> +
> + opts->arg = addr;
> + opts->size_arg = sizeof(QPCIAddress);
> +}
> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> index 429c382282..5fb3c550c5 100644
> --- a/tests/libqos/pci.h
> +++ b/tests/libqos/pci.h
> @@ -14,6 +14,7 @@
> #define LIBQOS_PCI_H
>
> #include "libqtest.h"
> +#include "libqos/qgraph.h"
>
> #define QPCI_PIO_LIMIT 0x10000
>
> @@ -22,6 +23,7 @@
> typedef struct QPCIDevice QPCIDevice;
> typedef struct QPCIBus QPCIBus;
> typedef struct QPCIBar QPCIBar;
> +typedef struct QPCIAddress QPCIAddress;
>
> struct QPCIBus {
> uint8_t (*pio_readb)(QPCIBus *bus, uint32_t addr);
> @@ -51,6 +53,8 @@ struct QPCIBus {
> QTestState *qts;
> uint16_t pio_alloc_ptr;
> uint64_t mmio_alloc_ptr, mmio_limit;
> + bool has_buggy_msi; /* TRUE for spapr, FALSE for pci */
as above, introduce it when you need it.
> +
> };
>
> struct QPCIBar {
> @@ -66,10 +70,19 @@ struct QPCIDevice
> uint64_t msix_table_off, msix_pba_off;
> };
>
> +struct QPCIAddress {
> + uint32_t devfn;
> + uint16_t vendor_id;
> + uint16_t device_id;
> +};
> +
> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> void (*func)(QPCIDevice *dev, int devfn, void
> *data),
> void *data);
> QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn);
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr);
> +/* returns the bus has_buggy_msi flag */
> +bool qpci_has_buggy_msi(QPCIDevice *dev);
>
> void qpci_device_enable(QPCIDevice *dev);
> uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id);
> @@ -112,4 +125,6 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
> void qpci_plug_device_test(const char *driver, const char *id,
> uint8_t slot, const char *opts);
> void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
> +
> +void add_qpci_address(QOSGraphEdgeOptions *opts, QPCIAddress *addr);
> #endif
>
[Qemu-devel] [PATCH v2 04/34] tests/qgraph: x86_64/pc machine node, Emanuele Giuseppe Esposito, 2018/08/06
[Qemu-devel] [PATCH v2 05/34] tests/qgraph: sdhci driver and interface nodes, Emanuele Giuseppe Esposito, 2018/08/06