Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Jiri Denemark]

On Fri, May 10, 2019 at 17:23:13 -0300, Eduardo Habkost wrote:
> On Fri, May 10, 2019 at 01:33:03PM +0200, Jiri Denemark wrote:
> > On Thu, May 09, 2019 at 13:08:25 -0300, Eduardo Habkost wrote:
> > > On Thu, May 09, 2019 at 05:26:16PM +0200, Jiri Denemark wrote:
> > > > On Thu, May 09, 2019 at 10:56:17 -0300, Eduardo Habkost wrote:
> > > > > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > > > > Would this unavailable-features property contain only canonical 
> > > > > > names of
> > > > > > the features or all possible aliases of all features? For example,
> > > > > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > > > > query-cpu-model-expansion, we have a way to request all variants by
> > > > > > running full expansion on the result of a previous static 
> > > > > > expansion. Can
> > > > > > we get something like this for unavailable-features too?
> > > > > 
> > > > > I'd like to avoid that, and refer only to the canonical names.
> > > > > 
> > > > > Could you explain the use case you have in mind, so we can look
> > > > > for alternatives?
> > > > 
> > > > Libvirt only knows about a single spelling for each CPU feature and
> > > > quite often it is not the canonical variant. Thus libvirt would only
> > > > recognize features for which the name known by libvirt is the canonical
> > > > name.
> > > > 
> > > > We could theoretically make the translation in libvirt, but it's not
> > > > going to be future proof. If a new spelling is introduced, it's because
> > > > the old one is not considered correct and the new one becomes the
> > > > canonical version. When libvirt doesn't have the translation (libvirt is
> > > > older or just didn't catch up yet) we have a big problem.
> > > > 
> > > > I guess a good alternative would be some way of querying all CPU feature
> > > > names and their aliases. If I'm not mistaken, we can either get a list
> > > > of canonical names or all names, but without any clue which names
> > > > actually refer to a single feature.
> > > 
> > > Right.  But (as described below) if you want to know the status a
> > > specific feature you already know about (even if you are using
> > > the old spelling), qom-get should work for you.
> > 
> > Yeah, since we'll be checking all features anyway, we can detect enabled
> > and disabled features at the same time. However, we don't know whether a
> > specific feature was disabled because we did not ask for it to be
> > enabled (remember, CPU model definition may differ between libvirt and
> > QEMU) or because it was filtered out.
> > 
> > Depending on the domain XML used to start a domain, libvirt may not (and
> > usually will not) refuse to start a domain for which QEMU filtered out
> > some CPU features. Of course, once the domain is running, the checking
> > becomes very strict and libvirt would refuse to start the domain on
> > another host during migration if any feature is filtered out.
> > 
> > Thus libvirt stores all features QEMU filtered out when a domain was
> > started in the non-strict way. So we need to match the features in the
> > unavailable-features list with our features. Just checking for the list
> > being empty is not sufficient.
> 
> OK, I understand you want to translate the canonical names on
> unavailable-features back to the old names on some cases.
> 
> But I really prefer Igor's suggestion of deprecating aliases and
> getting rid of them in the future, instead of increasing the
> complexity of our QMP interfaces just to accommodate the existing
> aliases.

OK, I think you can go on and implement the unavailable-features
property the way you suggested and we'll deal with the translation
internally in libvirt.

Jirka