Re: [PATCH 6/6] tests/qtest/libqos: Add pci-arm and add a pci-arm producer in arm-virt machine

[Paolo Bonzini <pbonzini@redhat.com>]

On 1/10/22 22:19, Eric Auger wrote:
> Up to now the virt-machine node contains a virtio-mmio node.
> However no driver produces any PCI interface node. Hence, PCI
> tests cannot be run with aarch64 binary.
> 
> Add a GPEX driver node that produces a pci interface node. This latter
> then can be consumed by all the pci tests. One of the first motivation
> was to be able to run the virtio-iommu-pci tests.
> 
> We still face an issue with pci hotplug tests as hotplug cannot happen
> on the pcie root bus and require a generic root port. This will be
> addressed later on.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

Hey Eric,

it's great to have gpex support in libqos/qgraph!  On the next versions 
you might also Cc Emanuele since he was the author of the framework.

> ---
>   tests/qtest/libqos/arm-virt-machine.c |  47 +++++-
>   tests/qtest/libqos/meson.build        |   3 +
>   tests/qtest/libqos/pci-arm.c          | 219 ++++++++++++++++++++++++++
>   tests/qtest/libqos/pci-arm.h          |  56 +++++++
>   tests/qtest/libqos/pci.h              |   1 +
>   tests/qtest/libqos/qgraph.c           |   7 +
>   tests/qtest/libqos/qgraph.h           |  15 ++
>   7 files changed, 344 insertions(+), 4 deletions(-)
>   create mode 100644 tests/qtest/libqos/pci-arm.c
>   create mode 100644 tests/qtest/libqos/pci-arm.h
> 
> diff --git a/tests/qtest/libqos/arm-virt-machine.c b/tests/qtest/libqos/arm-virt-machine.c
> index e0f59322845..130c45c51e2 100644
> --- a/tests/qtest/libqos/arm-virt-machine.c
> +++ b/tests/qtest/libqos/arm-virt-machine.c
> @@ -22,6 +22,8 @@
>   #include "malloc.h"
>   #include "qgraph.h"
>   #include "virtio-mmio.h"
> +#include "pci-arm.h"
> +#include "hw/pci/pci_regs.h"
>   
>   #define ARM_PAGE_SIZE               4096
>   #define VIRTIO_MMIO_BASE_ADDR       0x0A003E00
> @@ -30,13 +32,40 @@
>   #define VIRTIO_MMIO_SIZE            0x00000200
>   
>   typedef struct QVirtMachine QVirtMachine;
> +typedef struct QGenericPCIHost QGenericPCIHost;
> +
> +struct QGenericPCIHost {
> +    QOSGraphObject obj;
> +    QPCIBusARM pci;
> +};

You can rename QPCIBusARM to QGenericPCIBus and move QGenericPCIHost to 
the same file.  There's nothing ARM specific in either file, and nothing 
specific to -M virt in QGenericPCIHost.

>   struct QVirtMachine {
>       QOSGraphObject obj;
>       QGuestAllocator alloc;
>       QVirtioMMIODevice virtio_mmio;
> +    QGenericPCIHost bridge;
>   };
>   
> +/* QGenericPCIHost */
> +
> +static QOSGraphObject *generic_pcihost_get_device(void *obj, const char *device)
> +{
> +    QGenericPCIHost *host = obj;
> +    if (!g_strcmp0(device, "pci-bus-arm")) {
> +        return &host->pci.obj;
> +    }
> +    fprintf(stderr, "%s not present in generic-pcihost\n", device);
> +    g_assert_not_reached();
> +}
> +
> +static void qos_create_generic_pcihost(QGenericPCIHost *host,
> +                                       QTestState *qts,
> +                                       QGuestAllocator *alloc)
> +{
> +    host->obj.get_device = generic_pcihost_get_device;
> +    qpci_init_arm(&host->pci, qts, alloc, false);
> +}
> +
>   static void virt_destructor(QOSGraphObject *obj)
>   {
>       QVirtMachine *machine = (QVirtMachine *) obj;

This should also be in the same file as the bus implementation.

> +    qos_node_create_driver("generic-pcihost", NULL);
> +    qos_node_contains("generic-pcihost", "pci-bus-arm", NULL);

This too, with a new libqos_init.


> +static uint8_t qpci_arm_config_readb(QPCIBus *bus, int devfn, uint8_t offset)
> +{
> +    uint64_t addr = bus->ecam_alloc_ptr + ((0 << 20) | (devfn << 12) | offset);

ecam_alloc_ptr should be in QPCIBusARM (to be renamed to 
QGenericPCIBus), which you can retrieve from the "bus" QPCIBus* via 
container_of.

> diff --git a/tests/qtest/libqos/pci-arm.h b/tests/qtest/libqos/pci-arm.h
> new file mode 100644
> index 00000000000..8cd49ec2969
> --- /dev/null
> +++ b/tests/qtest/libqos/pci-arm.h
> @@ -0,0 +1,56 @@
> +/*
> + * libqos PCI bindings for ARM
> + *
> + * Copyright Red Hat Inc., 2021
> + *
> + * Authors:
> + *  Eric Auger   <eric.auger@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef LIBQOS_PCI_ARM_H
> +#define LIBQOS_PCI_ARM_H
> +
> +#include "pci.h"
> +#include "malloc.h"
> +#include "qgraph.h"
> +
> +typedef struct QPCIBusARM {
> +    QOSGraphObject obj;
> +    QPCIBus bus;
> +    uint64_t gpex_pio_base;
> +} QPCIBusARM;
> +
> +/*
> + * qpci_init_arm():
> + * @ret: A valid QPCIBusARM * pointer
> + * @qts: The %QTestState for this ARM machine
> + * @alloc: A previously initialized @alloc providing memory for @qts
> + * @bool: devices can be hotplugged on this bus
> + *
> + * This function initializes an already allocated
> + * QPCIBusARM object.
> + */
> +void qpci_init_arm(QPCIBusARM *ret, QTestState *qts,
> +                   QGuestAllocator *alloc, bool hotpluggable);
> +
> +/*
> + * qpci_arm_new():
> + * @qts: The %QTestState for this ARM machine
> + * @alloc: A previously initialized @alloc providing memory for @qts
> + * @hotpluggable: the pci bus is hotpluggable
> + *
> + * This function creates a new QPCIBusARM object,
> + * and properly initialize its fields.
> + *
> + * Returns the QPCIBus *bus field of a newly
> + * allocated QPCIBusARM.
> + */
> +QPCIBus *qpci_new_arm(QTestState *qts, QGuestAllocator *alloc,
> +                      bool hotpluggable);
> +
> +void qpci_free_arm(QPCIBus *bus);

These two functions are not needed now.  I'm not opposing non-qgraph 
tests that use the gpex device, but the functions should be introduced 
together with their users.

> diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
> index 109ff04e1e8..e03fad35241 100644
> --- a/tests/qtest/libqos/qgraph.c
> +++ b/tests/qtest/libqos/qgraph.c
> @@ -667,6 +667,13 @@ void qos_node_produces(const char *producer, const char *interface)
>       add_edge(producer, interface, QEDGE_PRODUCES, NULL);
>   }
>   
> +void qos_node_produces_opts(const char *producer, const char *interface,
> +                            QOSGraphEdgeOptions *opts)
> +{
> +    create_interface(interface);
> +    add_edge(producer, interface, QEDGE_PRODUCES, opts);
> +}
> +

This is not needed, I think.

Paolo