| Message ID | 5837d14a-b562-c382-f95a-32ef12a4e25c@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08.05.2018 07:33, Thomas Huth wrote: > On 07.05.2018 21:32, Eduardo Habkost wrote: >> On Mon, May 07, 2018 at 09:13:57PM +0200, Thomas Huth wrote: >>> On 07.05.2018 20:21, Eduardo Habkost wrote: >>>> On Mon, May 07, 2018 at 06:50:35PM +0200, Markus Armbruster wrote: >>>>> Eduardo Habkost <ehabkost@redhat.com> writes: >>>>> >>>>>> On Fri, Apr 27, 2018 at 08:31:58AM +0200, Markus Armbruster wrote: >>>>>>> Eduardo Habkost <ehabkost@redhat.com> writes: >>>>>>> >>>>>>>> On Thu, Apr 26, 2018 at 05:20:25PM +0200, Thomas Huth wrote: >>>>>>>>> On 26.04.2018 13:45, Markus Armbruster wrote: >>>>>>>>>> Thomas Huth <thuth@redhat.com> writes: >>>>>>>>> [...] >>>>>>>>>>> @@ -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); >>>>>>>>>> >>>>>>>>>> This runs test_device_intro_concrete() with "-machine M" for all machine >>>>>>>>>> types M, in SPEED=slow mode. >>>>>>>>>> >>>>>>>>>>> + 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? >>>>>>>>> >>>>>>>>> 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. >>>>>> >>>>>> 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 >>>> >>>> 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 ;-) ) >> >> 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
On Tue, May 08, 2018 at 07:33:46AM +0200, Thomas Huth wrote: > On 07.05.2018 21:32, Eduardo Habkost wrote: > > On Mon, May 07, 2018 at 09:13:57PM +0200, Thomas Huth wrote: > >> On 07.05.2018 20:21, Eduardo Habkost wrote: > >>> On Mon, May 07, 2018 at 06:50:35PM +0200, Markus Armbruster wrote: > >>>> Eduardo Habkost <ehabkost@redhat.com> writes: > >>>> > >>>>> On Fri, Apr 27, 2018 at 08:31:58AM +0200, Markus Armbruster wrote: > >>>>>> Eduardo Habkost <ehabkost@redhat.com> writes: > >>>>>> > >>>>>>> On Thu, Apr 26, 2018 at 05:20:25PM +0200, Thomas Huth wrote: > >>>>>>>> On 26.04.2018 13:45, Markus Armbruster wrote: > >>>>>>>>> Thomas Huth <thuth@redhat.com> writes: > >>>>>>>> [...] > >>>>>>>>>> @@ -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); > >>>>>>>>> > >>>>>>>>> This runs test_device_intro_concrete() with "-machine M" for all machine > >>>>>>>>> types M, in SPEED=slow mode. > >>>>>>>>> > >>>>>>>>>> + 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? > >>>>>>>> > >>>>>>>> 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. > >>>>> > >>>>> 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 > >>> > >>> 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 ;-) ) > > > > 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. > > > * 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.: > > diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c > --- a/hw/m68k/mcf5208.c > +++ b/hw/m68k/mcf5208.c > @@ -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? 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.) > > > * 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. > > > 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? > 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. > > > 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. 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.
On 08.05.2018 18:40, Eduardo Habkost wrote: > On Tue, May 08, 2018 at 07:33:46AM +0200, Thomas Huth wrote: >> On 07.05.2018 21:32, Eduardo Habkost wrote: >>> On Mon, May 07, 2018 at 09:13:57PM +0200, Thomas Huth wrote: [...] >>>> 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; >> >> 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. >> >>> * 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.: >> >> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c >> --- a/hw/m68k/mcf5208.c >> +++ b/hw/m68k/mcf5208.c >> @@ -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? > > 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.) > >> >>> * 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. > >> >>> 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? 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. >> 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. > 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
Thomas Huth <thuth@redhat.com> writes: > On 08.05.2018 18:40, Eduardo Habkost wrote: >> On Tue, May 08, 2018 at 07:33:46AM +0200, Thomas Huth wrote: >>> On 07.05.2018 21:32, Eduardo Habkost wrote: >>>> On Mon, May 07, 2018 at 09:13:57PM +0200, Thomas Huth wrote: > [...] >>>>> 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; >>> >>> 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. >>> >>>> * 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.: >>> >>> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c >>> --- a/hw/m68k/mcf5208.c >>> +++ b/hw/m68k/mcf5208.c >>> @@ -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? >> >> 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.) >> >>> >>>> * 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. >> >>> >>>> 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? > > 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. > >>> 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. -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. >> 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... Different name, same thing, not worth changing.
diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c --- a/hw/m68k/mcf5208.c +++ b/hw/m68k/mcf5208.c @@ -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");