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

[Philippe Mathieu-Daudé]

On 07/11/2018 12:30 PM, Paolo Bonzini wrote:
> On 11/07/2018 16:59, Stefan Hajnoczi wrote:
>>> +machine->obj.get_device = raspi2_get_device;
>>> +machine->obj.destructor = raspi2_destroy;
>>> +qos_create_sdhci_mm(>sdhci, 0x3f30, &(QSDHCIProperties) {
>>> +.version = 3,
>>> +.baseclock = 52,
>>> +.capab.sdma = false,
>>> +.capab.reg = 0x052134b4
>>> +});
>>> +return >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.
> 
> That doesn't provide enough information yet.  However, PCI devices are
> already discovered automatically and the next step (next year :)) could
> be to use device tree or ACPI to discover embedded devices.
> 
> Using QOM properties instead of duplicating things like QSDHCIProperties
> is a great idea.  Of course the duplication was already there ("old
> sdhci_t structure" in patch 7), so I don't think it should be a blocker,
> but indeed there's a better way to do it.
> 
>> 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 is exactly the opposite problem that we have now: when you write a
> test you are interested typically only in one machine, and the
> "if(x86)elseif(arm)" gets in the way.  (Also: it's also difficult to
> split the ifs across files, i.e. it scales worse; and there's lots of
> duplicated code across tests to do "for" loops of g_test_add, because
> the ifs don't lend itself to "automatic" generation of test cases via
> path walk).
> 
> The advantage is that (just like for OS vs. QEMU) the structure of this
> file matches closely the board files in hw/arm/, which is something you
> should already be familiar with if you're adding a new board to QEMU.

Some projects organize their tests alongside the source to be tested.
This is often messy, but might be clearer to see source testing coverage.
That said, I'd rather keep it out of 'source' tree (as of now). Using a
simple script we can notice the same (which device models matches a test).

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

[Paolo Bonzini]

On 11/07/2018 16:59, Stefan Hajnoczi wrote:
>> +machine->obj.get_device = raspi2_get_device;
>> +machine->obj.destructor = raspi2_destroy;
>> +qos_create_sdhci_mm(>sdhci, 0x3f30, &(QSDHCIProperties) {
>> +.version = 3,
>> +.baseclock = 52,
>> +.capab.sdma = false,
>> +.capab.reg = 0x052134b4
>> +});
>> +return >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.

That doesn't provide enough information yet.  However, PCI devices are
already discovered automatically and the next step (next year :)) could
be to use device tree or ACPI to discover embedded devices.

Using QOM properties instead of duplicating things like QSDHCIProperties
is a great idea.  Of course the duplication was already there ("old
sdhci_t structure" in patch 7), so I don't think it should be a blocker,
but indeed there's a better way to do it.

> 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 is exactly the opposite problem that we have now: when you write a
test you are interested typically only in one machine, and the
"if(x86)elseif(arm)" gets in the way.  (Also: it's also difficult to
split the ifs across files, i.e. it scales worse; and there's lots of
duplicated code across tests to do "for" loops of g_test_add, because
the ifs don't lend itself to "automatic" generation of test cases via
path walk).

The advantage is that (just like for OS vs. QEMU) the structure of this
file matches closely the board files in hw/arm/, which is something you
should already be familiar with if you're adding a new board to QEMU.

Paolo

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 
> ---
>  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 00..47f024076f
> --- /dev/null
> +++ b/tests/libqos/raspi2-machine.c
> @@ -0,0 +1,68 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito 
> 
> + *
> + * 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 
> 
> + */
> +
> +#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 >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(>sdhci, 0x3f30, &(QSDHCIProperties) {
> +.version = 3,
> +.baseclock = 52,
> +.capab.sdma = false,
> +.capab.reg = 0x052134b4
> +});
> +return >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