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(a)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(a)n0ano.com
Ph: 303/443-3786