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

[Laszlo Ersek]

On 04/20/18 14:53, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:

[snip]

>> 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 @address@hidden 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.

OK, so here's my understanding:

(1) I'll introduce a new @Target enum in misc.json. I'll inherit /
include it in firmware.json. This is compatible with what Dan said.

(2) I'll change @address@hidden to the new type. I believe this will
break the compilation of qmp_query_target(); I'll hack on that until it
builds again, with the lookup. :)

(3) Regarding the addition of @target to CpuInfo (accompanying @arch)
doesn't look hard; what *does* look hard is changing the union
discriminator from @arch to @target. @target has many more values, and I
would have to map all of them to the (fewer) @arch values that currently
do *not* select @CpuInfoOther. Here's an example:

- Both @i386 and @x86_64 from @target mean @x86 in @arch,
- because @x86 currently selects @CpuInfoX86, not @CpuInfoOther,
  both @i386 and @x86_64 must be assigned @CpuInfoX86.

This depends on the knowledge that "x86" actually *means* "i386 plus
x86_64", and I totally don't have that knowledge for the rest of the
arches / targets.

So, the modification of @CpuInfo I would indeed like to pass off to
someone else. (Well, if all the architecture maintainers follow up and
tell me what emulators exactly fall under the umbrella of each
individual @arch value, I can post the patch.) BTW... I wonder how
compatibility would be affected if we just added @target to @CpuInfo,
even without making it the new discriminator.

... Anyway, I think I've gotten a huge amount of useful and actionable
feedback; thanks everyone, let me work on RFCv3 and post it soon. :)

Thanks!
Laszlo