Re: [libvirt] [PATCH 3/3] qemu_process: add check for hyperv features
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: 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu_process: add check for hyperv features
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu_process: add check for hyperv features
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu_process: add check for hyperv features
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
---
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 = 0x4001, .eax =
0x0080 }},
{VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT,
{ .function = 0x4001, .eax =
0x0100 }},
+{VIR_CPU_x86_KVM_HV_RUNTIME, { .function = 0x4003, .eax = 0x0001
}},
+{VIR_CPU_x86_KVM_HV_SYNIC, { .function = 0x4003, .eax = 0x0004
}},
+{VIR_CPU_x86_KVM_HV_STIMER,{ .function = 0x4003, .eax = 0x0008
}},
+{VIR_CPU_x86_KVM_HV_RELAXED, { .function = 0x4003, .eax = 0x0020
}},
+{VIR_CPU_x86_KVM_HV_SPINLOCK, { .function = 0x4003, .eax = 0x0022
}},
+{VIR_CPU_x86_KVM_HV_VAPIC, { .function = 0x4003, .eax = 0x0030
}},
+{VIR_CPU_x86_KVM_HV_VPINDEX, { .function = 0x4003, .eax = 0x0040
}},
+{VIR_CPU_x86_KVM_HV_RESET, { .function = 0x4003, .eax = 0x0080
}},
};
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_rel
Re: [libvirt] [PATCH 3/3] qemu_process: add check for hyperv features
[...] >> >> 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu_process: add check for hyperv features
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 > > --- > > 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu_process: add check for hyperv features
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
> ---
> 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 = 0x4001, .eax =
> 0x0080 }},
> {VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT,
> { .function = 0x4001, .eax =
> 0x0100 }},
> +{VIR_CPU_x86_KVM_HV_RUNTIME, { .function = 0x4003, .eax =
> 0x0001 }},
> +{VIR_CPU_x86_KVM_HV_SYNIC, { .function = 0x4003, .eax =
> 0x0004 }},
> +{VIR_CPU_x86_KVM_HV_STIMER,{ .function = 0x4003, .eax =
> 0x0008 }},
> +{VIR_CPU_x86_KVM_HV_RELAXED, { .function = 0x4003, .eax =
> 0x0020 }},
> +{VIR_CPU_x86_KVM_HV_SPINLOCK, { .function = 0x4003, .eax =
> 0x0022 }},
> +{VIR_CPU_x86_KVM_HV_VAPIC, { .function = 0x4003, .eax =
> 0x0030 }},
> +{VIR_CPU_x86_KVM_HV_VPINDEX, { .function = 0x4003, .eax =
> 0x0040 }},
> +{VIR_CPU_x86_KVM_HV_RESET, { .function = 0x4003, .eax =
> 0x0080 }},
> };
>
> 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 c
