[libvirt] [PATCH 0/3] some additional hyper-v features fixes
Pavel Hrdina (3): docs: fix qemu version for hyperv features qemu_process: skip only cpu features qemu_process: add check for hyperv features docs/formatdomain.html.in | 4 ++-- src/cpu/cpu_x86.c | 8 +++++++ src/cpu/cpu_x86_data.h | 8 +++++++ src/qemu/qemu_process.c | 57 +++++++++++++++++++++++++++++++++++------------ 4 files changed, 61 insertions(+), 16 deletions(-) -- 2.7.4
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.html.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a1d7c4a..957b839 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1558,13 +1558,13 @@ <td>synic</td> <td>Enable Synthetic Interrupt Controller (SyNIC)</td> <td> on, off</td> - <td><span class="since">1.3.3 (QEMU 2.5)</span></td> + <td><span class="since">1.3.3 (QEMU 2.6)</span></td> </tr> <tr> <td>stimer</td> <td>Enable SyNIC timers</td> <td> on, off</td> - <td><span class="since">1.3.3 (QEMU 2.5)</span></td> + <td><span class="since">1.3.3 (QEMU 2.6)</span></td> </tr> <tr> <td>reset</td> -- 2.7.4
On 03/29/2016 09:31 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.html.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK - Read that one wrong yesterday John
This check is there to allow restore saved domain with older libvirt where we included invtsc by default for host-passthrough model. Don't skip the whole function, but only the part that checks for invtsc. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_process.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 205d9ae..9334a75 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3904,11 +3904,6 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, bool ret = false; size_t i; - /* no features are passed to QEMU with -cpu host - * so it makes no sense to verify them */ - if (def->cpu && def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) - return true; - switch (arch) { case VIR_ARCH_I686: case VIR_ARCH_X86_64: @@ -3933,17 +3928,20 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, } } - for (i = 0; def->cpu && i < def->cpu->nfeatures; i++) { - virCPUFeatureDefPtr feature = &def->cpu->features[i]; - if (feature->policy != VIR_CPU_FEATURE_REQUIRE) - continue; + if (def->cpu && def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + for (i = 0; i < def->cpu->nfeatures; i++) { + virCPUFeatureDefPtr feature = &def->cpu->features[i]; - if (STREQ(feature->name, "invtsc") && - !cpuHasFeature(guestcpu, feature->name)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("host doesn't support invariant TSC")); - goto cleanup; + if (feature->policy != VIR_CPU_FEATURE_REQUIRE) + continue; + + if (STREQ(feature->name, "invtsc") && + !cpuHasFeature(guestcpu, feature->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("host doesn't support invariant TSC")); + goto cleanup; + } } } break; -- 2.7.4
Commit 7068b56c introduced several hyperv features. Not all hyperv features are supported by old enough kernels and we shouldn't allow to start a guest if kernel doesn't support any of the hyperv feature. There is one exception, for backward compatibility we cannot error out if one of the RELAXED, VAPIC or SPINLOCKS isn't supported, for the same reason we ignore invtsc, to not break restoring saved domains with older libvirt. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/cpu/cpu_x86.c | 8 ++++++++ src/cpu/cpu_x86_data.h | 8 ++++++++ src/qemu/qemu_process.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 90949f6..b7f1690 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -75,6 +75,14 @@ static const struct x86_kvm_feature x86_kvm_features[] = {VIR_CPU_x86_KVM_PV_UNHALT, { .function = 0x40000001, .eax = 0x00000080 }}, {VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT, { .function = 0x40000001, .eax = 0x01000000 }}, + {VIR_CPU_x86_KVM_HV_RUNTIME, { .function = 0x40000003, .eax = 0x00000001 }}, + {VIR_CPU_x86_KVM_HV_SYNIC, { .function = 0x40000003, .eax = 0x00000004 }}, + {VIR_CPU_x86_KVM_HV_STIMER, { .function = 0x40000003, .eax = 0x00000008 }}, + {VIR_CPU_x86_KVM_HV_RELAXED, { .function = 0x40000003, .eax = 0x00000020 }}, + {VIR_CPU_x86_KVM_HV_SPINLOCK, { .function = 0x40000003, .eax = 0x00000022 }}, + {VIR_CPU_x86_KVM_HV_VAPIC, { .function = 0x40000003, .eax = 0x00000030 }}, + {VIR_CPU_x86_KVM_HV_VPINDEX, { .function = 0x40000003, .eax = 0x00000040 }}, + {VIR_CPU_x86_KVM_HV_RESET, { .function = 0x40000003, .eax = 0x00000080 }}, }; struct x86_model { diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h index 88dccf6..777cc8d 100644 --- a/src/cpu/cpu_x86_data.h +++ b/src/cpu/cpu_x86_data.h @@ -48,6 +48,14 @@ struct _virCPUx86CPUID { # define VIR_CPU_x86_KVM_PV_EOI "__kvm_pv_eoi" # define VIR_CPU_x86_KVM_PV_UNHALT "__kvm_pv_unhalt" # define VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT "__kvm_clocksource_stable" +# define VIR_CPU_x86_KVM_HV_RUNTIME "__kvm_hv_runtime" +# define VIR_CPU_x86_KVM_HV_SYNIC "__kvm_hv_synic" +# define VIR_CPU_x86_KVM_HV_STIMER "__kvm_hv_stimer" +# define VIR_CPU_x86_KVM_HV_RELAXED "__kvm_hv_relaxed" +# define VIR_CPU_x86_KVM_HV_SPINLOCK "__kvm_hv_spinlock" +# define VIR_CPU_x86_KVM_HV_VAPIC "__kvm_hv_vapic" +# define VIR_CPU_x86_KVM_HV_VPINDEX "__kvm_hv_vpindex" +# define VIR_CPU_x86_KVM_HV_RESET "__kvm_hv_reset" typedef struct _virCPUx86Data virCPUx86Data; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9334a75..07b9df2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3928,6 +3928,37 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, } } + for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { + if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON) { + char *cpuFeature; + if (virAsprintf(&cpuFeature, "__kvm_hv_%s", + virDomainHypervTypeToString(i)) < 0) + goto cleanup; + if (!cpuHasFeature(guestcpu, cpuFeature)) { + switch ((virDomainHyperv) i) { + case VIR_DOMAIN_HYPERV_RELAXED: + case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_SPINLOCKS: + VIR_WARN("host doesn't support hyperv '%s' feature", + virDomainHypervTypeToString(i)); + break; + case VIR_DOMAIN_HYPERV_VPINDEX: + case VIR_DOMAIN_HYPERV_RUNTIME: + case VIR_DOMAIN_HYPERV_SYNIC: + case VIR_DOMAIN_HYPERV_STIMER: + case VIR_DOMAIN_HYPERV_RESET: + case VIR_DOMAIN_HYPERV_VENDOR_ID: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("host doesn't support hyperv '%s' feature"), + virDomainHypervTypeToString(i)); + goto cleanup; + break; + case VIR_DOMAIN_HYPERV_LAST: + break; + } + } + } + } if (def->cpu && def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { for (i = 0; i < def->cpu->nfeatures; i++) { -- 2.7.4
On 03/29/2016 09:31 AM, Pavel Hrdina wrote:
Commit 7068b56c introduced several hyperv features. Not all hyperv features are supported by old enough kernels and we shouldn't allow to start a guest if kernel doesn't support any of the hyperv feature.
There is one exception, for backward compatibility we cannot error out if one of the RELAXED, VAPIC or SPINLOCKS isn't supported, for the same reason we ignore invtsc, to not break restoring saved domains with older libvirt.
From yesterday's dialog, there's also commit id '59fc0d06' which adds "hv_crash" and commit id '600bca59' which adds "hv_time".
Neither is handled via these bits, but wouldn't both fall into the same trap since both were added after commit '2e8f9080'?
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/cpu/cpu_x86.c | 8 ++++++++ src/cpu/cpu_x86_data.h | 8 ++++++++ src/qemu/qemu_process.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+)
Now I was going to ask about a capability bit for this, but seeing none in previous commits, I thought I was safe... I guess not. The rest looks OK to me, although I wonder if there's a way to avoid missing this for future changes John
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 90949f6..b7f1690 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -75,6 +75,14 @@ static const struct x86_kvm_feature x86_kvm_features[] = {VIR_CPU_x86_KVM_PV_UNHALT, { .function = 0x40000001, .eax = 0x00000080 }}, {VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT, { .function = 0x40000001, .eax = 0x01000000 }}, + {VIR_CPU_x86_KVM_HV_RUNTIME, { .function = 0x40000003, .eax = 0x00000001 }}, + {VIR_CPU_x86_KVM_HV_SYNIC, { .function = 0x40000003, .eax = 0x00000004 }}, + {VIR_CPU_x86_KVM_HV_STIMER, { .function = 0x40000003, .eax = 0x00000008 }}, + {VIR_CPU_x86_KVM_HV_RELAXED, { .function = 0x40000003, .eax = 0x00000020 }}, + {VIR_CPU_x86_KVM_HV_SPINLOCK, { .function = 0x40000003, .eax = 0x00000022 }}, + {VIR_CPU_x86_KVM_HV_VAPIC, { .function = 0x40000003, .eax = 0x00000030 }}, + {VIR_CPU_x86_KVM_HV_VPINDEX, { .function = 0x40000003, .eax = 0x00000040 }}, + {VIR_CPU_x86_KVM_HV_RESET, { .function = 0x40000003, .eax = 0x00000080 }}, };
struct x86_model { diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h index 88dccf6..777cc8d 100644 --- a/src/cpu/cpu_x86_data.h +++ b/src/cpu/cpu_x86_data.h @@ -48,6 +48,14 @@ struct _virCPUx86CPUID { # define VIR_CPU_x86_KVM_PV_EOI "__kvm_pv_eoi" # define VIR_CPU_x86_KVM_PV_UNHALT "__kvm_pv_unhalt" # define VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT "__kvm_clocksource_stable" +# define VIR_CPU_x86_KVM_HV_RUNTIME "__kvm_hv_runtime" +# define VIR_CPU_x86_KVM_HV_SYNIC "__kvm_hv_synic" +# define VIR_CPU_x86_KVM_HV_STIMER "__kvm_hv_stimer" +# define VIR_CPU_x86_KVM_HV_RELAXED "__kvm_hv_relaxed" +# define VIR_CPU_x86_KVM_HV_SPINLOCK "__kvm_hv_spinlock" +# define VIR_CPU_x86_KVM_HV_VAPIC "__kvm_hv_vapic" +# define VIR_CPU_x86_KVM_HV_VPINDEX "__kvm_hv_vpindex" +# define VIR_CPU_x86_KVM_HV_RESET "__kvm_hv_reset"
typedef struct _virCPUx86Data virCPUx86Data; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9334a75..07b9df2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3928,6 +3928,37 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, } }
+ for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { + if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON) { + char *cpuFeature; + if (virAsprintf(&cpuFeature, "__kvm_hv_%s", + virDomainHypervTypeToString(i)) < 0) + goto cleanup; + if (!cpuHasFeature(guestcpu, cpuFeature)) { + switch ((virDomainHyperv) i) { + case VIR_DOMAIN_HYPERV_RELAXED: + case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_SPINLOCKS: + VIR_WARN("host doesn't support hyperv '%s' feature", + virDomainHypervTypeToString(i)); + break; + case VIR_DOMAIN_HYPERV_VPINDEX: + case VIR_DOMAIN_HYPERV_RUNTIME: + case VIR_DOMAIN_HYPERV_SYNIC: + case VIR_DOMAIN_HYPERV_STIMER: + case VIR_DOMAIN_HYPERV_RESET: + case VIR_DOMAIN_HYPERV_VENDOR_ID: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("host doesn't support hyperv '%s' feature"), + virDomainHypervTypeToString(i)); + goto cleanup; + break; + case VIR_DOMAIN_HYPERV_LAST: + break; + } + } + } + }
if (def->cpu && def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { for (i = 0; i < def->cpu->nfeatures; i++) {
On Tue, Mar 29, 2016 at 11:10:42AM -0400, John Ferlan wrote:
On 03/29/2016 09:31 AM, Pavel Hrdina wrote:
Commit 7068b56c introduced several hyperv features. Not all hyperv features are supported by old enough kernels and we shouldn't allow to start a guest if kernel doesn't support any of the hyperv feature.
There is one exception, for backward compatibility we cannot error out if one of the RELAXED, VAPIC or SPINLOCKS isn't supported, for the same reason we ignore invtsc, to not break restoring saved domains with older libvirt.
From yesterday's dialog, there's also commit id '59fc0d06' which adds "hv_crash" and commit id '600bca59' which adds "hv_time".
Neither is handled via these bits, but wouldn't both fall into the same trap since both were added after commit '2e8f9080'?
Even though libvirt passes those options to QEMU in the same -cpu argument, they are unrelated to this patch series. And for the same reason as for RELAXED, VAPIC and SPINLOCKS, we cannot error out to not break things so there is nothing to do about it.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/cpu/cpu_x86.c | 8 ++++++++ src/cpu/cpu_x86_data.h | 8 ++++++++ src/qemu/qemu_process.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+)
Now I was going to ask about a capability bit for this, but seeing none in previous commits, I thought I was safe... I guess not.
I don't know what you mean by this, what capability? Pavel
[...]
Now I was going to ask about a capability bit for this, but seeing none in previous commits, I thought I was safe... I guess not.
I don't know what you mean by this, what capability?
Sorry - context was I was thinking about the code I looked at yesterday that didn't have any capability checking for the bits in previous commits for hyperv feature bits. Not this patch. John
29.03.2016 16:31, Pavel Hrdina пишет:
Commit 7068b56c introduced several hyperv features. Not all hyperv features are supported by old enough kernels and we shouldn't allow to start a guest if kernel doesn't support any of the hyperv feature.
There is one exception, for backward compatibility we cannot error out if one of the RELAXED, VAPIC or SPINLOCKS isn't supported, for the same reason we ignore invtsc, to not break restoring saved domains with older libvirt.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/cpu/cpu_x86.c | 8 ++++++++ src/cpu/cpu_x86_data.h | 8 ++++++++ src/qemu/qemu_process.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 90949f6..b7f1690 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -75,6 +75,14 @@ static const struct x86_kvm_feature x86_kvm_features[] = {VIR_CPU_x86_KVM_PV_UNHALT, { .function = 0x40000001, .eax = 0x00000080 }}, {VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT, { .function = 0x40000001, .eax = 0x01000000 }}, + {VIR_CPU_x86_KVM_HV_RUNTIME, { .function = 0x40000003, .eax = 0x00000001 }}, + {VIR_CPU_x86_KVM_HV_SYNIC, { .function = 0x40000003, .eax = 0x00000004 }}, + {VIR_CPU_x86_KVM_HV_STIMER, { .function = 0x40000003, .eax = 0x00000008 }}, + {VIR_CPU_x86_KVM_HV_RELAXED, { .function = 0x40000003, .eax = 0x00000020 }}, + {VIR_CPU_x86_KVM_HV_SPINLOCK, { .function = 0x40000003, .eax = 0x00000022 }}, + {VIR_CPU_x86_KVM_HV_VAPIC, { .function = 0x40000003, .eax = 0x00000030 }}, + {VIR_CPU_x86_KVM_HV_VPINDEX, { .function = 0x40000003, .eax = 0x00000040 }}, + {VIR_CPU_x86_KVM_HV_RESET, { .function = 0x40000003, .eax = 0x00000080 }}, };
struct x86_model { diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h index 88dccf6..777cc8d 100644 --- a/src/cpu/cpu_x86_data.h +++ b/src/cpu/cpu_x86_data.h @@ -48,6 +48,14 @@ struct _virCPUx86CPUID { # define VIR_CPU_x86_KVM_PV_EOI "__kvm_pv_eoi" # define VIR_CPU_x86_KVM_PV_UNHALT "__kvm_pv_unhalt" # define VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT "__kvm_clocksource_stable" +# define VIR_CPU_x86_KVM_HV_RUNTIME "__kvm_hv_runtime" +# define VIR_CPU_x86_KVM_HV_SYNIC "__kvm_hv_synic" +# define VIR_CPU_x86_KVM_HV_STIMER "__kvm_hv_stimer" +# define VIR_CPU_x86_KVM_HV_RELAXED "__kvm_hv_relaxed" +# define VIR_CPU_x86_KVM_HV_SPINLOCK "__kvm_hv_spinlock" +# define VIR_CPU_x86_KVM_HV_VAPIC "__kvm_hv_vapic" +# define VIR_CPU_x86_KVM_HV_VPINDEX "__kvm_hv_vpindex" +# define VIR_CPU_x86_KVM_HV_RESET "__kvm_hv_reset"
typedef struct _virCPUx86Data virCPUx86Data; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9334a75..07b9df2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3928,6 +3928,37 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, } }
+ for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { + if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON) { + char *cpuFeature; + if (virAsprintf(&cpuFeature, "__kvm_hv_%s", + virDomainHypervTypeToString(i)) < 0) + goto cleanup; + if (!cpuHasFeature(guestcpu, cpuFeature)) { + switch ((virDomainHyperv) i) { + case VIR_DOMAIN_HYPERV_RELAXED: + case VIR_DOMAIN_HYPERV_VAPIC: + case VIR_DOMAIN_HYPERV_SPINLOCKS: + VIR_WARN("host doesn't support hyperv '%s' feature", + virDomainHypervTypeToString(i)); + break; + case VIR_DOMAIN_HYPERV_VPINDEX: + case VIR_DOMAIN_HYPERV_RUNTIME: + case VIR_DOMAIN_HYPERV_SYNIC: + case VIR_DOMAIN_HYPERV_STIMER: + case VIR_DOMAIN_HYPERV_RESET: + case VIR_DOMAIN_HYPERV_VENDOR_ID: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("host doesn't support hyperv '%s' feature"), + virDomainHypervTypeToString(i)); + goto cleanup; + break; + case VIR_DOMAIN_HYPERV_LAST: + break; + } + } + } + }
if (def->cpu && def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { for (i = 0; i < def->cpu->nfeatures; i++) {
Hmm, qemu already checks them and simply ignores most of them and doesn't prevent guest from starting in case they are not supported and optional. In case they are reqired it fails. Why should we check them here? At least we should follow the logic qemu has. Extract from target-i386/kvm.c does the following: if (cpu->hyperv_relaxed_timing) { c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE; } if (cpu->hyperv_vapic) { c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE; c->eax |= HV_X64_MSR_APIC_ACCESS_AVAILABLE; has_msr_hv_vapic = true; } ... if (cpu->hyperv_crash && has_msr_hv_crash) { c->edx |= HV_X64_GUEST_CRASH_MSR_AVAILABLE; } if (cpu->hyperv_reset && has_msr_hv_reset) { c->eax |= HV_X64_MSR_RESET_AVAILABLE; } if (cpu->hyperv_vpindex && has_msr_hv_vpindex) { c->eax |= HV_X64_MSR_VP_INDEX_AVAILABLE; } if (cpu->hyperv_runtime && has_msr_hv_runtime) { c->eax |= HV_X64_MSR_VP_RUNTIME_AVAILABLE; } thus, features hv_vapic, hv_relaxed, hv_reset, hv_runtime, hv_crash, hv_vpindex are ignored if not supported. Only absence of hv_synic, hv_stimer leads to error. Maxim
On Tue, Mar 29, 2016 at 07:06:14PM +0300, Maxim Nestratov wrote:
29.03.2016 16:31, Pavel Hrdina пишет:
Commit 7068b56c introduced several hyperv features. Not all hyperv features are supported by old enough kernels and we shouldn't allow to start a guest if kernel doesn't support any of the hyperv feature.
There is one exception, for backward compatibility we cannot error out if one of the RELAXED, VAPIC or SPINLOCKS isn't supported, for the same reason we ignore invtsc, to not break restoring saved domains with older libvirt.
[...]
Hmm, qemu already checks them and simply ignores most of them and doesn't prevent guest from starting in case they are not supported and optional. In case they are reqired it fails. Why should we check them here? At least we should follow the logic qemu has.
Yes, that's true that QEMU do some checks and ignores most of the features missing in kernel, but that's no reason why we should do the same. Libvirt tries to present in domain XML only those features and devices that are actually present in the guest. Thus if you tell libvirt that you want some hyperv feature but your host kernel doesn't support it, we should let the user know that this feature isn't supported instead of ignoring that fact and start the guest anyway. Pavel
30.03.2016 10:50, Pavel Hrdina пишет:
On Tue, Mar 29, 2016 at 07:06:14PM +0300, Maxim Nestratov wrote:
29.03.2016 16:31, Pavel Hrdina пишет:
Commit 7068b56c introduced several hyperv features. Not all hyperv features are supported by old enough kernels and we shouldn't allow to start a guest if kernel doesn't support any of the hyperv feature.
There is one exception, for backward compatibility we cannot error out if one of the RELAXED, VAPIC or SPINLOCKS isn't supported, for the same reason we ignore invtsc, to not break restoring saved domains with older libvirt.
[...]
Hmm, qemu already checks them and simply ignores most of them and doesn't prevent guest from starting in case they are not supported and optional. In case they are reqired it fails. Why should we check them here? At least we should follow the logic qemu has. Yes, that's true that QEMU do some checks and ignores most of the features missing in kernel, but that's no reason why we should do the same. Libvirt tries to present in domain XML only those features and devices that are actually present in the guest. Thus if you tell libvirt that you want some hyperv feature but your host kernel doesn't support it, we should let the user know that this feature isn't supported instead of ignoring that fact and start the guest anyway.
Pavel I see your point. And what if a user wants to define some features just to ask libvirt/qemu to do its best if possible and ignore if it isn't without redefining domain xml? Does such case make sense?
Maxim
On Wed, Mar 30, 2016 at 12:05:28PM +0300, Maxim Nestratov wrote:
30.03.2016 10:50, Pavel Hrdina пишет:
On Tue, Mar 29, 2016 at 07:06:14PM +0300, Maxim Nestratov wrote:
29.03.2016 16:31, Pavel Hrdina пишет:
Commit 7068b56c introduced several hyperv features. Not all hyperv features are supported by old enough kernels and we shouldn't allow to start a guest if kernel doesn't support any of the hyperv feature.
There is one exception, for backward compatibility we cannot error out if one of the RELAXED, VAPIC or SPINLOCKS isn't supported, for the same reason we ignore invtsc, to not break restoring saved domains with older libvirt.
[...]
Hmm, qemu already checks them and simply ignores most of them and doesn't prevent guest from starting in case they are not supported and optional. In case they are reqired it fails. Why should we check them here? At least we should follow the logic qemu has. Yes, that's true that QEMU do some checks and ignores most of the features missing in kernel, but that's no reason why we should do the same. Libvirt tries to present in domain XML only those features and devices that are actually present in the guest. Thus if you tell libvirt that you want some hyperv feature but your host kernel doesn't support it, we should let the user know that this feature isn't supported instead of ignoring that fact and start the guest anyway.
Pavel I see your point. And what if a user wants to define some features just to ask libvirt/qemu to do its best if possible and ignore if it isn't without redefining domain xml? Does such case make sense?
Maxim
Sure, this makes sense and we do this for cpu features [1], for example: <feature policy='optional' name='lahf_lm'/> But I'm not that sure whether we want to do this also for guest features like those hyperv features. If your guest OS is Windows you probably want to know what features are supported and to be sure, that all of the specified features are enabled. [1] http://libvirt.org/formatdomain.html#elementsCPU Pavel
On Tue, Mar 29, 2016 at 15:31:14 +0200, Pavel Hrdina wrote:
Pavel Hrdina (3): docs: fix qemu version for hyperv features qemu_process: skip only cpu features qemu_process: add check for hyperv features
docs/formatdomain.html.in | 4 ++-- src/cpu/cpu_x86.c | 8 +++++++ src/cpu/cpu_x86_data.h | 8 +++++++ src/qemu/qemu_process.c | 57 +++++++++++++++++++++++++++++++++++------------ 4 files changed, 61 insertions(+), 16 deletions(-)
ACK series Jirka
participants (4)
-
Jiri Denemark -
John Ferlan -
Maxim Nestratov -
Pavel Hrdina