[libvirt] [Qemu-devel] [qemu RFC v2] qapi: add "firmware.json"

[Markus Armbruster]

Laszlo Ersek <lersek at redhat.com> writes:

> On 04/19/18 09:56, Daniel P. Berrangé wrote:
>> On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote:
>>> Laszlo Ersek <lersek at redhat.com> writes:
>>>
>>>> On 04/18/18 10:47, Markus Armbruster wrote:
>>>>> Laszlo Ersek <lersek at redhat.com> writes:
>>>>
>>>>>> +##
>>>>>> +# @FirmwareArchitecture:
>>>>>> +#
>>>>>> +# Defines the target architectures (emulator binaries) that firmware may
>>>>>> +# execute on.
>>>>>> +#
>>>>>> +# @aarch64: The firmware can be executed by the qemu-system-aarch64 emulator.
>>>>>> +#
>>>>>> +# @arm: The firmware can be executed by the qemu-system-arm emulator.
>>>>>> +#
>>>>>> +# @i386: The firmware can be executed by the qemu-system-i386 emulator.
>>>>>> +#
>>>>>> +# @x86_64: The firmware can be executed by the qemu-system-x86_64 emulator.
>>>>>> +#
>>>>>> +# Since: 2.13
>>>>>> +##
>>>>>> +{ 'enum' : 'FirmwareArchitecture',
>>>>>> +  'data' : [ 'aarch64', 'arm', 'i386', 'x86_64' ] }
>>>>>
>>>>> Partial dupe of CpuInfoArch from misc.json.  Neither covers all our
>>>>> target architectures.  Should we have one that does in common.json
>>>>> instead?
>>>>
>>>> If we had one there, I'd use it here :)
>>>>
>>>> For collecting the arch identifiers, is it OK to run "./configure
>>>> --help", and look for the "*-softmmu" options under
>>>> "--target-list=LIST"? Because that's what I need here; the qemu-system-*
>>>> emulators.
>>>
>>> configure gets its list like this:
>>>
>>>     default_target_list=""
>>>
>>>     mak_wilds=""
>>>
>>>     if [ "$softmmu" = "yes" ]; then
>>>         mak_wilds="${mak_wilds} $source_path/default-configs/*-softmmu.mak"
>>>     fi
>>>     if [ "$linux_user" = "yes" ]; then
>>>         mak_wilds="${mak_wilds} $source_path/default-configs/*-linux-user.mak"
>>>     fi
>>>     if [ "$bsd_user" = "yes" ]; then
>>>         mak_wilds="${mak_wilds} $source_path/default-configs/*-bsd-user.mak"
>>>     fi
>>>
>>>     for config in $mak_wilds; do
>>>         default_target_list="${default_target_list} $(basename "$config" .mak)"
>>>     done
>>>
>>> Since we use QMP only in system emulation, a QAPI enum limited to the
>>> system emulation targets makes sense.
>>>
>>> Replacing CpuInfoArch by such an enum will change the discriminator
>>> value from "other" to the real architecture, with the obvious
>>> compatibility concerns.  But we've accepted similar changes twice
>>> already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0.
>>>
>>> "other" was a bad idea.  Hindsight 20/20.
>>>
>>> Getting rid of it in one go rather than piecemeal seems like the least
>>> bad way out.  Too late for 2.12, though.  Eric, what do you think?
>> 
>> Given the context in which this "other" value is used, I think it is
>> reasonable to kill it and put a full arch list in there.
>> 
>> No app is likely to be accessing the struct under "other" because it
>> is just an empty placeholder.
>
> Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the
> potential to confuse QMP clients that didn't expect "s390", but
> otherwise it didn't mess with preexistent enum values / structures.
>
> The same applies to commit 25fa194b7b1, just with "riscv" /
> "CpuInfoRISCV" substituted.
>
> Removing "other" might confuse QMP clients that expect it, except
> (according to Daniel) no such client exists, probably.

It's a done deal anyway; we're not going to hold 2.12 to avoid this QMP
compatibility break.

> However, replacing the current list of CpuInfoArch constants with the
> system emulation target list would be more intrusive than all three of
> the above. In particular there is no "x86" target; only i386 and x86_64
> targets exist. For the firmware schema, this distinction is important,
> but it would break QMP clients that expect "x86" (and such clients must
> certainly exist).

You're right.  Another nice mess.

> The targets with softmmu are: aarch64, alpha, arm, cris, hppa, i386,
> lm32, m68k, microblaze, microblazeel, mips, mips64, mips64el, mipsel,
> moxie, nios2, or1k, ppc, ppc64, ppcemb, riscv32, riscv64, s390x, sh4,
> sh4eb, sparc, sparc64, tricore, unicore32, x86_64, xtensa, xtensaeb.
>
> That is, at least the following constants in CpuInfoArch have no
> corresponding (identical) mapping -- I'll state to the right of the
> arrow the emulation targets which I know or presume to be associated
> with the CpuInfoArch constant:
> - x86 -> i386, x86_64
> - sparc -> sparc, sparc64
> - ppc -> ppc, ppc64, ppcemb
> - mips -> mips, mips64, mips64el, mipsel
> - s390 -> s390x
> - riscv -> riscv32, riscv64
>
> The only constant that seems to have a 1:1 mapping is "tricore", but I
> could perfectly well be thinking even that just because I have no clue
> about "tricore".
>
> So, I don't think CpuInfoArch can be updated so that it both remains
> compatible with current QMP clients, and serves "firmware.json". In the
> firmware schema we don't just need CPU architecture, but actual emulator
> names (and board / machine types).

The completely orthodox fix for CpuInfo would be:

* Keep @arch unchanged.  In particular, keep "other" for all targets
  other than 'x86', 'sparc', 'ppc', 'mips', 'tricore'.

* Add a new member @target of new enum type Target, whose values match
  configures idea of targets exactly.

* Make the new member the union tag.

It's too late for complete orthodoxy; we already changed @arch.

A common enum type Target makes sense anyway, I think.

Using it in CpuInfo as described above may make sense, too.  Could be
left to a follow-up patch.

> I grepped 'qapi/*json' for the whole word "x86_64", and the only thing
> that remotely matches is the @TargetInfo structure, in which the @arch
> field is a string, coming with the example "x86_64". The example also
> names "i386" separately.

Well spotted.

TargetInfo member @arch is set to TARGET_NAME, which matches configure's
idea of the target.  If we add enum Target, we should change @arch's
type from str to Target.

> So what might make sense is to introduce a separate enum in
> "qapi/misc.json" with all the softmmu targets I listed above, and change
> the type of @TargetInfo. at arch to that enum.

I arrived at this conclusion, too.

>                                             In parallel,
> qmp_query_target() would have to be updated to look up the enum value
> matching TARGET_NAME. (I do think we can ignore linux-user etc emulators
> for collecting the relevant arches here: @TargetInfo is only used in the
> "query-target" QMP command, and Markus said above that QMP is only used
> with system emulation.)
>
> Should I do this?

Yes, please.