[libvirt] [PATCH] Enable support for nested SVM

[Eric Blake]

On 10/12/2010 04:46 AM, Daniel P. Berrange wrote:
> This enables support for nested SVM using the regular CPU
> model/features block. If the CPU model or features include
> 'svm', then the '-enable-nesting' flag will be added to the
> QEMU command line. Latest out of tree patches for nested
> 'vmx', no longer require the '-enable-nesting' flag. They
> instead just look at the cpu features. Several of the models
> already include svm support, but QEMU was just masking out
> the svm bit silently. So this will enable SVM on such
> models
>
> +
> +bool
> +cpuHasFeature(const char *arch,
> +              const union cpuData *data,
> +              const char *feature)
> +{
> +    struct cpuArchDriver *driver;
> +
> +    VIR_DEBUG("arch=%s, data=%p, feature=%s",
> +              arch, data, feature);
> +
> +    if ((driver = cpuGetSubDriver(arch)) == NULL)
> +        return -1;

Ouch.  -1 promotes to true.

> +
> +    if (driver->hasFeature == NULL) {
> +        virCPUReportError(VIR_ERR_NO_SUPPORT,
> +                _("cannot check guest CPU data for %s architecture"),
> +                          arch);
> +        return -1;

Likewise.

> +    }
> +
> +    return driver->hasFeature(data, feature);
> +}

You either need to change the return type to int and take a bool* 
parameter (return -1 on failure, 0 on success when *param was written 
to), or return an int value rather than a bool; since the caller needs 
to distinguish between feature-not-present and error-message-reported.

> +
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index a745917..3a7b996 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -82,6 +82,10 @@ typedef int
>   (*cpuArchUpdate)    (virCPUDefPtr guest,
>                        const virCPUDefPtr host);
>
> +typedef bool
> +(*cpuArchHasFeature) (const union cpuData *data,
> +                      const char *feature);

But this typedef is okay.

...
> +    for (i = 0 ; i<  feature->ncpuid ; i++) {
> +        struct cpuX86cpuid *cpuid;
> +
> +        cpuid = x86DataCpuid(data, feature->cpuid[i].function);
> +        if (cpuid&&  x86cpuidMatchMasked(cpuid, feature->cpuid + i)) {
> +            ret = true;
> +            goto cleanup;

I probably would have used 'break' rather than 'goto cleanup', since 
it's shorter, but since you already have to have the label due to 
earlier code in the method, either way is fine.

> +        }
> +    }
> +
> +cleanup:
> +    x86MapFree(map);
> +    return ret;
> +}


> +++ b/src/qemu/qemu_conf.c
> @@ -1210,6 +1210,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help,
>           flags |= QEMUD_CMD_FLAG_NO_KVM_PIT;
>       if (strstr(help, "-tdf"))
>           flags |= QEMUD_CMD_FLAG_TDF;
> +    if (strstr(help, "-enable-nesting"))
> +        flags |= QEMUD_CMD_FLAG_NESTING;
>       if (strstr(help, ",menu=on"))
>           flags |= QEMUD_CMD_FLAG_BOOT_MENU;
 >

Any reason you didn't put the new feature at the end of the list, in 
enum order?

> @@ -3555,6 +3560,12 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver,
>           if (cpuDecode(guest, data, cpus, ncpus, preferred)<  0)
>               goto cleanup;
>
> +        /* Only 'svm' requires --enable-nesting. The out-of-tree
> +         * 'vmx' patches now simply hook off the CPU features

This comment will be out-of-date once the vmx patches are no longer out 
of tree.  Should we just say:

"Nested vmx support in qemu simply hooks off the CPU features"

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org