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