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

[Eric Auger <eric.auger@redhat.com>]

Hi Paolo,

On 1/15/22 5:01 PM, Paolo Bonzini wrote:
> 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.
sure!
>
>> ---
>>   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.

done
>
>>   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.
done
>
>> +    qos_node_create_driver("generic-pcihost", NULL);
>> +    qos_node_contains("generic-pcihost", "pci-bus-arm", NULL);
>
> This too, with a new libqos_init.
done
>
>
>> +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.
done
>
>> 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.
removed
>
>> 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.
indeed, removed.

So you should see the above comments taken into account in just posted v2.

Thank you for the review!

Eric
>
> Paolo
>