[libvirt] [PATCH] Expose all CPU features in host definition

[Don Dugger]

On Sat, May 25, 2013 at 11:45:13PM +0200, Martin Kletzander wrote:
> On 05/25/2013 12:44 AM, Don Dugger wrote:
> > I've opened BZ 697141 on this as I would consider it more
> > a bug than a feature request.  Anyway, to re-iterate my
> > rationale from the BZ:
> > 
> > 
> > The virConnectGetCapabilities API describes the host capabilities
> > by returning an XML description that includes the CPU model name
> > and a set of CPU features.  The problem is that any features that
> > are part of the CPU model are not explicitly listed, they are
> > assumed to be part of the definition of that CPU model.  This
> > makes it extremely difficult for the caller of this API to check
> > for the presence of a specific CPU feature, the caller would have
> > to know what features are part of which CPU models, a very
> > daunting task.
> > 
> > This patch solves this problem by having the API return a model
> > name, as it currently does, but it will also explicitly list all
> > of the CPU features that are present.  This would make it much
> > easier for a caller of this API to check for specific features.
> > 
> > Signed-off-by: Don Dugger <donald.d.dugger at intel.com>
> > 
> 
> I'm generally not against exposing CPU model features in capabilities,
> but if we do this, such features should be distinguishable from those
> not in the model.  Of course we don't want users to go to
> /usr/share/libvirt/cpu_map.xml all the time, but maybe there could be
> separate API for that.  If not, then it should be encapsulated somewhere
> else than side by side the other features.

I guess I don't understand why there's a need to distinguish between
features in a model and other features.  The important bits are the
actual features themselves.  A model is a convenient shorthand for
a set of features but that's all it is, a shorthand.  The real
information is the specific features that are present on that CPU.

Knowing that a CPU is a Westmere model is interesting but what I
really want to know is whether the CPU supports SSE3 instructions
so that I know this is an appropriate CPU to be running my
multimedia application on.  Listing all the features provides that
info in an easy to consume fashion.

> 
> > ---
> >  src/cpu/cpu_x86.c |   30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> > index 5d479c2..b2e16df 100644
> > --- a/src/cpu/cpu_x86.c
> > +++ b/src/cpu/cpu_x86.c
> > @@ -1296,6 +1296,35 @@ x86GuestData(virCPUDefPtr host,
> >      return x86Compute(host, guest, data, message);
> >  }
> >  
> > +static void
> > +x86AddFeatures(virCPUDefPtr cpu,
> > +	       struct x86_map *map)
> > +{
> > +    const struct x86_model *candidate;
> > +    const struct x86_feature *feature = map->features;
> > +
> > +    candidate = map->models;
> > +    while (candidate != NULL) {
> > +	if (STREQ(cpu->model, candidate->name))
> 
> Don't indent with TABs, there's even a 'make syntax-check' rule for that.

I was raised that tabs are the one true indention style but,
if the libvirt code base prefers spaces, I'll bite my tongue
and do it :-)

> 
> > +	    break;
> > +	candidate = candidate->next;
> > +    }
> > +    if (!candidate) {
> > +	VIR_WARN("Odd, %s not a known CPU model\n", cpu->model);
> > +	return;
> 
> Warning seems inappropriate here as this is actually an error.

Agreed.

> 
> > +    }
> > +    while (feature != NULL) {
> > +	if (x86DataIsSubset(candidate->data, feature->data)) {
> > +	    if (virCPUDefAddFeature(cpu, feature->name, VIR_CPU_FEATURE_DISABLE) < 0) {
> > +		VIR_WARN("CPU model %s, no room for feature %s", cpu->model, feature->name);
> > +		return;
> 
> This code path shadows an error and means the feature will not be
> mentioned in the capabilities, but the API will end successfully.

I was trying to not through fatal errors but, on reflection, I think
I agree here also.  I'll spin a new patch incorporating these
suggestions.

> 
> Martin
> 

-- 
Don Dugger
"Censeo Toto nos in Kansa esse decisse." - D. Gale
n0ano at n0ano.com
Ph: 303/443-3786