[libvirt] [PATCH v3 5/6] xenconfig: add CPUID handling to domXML <-> xl.cfg conversion

[Jim Fehlig]

On 12/09/2017 07:10 PM, Marek Marczykowski-Górecki wrote:
> Only "libxl" format supported for now. Special care needed around
> vmx/svm, because those two are translated into "nestedhvm" setting.
> 
> ---
> Changes since v2:
>   - new patch
> ---
>   src/xenconfig/xen_xl.c | 168 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 168 insertions(+)
> 
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 356ca3a..dc06d4a 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -259,6 +259,94 @@ xenTranslateCPUFeature(const char *feature_name, bool from_libxl)
>       return feature_name;
>   }
>   
> +static int
> +xenParseXLCPUID(virConfPtr conf, virDomainDefPtr def)
> +{
> +    const char *cpuid_str = NULL;
> +    char **cpuid_pairs = NULL;
> +    char **name_and_value = NULL;
> +    size_t i;
> +    int ret = -1;
> +    int policy;
> +
> +    if (xenConfigGetString(conf, "cpuid", &cpuid_str, NULL) < 0)
> +        return -1;
> +
> +    if (!cpuid_str)
> +        return 0;
> +
> +    if (!def->cpu) {
> +        if (VIR_ALLOC(def->cpu) < 0)
> +            goto cleanup;
> +        def->cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
> +        def->cpu->type = VIR_CPU_TYPE_GUEST;
> +        def->cpu->nfeatures = 0;
> +        def->cpu->nfeatures_max = 0;
> +    }
> +
> +    cpuid_pairs = virStringSplit(cpuid_str, ",", 0);
> +    if (!cpuid_pairs)
> +        return -1;

goto cleanup; ?

> +
> +    if (!cpuid_pairs[0]) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Empty cpuid string"));
> +        goto cleanup;
> +    }

Does xl ignore an empty cpuid string? If so, we should ignore it too.

> +
> +    if (STRNEQ(cpuid_pairs[0], "host")) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("cpuid starting with %s is not supported, only libxl format is"),
> +                       cpuid_pairs[0]);
> +        goto cleanup;
> +    }

It looks like we use VIR_ERR_INTERNAL_ERROR liberally throughout this file even 
when a more specific error could be reported. E.g. in this case VIR_ERR_CONF_SYNTAX.

> +
> +    for (i = 1; cpuid_pairs[i]; i++) {
> +        name_and_value = virStringSplit(cpuid_pairs[i], "=", 2);
> +        if (!name_and_value)
> +            goto cleanup;
> +        if (!name_and_value[0] || !name_and_value[1]) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Invalid libxl cpuid key=value element: %s"),
> +                           cpuid_pairs[i]);

VIR_ERR_CONF_SYNTAX?

> +            goto cleanup;
> +        }
> +        if (STREQ(name_and_value[1], "1")) {
> +            policy = VIR_CPU_FEATURE_FORCE;
> +        } else if (STREQ(name_and_value[1], "0")) {
> +            policy = VIR_CPU_FEATURE_DISABLE;
> +        } else if (STREQ(name_and_value[1], "x")) {
> +            policy = VIR_CPU_FEATURE_OPTIONAL;
> +        } else if (STREQ(name_and_value[1], "k")) {
> +            policy = VIR_CPU_FEATURE_OPTIONAL;
> +        } else if (STREQ(name_and_value[1], "s")) {
> +            policy = VIR_CPU_FEATURE_OPTIONAL;
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Invalid libxl cpuid value: %s"),
> +                           cpuid_pairs[i]);

Same.

> +            goto cleanup;
> +        }
> +
> +        if (virCPUDefAddFeature(def->cpu,
> +                                xenTranslateCPUFeature(name_and_value[0], true),
> +                                policy) < 0)
> +            goto cleanup;
> +
> +        virStringListFree(name_and_value);
> +        name_and_value = NULL;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (name_and_value)
> +        virStringListFree(name_and_value);
> +    if (cpuid_pairs)
> +        virStringListFree(cpuid_pairs);

It's safe to call virStringListFree with NULL.

> +    return ret;
> +}
> +
>   
>   static int
>   xenParseXLSpice(virConfPtr conf, virDomainDefPtr def)
> @@ -1094,6 +1182,9 @@ xenParseXL(virConfPtr conf,
>           goto cleanup;
>   #endif
>   
> +    if (xenParseXLCPUID(conf, def) < 0)
> +        goto cleanup;
> +
>       if (xenParseXLDisk(conf, def) < 0)
>           goto cleanup;
>   
> @@ -1243,6 +1334,80 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def)
>       return 0;
>   }
>   
> +static int
> +xenFormatXLCPUID(virConfPtr conf, virDomainDefPtr def)
> +{
> +    char **cpuid_pairs = NULL;
> +    char *cpuid_string = NULL;
> +    size_t i, j;
> +    int ret = -1;
> +
> +    if (!def->cpu)
> +        return 0;
> +
> +    if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("'%s' CPU mode not supported, only host-passthrough is"),
> +                       virCPUModeTypeToString(def->cpu->mode));
> +        goto cleanup;
> +    }
> +
> +    /* "host" + all features + NULL */
> +    if (VIR_ALLOC_N(cpuid_pairs, def->cpu->nfeatures + 2) < 0)
> +        goto cleanup;

Nit: no need to jump to cleanup before this point since there is nothing to cleanup.

Regards,
Jim

> +
> +    if (VIR_STRDUP(cpuid_pairs[0], "host") < 0)
> +        goto cleanup;
> +
> +    j = 1;
> +    for (i = 0; i < def->cpu->nfeatures; i++) {
> +        const char *feature_name = xenTranslateCPUFeature(
> +                def->cpu->features[i].name,
> +                false);
> +        const char *policy = NULL;
> +
> +        if (STREQ(feature_name, "vmx") || STREQ(feature_name, "svm"))
> +            /* ignore vmx/svm in cpuid option, translated into nestedhvm
> +             * elsewhere */
> +            continue;
> +
> +        switch (def->cpu->features[i].policy) {
> +            case VIR_CPU_FEATURE_FORCE:
> +            case VIR_CPU_FEATURE_REQUIRE:
> +                policy = "1";
> +                break;
> +            case VIR_CPU_FEATURE_OPTIONAL:
> +                policy = "x";
> +                break;
> +            case VIR_CPU_FEATURE_DISABLE:
> +            case VIR_CPU_FEATURE_FORBID:
> +                policy = "0";
> +                break;
> +        }
> +        if (virAsprintf(&cpuid_pairs[j++], "%s=%s",
> +                        feature_name,
> +                        policy) < 0)
> +            goto cleanup;
> +    }
> +    cpuid_pairs[j] = NULL;
> +
> +    if (j > 1) {
> +        cpuid_string = virStringListJoin((const char **)cpuid_pairs, ",");
> +        if (!cpuid_string)
> +            goto cleanup;
> +
> +        if (xenConfigSetString(conf, "cpuid", cpuid_string) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    virStringListFree(cpuid_pairs);
> +    VIR_FREE(cpuid_string);
> +    return ret;
> +}
> +
>   #ifdef LIBXL_HAVE_VNUMA
>   static int
>   xenFormatXLVnode(virConfValuePtr list,
> @@ -2008,6 +2173,9 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn)
>       if (xenFormatXLOS(conf, def) < 0)
>           goto cleanup;
>   
> +    if (xenFormatXLCPUID(conf, def) < 0)
> +        goto cleanup;
> +
>   #ifdef LIBXL_HAVE_VNUMA
>       if (xenFormatXLDomainVnuma(conf, def) < 0)
>           goto cleanup;
>