[libvirt] [PATCH] Enable support for nested SVM
Eric Blake
eblake at redhat.com
Tue Oct 12 14:23:21 UTC 2010
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
More information about the libvir-list
mailing list