Re: [Qemu-devel] [PATCH 6/6] qapi: discriminate CpuInfo[Fast] on SysEmuTarget, not CpuInfoArch

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 04/26/18 17:51, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> On 04/26/2018 09:34 AM, Markus Armbruster wrote:
>>>>>
>>>>> Acerbic discovery of the day: @CpuInfoArch has "x86", while configure
>>>>> produces:
>>>>>
>>>>>   TARGET_NAME  TARGET_BASE_ARCH
>>>>>          i386              i386
>>>>>        x86_64              i386
>>>>>
>>>>> Note how "i386" does not match "x86".
>>>>
>>>> Review fail.
>>>>
>>>> Just three weeks ago, we could still have fixed query-cpus-fast...
>>>
>>> Actually, I think we still can.  We already documented in the 2.12
>>> release notes that the "arch" field of query-cpus-fast is known to be
>>> broken for all but "s390x" (which is really the only arch field that
>>> MUST be correct, as that is the only time we send additional
>>> information).  And introspection can easily see both the enum contents
>>> (if we add something) as well as any other additions to the
>>> query-cpus-fast output union (although that is less likely), to use
>>> those as a witness for whether qemu is new enough to have fixed the
>>> bogus "arch" values.  I'd argue that if we change things right now, with
>>> intent to include the change in 2.12.1, before people start relying on
>>> the bogus "arch" of 2.12, then we should feel free to make
>>> query-cpus-fast output whatever we want for all architectures other than
>>> "s390x", even if it changes the current output of "x86".
>> 
>> In other words, we managed to screw up query-cpus-fast sufficiently to
>> let us fix it even now.  Cool, let's do it!
>> 
>
> Let me clarify a little.
>
> The @CpuInfoArch enum has the following constants now:
>
> ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'riscv', 'other' ]
>
> This enum was originally introduced in 2.6 (according to the
> documentation), and only the @s390 and @riscv constants were added in 2.12.
>
> The enum constants show up in the following two places, *on the wire*:
>
> - in @address@hidden, only produced by @query-cpus,
> - in @address@hidden, only produced by @query-cpus-fast.
>
> The plan would be to rewrite *all* those enum constants of @CpuInfoArch
> to which the respective TARGET_BASE_ARCH values (from "configure") do
> not map *identically*. Here's the mapping:
>
>   TARGET_BASE_ARCH  CpuInfoArch  CpuInfoArch needs change
>   ----------------  -----------  ------------------------
>   i386              x86          YES
>   sparc             sparc        no
>   ppc               ppc          no
>   mips              mips         no
>   tricore           tricore      no
>   s390x             s390         YES
>   riscv             riscv        no
>
> In other words, @CpuInfoArch would have to be changed to the following:
>
> ['i386', 'sparc', 'ppc', 'mips', 'tricore', 's390x', 'riscv', 'other' ]
>  ^^^^^^                                     ^^^^^^^
>
> This means that the @arch field, returned by @query-cpus and
> @query-cpus-fast, would change incompatibly for those QAPI clients that
> look specifically for "x86" or "s390".
>
> Is this a safe change?
>
> I would say, because of the 's390' -> 's390x' change, that it isn't.
>
> (Also, to confirm, the wiki section at
> <https://wiki.qemu.org/Planning/2.12#Issues_that_will_not_be_fixed> states,
>
>   * the query-cpus-fast QMP command reports bogus arch data for all
>     architectures except x86 and s390; applications should be careful to
>     not rely on the bogus information
>
> It (correctly) refers to "s390". That value would change.)

You're right, that's a compatibility break.

We could perhaps still declare *all* @arch values useless in v2.12.0,
then fix them in v2.12.1.

Or we deprecate @arch right when we introduce @target, and drop it later
in accordance with our deprecation policy (qemu-doc.texi @appendix
Deprecated features).  That way, the rather ridiculous code to compute
it will be temporary.  I think that's cleaner.

   @arch in     query-cpus      query-cpus-fast
   before 2.6   nonexistent
   2.6 - 2.11   CpuInfoArch
   2.12         cmd deprecated  CpuInfoArch
   2.13         cmd deprecated  memb deprecated
   2.14         cmd gone        memb deprecated
   2.15         cmd gone        memb gone

Opinions?