[libvirt] [PATCH 0/4] qemu: Validate guest CPU features before starting a domain
CPU features are usually checked by libvirt, but not if libvirt decides it should not check the CPU at all, which happens with host-passthrough CPUs, for example. Let's check all used CPU features are valid for all CPU definitions. https://bugzilla.redhat.com/show_bug.cgi?id=1460086 Jiri Denemark (4): cpu: Introduce virCPUValidateFeatures qemu: Validate guest CPU features before starting a domain cpu_s390: Implement virCPUValidateFeatures cpu_x86: Implement virCPUValidateFeatures src/cpu/cpu.c | 29 +++++++++++++++++++++++++++++ src/cpu/cpu.h | 9 +++++++++ src/cpu/cpu_s390.c | 29 +++++++++++++++++++++-------- src/cpu/cpu_x86.c | 23 +++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 4 ++++ 6 files changed, 87 insertions(+), 8 deletions(-) -- 2.14.1
This new API may be used to check whether all features used in a CPU definition are valid (e.g., libvirt knows their name, their policy is supported, etc.). Leaving this API unimplemented in an arch subdriver means libvirt does not restrict CPU features usable on the associated architectures. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu.c | 29 +++++++++++++++++++++++++++++ src/cpu/cpu.h | 9 +++++++++ src/libvirt_private.syms | 1 + 3 files changed, 39 insertions(+) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index a7c7c381b9..dc72ed42d8 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -1076,3 +1076,32 @@ virCPUCopyMigratable(virArch arch, else return virCPUDefCopy(cpu); } + + +/** + * virCPUValidateFeatures: + * + * @arch: CPU architecture + * @cpu: CPU definition to be checked + * + * Checks whether all CPU features specified in @cpu are valid. + * + * Returns 0 on success (all features are valid), -1 on error. + */ +int +virCPUValidateFeatures(virArch arch, + virCPUDefPtr cpu) +{ + struct cpuArchDriver *driver; + + VIR_DEBUG("arch=%s, cpu=%p, nfeatures=%zu", + virArchToString(arch), cpu, cpu->nfeatures); + + if (!(driver = cpuGetSubDriver(arch))) + return -1; + + if (driver->validateFeatures) + return driver->validateFeatures(cpu); + else + return 0; +} diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 5dda46ee70..5daff186c4 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -121,6 +121,9 @@ typedef int typedef virCPUDefPtr (*virCPUArchCopyMigratable)(virCPUDefPtr cpu); +typedef int +(*virCPUArchValidateFeatures)(virCPUDefPtr cpu); + struct cpuArchDriver { const char *name; const virArch *arch; @@ -142,6 +145,7 @@ struct cpuArchDriver { virCPUArchConvertLegacy convertLegacy; virCPUArchExpandFeatures expandFeatures; virCPUArchCopyMigratable copyMigratable; + virCPUArchValidateFeatures validateFeatures; }; @@ -258,6 +262,11 @@ virCPUDefPtr virCPUCopyMigratable(virArch arch, virCPUDefPtr cpu); +int +virCPUValidateFeatures(virArch arch, + virCPUDefPtr cpu) + ATTRIBUTE_NONNULL(2); + /* virCPUDataFormat and virCPUDataParse are implemented for unit tests only and * have no real-life usage */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 888e4e329b..6670c5fc92 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1097,6 +1097,7 @@ virCPUProbeHost; virCPUTranslate; virCPUUpdate; virCPUUpdateLive; +virCPUValidateFeatures; # cpu/cpu_x86.h -- 2.14.1
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7e9b406b6a..d3155e4e75 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4659,6 +4659,10 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, if (qemuProcessStartValidateShmem(vm) < 0) return -1; + if (vm->def->cpu && + virCPUValidateFeatures(vm->def->os.arch, vm->def->cpu) < 0) + return -1; + VIR_DEBUG("Checking for any possible (non-fatal) issues"); qemuProcessStartWarnShmem(vm); -- 2.14.1
Only feature policy is checked on s390, which was previously done in virCPUUpdate, but that's not the correct place for the check once we have virCPUValidateFeatures. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_s390.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c index 2ef03367d7..3d10f920ba 100644 --- a/src/cpu/cpu_s390.c +++ b/src/cpu/cpu_s390.c @@ -78,14 +78,6 @@ virCPUs390Update(virCPUDefPtr guest, goto cleanup; for (i = 0; i < guest->nfeatures; i++) { - if (guest->features[i].policy == VIR_CPU_FEATURE_OPTIONAL) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("only cpu feature policies 'require' and " - "'disable' are supported for %s"), - guest->features[i].name); - goto cleanup; - } - if (virCPUDefUpdateFeature(updated, guest->features[i].name, guest->features[i].policy) < 0) @@ -102,6 +94,26 @@ virCPUs390Update(virCPUDefPtr guest, return ret; } + +static int +virCPUs390ValidateFeatures(virCPUDefPtr cpu) +{ + size_t i; + + for (i = 0; i < cpu->nfeatures; i++) { + if (cpu->features[i].policy == VIR_CPU_FEATURE_OPTIONAL) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("only cpu feature policies 'require' and " + "'disable' are supported for %s"), + cpu->features[i].name); + return -1; + } + } + + return 0; +} + + struct cpuArchDriver cpuDriverS390 = { .name = "s390", .arch = archs, @@ -111,4 +123,5 @@ struct cpuArchDriver cpuDriverS390 = { .encode = NULL, .baseline = NULL, .update = virCPUs390Update, + .validateFeatures = virCPUs390ValidateFeatures, }; -- 2.14.1
The function checks whether all CPU features used in a CPU definition are specified in cpu_map.xml. https://bugzilla.redhat.com/show_bug.cgi?id=1460086 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 2864454211..5ce205f9c1 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2925,6 +2925,28 @@ virCPUx86CopyMigratable(virCPUDefPtr cpu) } +static int +virCPUx86ValidateFeatures(virCPUDefPtr cpu) +{ + virCPUx86MapPtr map; + size_t i; + + if (!(map = virCPUx86GetMap())) + return -1; + + for (i = 0; i < cpu->nfeatures; i++) { + if (!x86FeatureFind(map, cpu->features[i].name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown CPU feature: %s"), + cpu->features[i].name); + return -1; + } + } + + return 0; +} + + int virCPUx86DataAddCPUID(virCPUDataPtr cpuData, const virCPUx86CPUID *cpuid) @@ -3001,4 +3023,5 @@ struct cpuArchDriver cpuDriverX86 = { .translate = virCPUx86Translate, .expandFeatures = virCPUx86ExpandFeatures, .copyMigratable = virCPUx86CopyMigratable, + .validateFeatures = virCPUx86ValidateFeatures, }; -- 2.14.1
On Thu, Sep 14, 2017 at 04:22:59PM +0200, Jiri Denemark wrote:
CPU features are usually checked by libvirt, but not if libvirt decides it should not check the CPU at all, which happens with host-passthrough CPUs, for example. Let's check all used CPU features are valid for all CPU definitions.
https://bugzilla.redhat.com/show_bug.cgi?id=1460086
Jiri Denemark (4): cpu: Introduce virCPUValidateFeatures qemu: Validate guest CPU features before starting a domain cpu_s390: Implement virCPUValidateFeatures cpu_x86: Implement virCPUValidateFeatures
src/cpu/cpu.c | 29 +++++++++++++++++++++++++++++ src/cpu/cpu.h | 9 +++++++++ src/cpu/cpu_s390.c | 29 +++++++++++++++++++++-------- src/cpu/cpu_x86.c | 23 +++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 4 ++++ 6 files changed, 87 insertions(+), 8 deletions(-)
ACK series Jan
participants (2)
-
Jiri Denemark -
Ján Tomko