[libvirt PATCH 0/3] cpu: Honor check='full' for host-passthrough CPUs
See patch 3/3 for explanation. Jiri Denemark (3): cpu: Change control flow in virCPUUpdateLive cpu_x86: Prepare virCPUx86UpdateLive for easier extension cpu: Honor check='full' for host-passthrough CPUs src/cpu/cpu.c | 12 +++++++----- src/cpu/cpu_x86.c | 20 +++++++++++++++++--- 2 files changed, 24 insertions(+), 8 deletions(-) -- 2.25.1
The updateLive CPU sub-driver function is supposed to be called only for a subset of CPU definitions. Let's make it more obvious by turning a negative test and return into a positive check. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 6d6191fe4e..c461c4839d 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -647,13 +647,14 @@ virCPUUpdateLive(virArch arch, if (!driver->updateLive) return 1; - if (cpu->mode != VIR_CPU_MODE_CUSTOM) - return 1; + if (cpu->mode == VIR_CPU_MODE_CUSTOM) { + if (driver->updateLive(cpu, dataEnabled, dataDisabled) < 0) + return -1; - if (driver->updateLive(cpu, dataEnabled, dataDisabled) < 0) - return -1; + return 0; + } - return 0; + return 1; } -- 2.25.1
Adding more checks into the existing if statements would turn them into an unreadable mess. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index dca9ed2979..5a6b7bb1d8 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3036,9 +3036,15 @@ virCPUx86UpdateLive(virCPUDefPtr cpu, for (i = 0; i < map->nfeatures; i++) { virCPUx86FeaturePtr feature = map->features[i]; + virCPUFeaturePolicy expected = VIR_CPU_FEATURE_LAST; - if (x86DataIsSubset(&enabled, &feature->data) && - !x86DataIsSubset(&model->data, &feature->data)) { + if (x86DataIsSubset(&model->data, &feature->data)) + expected = VIR_CPU_FEATURE_REQUIRE; + else + expected = VIR_CPU_FEATURE_DISABLE; + + if (expected == VIR_CPU_FEATURE_DISABLE && + x86DataIsSubset(&enabled, &feature->data)) { VIR_DEBUG("Feature '%s' enabled by the hypervisor", feature->name); if (cpu->check == VIR_CPU_CHECK_FULL) virBufferAsprintf(&bufAdded, "%s,", feature->name); @@ -3048,7 +3054,7 @@ virCPUx86UpdateLive(virCPUDefPtr cpu, } if (x86DataIsSubset(&disabled, &feature->data) || - (x86DataIsSubset(&model->data, &feature->data) && + (expected == VIR_CPU_FEATURE_REQUIRE && !x86DataIsSubset(&enabled, &feature->data))) { VIR_DEBUG("Feature '%s' disabled by the hypervisor", feature->name); if (cpu->check == VIR_CPU_CHECK_FULL) -- 2.25.1
The check attribute was completely ignored for host-passthrough CPUs even if they explicitly requested some features to be enabled. For example, a domain with the following CPU definition <cpu mode='host-passthrough' check='full'> <feature policy='require' name='svm'/> </cpu> would happily start even when 'svm' cannot be enabled. Let's call virCPUArchUpdateLive for host-passthrough CPUs with VIR_CPU_CHECK_FULL to make sure the architecture specific code can validate the provided virtual CPU against the desired definition. https://bugzilla.redhat.com/show_bug.cgi?id=1515677 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu.c | 3 ++- src/cpu/cpu_x86.c | 10 +++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index c461c4839d..631c755391 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -647,7 +647,8 @@ virCPUUpdateLive(virArch arch, if (!driver->updateLive) return 1; - if (cpu->mode == VIR_CPU_MODE_CUSTOM) { + if (cpu->mode == VIR_CPU_MODE_CUSTOM || + cpu->check == VIR_CPU_CHECK_FULL) { if (driver->updateLive(cpu, dataEnabled, dataDisabled) < 0) return -1; diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 5a6b7bb1d8..7a8a2e3f3b 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3009,8 +3009,10 @@ virCPUx86UpdateLive(virCPUDefPtr cpu, virCPUDataPtr dataEnabled, virCPUDataPtr dataDisabled) { + bool hostPassthrough = cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH; virCPUx86MapPtr map; virCPUx86ModelPtr model = NULL; + virCPUx86ModelPtr modelDisabled = NULL; virCPUx86Data enabled = VIR_CPU_X86_DATA_INIT; virCPUx86Data disabled = VIR_CPU_X86_DATA_INIT; virBuffer bufAdded = VIR_BUFFER_INITIALIZER; @@ -3026,6 +3028,10 @@ virCPUx86UpdateLive(virCPUDefPtr cpu, if (!(model = x86ModelFromCPU(cpu, map, -1))) goto cleanup; + if (hostPassthrough && + !(modelDisabled = x86ModelFromCPU(cpu, map, VIR_CPU_FEATURE_DISABLE))) + goto cleanup; + if (dataEnabled && x86DataCopy(&enabled, &dataEnabled->data.x86) < 0) goto cleanup; @@ -3040,7 +3046,8 @@ virCPUx86UpdateLive(virCPUDefPtr cpu, if (x86DataIsSubset(&model->data, &feature->data)) expected = VIR_CPU_FEATURE_REQUIRE; - else + else if (!hostPassthrough || + x86DataIsSubset(&modelDisabled->data, &feature->data)) expected = VIR_CPU_FEATURE_DISABLE; if (expected == VIR_CPU_FEATURE_DISABLE && @@ -3101,6 +3108,7 @@ virCPUx86UpdateLive(virCPUDefPtr cpu, cleanup: x86ModelFree(model); + x86ModelFree(modelDisabled); virCPUx86DataClear(&enabled); virCPUx86DataClear(&disabled); VIR_FREE(added); -- 2.25.1
On a Thursday in 2020, Jiri Denemark wrote:
See patch 3/3 for explanation.
Jiri Denemark (3): cpu: Change control flow in virCPUUpdateLive cpu_x86: Prepare virCPUx86UpdateLive for easier extension cpu: Honor check='full' for host-passthrough CPUs
src/cpu/cpu.c | 12 +++++++----- src/cpu/cpu_x86.c | 20 +++++++++++++++++--- 2 files changed, 24 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jiri Denemark -
Ján Tomko