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.dug...@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 n0...@n0ano.com Ph: 303/443-3786 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list