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 <ehabk...@redhat.com> writes:
> >>>>
> >>>>> On Fri, Apr 27, 2018 at 08:31:58AM +0200, Markus Armbruster wrote:
> >>>>>> Eduardo Habkost <ehabk...@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 <th...@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.
--
Eduardo
