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

[Cleber Rosa <crosa@redhat.com>]

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 <crosa@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>  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?

Thanks!
- Cleber.

>>
>> Thoughts?
>>
>> Thanks!
>> - Cleber.
>>
>> [1] - https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00591.html
>> [2] -
>> https://github.com/avocado-framework/avocado-vt/blob/65.0/virttest/utils_misc.py#L2105
>> [3] - http://lists.gnu.org/archive/html/qemu-devel/2017-07/msg06757.html
>>
>>> OK.
>>>
>>>> About our default testing configuration for acceptance tests:
>>>> should acceptance tests run against PC by default?  Should it
>>>> test Q35?  Should we test both PC and Q35?  I'm not sure what's
>>>> the answer, but I think these decisions shouldn't affect the
>>>> qemu.py API at all.
>>>>
>>>
>>> OK.
>>>
>>> To make sure we're on the same page, we're still going to have default
>>> machine types, based on the arch, for those targets that don't provide
>>> one (aarch64 is one example).  Right?
>>>
>>> - Cleber.
>>>
>