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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4497 bytes --]

On Fri, Apr 20, 2018 at 10:34:57AM +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 20, 2018 at 10:11:08AM +0200, Laszlo Ersek wrote:
> > On 04/19/18 11:12, Daniel P. Berrangé wrote:
> > > On Thu, Apr 19, 2018 at 10:39:32AM +0200, Laszlo Ersek wrote:
> > >> 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:
> > >>>> 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.
> > > 
> > > NB, qemu-system-s390x would previously have returned "other" in
> > > this field, and now it returns "s390".  So while it didn't
> > > remove "other" from the list of things that could potentially
> > > exist, it did change what the s390x binary will actually report.
> > > 
> > >> 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.
> > > 
> > > When I say removing "other", I imply that we add an explicit arch
> > > for all those which we currently are missing. IOW, all qemu-system-XXX
> > > binaries which currently report "other" would change to report their
> > > respective "XXX" values.
> > > 
> > > So in this way, it is exactly the same as what we did when we
> > > introduced "s390" as an option.
> > > 
> > > The only difference is that once we have every binary reporting the
> > > correct arch, we can now also remove "other" from the schema itself
> > > as it will then be unused.
> > 
> > Can we please translate this into more actionable items for me, because
> > I'm getting confused :)
> > 
> > First, if I add "i386" and "x86_64" to the enum list, we'll have all
> > three of "i386", "x86_64" and "x86". Is that useful? How will that work?
> 
> Hmm, yes, on closer look this is a big mess as it is. We've been using
> generic terms for covering multiple architectures :-(  'x86' for both
> i386 and x86_64,  'sparc' for sparc and sparc64, etc. If we try to fix
> that we'll be entering a world of backcompat hurt :-(
> 
> Since your schema is likely to end up just being a file in docs/specs,
> rather than directly part of our existnig qapi schema, I suggest we just
> ignore whats there. Just define an arch enum in your spec which is right,
> and let someone else worry about fixing the mess

Trouble is, for these "biarch" cases, I'm not sure it's always clear
what the right value for a firmware is.  While whether a userspace
binary is i386 or x86_64 is clear and well-defined, a firmware could
well be responsible for switching the CPU from its reset mode into the
more modern 64-bit mode, and would therefore have at least some code
in both archs.

> > Second, assuming I add constants for the ~10 (?) softmmu arches, can I
> > still use @CpuInfoOther as the type for the corresponding new members in
> > @CpuInfo? What C code changes will be necessary?
> 
> Yes, we could still use the CpuInfoOther struct, since struct names are
> invisible to consumers, but as above, lets ignore the mess
> 
> Regards,
> Daniel

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]