Re: [Qemu-devel] [PATCH v2 4/7] scripts/qemu.py: set predefined machine type based on arch

[Eduardo Habkost]

On Thu, Oct 11, 2018 at 12:43:06AM -0400, Cleber Rosa wrote:
> 
> 
> On 10/10/18 11:42 PM, Eduardo Habkost wrote:
> > On Wed, Oct 10, 2018 at 08:17:26PM -0400, Cleber Rosa wrote:
> >>
> >>
> >> On 10/10/18 11:47 AM, Cleber Rosa wrote:
> >>>
> >>>
> >>> On 10/10/18 10:28 AM, Eduardo Habkost wrote:
> >>>> On Wed, Oct 10, 2018 at 10:15:15AM -0400, Cleber Rosa wrote:
> >>>>>
> >>>>>
> >>>>> On 10/10/18 9:59 AM, Cleber Rosa wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 10/10/18 9:46 AM, Eduardo Habkost wrote:
> >>>>>>> On Wed, Oct 10, 2018 at 08:35:38AM -0400, Cleber Rosa wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 10/10/18 7:00 AM, Philippe Mathieu-Daudé wrote:
> >>>>>>>>> On 10/10/2018 01:26, Cleber Rosa wrote:
> >>>>>>>>>> Some targets require a machine type to be set, as there's no 
> >>>>>>>>>> default
> >>>>>>>>>> (aarch64 is one example).  To give a consistent interface to users 
> >>>>>>>>>> of
> >>>>>>>>>> this API, this changes set_machine() so that a predefined default 
> >>>>>>>>>> can
> >>>>>>>>>> be used, if one is not given.  The approach used is exactly the 
> >>>>>>>>>> same
> >>>>>>>>>> with the console device type.
> >>>>>>>>>>
> >>>>>>>>>> Also, even when there's a default machine type, for some purposes,
> >>>>>>>>>> testing included, it's better if outside code is explicit about the
> >>>>>>>>>> machine type, instead of relying on whatever is set internally.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Cleber Rosa <address@hidden>
> >>>>>>>>>> ---
> >>>>>>>>>>  scripts/qemu.py | 22 +++++++++++++++++++++-
> >>>>>>>>>>  1 file changed, 21 insertions(+), 1 deletion(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/scripts/qemu.py b/scripts/qemu.py
> >>>>>>>>>> index d9e24a0c1a..fca9b76990 100644
> >>>>>>>>>> --- a/scripts/qemu.py
> >>>>>>>>>> +++ b/scripts/qemu.py
> >>>>>>>>>> @@ -36,6 +36,15 @@ CONSOLE_DEV_TYPES = {
> >>>>>>>>>>      r'^s390-ccw-virtio.*': 'sclpconsole',
> >>>>>>>>>>      }
> >>>>>>>>>>  
> >>>>>>>>>> +#: Maps archictures to the preferred machine type
> >>>>>>>>>> +MACHINE_TYPES = {
> >>>>>>>>>> +    r'^aarch64$': 'virt',
> >>>>>>>>>> +    r'^ppc$': 'g3beige',
> >>>>>>>>>> +    r'^ppc64$': 'pseries',
> >>>>>>>>>> +    r'^s390x$': 's390-ccw-virtio',
> >>>>>>>>>> +    r'^x86_64$': 'q35',
> >>>>>>>>>
> >>>>>>>>> Why choose Q35 rather than PC (the default)?
> >>>>>>>>>
> >>>>>>>>> I was wondering about how to generate variants/machines.json but 
> >>>>>>>>> this is
> >>>>>>>>> definitively something we want to do via a QMP query.
> >>>>>>>>>
> >>>>>>>>> Eduardo what do you think?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> It was motivated by Eduardo's initiative to make q35 the default 
> >>>>>>>> "across
> >>>>>>>> the board".  He can confirm and give more details.
> >>>>>>>
> >>>>>>> Making Q35 the default on applications using QEMU and libvirt is
> >>>>>>> something I'd like to happen.  But I think the simplest way to do
> >>>>>>> that is to change the QEMU default.  This way you won't need this
> >>>>>>> table on qemu.py: you can just use the default provided by QEMU.
> >>>>>>>
> >>>>>>
> >>>>>> The idea is to bring consistency on how we're calling
> >>>>>> "qemu-system-$(ARCH)", and at the same time apply the "explicit is
> >>>>>> better than implicit" rule.
> >>>>>>
> >>>>>> The most important fact is that some targets do not (currently) have
> >>>>>> "the default provided by QEMU", aarch64 is one of them.
> >>>>>>
> >>>>>> - Cleber.
> >>>>>>
> >>>>>
> >>>>> So I ended up not relaying the question properly: should we default
> >>>>> (even if explicitly adding "-machine") to "pc"?
> >>>>
> >>>> I think using the default machine-type (when QEMU has a default)
> >>>> would be less surprising for users of the qemu.py API.
> >>>>
> >>>
> >>> OK, agreed.
> >>>
> >>>> Implicitly adding -machine when there's no default is also
> >>>> surprising, but then it's a nice surprise: instead of crashing
> >>>> you get a running VM.
> >>>>
> >>>> Now, there are two other questions related to this:
> >>>>
> >>>> If using 'pc' as default, should we always add -machine, or just
> >>>> omit the machine-type name?  I think we should omit it unless the
> >>>> caller asked for a specific machine-type name (because it would
> >>>> be less surprising for users of the API).
> >>>>
> >>>
> >>
> >> Getting down to business, trying to apply those changes, I was faced
> >> with a situation.  Actually, the same situation I faced a few months
> >> ago.  Handling it was defered until it was *really* a blocker.
> >> Basically the issue is: the set_console() method, which gives tests a
> >> ready to use console, depends on knowing the machine type (see
> >> CONSOLE_DEV_TYPES).
> >>
> >> As a case study, let's look at "boot_console_linux.py":
> >>  1) it sets the machine type explicitly
> >>  2) it has nothing to do with the specific machine type
> >>  3) the setting of a machine type is boiler plate code to set a console
> >>  4) the console is used on the test's real purpose: verifying the Linux
> >> kernel booted
> >>
> >> Now, to be able to run the same test -- booting a Linux kernel -- on
> >> *other target archs*, we need the same machinery.  Even more important:
> >> to have similar tests we'll need to either abstract those features or
> >> duplicate them.  This can be seen, at least in part, on the firmware
> >> tests that Philippe sent to the list: they would also benefit from
> >> having a console device ready to be used on the configured machine type[1]:
> >>
> >> Assuming that we want to provide this type of machinery for free (or as
> >> close as that) to the acceptance/functional tests, we need some source
> >> of "known good" configuration for the targets we aim to support.
> >>
> >> Let's restrict the discussion to the issue at hand, machine types, while
> >> keeping in mind that the same pattern happened with devices types to use
> >> as console before, and my experience running into default network device
> >> types in further work (tests that interact with the guest by ssh'ing
> >> into it).
> >>
> >> The solutions I can think of are:
> >>
> >>  1) run the target binary previous to the "real" run, and query
> >> information -- this is what Avocado-VT does[2], and something I tried on
> >> earlier versions of the acceptance tests infrastructure code
> >>
> >>  2) attempt to get this information from the build system[3]
> >>
> >>  3) hard code the "known" good configuration
> >>
> >> I've previously worked on solutions along the lines of #1 and #2, but
> >> the general feedback wasn't that positive, for valid reasons.  Eduardo
> >> probably remembers this.
> > 
> > I don't remember seeing negative feedback for #1.  It sounds like
> > the best solution.
> > 
> 
> IIRC, it was mostly related to the fact that the reliable way to query a
> target information (instead of parsing a human oriented output) would be
> to use QMP.
> 
> Then, the question of using QEMUMachine for that (and the possible
> chicken-and-egg situations, having a different set of base args, etc)
> led me into prototyping a simplified version:
> 
> https://github.com/clebergnu/qemu/commit/b769b3d969b9e29c644d12c30cd0bb7f312e4fbc#diff-07b9b989eb85128c2c8d2a8f17685698R46
> 
> Which would still be duplicating some code.  I'm not particularly happy
> about either approaches TBH.
> 
> >>
> >> So, I'm tempted to try solution #3.  As much as duplicating target
> >> defaults in qemu.py doesn't sound perfect, it seems to be the more
> >> predictable and attainable solution at this point.
> > 
> > I consider #3 to be acceptable just as a temporary solution until
> > we implement #1.
> > 
> > 
> 
> So, with the extra information given here, would you recommend doing #3?
> Or pause this, and work on a #1 of sorts?

Getting more working tests is more important to me.  I won't mind
implementing #3 on the avocado_qemu module if implementing #1
first would delay the inclusion of tests on QEMU 3.1.

-- 
Eduardo