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

[Eduardo Habkost <ehabkost@redhat.com>]

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.

-- 
Eduardo