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

[Eduardo Habkost]

On Wed, May 22, 2019 at 10:42:56AM +0200, Jiri Denemark wrote:
> 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.

OK, I'm queueing this series on x86-next.  Thanks for the
feedback!

-- 
Eduardo