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