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

[Laszlo Ersek]

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 <address@hidden> writes:
>>
>>> On 04/18/18 10:47, Markus Armbruster wrote:
>>>> Laszlo Ersek <address@hidden> 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.

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).

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).

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.

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 @address@hidden to that enum. 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?

Thanks,
Laszlo