Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:33AM +0200, Emanuele Giuseppe Esposito wrote:
> Add arm/raspi2 machine to the graph. This machine contains a generic-sdhci, so
> its constructor must take care of setting it properly when called.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <address@hidden>
> ---
>  tests/Makefile.include        |  3 +-
>  tests/libqos/raspi2-machine.c | 68 +++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 tests/libqos/raspi2-machine.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index acbf704a8a..de75a7394e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -770,7 +770,8 @@ 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-obj-y = tests/libqos/qgraph.o
> -libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o
> +libqgraph-pc-obj-y = $(libqos-pc-obj-y) $(libqgraph-obj-y) 
> tests/libqos/sdhci.o
> +libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o
>  
>  check-unit-y += tests/test-qgraph$(EXESUF)
>  tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
> diff --git a/tests/libqos/raspi2-machine.c b/tests/libqos/raspi2-machine.c
> new file mode 100644
> index 0000000000..47f024076f
> --- /dev/null
> +++ b/tests/libqos/raspi2-machine.c
> @@ -0,0 +1,68 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qgraph.h"
> +#include "sdhci.h"
> +
> +typedef struct QRaspi2Machine QRaspi2Machine;
> +
> +struct QRaspi2Machine {
> +    QOSGraphObject obj;
> +    QSDHCI_MemoryMapped sdhci;
> +};
> +
> +static void raspi2_destroy(QOSGraphObject *obj)
> +{
> +    g_free(obj);
> +}
> +
> +static QOSGraphObject *raspi2_get_device(void *obj, const char *device)
> +{
> +    QRaspi2Machine *machine = obj;
> +    if (!g_strcmp0(device, "generic-sdhci")) {
> +        return &machine->sdhci.obj;
> +    }
> +
> +    printf("%s not present in arm/raspi2", device);
> +    abort();
> +}
> +
> +static void *qos_create_machine_arm_raspi2(void)
> +{
> +    QRaspi2Machine *machine = g_new0(QRaspi2Machine, 1);
> +
> +    machine->obj.get_device = raspi2_get_device;
> +    machine->obj.destructor = raspi2_destroy;
> +    qos_create_sdhci_mm(&machine->sdhci, 0x3f300000, &(QSDHCIProperties) {
> +        .version = 3,
> +        .baseclock = 52,
> +        .capab.sdma = false,
> +        .capab.reg = 0x052134b4
> +    });
> +    return &machine->obj;
> +}
> +
> +static void raspi2(void)
> +{
> +    qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
> +    qos_node_contains("arm/raspi2", "generic-sdhci");
> +}
> +
> +libqos_init(raspi2);

Hmm...I didn't realize it would be necessary to manually define each
machine type.  I thought qgraph would use qemu-system-x86_64
-machine/-device/info qtree to introspect QEMU and instantiate the
appropriate drivers.

My concern is that this manual approach of defining machines complicates
qtests.  This file will need to be modified by multiple people - each of
them will be interested in a specific driver and not interested in the
driver that other people have added.

This makes things more complicated where previous qtests simply had an
if (x86) { add_test(test_x86_foo); } else if (arm) {
add_test(test_arm_foo); } in their source file.  It's ugly but it's
obvious.

Stefan

signature.asc
Description: PGP signature