Skip to contentSkip to conversation

tests/device-introspect: Test devices with all machines, not only with "none"

RFC PATCH In review Thomas Huth 4 participants 36 messages Mar 19, 2018 – May 9, 2018

lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 tests/device-introspect: Test devices with all machines, not only with "none"

Many device introspection crashes only happen if you are using a certain machine, e.g.:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)

To be able to catch these problems, let's extend the device-introspect test to check the devices on all machine types. Since this is a rather slow operation, the test is only run in "SPEED=slow" mode.

---
In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.
For Example:

 $ make check-qtest SPEED=slow
  GTESTER check-qtest-aarch64
 RAMBlock "integrator.flash" already registered, abort!
 Broken pipe
 GTester: last random seed: R02S8e52709605790d290d2c8261cefb8b0e
 Unsupported NIC model: lan9118
 Broken pipe
 GTester: last random seed: R02S326d4ea43bfce860ebe2d554192540f7
 qemu-system-aarch64: warning: nic lan9118.0 has no peer
 Unsupported NIC model: smc91c111
 Broken pipe
 GTester: last random seed: R02Se9783b450806f350a14e757b175e3dc4
 qemu-system-aarch64: missing SecureDigital device
 Broken pipe
 GTester: last random seed: R02S5c718b8f4c4fd48a358de8daafcf1b6f
 qemu-system-aarch64: warning: nic lan9118.0 has no peer
 Unexpected error in error_set_from_qdev_prop_error() at hw/core/qdev-properties.c:1095:
 Property 'allwinner-emac.netdev' can't take value 'hub0port0', it's in use
 Broken pipe
 GTester: last random seed: R02S597848ddcfdc76a695a946a9d4e50146
 qemu-system-aarch64: warning: nic ftgmac100.0 has no peer
 GTester: last random seed: R02Seea0f0b769a2161fa53a50479fd68d84
 qemu-system-aarch64: warning: nic imx.fec.0 has no peer
 qemu-system-aarch64: missing SecureDigital device
 Broken pipe
 GTester: last random seed: R02S9c2d3e34427162e7a56aa4ac859f1a6b
 Unsupported NIC model: virtio-net-pci
 Broken pipe
 GTester: last random seed: R02Sd61c0e9ed52d50a17c784213e5c6590c
 Unsupported NIC model: mv88w8618
 Broken pipe
 GTester: last random seed: R02Sbfaecfe58dd643f2faca218e3051d464
 qemu-system-aarch64: warning: nic mv88w8618_eth.0 has no peer
 qemu-system-aarch64: missing SecureDigital device
 Broken pipe
 Unsupported NIC model: xgmac
 Broken pipe
 GTester: last random seed: R02Sc61e65e884e364652c3a0c4190023565
 fsl,imx7: Only 2 CPUs are supported (4 requested)
 Broken pipe
 GTester: last random seed: R02S0cfda43bc17e3e052d5a994b2c96457b
 etc.
 tests/device-introspect-test.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)
tests/device-introspect-test.c +30 -3
index b80058f..a9b9cf7 100644
@@ -105,6 +105,8 @@ static void test_one_device(const char *type)
     QDict *resp;
     char *help, *qom_tree;
 
+    g_debug("Testing device '%s'", type);
+
     resp = qmp("{'execute': 'device-list-properties',"
                " 'arguments': {'typename': %s}}",
                type);
@@ -206,13 +208,13 @@ static void test_device_intro_abstract(void)
     qtest_end();
 }
 
-static void test_device_intro_concrete(void)
+static void test_device_intro_concrete(gconstpointer args)
 {
     QList *types;
     QListEntry *entry;
     const char *type;
 
-    qtest_start(common_args);
+    qtest_start((const char *)args);
     types = device_type_list(false);
 
     QLIST_FOREACH_ENTRY(types, entry) {
@@ -224,6 +226,7 @@ static void test_device_intro_concrete(void)
 
     QDECREF(types);
     qtest_end();
+    g_free((void *)args);
 }
 
 static void test_abstract_interfaces(void)
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
+    g_free(path);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -268,8 +291,12 @@ int main(int argc, char **argv)
     qtest_add_func("device/introspect/list-fields", test_qom_list_fields);
     qtest_add_func("device/introspect/none", test_device_intro_none);
     qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
-    qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
     qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces);
+    qtest_add_data_func("device/introspect/concrete", g_strdup(common_args),
+                        test_device_intro_concrete);
+    if (g_test_slow()) {
+        qtest_cb_for_every_machine(add_machine_test_case);
+    }
 
     return g_test_run();
 }
Signature · 1 line
1.8.3.1
#1 lore raw
Quoted 18 lines · Thomas Huth

Many device introspection crashes only happen if you are using a certain machine, e.g.:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)

To be able to catch these problems, let's extend the device-introspect test to check the devices on all machine types. Since this is a rather slow operation, the test is only run in "SPEED=slow" mode.

Quoted 7 lines

---
In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.

[...]

This looks very useful, thanks!

I wonder if we could have something that would make it simpler for us to cover more command-line combinations + QMP commands in simple "validate output and check if QEMU won't crash" test cases without writing extra C or Python code every time.

device-crash-test could be used for that, but I'd like to make it simpler to extend.

Signature · 1 line
Eduardo
#2 lore raw
In reply to Thomas Huth
Quoted 14 lines · Thomas Huth

Many device introspection crashes only happen if you are using a certain machine, e.g.:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)
Quoted 3 lines

To be able to catch these problems, let's extend the device-introspect test to check the devices on all machine types. Since this is a rather slow operation, the test is only run in "SPEED=slow" mode.

If the device works with one machine type, it has a decent chance to work with others, too. Thus, testing each device with every machine type is overkill. I appreciate having overkill as an option :)

What I'd like to see for a quick "make check" is testing each device once. That should flush out most bugs.

Quoted 7 lines

---
In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.

Yes, that's a common class of bugs. There's little guidance on what kind of work belongs where, and plenty of bad examples. Some of the bad examples crash (as you found). Some work fine, typically because the device doesn't support unplug.

Bad examples breed more bad code, so we better fix them all. The ones that work are harder to find...

#3 lore raw
Quoted 11 lines · Markus Armbruster
Quoted 5 lines · Thomas Huth

In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.

Yes, that's a common class of bugs. There's little guidance on what kind of work belongs where, and plenty of bad examples. Some of the bad examples crash (as you found). Some work fine, typically because the device doesn't support unplug.

I've been vaguely wondering if we should start to recommend that all devices have a correctly implemented code path for destroying them post-realize, even if they don't actually implement hotplug...

thanks
-- PMM

#4 lore raw
Quoted 8 lines · Peter Maydell
Quoted 7 lines · Markus Armbruster
Quoted 5 lines · Thomas Huth

In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.

Quoted 8 lines
Quoted 4 lines

Yes, that's a common class of bugs. There's little guidance on what kind of work belongs where, and plenty of bad examples. Some of the bad examples crash (as you found). Some work fine, typically because the device doesn't support unplug.

I've been vaguely wondering if we should start to recommend that all devices have a correctly implemented code path for destroying them post-realize, even if they don't actually implement hotplug...

Possibly crazy idea: make devices implement *cold* unplug. Not really useful in practice, but it would make the destroy path testable.

#5 lore raw
In reply to Markus Armbruster
Quoted 20 lines · Markus Armbruster
Quoted 14 lines · Thomas Huth

Many device introspection crashes only happen if you are using a certain machine, e.g.:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)
Quoted 3 lines

To be able to catch these problems, let's extend the device-introspect test to check the devices on all machine types. Since this is a rather slow operation, the test is only run in "SPEED=slow" mode.

Quoted 6 lines

If the device works with one machine type, it has a decent chance to work with others, too. Thus, testing each device with every machine type is overkill. I appreciate having overkill as an option :)

What I'd like to see for a quick "make check" is testing each device once. That should flush out most bugs.

That's already done with the "none" machine.

Anyway, do you think my patch here is useful and has a chance of getting included? I.e. shall I re-spin this as a non-RFC patch? Or shall we rather wait for Eduardo's python-based tests to get included into the repository?

Quoted 10 lines
Quoted 7 lines

---
In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.

Yes, that's a common class of bugs. There's little guidance on what kind of work belongs where, and plenty of bad examples.

I think we urgently need a file in doc/devel/ that describes the various states / functions of a device, where we should properly describe the differences between instance_init and realize. ... I'll try to come up with something when I've got some spare time (unless somebody else volunteers to do that first).

Thomas

#6 lore raw
Quoted 21 lines · Thomas Huth
Quoted 20 lines · Markus Armbruster
Quoted 17 lines · Thomas Huth

Many device introspection crashes only happen if you are using a certain machine, e.g.:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)

To be able to catch these problems, let's extend the device-introspect test to check the devices on all machine types. Since this is a rather slow operation, the test is only run in "SPEED=slow" mode.

Quoted 8 lines
Quoted 6 lines

If the device works with one machine type, it has a decent chance to work with others, too. Thus, testing each device with every machine type is overkill. I appreciate having overkill as an option :)

What I'd like to see for a quick "make check" is testing each device once. That should flush out most bugs.

That's already done with the "none" machine.

I was too terse. We test each device with -machine none for every target. Fine if that's quick enough. If not, we might want to reduce redundancy there.

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

Quoted 4 lines

Anyway, do you think my patch here is useful and has a chance of getting included? I.e. shall I re-spin this as a non-RFC patch? Or shall we rather wait for Eduardo's python-based tests to get included into the repository?

I don't mind having make check SPEED=slow run more extensive tests. Assuming we actually run them at least once in a while, which seems doubtful.

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

---
In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.

Quoted 8 lines
Quoted 2 lines

Yes, that's a common class of bugs. There's little guidance on what kind of work belongs where, and plenty of bad examples.

I think we urgently need a file in doc/devel/ that describes the various states / functions of a device, where we should properly describe the differences between instance_init and realize. ... I'll try to come up with something when I've got some spare time (unless somebody else volunteers to do that first).

Please do.

Widen the scope from just TYPE_DEVICE to all of QOM?

#7 lore raw
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 2 lines

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

I guess we could also change the logic in qom-tester to only run with all machines if we're in SPEED=slow mode, and rather only use the "none" machine by default?

Quoted 8 lines
Quoted 4 lines

Anyway, do you think my patch here is useful and has a chance of getting included? I.e. shall I re-spin this as a non-RFC patch? Or shall we rather wait for Eduardo's python-based tests to get included into the repository?

I don't mind having make check SPEED=slow run more extensive tests. Assuming we actually run them at least once in a while, which seems doubtful.

If some developers (like myself) are running it at least every couple of weeks manually, that's already much better than nothing!

Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

---
In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.

Quoted 8 lines
Quoted 2 lines

Yes, that's a common class of bugs. There's little guidance on what kind of work belongs where, and plenty of bad examples.

I think we urgently need a file in doc/devel/ that describes the various states / functions of a device, where we should properly describe the differences between instance_init and realize. ... I'll try to come up with something when I've got some spare time (unless somebody else volunteers to do that first).

Quoted 3 lines

Please do.

Widen the scope from just TYPE_DEVICE to all of QOM?

I don't have that much experience with QOM yet that I'd dare to write a doc about it. Would you maybe be interested in writing something up about QOM?

Thomas

#8 lore raw
In reply to Markus Armbruster
Quoted 39 lines · Markus Armbruster
Quoted 21 lines · Thomas Huth
Quoted 20 lines · Markus Armbruster
Quoted 17 lines · Thomas Huth

Many device introspection crashes only happen if you are using a certain machine, e.g.:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)

To be able to catch these problems, let's extend the device-introspect test to check the devices on all machine types. Since this is a rather slow operation, the test is only run in "SPEED=slow" mode.

Quoted 8 lines
Quoted 6 lines

If the device works with one machine type, it has a decent chance to work with others, too. Thus, testing each device with every machine type is overkill. I appreciate having overkill as an option :)

What I'd like to see for a quick "make check" is testing each device once. That should flush out most bugs.

That's already done with the "none" machine.

I was too terse. We test each device with -machine none for every target. Fine if that's quick enough. If not, we might want to reduce redundancy there.

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

Quoted 8 lines
Quoted 4 lines

Anyway, do you think my patch here is useful and has a chance of getting included? I.e. shall I re-spin this as a non-RFC patch? Or shall we rather wait for Eduardo's python-based tests to get included into the repository?

I don't mind having make check SPEED=slow run more extensive tests. Assuming we actually run them at least once in a while, which seems doubtful.

The infrastructure for Python-based tests might take a while to be included, as I'm busy with other stuff right now. I wouldn't mind including this patch, as long as you don't mind seeing it deleted after we reimplement it in Python.

Signature · 1 line
Eduardo
#9 lore raw
Quoted 45 lines · Eduardo Habkost
Quoted 44 lines · Markus Armbruster
Quoted 21 lines · Thomas Huth
Quoted 20 lines · Markus Armbruster
Quoted 17 lines · Thomas Huth

Many device introspection crashes only happen if you are using a certain machine, e.g.:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)

To be able to catch these problems, let's extend the device-introspect test to check the devices on all machine types. Since this is a rather slow operation, the test is only run in "SPEED=slow" mode.

Quoted 8 lines
Quoted 6 lines

If the device works with one machine type, it has a decent chance to work with others, too. Thus, testing each device with every machine type is overkill. I appreciate having overkill as an option :)

What I'd like to see for a quick "make check" is testing each device once. That should flush out most bugs.

That's already done with the "none" machine.

I was too terse. We test each device with -machine none for every target. Fine if that's quick enough. If not, we might want to reduce redundancy there.

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

Quoted 4 lines

Anyway, do you think my patch here is useful and has a chance of getting included? I.e. shall I re-spin this as a non-RFC patch? Or shall we rather wait for Eduardo's python-based tests to get included into the repository?

Quoted 8 lines
Quoted 3 lines

I don't mind having make check SPEED=slow run more extensive tests. Assuming we actually run them at least once in a while, which seems doubtful.

The infrastructure for Python-based tests might take a while to be included, as I'm busy with other stuff right now. I wouldn't mind including this patch, as long as you don't mind seeing it deleted after we reimplement it in Python.

Fine for me.

Thomas

#10 lore raw

[Qemu-arm] [Qemu-devel] qom-test (was: [RFC PATCH] tests/device-introspect: Test devices with all machines, not only with "none")

In reply to Markus Armbruster

On 26.04.2018 13:54, Markus Armbruster wrote:
[...]

Quoted 2 lines

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

Shall we change qom-test to also only test with the "none" machine in the normal "make check" mode and only do the full test with all machines in "make check SPEED=slow" ?

Thomas

#11 lore raw

Re: [Qemu-arm] [Qemu-devel] qom-test

Quoted 8 lines · Thomas Huth

On 26.04.2018 13:54, Markus Armbruster wrote:
[...]

Quoted 2 lines

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

Shall we change qom-test to also only test with the "none" machine in the normal "make check" mode and only do the full test with all machines in "make check SPEED=slow" ?

I suggest you give it a try and see how much time it saves on "make check" without SPEED=slow. If you like the result, find out which devices this no longer tests, if any.

#12 lore raw

Re: [Qemu-arm] [Qemu-devel] qom-test (was: [RFC PATCH] tests/device-introspect: Test devices with all machines, not only with "none")

In reply to Thomas Huth
Quoted 8 lines · Thomas Huth

On 26.04.2018 13:54, Markus Armbruster wrote:
[...]

Quoted 2 lines

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

Shall we change qom-test to also only test with the "none" machine in the normal "make check" mode and only do the full test with all machines in "make check SPEED=slow" ?

We definitely want something that tries to instantiate every machine, because that does catch bugs.

thanks
-- PMM

#13 lore raw

Re: [Qemu-devel] [Qemu-arm] qom-test

Quoted 6 lines · Peter Maydell
Quoted 5 lines · Thomas Huth

On 26.04.2018 13:54, Markus Armbruster wrote:
[...]

Quoted 2 lines

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

Quoted 6 lines
Quoted 3 lines

Shall we change qom-test to also only test with the "none" machine in the normal "make check" mode and only do the full test with all machines in "make check SPEED=slow" ?

We definitely want something that tries to instantiate every machine, because that does catch bugs.

Yes, after having a closer look at this one, I also think that we should *not* change it to run with "none" by default only. The 'qom-list' command results in quite a different output depending on which machine you run it on.

Thomas

#14 lore raw

Re: [Qemu-arm] [Qemu-devel] qom-test

Quoted 6 lines · Thomas Huth
Quoted 5 lines · Peter Maydell
Quoted 4 lines · Thomas Huth

On 26.04.2018 13:54, Markus Armbruster wrote:
[...]

Quoted 2 lines

Actually, a worse offender in the "waste everybody's time via redunancy" department could be qom-test.

Supporting numbers:

$ time for i in *-softmmu/qemu-system-*; do [ -x $i ] || continue; QTEST_QEMU_BINARY=$i QTEST_QEMU_IMG=qemu-img MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} gtester -k --verbose -m=quick  tests/qom-test ; done
[...]
real	3m27.427s
user	2m7.141s
sys	1m44.354s

aarch64, arm, i386, x86_64 each take more than 30s.

For each target, we walk /machine and qom-get every property. The test passes if qom-get doesn't crash, the values we get don't matter.

For x86_64 alone, qom-test executes qom-get more than 45,000 times to test almost 9,500 objects. It gets the properties of more than 5000 qemu:memory-region objects, more than 2500 irq objects, almost 300 smbus-eeprom objects, 110 IDE objects, ... It's nice we can test qom-get doesn't crash on any of IDE's properties in 110 very slight variations. But most of the time, one of the variations would be enough.

Quoted 11 lines
Quoted 6 lines
Quoted 3 lines

Shall we change qom-test to also only test with the "none" machine in the normal "make check" mode and only do the full test with all machines in "make check SPEED=slow" ?

We definitely want something that tries to instantiate every machine, because that does catch bugs.

Yes, after having a closer look at this one, I also think that we should *not* change it to run with "none" by default only. The 'qom-list' command results in quite a different output depending on which machine you run it on.

Only running "none" is too naive.

#15 lore raw

Re: [Qemu-arm] [Qemu-ppc] [Qemu-devel] qom-test

Quoted 4 lines · Markus Armbruster
Quoted 2 lines · Thomas Huth
Quoted 1 line · Peter Maydell

On 27 April 2018 at 07:06, Thomas Huth <[email protected]> wrote:

[...]

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

Shall we change qom-test to also only test with the "none" machine in the normal "make check" mode and only do the full test with all machines in "make check SPEED=slow" ?

We definitely want something that tries to instantiate every machine, because that does catch bugs.

Quoted 6 lines
Quoted 4 lines

Yes, after having a closer look at this one, I also think that we should *not* change it to run with "none" by default only. The 'qom-list' command results in quite a different output depending on which machine you run it on.

Only running "none" is too naive.

For the targets that have "versioned" machine types, I think we could skip all the older machine versions, so that we only test with pc-i440fx-2.12 but not with pc-i440fx-2.11 and older anymore. That would need some more or less clever algorithm to detect the latest version, though.

Thomas

#16 lore raw
In reply to Thomas Huth
Quoted 86 lines · Thomas Huth

Many device introspection crashes only happen if you are using a certain machine, e.g.:

$ ppc-softmmu/qemu-system-ppc -S -M ref405ep,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{ 'execute': 'qmp_capabilities' }
{"return": {}}
{ 'execute': 'device-list-properties',
  'arguments': {'typename': 'macio-newworld'}}
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
Device 'serial0' is in use
Aborted (core dumped)

To be able to catch these problems, let's extend the device-introspect test to check the devices on all machine types. Since this is a rather slow operation, the test is only run in "SPEED=slow" mode.

---
In case someone wants to help with creating some bug fix patches during the QEMU hard freeze phase: This test can now be used to trigger lots of introspection bugs that we were not aware of yet. I think most of the bugs are due to wrong handling of instance_init vs. realize functions.
For Example:

 $ make check-qtest SPEED=slow
  GTESTER check-qtest-aarch64
 RAMBlock "integrator.flash" already registered, abort!
 Broken pipe
 GTester: last random seed: R02S8e52709605790d290d2c8261cefb8b0e
 Unsupported NIC model: lan9118
 Broken pipe
 GTester: last random seed: R02S326d4ea43bfce860ebe2d554192540f7
 qemu-system-aarch64: warning: nic lan9118.0 has no peer
 Unsupported NIC model: smc91c111
 Broken pipe
 GTester: last random seed: R02Se9783b450806f350a14e757b175e3dc4
 qemu-system-aarch64: missing SecureDigital device
 Broken pipe
 GTester: last random seed: R02S5c718b8f4c4fd48a358de8daafcf1b6f
 qemu-system-aarch64: warning: nic lan9118.0 has no peer
 Unexpected error in error_set_from_qdev_prop_error() at hw/core/qdev-properties.c:1095:
 Property 'allwinner-emac.netdev' can't take value 'hub0port0', it's in use
 Broken pipe
 GTester: last random seed: R02S597848ddcfdc76a695a946a9d4e50146
 qemu-system-aarch64: warning: nic ftgmac100.0 has no peer
 GTester: last random seed: R02Seea0f0b769a2161fa53a50479fd68d84
 qemu-system-aarch64: warning: nic imx.fec.0 has no peer
 qemu-system-aarch64: missing SecureDigital device
 Broken pipe
 GTester: last random seed: R02S9c2d3e34427162e7a56aa4ac859f1a6b
 Unsupported NIC model: virtio-net-pci
 Broken pipe
 GTester: last random seed: R02Sd61c0e9ed52d50a17c784213e5c6590c
 Unsupported NIC model: mv88w8618
 Broken pipe
 GTester: last random seed: R02Sbfaecfe58dd643f2faca218e3051d464
 qemu-system-aarch64: warning: nic mv88w8618_eth.0 has no peer
 qemu-system-aarch64: missing SecureDigital device
 Broken pipe
 Unsupported NIC model: xgmac
 Broken pipe
 GTester: last random seed: R02Sc61e65e884e364652c3a0c4190023565
 fsl,imx7: Only 2 CPUs are supported (4 requested)
 Broken pipe
 GTester: last random seed: R02S0cfda43bc17e3e052d5a994b2c96457b
 etc.
 tests/device-introspect-test.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)
tests/device-introspect-test.c +2 -0
index b80058f..a9b9cf7 100644
@@ -105,6 +105,8 @@ static void test_one_device(const char *type)
     QDict *resp;
     char *help, *qom_tree;
 
+    g_debug("Testing device '%s'", type);
+
     resp = qmp("{'execute': 'device-list-properties',"
                " 'arguments': {'typename': %s}}",
                type);

tests/device-introspect-test.c static void test_device_intro_abstract(void) · line 211

Quoted 5 lines
tests/device-introspect-test.c +1 -1
@@ -206,13 +208,13 @@ static void test_device_intro_abstract(void)
     qtest_end();
 }
 
-static void test_device_intro_concrete(void)
+static void test_device_intro_concrete(gconstpointer args)

const void *, please, because that's what qtest_add_data_func() takes.

Quoted 24 lines
tests/device-introspect-test.c +4 -1
 {
     QList *types;
     QListEntry *entry;
     const char *type;
 
-    qtest_start(common_args);
+    qtest_start((const char *)args);
     types = device_type_list(false);
 
     QLIST_FOREACH_ENTRY(types, entry) {
@@ -224,6 +226,7 @@ static void test_device_intro_concrete(void)
 
     QDECREF(types);
     qtest_end();
+    g_free((void *)args);
 }
 
 static void test_abstract_interfaces(void)
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

Please mention "with and without -nodefaults" in the commit message.

I'd try "with -nodefaults" before "without", because "with" is the simpler test case.

Quoted 7 lines
tests/device-introspect-test.c +3 -0
+    g_free(path);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);

tests/device-introspect-test.c int main(int argc, char **argv) · line 296

Quoted 7 lines
tests/device-introspect-test.c +2 -1
@@ -268,8 +291,12 @@ int main(int argc, char **argv)
     qtest_add_func("device/introspect/list-fields", test_qom_list_fields);
     qtest_add_func("device/introspect/none", test_device_intro_none);
     qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
-    qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
     qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces);
+    qtest_add_data_func("device/introspect/concrete", g_strdup(common_args),
+                        test_device_intro_concrete);

This runs test_device_intro_concrete() with "-nodefaults -machine none". Duplicate in SPEED=slow mode?

Quoted 6 lines
tests/device-introspect-test.c +3 -0
+    if (g_test_slow()) {
+        qtest_cb_for_every_machine(add_machine_test_case);
+    }
 
     return g_test_run();
 }
#17 lore raw
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
tests/device-introspect-test.c +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

So we'd either need a rather big black list for the machines that do not work, or simply drop the "-nodefaults" tests from this patch.

Quoted 4 lines

Please mention "with and without -nodefaults" in the commit message.

I'd try "with -nodefaults" before "without", because "with" is the simpler test case.

For most boards, it seems rather to be the more "difficult" setting since most boards are only tested without "-nodefaults" obviously.

Quoted 9 lines
Quoted 9 lines
tests/device-introspect-test.c +3 -0
+    g_free(path);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -268,8 +291,12 @@ int main(int argc, char **argv)
     qtest_add_func("device/introspect/list-fields", test_qom_list_fields);
     qtest_add_func("device/introspect/none", test_device_intro_none);
Quoted 8 lines

tests/device-introspect-test.c int main(int argc, char **argv) · line 296

Quoted 5 lines
tests/device-introspect-test.c +2 -1
     qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
-    qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
     qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces);
+    qtest_add_data_func("device/introspect/concrete", g_strdup(common_args),
+                        test_device_intro_concrete);

This runs test_device_intro_concrete() with "-nodefaults -machine none". Duplicate in SPEED=slow mode?

Yes, it's a duplicate, we should skip that in SPEED=slow mode.

Thomas

#18 lore raw
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
tests/device-introspect-test.c +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

So we'd either need a rather big black list for the machines that do not work, or simply drop the "-nodefaults" tests from this patch.

Or we could try to test all machines anyway, but not consider it an error if QEMU just does exit(1). Can the qtest C API give us that information?

(Or we could simply let -nodefaults aside by now, and do this after we implement this test case in Python.)

Signature · 1 line
Eduardo
#19 lore raw
Quoted 43 lines · Eduardo Habkost
Quoted 42 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
tests/device-introspect-test.c +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device
Quoted 6 lines
Quoted 2 lines

So we'd either need a rather big black list for the machines that do not work, or simply drop the "-nodefaults" tests from this patch.

Or we could try to test all machines anyway, but not consider it an error if QEMU just does exit(1). Can the qtest C API give us that information?

At a first glance, I haven't seen an easy way to do this. I guess we could do some polling with waitid() or do something with SIGCHLD, but...

Quoted 2 lines

(Or we could simply let -nodefaults aside by now, and do this after we implement this test case in Python.)

... I'd rather prefer that for now, assuming that the test will later get replaced by the python test anyway.

Thomas

#20 lore raw
In reply to Eduardo Habkost
Quoted 35 lines · Eduardo Habkost
Quoted 34 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 30 lines
Quoted 6 lines
tests/device-introspect-test.c +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

Quoted 7 lines
Quoted 7 lines

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 9 lines
Quoted 2 lines

So we'd either need a rather big black list for the machines that do not work, or simply drop the "-nodefaults" tests from this patch.

Or we could try to test all machines anyway, but not consider it an error if QEMU just does exit(1). Can the qtest C API give us that information?

(Or we could simply let -nodefaults aside by now, and do this after we implement this test case in Python.)

Or we could fix the bugs.

#21 lore raw
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
tests/device-introspect-test.c +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Even if we fix all the issues, there is still another cosmetic problem:
Since there are no entries in nd_table[], all the boards with embedded NICs start to spill out "warning: nic XYZ has no peer". Should we simply suppress that warning in qtest mode?

Thomas

#22 lore raw
Quoted 46 lines · Thomas Huth
Quoted 45 lines · Markus Armbruster
Quoted 42 lines · Eduardo Habkost
Quoted 41 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
tests/device-introspect-test.c +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device
Quoted 7 lines
Quoted 2 lines

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Even if we fix all the issues, there is still another cosmetic problem:
Since there are no entries in nd_table[], all the boards with embedded NICs start to spill out "warning: nic XYZ has no peer". Should we simply suppress that warning in qtest mode?

Makes sense to me.

#23 lore raw
In reply to Markus Armbruster
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
tests/device-introspect-test.c +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

Signature · 1 line
Eduardo
#24 lore raw
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
tests/device-introspect-test.c +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
tests/device-introspect-test.c +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

tests/device-introspect-test.c static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
tests/device-introspect-test.c +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Does that make some sense?

#25 lore raw
Quoted 61 lines · Markus Armbruster
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
(quoted diff) +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

Review static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
(quoted diff) +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

Review static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
(quoted diff) +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

I propose:

Quoted 8 lines

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Does that make some sense?

Makes sense. On a real machine, you could likely also remove the SD card and load a kernel by other means, e.g. with a JTAG debug connector. So it makes sense that you could also start the machine in QEMU without SD card and load a kernel e.g. with the gdb stub instead.

Thomas

#26 lore raw
In reply to Markus Armbruster
Quoted 10 lines · Markus Armbruster

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Does that make some sense?

We might also want
* Stuff that you can't add back in with an appropriate command line argument is not suppressed

though we might optimistically hope that the category is empty :-)

thanks
-- PMM

#27 lore raw
In reply to Markus Armbruster
Quoted 59 lines · Markus Armbruster
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
(quoted diff) +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

Review static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
(quoted diff) +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

Review static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
(quoted diff) +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

Quoted 8 lines

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Makes sense to me. It looks like the only obstacle for tests/device-introspect and device-crash-test is the first rule. "Guest won't boot" isn't a problem, but "QEMU won't run" is.

The first rule is easily testable, too: running "$QEMU -machine $MACHINE -nodefaults" and not having a working QMP monitor should be reported as a bug by automated tests.

Do we have an up-to-date list of machines that break this rule? We can add this to
<https://wiki.qemu.org/Contribute/BiteSizedTasks>.

Signature · 1 line
Eduardo
#28 lore raw
Quoted 69 lines · Eduardo Habkost
Quoted 59 lines · Markus Armbruster
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
(quoted diff) +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

Review static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
(quoted diff) +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

Review static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
(quoted diff) +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

Quoted 8 lines

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Quoted 7 lines

Makes sense to me. It looks like the only obstacle for tests/device-introspect and device-crash-test is the first rule. "Guest won't boot" isn't a problem, but "QEMU won't run" is.

The first rule is easily testable, too: running "$QEMU -machine $MACHINE -nodefaults" and not having a working QMP monitor should be reported as a bug by automated tests.

You mean with "-accel qtest" or without? With "-accel qtest" we should pretty soon be fine, after Peter's current PULL request has been merged (which contains a patch from me for fixing these SD card problems with ARM machines).
Without "-accel qtest", things are not that easy, unfortunately. Lots of boards require "-kernel" or "-bios" and refuse to work without. So you can hardly test "-nodefaults" automatically in the normal tcg mode. (But maybe all boards should allow to start QEMU in case you've at least also specified "-S" ? ... in that case we've got plenty of work for BiteSizeTasks ;-) )

Thomas

#29 lore raw
Quoted 78 lines · Thomas Huth
Quoted 69 lines · Eduardo Habkost
Quoted 59 lines · Markus Armbruster
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
(quoted diff) +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

Review static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
(quoted diff) +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

Review static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
(quoted diff) +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

Quoted 8 lines

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Quoted 7 lines

Makes sense to me. It looks like the only obstacle for tests/device-introspect and device-crash-test is the first rule. "Guest won't boot" isn't a problem, but "QEMU won't run" is.

The first rule is easily testable, too: running "$QEMU -machine $MACHINE -nodefaults" and not having a working QMP monitor should be reported as a bug by automated tests.

Quoted 10 lines

You mean with "-accel qtest" or without? With "-accel qtest" we should pretty soon be fine, after Peter's current PULL request has been merged (which contains a patch from me for fixing these SD card problems with ARM machines).
Without "-accel qtest", things are not that easy, unfortunately. Lots of boards require "-kernel" or "-bios" and refuse to work without. So you can hardly test "-nodefaults" automatically in the normal tcg mode. (But maybe all boards should allow to start QEMU in case you've at least also specified "-S" ? ... in that case we've got plenty of work for BiteSizeTasks ;-) )

Hmm, maybe it's not a bite-sized task after all. :)

Should we do this gradually?

* Working with -accel qtest is useful, and sounds like an easier goal;
* working with -S seems desirable too;
* working without -S (even if the emulated CPU crashes and burns) would be interesting.

Related question: what are the use cases where we require "-accel qtest" and "-S" wouldn't work?

Are the requirements and goals of "-accel qtest" documented somewhere? Without documentation, it's hard to say when a given qtest_enabled() call in the code is reasonable, or a hack we want to get rid of.

Signature · 1 line
Eduardo
#30 lore raw

Re: [Qemu-arm] [Qemu-devel] Running QEMU without default devices / kernel / bios (was: Test devices with all machines, not only with "none")

Quoted 90 lines · Eduardo Habkost
Quoted 78 lines · Thomas Huth
Quoted 69 lines · Eduardo Habkost
Quoted 59 lines · Markus Armbruster
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
(quoted diff) +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

Review static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
(quoted diff) +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

Review static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
(quoted diff) +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

Quoted 8 lines

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Quoted 7 lines

Makes sense to me. It looks like the only obstacle for tests/device-introspect and device-crash-test is the first rule. "Guest won't boot" isn't a problem, but "QEMU won't run" is.

The first rule is easily testable, too: running "$QEMU -machine $MACHINE -nodefaults" and not having a working QMP monitor should be reported as a bug by automated tests.

Quoted 10 lines

You mean with "-accel qtest" or without? With "-accel qtest" we should pretty soon be fine, after Peter's current PULL request has been merged (which contains a patch from me for fixing these SD card problems with ARM machines).
Without "-accel qtest", things are not that easy, unfortunately. Lots of boards require "-kernel" or "-bios" and refuse to work without. So you can hardly test "-nodefaults" automatically in the normal tcg mode. (But maybe all boards should allow to start QEMU in case you've at least also specified "-S" ? ... in that case we've got plenty of work for BiteSizeTasks ;-) )

Quoted 5 lines

Hmm, maybe it's not a bite-sized task after all. :)

Should we do this gradually?

* Working with -accel qtest is useful, and sounds like an easier goal;

We're pretty much there already. Apart from the SD card problem (and the xen boards), all machines should work with -nodefaults in qtest mode now.

Quoted 1 line

* working with -S seems desirable too;

Yes, it could be interesting to load the firmware / OS via HMP or GDB after QEMU has been started.

Maybe we'd simply need a new function a la:

bool cpu_starts_automatically()

{
    return autostart && !qtest_enabled();
}

And then replace all spots where we exit due to missing -kernel or -bios parameters, e.g.:

hw/m68k/mcf5208.c +1 -1
@@ -286,7 +286,7 @@ static void mcf5208evb_init(MachineState *machine)
 
     /* Load kernel.  */
     if (!kernel_filename) {
-        if (qtest_enabled()) {
+        if (!cpu_starts_automatically()) {
             return;
         }
         error_report("Kernel image must be specified");

Does that sound like a plan?

Quoted 2 lines

* working without -S (even if the emulated CPU crashes and burns) would be interesting.

Not sure whether we really need this. It's likely better to give the user a proper error message to use "-kernel" instead of just showing a crash.

Quoted 2 lines

Related question: what are the use cases where we require "-accel qtest" and "-S" wouldn't work?

Maybe there are some boards where you can not load code via HMP or GDB once you've started QEMU with "-S"? You'd end up with a mostly useless HMP prompt in that case, which is a little bit ugly, but not fatal. Apart from that ... I can't think of a case where "-S" would not work at all once we've introduce something like cpu_starts_automatically().

Quoted 4 lines

Are the requirements and goals of "-accel qtest" documented somewhere? Without documentation, it's hard to say when a given qtest_enabled() call in the code is reasonable, or a hack we want to get rid of.

There's some few documentation in docs/devel/testing.rst and
https://wiki.qemu.org/Features/QTest - but the quintessence is that qtest basically rips out the CPU of the emulated machine and replaces it with the qtest stub. So in this case there is no CPU that can run code automatically. Thus I think if we abstract this with a function a la cpu_starts_automatically(), we should be fine.

Thomas

#31 lore raw

Re: [Qemu-devel] Running QEMU without default devices / kernel / bios

Quoted 91 lines · Thomas Huth
Quoted 90 lines · Eduardo Habkost
Quoted 88 lines · Thomas Huth
Quoted 69 lines · Eduardo Habkost
Quoted 59 lines · Markus Armbruster
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
(quoted diff) +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

Review static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
(quoted diff) +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

Review static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
(quoted diff) +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

Quoted 8 lines

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Quoted 7 lines

Makes sense to me. It looks like the only obstacle for tests/device-introspect and device-crash-test is the first rule. "Guest won't boot" isn't a problem, but "QEMU won't run" is.

The first rule is easily testable, too: running "$QEMU -machine $MACHINE -nodefaults" and not having a working QMP monitor should be reported as a bug by automated tests.

You mean with "-accel qtest" or without? With "-accel qtest" we should pretty soon be fine, after Peter's current PULL request has been merged (which contains a patch from me for fixing these SD card problems with ARM machines).
Without "-accel qtest", things are not that easy, unfortunately. Lots of boards require "-kernel" or "-bios" and refuse to work without. So you can hardly test "-nodefaults" automatically in the normal tcg mode. (But maybe all boards should allow to start QEMU in case you've at least also specified "-S" ? ... in that case we've got plenty of work for BiteSizeTasks ;-) )

Quoted 8 lines
Quoted 5 lines

Hmm, maybe it's not a bite-sized task after all. :)

Should we do this gradually?

* Working with -accel qtest is useful, and sounds like an easier goal;

We're pretty much there already. Apart from the SD card problem (and the xen boards), all machines should work with -nodefaults in qtest mode now.

I forgot to mention that some sparc machines also still have problems. Patches are available here:

https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00543.html

and

https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00546.html

... but they have not been merged yet.

Thomas

#32 lore raw

Re: [Qemu-arm] [Qemu-devel] Running QEMU without default devices / kernel / bios (was: Test devices with all machines, not only with "none")

In reply to Thomas Huth
Quoted 128 lines · Thomas Huth
Quoted 90 lines · Eduardo Habkost
Quoted 78 lines · Thomas Huth
Quoted 69 lines · Eduardo Habkost
Quoted 59 lines · Markus Armbruster
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
(quoted diff) +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

Review static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
(quoted diff) +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

Review static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
(quoted diff) +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

Quoted 8 lines

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Quoted 7 lines

Makes sense to me. It looks like the only obstacle for tests/device-introspect and device-crash-test is the first rule. "Guest won't boot" isn't a problem, but "QEMU won't run" is.

The first rule is easily testable, too: running "$QEMU -machine $MACHINE -nodefaults" and not having a working QMP monitor should be reported as a bug by automated tests.

Quoted 10 lines

You mean with "-accel qtest" or without? With "-accel qtest" we should pretty soon be fine, after Peter's current PULL request has been merged (which contains a patch from me for fixing these SD card problems with ARM machines).
Without "-accel qtest", things are not that easy, unfortunately. Lots of boards require "-kernel" or "-bios" and refuse to work without. So you can hardly test "-nodefaults" automatically in the normal tcg mode. (But maybe all boards should allow to start QEMU in case you've at least also specified "-S" ? ... in that case we've got plenty of work for BiteSizeTasks ;-) )

Quoted 5 lines

Hmm, maybe it's not a bite-sized task after all. :)

Should we do this gradually?

* Working with -accel qtest is useful, and sounds like an easier goal;

We're pretty much there already. Apart from the SD card problem (and the xen boards), all machines should work with -nodefaults in qtest mode now.

Quoted 1 line

* working with -S seems desirable too;

Yes, it could be interesting to load the firmware / OS via HMP or GDB after QEMU has been started.

Maybe we'd simply need a new function a la:

bool cpu_starts_automatically()

{
    return autostart && !qtest_enabled();
}

And then replace all spots where we exit due to missing -kernel or -bios parameters, e.g.:

hw/m68k/mcf5208.c +1 -1
@@ -286,7 +286,7 @@ static void mcf5208evb_init(MachineState *machine)
 
     /* Load kernel.  */
     if (!kernel_filename) {
-        if (qtest_enabled()) {
+        if (!cpu_starts_automatically()) {
             return;
         }
         error_report("Kernel image must be specified");
Quoted 1 line

Does that sound like a plan?

Not sure. If a given command-line fails without -S, I would expect it to also fail if using -S and the "cont" monitor command is issued. (But not necessarily if "-S" is used and "cont" is never issued.)

Quoted 7 lines
Quoted 3 lines

* working without -S (even if the emulated CPU crashes and burns) would be interesting.

Not sure whether we really need this. It's likely better to give the user a proper error message to use "-kernel" instead of just showing a crash.

I think I agree.

Quoted 7 lines
Quoted 3 lines

Related question: what are the use cases where we require "-accel qtest" and "-S" wouldn't work?

Maybe there are some boards where you can not load code via HMP or GDB once you've started QEMU with "-S"? You'd end up with a mostly useless HMP prompt in that case, which is a little bit ugly, but not fatal.

You have a point. I guess the definition of "useless" here depend on what are the use cases we want to address with -S: are there reasonable use cases for using -S and never issuing "cont"?

Would it be OK if we reported errors like "kernel image must be specified" only when/if "cont" is issued?

Quoted 2 lines

Apart from that ... I can't think of a case where "-S" would not work at all once we've introduce something like cpu_starts_automatically().

I'm being convinced that "-accel qtest" and "-S" are not expected to be equivalent, so my main priority right now is to document what are the differences.

Quoted 6 lines
Quoted 5 lines

Are the requirements and goals of "-accel qtest" documented somewhere? Without documentation, it's hard to say when a given qtest_enabled() call in the code is reasonable, or a hack we want to get rid of.

Quoted 6 lines

There's some few documentation in docs/devel/testing.rst and
https://wiki.qemu.org/Features/QTest - but the quintessence is that qtest basically rips out the CPU of the emulated machine and replaces it with the qtest stub. So in this case there is no CPU that can run code automatically. Thus I think if we abstract this with a function a la cpu_starts_automatically(), we should be fine.

If ripping out the CPU is the only purpose of "-accel qtest", then it sounds equivalent to -S. But I have the impression that this is not its only purpose.

I'm reaching two conclusions from this thread:

1) "-accel qtest" has additional purposes other than the "don't run any guest code". We need to document them clearly, and it probably can't be replaced by -S directly.

2) It would be nice if we allowed the "cont" command to report errors. This would allow us to report things that prevent a VM from running through QMP instead of stderr. I have considered using that to report "host doesn't support CPU feature" errors in the past.

Signature · 1 line
Eduardo
#33 lore raw

Re: [Qemu-arm] [Qemu-ppc] [Qemu-devel] Running QEMU without default devices / kernel / bios

Quoted 3 lines · Eduardo Habkost
Quoted 2 lines · Thomas Huth
Quoted 1 line · Eduardo Habkost

On Mon, May 07, 2018 at 09:13:57PM +0200, Thomas Huth wrote:

[...]

Quoted 69 lines
Quoted 44 lines
Quoted 7 lines
Quoted 6 lines

Without "-accel qtest", things are not that easy, unfortunately. Lots of boards require "-kernel" or "-bios" and refuse to work without. So you can hardly test "-nodefaults" automatically in the normal tcg mode. (But maybe all boards should allow to start QEMU in case you've at least also specified "-S" ? ... in that case we've got plenty of work for BiteSizeTasks ;-) )

Quoted 5 lines

Hmm, maybe it's not a bite-sized task after all. :)

Should we do this gradually?

* Working with -accel qtest is useful, and sounds like an easier goal;

We're pretty much there already. Apart from the SD card problem (and the xen boards), all machines should work with -nodefaults in qtest mode now.

Quoted 1 line

* working with -S seems desirable too;

Yes, it could be interesting to load the firmware / OS via HMP or GDB after QEMU has been started.

Maybe we'd simply need a new function a la:

bool cpu_starts_automatically()

{
    return autostart && !qtest_enabled();
}

And then replace all spots where we exit due to missing -kernel or -bios parameters, e.g.:

hw/m68k/mcf5208.c +1 -1
@@ -286,7 +286,7 @@ static void mcf5208evb_init(MachineState *machine)
 
     /* Load kernel.  */
     if (!kernel_filename) {
-        if (qtest_enabled()) {
+        if (!cpu_starts_automatically()) {
             return;
         }
         error_report("Kernel image must be specified");
Quoted 1 line

Does that sound like a plan?

Not sure. If a given command-line fails without -S, I would expect it to also fail if using -S and the "cont" monitor command is issued. (But not necessarily if "-S" is used and "cont" is never issued.)

Quoted 7 lines
Quoted 3 lines

* working without -S (even if the emulated CPU crashes and burns) would be interesting.

Not sure whether we really need this. It's likely better to give the user a proper error message to use "-kernel" instead of just showing a crash.

I think I agree.

Quoted 7 lines
Quoted 3 lines

Related question: what are the use cases where we require "-accel qtest" and "-S" wouldn't work?

Maybe there are some boards where you can not load code via HMP or GDB once you've started QEMU with "-S"? You'd end up with a mostly useless HMP prompt in that case, which is a little bit ugly, but not fatal.

Quoted 8 lines

You have a point. I guess the definition of "useless" here depend on what are the use cases we want to address with -S: are there reasonable use cases for using -S and never issuing "cont"?

Would it be OK if we reported errors like "kernel image must be specified" only when/if "cont" is issued?

From a users point of view, this would be great, yes. You could start

QEMU with -S, set up your machine via HMP, QMP oder GDB, and then try to start with "cont". If you'd screw it up, "cont" would yell at you and you could try again.

Quoted 1 line

From a developers point of view, this sounds like a nightmare to get it

right with all the QEMU machines that we support, though.

Quoted 6 lines
Quoted 2 lines

Apart from that ... I can't think of a case where "-S" would not work at all once we've introduce something like cpu_starts_automatically().

I'm being convinced that "-accel qtest" and "-S" are not expected to be equivalent, so my main priority right now is to document what are the differences.

Hmmm, I think I originally slightly misunderstood your original question.... and until now, I also thought that "-accel qtest" would enable the qtest interface in qtest.c, but it seems like this is rather done by the "-qtest" parameter instead.

So as far as I can see, it theoretically should be possible to replace "-accel qtest" with "-S". But I'm also not an expert here.

Quoted 5 lines

I'm reaching two conclusions from this thread:

1) "-accel qtest" has additional purposes other than the "don't run any guest code". We need to document them clearly, and it probably can't be replaced by -S directly.

There are just two things that come to my mind why we could not immediately replace "-accel qtest" by "-S":

- There are some few qtest which override "-accel qtest" with "-accel tcg". But I think they could simply be changed to use the "cont" command instead.

- We should also consider that it is possible nowadays to build QEMU with --disable-tcg. In that case, you depend on KVM to be available as accelerator. As long as there's still "-accel qtest", it should be possible to run "make test" (just the tests that really need tcg don't work anymore). But if we remove "-accel qtest" and replace it with "-S", the tests can't be run anymore if the host machine does not offer KVM (e.g. on an automated builder machine). We could add an "-accel none" mode instead, but from a users point of view, that's pretty much the same as the current "-accel qtest" mode...

Thomas

#34 lore raw

Re: [Qemu-devel] [Qemu-ppc] Running QEMU without default devices / kernel / bios

Quoted 96 lines · Thomas Huth
Quoted 3 lines · Eduardo Habkost
Quoted 2 lines · Thomas Huth
Quoted 1 line · Eduardo Habkost

On Mon, May 07, 2018 at 09:13:57PM +0200, Thomas Huth wrote:

[...]

Quoted 69 lines
Quoted 44 lines
Quoted 7 lines
Quoted 6 lines

Without "-accel qtest", things are not that easy, unfortunately. Lots of boards require "-kernel" or "-bios" and refuse to work without. So you can hardly test "-nodefaults" automatically in the normal tcg mode. (But maybe all boards should allow to start QEMU in case you've at least also specified "-S" ? ... in that case we've got plenty of work for BiteSizeTasks ;-) )

Quoted 5 lines

Hmm, maybe it's not a bite-sized task after all. :)

Should we do this gradually?

* Working with -accel qtest is useful, and sounds like an easier goal;

We're pretty much there already. Apart from the SD card problem (and the xen boards), all machines should work with -nodefaults in qtest mode now.

Quoted 1 line

* working with -S seems desirable too;

Yes, it could be interesting to load the firmware / OS via HMP or GDB after QEMU has been started.

Maybe we'd simply need a new function a la:

bool cpu_starts_automatically()

{
    return autostart && !qtest_enabled();
}

And then replace all spots where we exit due to missing -kernel or -bios parameters, e.g.:

hw/m68k/mcf5208.c +1 -1
@@ -286,7 +286,7 @@ static void mcf5208evb_init(MachineState *machine)
 
     /* Load kernel.  */
     if (!kernel_filename) {
-        if (qtest_enabled()) {
+        if (!cpu_starts_automatically()) {
             return;
         }
         error_report("Kernel image must be specified");
Quoted 1 line

Does that sound like a plan?

Not sure. If a given command-line fails without -S, I would expect it to also fail if using -S and the "cont" monitor command is issued. (But not necessarily if "-S" is used and "cont" is never issued.)

Quoted 7 lines
Quoted 3 lines

* working without -S (even if the emulated CPU crashes and burns) would be interesting.

Not sure whether we really need this. It's likely better to give the user a proper error message to use "-kernel" instead of just showing a crash.

I think I agree.

Quoted 7 lines
Quoted 3 lines

Related question: what are the use cases where we require "-accel qtest" and "-S" wouldn't work?

Maybe there are some boards where you can not load code via HMP or GDB once you've started QEMU with "-S"? You'd end up with a mostly useless HMP prompt in that case, which is a little bit ugly, but not fatal.

Quoted 6 lines

You have a point. I guess the definition of "useless" here depend on what are the use cases we want to address with -S: are there reasonable use cases for using -S and never issuing "cont"?

Would it be OK if we reported errors like "kernel image must be specified" only when/if "cont" is issued?

From a users point of view, this would be great, yes. You could start QEMU with -S, set up your machine via HMP, QMP oder GDB, and then try to start with "cont". If you'd screw it up, "cont" would yell at you and you could try again.

From a developers point of view, this sounds like a nightmare to get it right with all the QEMU machines that we support, though.

Quoted 6 lines
Quoted 2 lines

Apart from that ... I can't think of a case where "-S" would not work at all once we've introduce something like cpu_starts_automatically().

I'm being convinced that "-accel qtest" and "-S" are not expected to be equivalent, so my main priority right now is to document what are the differences.

Quoted 7 lines

Hmmm, I think I originally slightly misunderstood your original question.... and until now, I also thought that "-accel qtest" would enable the qtest interface in qtest.c, but it seems like this is rather done by the "-qtest" parameter instead.

So as far as I can see, it theoretically should be possible to replace "-accel qtest" with "-S". But I'm also not an expert here.

-S makes QEMU remain in RUN_STATE_PRELAUNCH. Without it, QEMU enters RUN_STATE_RUNNING.

RUN_STATE_RUNNING with accel=qtest is not obviously equivalent to RUN_STATE_PRELAUNCH! The state transition does more than just resuming CPUs.

Quoted 13 lines
Quoted 5 lines

I'm reaching two conclusions from this thread:

1) "-accel qtest" has additional purposes other than the "don't run any guest code". We need to document them clearly, and it probably can't be replaced by -S directly.

There are just two things that come to my mind why we could not immediately replace "-accel qtest" by "-S":

- There are some few qtest which override "-accel qtest" with "-accel tcg". But I think they could simply be changed to use the "cont" command instead.

Quoted 9 lines

- We should also consider that it is possible nowadays to build QEMU with --disable-tcg. In that case, you depend on KVM to be available as accelerator. As long as there's still "-accel qtest", it should be possible to run "make test" (just the tests that really need tcg don't work anymore). But if we remove "-accel qtest" and replace it with "-S", the tests can't be run anymore if the host machine does not offer KVM (e.g. on an automated builder machine). We could add an "-accel none" mode instead, but from a users point of view, that's pretty much the same as the current "-accel qtest" mode...

Different name, same thing, not worth changing.

#35 lore raw
In reply to Eduardo Habkost
Quoted 90 lines · Eduardo Habkost
Quoted 78 lines · Thomas Huth
Quoted 69 lines · Eduardo Habkost
Quoted 59 lines · Markus Armbruster
Quoted 49 lines · Eduardo Habkost
Quoted 42 lines · Markus Armbruster
Quoted 40 lines · Eduardo Habkost
Quoted 39 lines · Thomas Huth
Quoted 1 line · Markus Armbruster

Thomas Huth <[email protected]> writes:

[...]

Quoted 26 lines
Quoted 6 lines
(quoted diff) +2 -0
@@ -260,6 +263,26 @@ static void test_abstract_interfaces(void)
     qtest_end();
 }
 
+static void add_machine_test_case(const char *mname)
+{

Review static void test_abstract_interfaces(void) · line 277

Quoted 10 lines
(quoted diff) +10 -0
+    char *path, *args;
+
+    /* Ignore blacklisted machines */
+    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
+        return;
+    }
+
+    path = g_strdup_printf("device/introspect/concrete-defaults-%s", mname);
+    args = g_strdup_printf("-machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);

This runs test_device_intro_concrete() with "-machine M" for all machine types M, in SPEED=slow mode.

Review static void test_abstract_interfaces(void) · line 282

Quoted 5 lines
(quoted diff) +5 -0
+    g_free(path);
+
+    path = g_strdup_printf("device/introspect/concrete-nodefaults-%s", mname);
+    args = g_strdup_printf("-nodefaults -machine %s", mname);
+    qtest_add_data_func(path, args, test_device_intro_concrete);
Quoted 4 lines

This runs test_device_intro_concrete() with "-nodefaults -machine M" for all machine types M, in SPEED=slow mode.

Has "without -nodefaults" exposed additional bugs?

After testing this with all machines, I had to discover that "-nodefaults" does not work so easily: A lot of the embedded machines (especially the ARM machines) simply refuse to work with "-nodefaults" and exit immediately instead. E.g.:

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
$ arm-softmmu/qemu-system-arm -nodefaults -nographic -M n810,accel=qtest
qemu-system-arm: missing SecureDigital device

These are all bugs. --nodefaults is supposed to suppress *optional* devices, not mandatory ones.

Quoted 7 lines

I'm not sure I understand the requirements. What exactly is the definition of "mandatory"?

A machine created by "qemu-system-x86_64 -machine pc -nodefaults" is useless because it has no any device to boot from. How is that different from a n810 machine not booting because there's no SD device?

Quoted 8 lines

I propose:

* Stuff that's required for QEMU to run is not suppressed by -nodefaults

* Stuff that a real machine has soldered on is also not suppressed

* Stuff that can be pulled out of a real machine may be suppressed, even when that means the guest won't run

Quoted 7 lines

Makes sense to me. It looks like the only obstacle for tests/device-introspect and device-crash-test is the first rule. "Guest won't boot" isn't a problem, but "QEMU won't run" is.

The first rule is easily testable, too: running "$QEMU -machine $MACHINE -nodefaults" and not having a working QMP monitor should be reported as a bug by automated tests.

Quoted 10 lines

You mean with "-accel qtest" or without? With "-accel qtest" we should pretty soon be fine, after Peter's current PULL request has been merged (which contains a patch from me for fixing these SD card problems with ARM machines).
Without "-accel qtest", things are not that easy, unfortunately. Lots of boards require "-kernel" or "-bios" and refuse to work without. So you can hardly test "-nodefaults" automatically in the normal tcg mode. (But maybe all boards should allow to start QEMU in case you've at least also specified "-S" ? ... in that case we've got plenty of work for BiteSizeTasks ;-) )

Quoted 5 lines

Hmm, maybe it's not a bite-sized task after all. :)

Should we do this gradually?

* Working with -accel qtest is useful, and sounds like an easier goal;

This is immediately useful.

Quoted 3 lines

* working with -S seems desirable too;
* working without -S (even if the emulated CPU crashes and burns) would be interesting.

Nice to have for consistency, I think.

Quoted 7 lines

Related question: what are the use cases where we require "-accel qtest" and "-S" wouldn't work?

Are the requirements and goals of "-accel qtest" documented somewhere? Without documentation, it's hard to say when a given qtest_enabled() call in the code is reasonable, or a hack we want to get rid of.

Good question.

    $ git-grep -l qtest docs/
    docs/devel/testing.rst

Its section QTest doesn't mention -accel.

The accelerator was added in commit c7f0f3b1c82. The commit message mentions it [lines wrapped for readability]:

The idea behind qtest is pretty simple. Instead of executing a CPU via TCG or KVM, rely on an external process to send events to the device model that the CPU would normally generate.

qtest presents itself as an accelerator. In addition, a new option is added to establish a qtest server (-qtest) that takes a character device. This is what allows the external process to send CPU events to the device model.

qtest uses a simple line based protocol to send the events. Documentation of that protocol is in qtest.c.

Less than clear.

In my understanding, the purpose of the qtest accelerator is to suppress guest execution. We later put it to secondary use of suppressing whatever stuff (such as warnings) gets in the way of the test suite.