Re: [libvirt] [PATCH v2] kvm: warn if num cpus is greater than num recommended
On Fri, Aug 23, 2013 at 03:24:37PM +0200, Andrew Jones wrote:
The comment in kvm_max_vcpus() states that it's using the recommended
procedure from the kernel API documentation to get the max number
of vcpus that kvm supports. It is, but by always returning the
maximum number supported. The maximum number should only be used
for development purposes. qemu should check KVM_CAP_NR_VCPUS for
the recommended number of vcpus. This patch adds a warning if a user
specifies a number of cpus between the recommended and max.
v2:
Incorporate tests for max_cpus, which specifies the maximum number
of hotpluggable cpus. An additional note is that the message for
the fail case was slightly changed, 'exceeds max cpus' to
'exceeds the maximum cpus'. If this is unacceptable change for
users like libvirt, then I'll need to spin a v3.
Signed-off-by: Andrew Jones drjo...@redhat.com
Applied, thanks.
---
kvm-all.c | 69
---
1 file changed, 40 insertions(+), 29 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c
index a2d49786365e3..021f5f47e53da 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1322,24 +1322,20 @@ static int kvm_irqchip_create(KVMState *s)
return 0;
}
-static int kvm_max_vcpus(KVMState *s)
+/* Find number of supported CPUs using the recommended
+ * procedure from the kernel API documentation to cope with
+ * older kernels that may be missing capabilities.
+ */
+static int kvm_recommended_vcpus(KVMState *s)
{
-int ret;
-
-/* Find number of supported CPUs using the recommended
- * procedure from the kernel API documentation to cope with
- * older kernels that may be missing capabilities.
- */
-ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS);
-if (ret) {
-return ret;
-}
-ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
-if (ret) {
-return ret;
-}
+int ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
+return (ret) ? ret : 4;
+}
-return 4;
+static int kvm_max_vcpus(KVMState *s)
+{
+int ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS);
+return (ret) ? ret : kvm_recommended_vcpus(s);
}
int kvm_init(void)
@@ -1347,11 +1343,19 @@ int kvm_init(void)
static const char upgrade_note[] =
Please upgrade to at least kernel 2.6.29 or recent kvm-kmod\n
(see http://sourceforge.net/projects/kvm).\n;
+struct {
+const char *name;
+int num;
+} num_cpus[] = {
+{ SMP, smp_cpus },
+{ hotpluggable, max_cpus },
+{ NULL, }
+}, *nc = num_cpus;
+int soft_vcpus_limit, hard_vcpus_limit;
KVMState *s;
const KVMCapabilityInfo *missing_cap;
int ret;
int i;
-int max_vcpus;
s = g_malloc0(sizeof(KVMState));
@@ -1392,19 +1396,26 @@ int kvm_init(void)
goto err;
}
-max_vcpus = kvm_max_vcpus(s);
-if (smp_cpus max_vcpus) {
-ret = -EINVAL;
-fprintf(stderr, Number of SMP cpus requested (%d) exceeds max cpus
-supported by KVM (%d)\n, smp_cpus, max_vcpus);
-goto err;
-}
+/* check the vcpu limits */
+soft_vcpus_limit = kvm_recommended_vcpus(s);
+hard_vcpus_limit = kvm_max_vcpus(s);
-if (max_cpus max_vcpus) {
-ret = -EINVAL;
-fprintf(stderr, Number of hotpluggable cpus requested (%d) exceeds
max cpus
-supported by KVM (%d)\n, max_cpus, max_vcpus);
-goto err;
+while (nc-name) {
+if (nc-num soft_vcpus_limit) {
+fprintf(stderr,
+Warning: Number of %s cpus requested (%d) exceeds
+the recommended cpus supported by KVM (%d)\n,
+nc-name, nc-num, soft_vcpus_limit);
+
+if (nc-num hard_vcpus_limit) {
+ret = -EINVAL;
+fprintf(stderr, Number of %s cpus requested (%d) exceeds
+the maximum cpus supported by KVM (%d)\n,
+nc-name, nc-num, hard_vcpus_limit);
+goto err;
+}
+}
+nc++;
}
s-vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
--
1.8.1.4
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] kvm: warn if num cpus is greater than num recommended
On Fri, Aug 23, 2013 at 03:24:37PM +0200, Andrew Jones wrote:
The comment in kvm_max_vcpus() states that it's using the recommended
procedure from the kernel API documentation to get the max number
of vcpus that kvm supports. It is, but by always returning the
maximum number supported. The maximum number should only be used
for development purposes. qemu should check KVM_CAP_NR_VCPUS for
the recommended number of vcpus. This patch adds a warning if a user
specifies a number of cpus between the recommended and max.
v2:
Incorporate tests for max_cpus, which specifies the maximum number
of hotpluggable cpus. An additional note is that the message for
the fail case was slightly changed, 'exceeds max cpus' to
'exceeds the maximum cpus'. If this is unacceptable change for
users like libvirt, then I'll need to spin a v3.
Looks good to me. Any ACKs, objections?
Signed-off-by: Andrew Jones drjo...@redhat.com
---
kvm-all.c | 69
---
1 file changed, 40 insertions(+), 29 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c
index a2d49786365e3..021f5f47e53da 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1322,24 +1322,20 @@ static int kvm_irqchip_create(KVMState *s)
return 0;
}
-static int kvm_max_vcpus(KVMState *s)
+/* Find number of supported CPUs using the recommended
+ * procedure from the kernel API documentation to cope with
+ * older kernels that may be missing capabilities.
+ */
+static int kvm_recommended_vcpus(KVMState *s)
{
-int ret;
-
-/* Find number of supported CPUs using the recommended
- * procedure from the kernel API documentation to cope with
- * older kernels that may be missing capabilities.
- */
-ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS);
-if (ret) {
-return ret;
-}
-ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
-if (ret) {
-return ret;
-}
+int ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
+return (ret) ? ret : 4;
+}
-return 4;
+static int kvm_max_vcpus(KVMState *s)
+{
+int ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS);
+return (ret) ? ret : kvm_recommended_vcpus(s);
}
int kvm_init(void)
@@ -1347,11 +1343,19 @@ int kvm_init(void)
static const char upgrade_note[] =
Please upgrade to at least kernel 2.6.29 or recent kvm-kmod\n
(see http://sourceforge.net/projects/kvm).\n;
+struct {
+const char *name;
+int num;
+} num_cpus[] = {
+{ SMP, smp_cpus },
+{ hotpluggable, max_cpus },
+{ NULL, }
+}, *nc = num_cpus;
+int soft_vcpus_limit, hard_vcpus_limit;
KVMState *s;
const KVMCapabilityInfo *missing_cap;
int ret;
int i;
-int max_vcpus;
s = g_malloc0(sizeof(KVMState));
@@ -1392,19 +1396,26 @@ int kvm_init(void)
goto err;
}
-max_vcpus = kvm_max_vcpus(s);
-if (smp_cpus max_vcpus) {
-ret = -EINVAL;
-fprintf(stderr, Number of SMP cpus requested (%d) exceeds max cpus
-supported by KVM (%d)\n, smp_cpus, max_vcpus);
-goto err;
-}
+/* check the vcpu limits */
+soft_vcpus_limit = kvm_recommended_vcpus(s);
+hard_vcpus_limit = kvm_max_vcpus(s);
-if (max_cpus max_vcpus) {
-ret = -EINVAL;
-fprintf(stderr, Number of hotpluggable cpus requested (%d) exceeds
max cpus
-supported by KVM (%d)\n, max_cpus, max_vcpus);
-goto err;
+while (nc-name) {
+if (nc-num soft_vcpus_limit) {
+fprintf(stderr,
+Warning: Number of %s cpus requested (%d) exceeds
+the recommended cpus supported by KVM (%d)\n,
+nc-name, nc-num, soft_vcpus_limit);
+
+if (nc-num hard_vcpus_limit) {
+ret = -EINVAL;
+fprintf(stderr, Number of %s cpus requested (%d) exceeds
+the maximum cpus supported by KVM (%d)\n,
+nc-name, nc-num, hard_vcpus_limit);
+goto err;
+}
+}
+nc++;
}
s-vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
--
1.8.1.4
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 4/7] target-i386/cpu: Introduce FeatureWord typedefs
On Mon, Jan 07, 2013 at 04:20:45PM -0200, Eduardo Habkost wrote:
This introduces a FeatureWord enum, FeatureWordInfo struct (with
generation information about a feature word), and a FeatureWordArray
typedef, and changes add_flagname_to_bitmaps() code and
cpu_x86_parse_featurestr() to use the new typedefs instead of separate
variables for each feature word.
This will help us keep the code at kvm_check_features_against_host(),
cpu_x86_parse_featurestr() and add_flagname_to_bitmaps() sane while
adding new feature name arrays.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 97
+++
target-i386/cpu.h | 15 +
2 files changed, 63 insertions(+), 49 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b09b625..7d62d48 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -124,6 +124,20 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
};
+typedef struct FeatureWordInfo {
+const char **feat_names;
+} FeatureWordInfo;
+
+static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
+[FEAT_1_EDX] = { .feat_names = feature_name },
+[FEAT_1_ECX] = { .feat_names = ext_feature_name },
+[FEAT_8000_0001_EDX] = { .feat_names = ext2_feature_name },
+[FEAT_8000_0001_ECX] = { .feat_names = ext3_feature_name },
+[FEAT_KVM] = { .feat_names = kvm_feature_name },
+[FEAT_SVM] = { .feat_names = svm_feature_name },
+[FEAT_7_0_EBX] = { .feat_names = cpuid_7_0_ebx_feature_name },
+};
+
const char *get_register_name_32(unsigned int reg)
{
static const char *reg_names[CPU_NB_REGS32] = {
@@ -271,23 +285,20 @@ static bool lookup_feature(uint32_t *pval, const char
*s, const char *e,
return found;
}
-static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
-uint32_t *ext_features,
-uint32_t *ext2_features,
-uint32_t *ext3_features,
-uint32_t *kvm_features,
-uint32_t *svm_features,
-uint32_t *cpuid_7_0_ebx_features)
+static void add_flagname_to_bitmaps(const char *flagname,
+FeatureWordArray words)
{
-if (!lookup_feature(features, flagname, NULL, feature_name)
-!lookup_feature(ext_features, flagname, NULL, ext_feature_name)
-!lookup_feature(ext2_features, flagname, NULL, ext2_feature_name)
-!lookup_feature(ext3_features, flagname, NULL, ext3_feature_name)
-!lookup_feature(kvm_features, flagname, NULL, kvm_feature_name)
-!lookup_feature(svm_features, flagname, NULL, svm_feature_name)
-!lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
-cpuid_7_0_ebx_feature_name))
-fprintf(stderr, CPU feature %s not found\n, flagname);
+FeatureWord w;
+for (w = 0; w FEATURE_WORDS; w++) {
+FeatureWordInfo *wi = feature_word_info[w];
+if (wi-feat_names
I would move patch 6 before that one and drop this check here, but if
you disagree it is your call.
+lookup_feature(words[w], flagname, NULL, wi-feat_names)) {
+break;
+}
+}
+if (w == FEATURE_WORDS) {
+fprintf(stderr, CPU feature %s not found\n, flagname);
+}
}
typedef struct x86_def_t {
@@ -1256,35 +1267,23 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
unsigned int i;
char *featurestr; /* Single 'key=value string being parsed */
/* Features to be added */
-uint32_t plus_features = 0, plus_ext_features = 0;
-uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
-uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0;
-uint32_t plus_7_0_ebx_features = 0;
+FeatureWordArray plus_features = {
+[FEAT_KVM] = kvm_default_features,
+};
/* Features to be removed */
-uint32_t minus_features = 0, minus_ext_features = 0;
-uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
-uint32_t minus_kvm_features = 0, minus_svm_features = 0;
-uint32_t minus_7_0_ebx_features = 0;
+FeatureWordArray minus_features = { 0 };
uint32_t numvalue;
-add_flagname_to_bitmaps(hypervisor, plus_features,
-plus_ext_features, plus_ext2_features, plus_ext3_features,
-plus_kvm_features, plus_svm_features, plus_7_0_ebx_features);
+add_flagname_to_bitmaps(hypervisor, plus_features);
featurestr = features ? strtok(features, ,) : NULL;
while (featurestr) {
char *val;
if (featurestr[0] == '+') {
-add_flagname_to_bitmaps(featurestr + 1, plus_features,
-
Re: [libvirt] [PATCH qom-cpu 0/7] disable kvm_mmu + -cpu enforce fixes (v3)
On Mon, Jan 07, 2013 at 04:20:41PM -0200, Eduardo Habkost wrote: Changes on v3: - Patches 3-9 from v2 are now already on qom-cpu tree - Remove CONFIG_KVM #ifdefs by declaring fake KVM_* #defines on sysemu/kvm.h - Refactor code that uses the feature word arrays (to make it easier to add a new feature name array) - Add feature name array for CPUID leaf 0xC001 Changes on v2: - Now both the kvm_mmu-disable and -cpu enforce changes are on the same series - Coding style fixes Git tree for reference: git://github.com/ehabkost/qemu-hacks.git cpu-enforce-all.v3 https://github.com/ehabkost/qemu-hacks/tree/cpu-enforce-all.v3 The changes are a bit intrusive, but: - The longer we take to make enforce strict as it should (and make libvirt finally use it), more users will have VMs with migration-unsafe unpredictable guest ABIs. For this reason, I would like to get this into QEMU 1.4. - The changes in this series should affect only users that are already using the enforce flag, and I believe whoever is using the enforce flag really want the strict behavior introduced by this series. Reviewed-by: Gleb Natapov g...@redhat.com Small comment on patch 4. Fill free to ignore. Eduardo Habkost (7): kvm: Add fake KVM constants to avoid #ifdefs on KVM-specific code target-i386: Don't set any KVM flag by default if KVM is disabled target-i386: Disable kvm_mmu by default target-i386/cpu: Introduce FeatureWord typedefs target-i386: kvm_check_features_against_host(): Use feature_word_info target-i386/cpu.c: Add feature name array for ext4_features target-i386: check/enforce: Check all feature words include/sysemu/kvm.h | 14 target-i386/cpu.c| 193 --- target-i386/cpu.h| 15 3 files changed, 150 insertions(+), 72 deletions(-) -- 1.7.11.7 -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [Qemu PATCH v2] add a boot option to do strict boot
On Wed, Jan 09, 2013 at 12:28:57PM -0500, Laine Stump wrote: On 01/09/2013 10:22 AM, Daniel P. Berrange wrote: On Wed, Jan 09, 2013 at 08:14:07AM -0700, Eric Blake wrote: On 01/09/2013 01:39 AM, Amos Kong wrote: Current seabios will try to boot from selected devices first, if they are all failed, seabios will also try to boot from un-selected devices. We need to make it configurable. I already posted a seabios patch to add a new device type to halt booting. Qemu can add HALT at the end of bootindex string, then seabios will halt booting after trying to boot from selected devices. This option only effects when boot priority is changed by bootindex options, the old style(-boot order=..) will still try to boot from un-selected devices. v2: add HALT entry in get_boot_devices_list() define boot_strict to bool Signed-off-by: Amos Kong ak...@redhat.com --- Libvirt will need to expose an attribute that lets the user control whether to use this new option; how do we probe via QMP whether the new -boot strict=on command-line option is available? While libvirt should make use of this, we don't need to expose it in the XML. This new behaviour is what we wanted to have all along, so we should just enable it. I agree that this is the way it *should* always work, but apparently there are people who depend on the old behavior, so just doing a blanket switch to the new behavior could lead to setups that no longer work properly after an upgrade, which unfortunately means that existing functionality needs to be maintained, and correct functionality must be triggered by a config switch (maybe Gleb can expand on the use cases that require this if more details are needed, as it's him I heard this from) It is common to configure PXE boot as highest prio for easy re-imaging, Usually boot from PXE fails and other bootable device is used instead, but if re-installation is needed it is as easy as configuring PXE server and rebooting the machine. With current behaviour it is enough to boost PXE priority, if we will change it setups that didn't explicitly specify all devices' priorities will stop working. The rule is easy: if exist command line that will work differently before and after the change you probably break someones setup with your patch. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote:
This is a cleanup that tries to solve two small issues:
- We don't need a separate kvm_pv_eoi_features variable just to keep a
constant calculated at compile-time, and this style would require
adding a separate variable (that's declared twice because of the
CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
by machine-type compat code.
- The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
even when KVM is disabled at runtime. This small incosistency in
the cpuid_kvm_features field isn't a problem today because
cpuid_kvm_features is ignored by the TCG code, but it may cause
unexpected problems later when refactoring the CPUID handling code.
This patch eliminates the kvm_pv_eoi_features variable and simply uses
CONFIG_KVM and kvm_enabled() inside the enable_kvm_pv_eoi() compat
function, so it enables kvm_pv_eoi only if KVM is enabled. I believe
this makes the behavior of enable_kvm_pv_eoi() clearer and easier to
understand.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: k...@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 82685dc..e6435da 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -145,15 +145,17 @@ static uint32_t kvm_default_features = (1
KVM_FEATURE_CLOCKSOURCE) |
(1 KVM_FEATURE_ASYNC_PF) |
(1 KVM_FEATURE_STEAL_TIME) |
(1 KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
-static const uint32_t kvm_pv_eoi_features = (0x1 KVM_FEATURE_PV_EOI);
#else
static uint32_t kvm_default_features = 0;
-static const uint32_t kvm_pv_eoi_features = 0;
#endif
void enable_kvm_pv_eoi(void)
{
-kvm_default_features |= kvm_pv_eoi_features;
+#ifdef CONFIG_KVM
You do not need ifdef here.
We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
set.
I could also write it as:
if (kvm_enabled()) {
#ifdef CONFIG_KVM
kvm_default_features |= (1UL KVM_FEATURE_PV_EOI);
#endif
}
But I find it less readable.
Why not define KVM_FEATURE_PV_EOI unconditionally?
+if (kvm_enabled()) {
+kvm_default_features |= (1UL KVM_FEATURE_PV_EOI);
+}
+#endif
}
void host_cpuid(uint32_t function, uint32_t count,
--
1.7.11.7
--
Gleb.
--
Eduardo
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 11/11] target-i386: check/enforce: Check all feature words
On Mon, Jan 07, 2013 at 10:06:21AM -0200, Eduardo Habkost wrote:
On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
This adds the following feature words to the list of flags to be checked
by kvm_check_features_against_host():
- cpuid_7_0_ebx_features
- ext4_features
- kvm_features
- svm_features
This will ensure the enforce flag works as it should: it won't allow
QEMU to be started unless every flag that was requested by the user or
defined in the CPU model is supported by the host.
This patch may cause existing configurations where enforce wasn't
preventing QEMU from being started to abort QEMU. But that's exactly the
point of this patch: if a flag was not supported by the host and QEMU
wasn't aborting, it was a bug in the enforce code.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: k...@vger.kernel.org
Cc: libvir-list@redhat.com
Cc: Jiri Denemark jdene...@redhat.com
CCing libvirt people, as this is directly related to the planned usage
of the enforce flag by libvirt.
The libvirt team probably has a problem in their hands: libvirt should
use enforce to make sure all requested flags are making their way into
the guest (so the resulting CPU is always the same, on any host), but
users may have existing working configurations where a flag is not
supported by the guest and the user really doesn't care about it. Those
configurations will necessarily break when libvirt starts using
enforce.
One example where it may cause trouble for common setups: pc-1.3 wants
the kvm_pv_eoi flag enabled by default (so enforce will make sure it
is enabled), but the user may have an existing VM running on a host
without pv_eoi support. That setup is unsafe today because
live-migration between different host kernel versions may enable/disable
pv_eoi silently (that's why we need the enforce flag to be used by
libvirt), but the user probably would like to be able to live-migrate
that VM anyway (and have libvirt to just do the right thing).
One possible solution to libvirt is to use enforce only on newer
machine-types, so existing machines with older machine-types will keep
the unsafe host-dependent-ABI behavior, but at least would keep
live-migration working in case the user is careful.
I really don't know what the libvirt team prefers, but that's the
situation today. The longer we take to make enforce strict as it
should and make libvirt finally use it, more users will have VMs with
migration-unsafe unpredictable guest ABIs.
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 876b0f6..52727ad 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -955,8 +955,9 @@ static int unavailable_host_feature(struct
model_features_t *f, uint32_t mask)
return 0;
}
-/* best effort attempt to inform user requested cpu flags aren't making
- * their way to the guest.
+/* Check if all requested cpu flags are making their way to the guest
+ *
+ * Returns 0 if all flags are supported by the host, non-zero otherwise.
*
* This function may be called only if KVM is enabled.
*/
@@ -973,7 +974,15 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
{guest_def-ext2_features, host_def.ext2_features,
ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
-ext3_feature_name, 0x8001, R_ECX}
+ext3_feature_name, 0x8001, R_ECX},
+{guest_def-ext4_features, host_def.ext4_features,
+NULL, 0xC001, R_EDX},
Since there is not name array for ext4_features they cannot be added or
removed on the command line hence no need to check them, no?
In theory, yes. But it won't hurt to check it, and it will be useful to
unify the list of feature words in a single place, so we can be sure the
checking/filtering/setting code at
kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(),
will all check/filter/set exactly the same feature words.
May be add a name array for the leaf? :)
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote:
On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote:
On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote:
This is a cleanup that tries to solve two small issues:
- We don't need a separate kvm_pv_eoi_features variable just to keep
a
constant calculated at compile-time, and this style would require
adding a separate variable (that's declared twice because of the
CONFIG_KVM ifdef) for each feature that's going to be
enabled/disable
by machine-type compat code.
- The pc-1.3 code is setting the kvm_pv_eoi flag on
cpuid_kvm_features
even when KVM is disabled at runtime. This small incosistency in
the cpuid_kvm_features field isn't a problem today because
cpuid_kvm_features is ignored by the TCG code, but it may cause
unexpected problems later when refactoring the CPUID handling code.
This patch eliminates the kvm_pv_eoi_features variable and simply uses
CONFIG_KVM and kvm_enabled() inside the enable_kvm_pv_eoi() compat
function, so it enables kvm_pv_eoi only if KVM is enabled. I believe
this makes the behavior of enable_kvm_pv_eoi() clearer and easier to
understand.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: k...@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 82685dc..e6435da 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -145,15 +145,17 @@ static uint32_t kvm_default_features = (1
KVM_FEATURE_CLOCKSOURCE) |
(1 KVM_FEATURE_ASYNC_PF) |
(1 KVM_FEATURE_STEAL_TIME) |
(1 KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
-static const uint32_t kvm_pv_eoi_features = (0x1
KVM_FEATURE_PV_EOI);
#else
static uint32_t kvm_default_features = 0;
-static const uint32_t kvm_pv_eoi_features = 0;
#endif
void enable_kvm_pv_eoi(void)
{
-kvm_default_features |= kvm_pv_eoi_features;
+#ifdef CONFIG_KVM
You do not need ifdef here.
We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
set.
I could also write it as:
if (kvm_enabled()) {
#ifdef CONFIG_KVM
kvm_default_features |= (1UL KVM_FEATURE_PV_EOI);
#endif
}
But I find it less readable.
Why not define KVM_FEATURE_PV_EOI unconditionally?
It comes from the KVM kernel headers, that are included only if
CONFIG_KVM is set, and probably won't even compile in non-Linux systems.
I have a dejavu feeling. I believe we had this exact problem before,
maybe about some other #defines that come from the Linux KVM headers and
won't be available in non-Linux systems.
It is better to hide all KVM related differences somewhere in the
headers where no one sees them instead of sprinkle them all over the
code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM
part. Or have one ifdef CONFIG_KVM at the beginning of the file and
define enable_kvm_pv_eoi() there and provide empty stub otherwise.
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 11/11] target-i386: check/enforce: Check all feature words
On Mon, Jan 07, 2013 at 10:19:15AM -0200, Eduardo Habkost wrote:
On Mon, Jan 07, 2013 at 02:06:38PM +0200, Gleb Natapov wrote:
On Mon, Jan 07, 2013 at 10:06:21AM -0200, Eduardo Habkost wrote:
On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
This adds the following feature words to the list of flags to be
checked
by kvm_check_features_against_host():
- cpuid_7_0_ebx_features
- ext4_features
- kvm_features
- svm_features
This will ensure the enforce flag works as it should: it won't allow
QEMU to be started unless every flag that was requested by the user or
defined in the CPU model is supported by the host.
This patch may cause existing configurations where enforce wasn't
preventing QEMU from being started to abort QEMU. But that's exactly
the
point of this patch: if a flag was not supported by the host and QEMU
wasn't aborting, it was a bug in the enforce code.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: k...@vger.kernel.org
Cc: libvir-list@redhat.com
Cc: Jiri Denemark jdene...@redhat.com
CCing libvirt people, as this is directly related to the planned usage
of the enforce flag by libvirt.
The libvirt team probably has a problem in their hands: libvirt should
use enforce to make sure all requested flags are making their way
into
the guest (so the resulting CPU is always the same, on any host), but
users may have existing working configurations where a flag is not
supported by the guest and the user really doesn't care about it.
Those
configurations will necessarily break when libvirt starts using
enforce.
One example where it may cause trouble for common setups: pc-1.3 wants
the kvm_pv_eoi flag enabled by default (so enforce will make sure it
is enabled), but the user may have an existing VM running on a host
without pv_eoi support. That setup is unsafe today because
live-migration between different host kernel versions may
enable/disable
pv_eoi silently (that's why we need the enforce flag to be used by
libvirt), but the user probably would like to be able to live-migrate
that VM anyway (and have libvirt to just do the right thing).
One possible solution to libvirt is to use enforce only on newer
machine-types, so existing machines with older machine-types will keep
the unsafe host-dependent-ABI behavior, but at least would keep
live-migration working in case the user is careful.
I really don't know what the libvirt team prefers, but that's the
situation today. The longer we take to make enforce strict as it
should and make libvirt finally use it, more users will have VMs with
migration-unsafe unpredictable guest ABIs.
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 876b0f6..52727ad 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -955,8 +955,9 @@ static int unavailable_host_feature(struct
model_features_t *f, uint32_t mask)
return 0;
}
-/* best effort attempt to inform user requested cpu flags aren't
making
- * their way to the guest.
+/* Check if all requested cpu flags are making their way to the guest
+ *
+ * Returns 0 if all flags are supported by the host, non-zero
otherwise.
*
* This function may be called only if KVM is enabled.
*/
@@ -973,7 +974,15 @@ static int
kvm_check_features_against_host(x86_def_t *guest_def)
{guest_def-ext2_features, host_def.ext2_features,
ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
-ext3_feature_name, 0x8001, R_ECX}
+ext3_feature_name, 0x8001, R_ECX},
+{guest_def-ext4_features, host_def.ext4_features,
+NULL, 0xC001, R_EDX},
Since there is not name array for ext4_features they cannot be added or
removed on the command line hence no need to check them, no?
In theory, yes. But it won't hurt to check it, and it will be useful to
unify the list of feature words in a single place, so we can be sure the
checking/filtering/setting code at
kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(),
will all check/filter/set exactly the same feature words.
May be add a name array for the leaf? :)
If anybody find reliable documentation about the 0xC001 CPUID bits,
I would
Re: [libvirt] [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
On Mon, Jan 07, 2013 at 10:30:40AM -0200, Eduardo Habkost wrote:
On Mon, Jan 07, 2013 at 02:15:59PM +0200, Gleb Natapov wrote:
On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote:
On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote:
On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote:
This is a cleanup that tries to solve two small issues:
- We don't need a separate kvm_pv_eoi_features variable just to
keep a
constant calculated at compile-time, and this style would
require
adding a separate variable (that's declared twice because of
the
CONFIG_KVM ifdef) for each feature that's going to be
enabled/disable
by machine-type compat code.
- The pc-1.3 code is setting the kvm_pv_eoi flag on
cpuid_kvm_features
even when KVM is disabled at runtime. This small incosistency
in
the cpuid_kvm_features field isn't a problem today because
cpuid_kvm_features is ignored by the TCG code, but it may cause
unexpected problems later when refactoring the CPUID handling
code.
This patch eliminates the kvm_pv_eoi_features variable and simply
uses
CONFIG_KVM and kvm_enabled() inside the enable_kvm_pv_eoi() compat
function, so it enables kvm_pv_eoi only if KVM is enabled. I
believe
this makes the behavior of enable_kvm_pv_eoi() clearer and easier
to
understand.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: k...@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 82685dc..e6435da 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -145,15 +145,17 @@ static uint32_t kvm_default_features = (1
KVM_FEATURE_CLOCKSOURCE) |
(1 KVM_FEATURE_ASYNC_PF) |
(1 KVM_FEATURE_STEAL_TIME) |
(1 KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
-static const uint32_t kvm_pv_eoi_features = (0x1
KVM_FEATURE_PV_EOI);
#else
static uint32_t kvm_default_features = 0;
-static const uint32_t kvm_pv_eoi_features = 0;
#endif
void enable_kvm_pv_eoi(void)
{
-kvm_default_features |= kvm_pv_eoi_features;
+#ifdef CONFIG_KVM
You do not need ifdef here.
We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM
is
set.
I could also write it as:
if (kvm_enabled()) {
#ifdef CONFIG_KVM
kvm_default_features |= (1UL KVM_FEATURE_PV_EOI);
#endif
}
But I find it less readable.
Why not define KVM_FEATURE_PV_EOI unconditionally?
It comes from the KVM kernel headers, that are included only if
CONFIG_KVM is set, and probably won't even compile in non-Linux systems.
I have a dejavu feeling. I believe we had this exact problem before,
maybe about some other #defines that come from the Linux KVM headers and
won't be available in non-Linux systems.
It is better to hide all KVM related differences somewhere in the
headers where no one sees them instead of sprinkle them all over the
code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM
part. Or have one ifdef CONFIG_KVM at the beginning of the file and
define enable_kvm_pv_eoi() there and provide empty stub otherwise.
If we had an empty enable_kvm_pv_eoi() stub, we would need an #ifdef
around the real implementation. I mean, I don't think this:
#ifdef CONFIG_KVM
int enable_kvm_pv_eoi() {
[...]
}
#endif
You already have #ifdef CONFIG_KVM just above enable_kvm_pv_eoi(). Put
everything KVM related there instead of adding #ifdef CONFIG_KVM all
over the file.
is any better than this:
int enable_kvm_pv_eoi() {
#ifdef CONFIG_KVM
[...]
#endif
}
So this is probably a good reason to duplicate the KVM_FEATURE_*
#defines in the QEMU code, instead?
Not even duplicate, they can be fake just to keep compiler happy.
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set
On Mon, Jan 07, 2013 at 02:15:14PM +0100, Igor Mammedov wrote:
On Mon, 7 Jan 2013 10:00:09 -0200
Eduardo Habkost ehabk...@redhat.com wrote:
On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,7 @@ static void kvm_cpu_fill_host(x86_def_t
*x86_cpu_def)
#endif /* CONFIG_KVM */
}
+#ifdef CONFIG_KVM
static int unavailable_host_feature(struct model_features_t *f,
uint32_t mask)
{
int i;
@@ -987,6 +988,7 @@ static int
kvm_check_features_against_host(x86_def_t *guest_def)
}
return rv;
}
+#endif
static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
*opaque,
const char *name, Error
**errp)
@@ -1410,10 +1412,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
x86_cpu_def-kvm_features = ~minus_kvm_features;
x86_cpu_def-svm_features = ~minus_svm_features;
x86_cpu_def-cpuid_7_0_ebx_features = ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
if (check_cpuid kvm_enabled()) {
if (kvm_check_features_against_host(x86_cpu_def)
enforce_cpuid)
goto error;
}
+#endif
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.
I will do. Igor probably will have to change his target-i386: move
kvm_check_features_against_host() check to realize time patch to use
the same approach, too.
Gleb,
Why do stub here? As result we will be adding more ifdef-s just in other
places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
Why will we be adding more ifdef-s in other places?
kvm_check_features_against_host() are bundled together in cpu.c so we could
instead ifdef whole block. Like here:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html
That's fine, but you can avoid things like:
if (kvm_enabled() name strcmp(name, host) == 0) {
+#ifdef CONFIG_KVM
kvm_cpu_fill_host(x86_cpu_def);
+#endif
in your patch by providing stub for kvm_cpu_fill_host() for !CONFIG_KVM
case. This is common practice really. Avoid ifdefs in the code.
For me code looks more readable with ifdef here, if we have stub, a reader
would have to look at kvm_check_features_against_host() body to see if it does
anything.
If reader cares about kvm it has to anyway. If he does not, there is
friendly kvm_enabled() (which is stub in case of !CONFIG_KVM BTW) to
tell him that he does not care. No need additional ifdef there.
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote:
This is a cleanup that tries to solve two small issues:
- We don't need a separate kvm_pv_eoi_features variable just to keep a
constant calculated at compile-time, and this style would require
adding a separate variable (that's declared twice because of the
CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
by machine-type compat code.
- The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
even when KVM is disabled at runtime. This small incosistency in
the cpuid_kvm_features field isn't a problem today because
cpuid_kvm_features is ignored by the TCG code, but it may cause
unexpected problems later when refactoring the CPUID handling code.
This patch eliminates the kvm_pv_eoi_features variable and simply uses
CONFIG_KVM and kvm_enabled() inside the enable_kvm_pv_eoi() compat
function, so it enables kvm_pv_eoi only if KVM is enabled. I believe
this makes the behavior of enable_kvm_pv_eoi() clearer and easier to
understand.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: k...@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 82685dc..e6435da 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -145,15 +145,17 @@ static uint32_t kvm_default_features = (1
KVM_FEATURE_CLOCKSOURCE) |
(1 KVM_FEATURE_ASYNC_PF) |
(1 KVM_FEATURE_STEAL_TIME) |
(1 KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
-static const uint32_t kvm_pv_eoi_features = (0x1 KVM_FEATURE_PV_EOI);
#else
static uint32_t kvm_default_features = 0;
-static const uint32_t kvm_pv_eoi_features = 0;
#endif
void enable_kvm_pv_eoi(void)
{
-kvm_default_features |= kvm_pv_eoi_features;
+#ifdef CONFIG_KVM
You do not need ifdef here.
+if (kvm_enabled()) {
+kvm_default_features |= (1UL KVM_FEATURE_PV_EOI);
+}
+#endif
}
void host_cpuid(uint32_t function, uint32_t count,
--
1.7.11.7
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 02/11] target-i386: Disable kvm_mmu_op by default on pc-1.4
On Fri, Jan 04, 2013 at 08:01:03PM -0200, Eduardo Habkost wrote:
The kvm_mmu_op feature was removed from the kernel since v3.3 (released
in March 2012), it was marked for removal since January 2011 and it's
slower than shadow or hardware assisted paging (see kernel commit
fb92045843). It doesn't make sense to keep it enabled by default.
Actually it was effectively removed Oct 1 2009 by a68a6a7282373. After 3
and a half years of not having it I think we can safely drop it without
trying to preserve it in older machine types.
Also, keeping it enabled by default would cause unnecessary hassle when
libvirt start using the enforce option.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: k...@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: libvir-list@redhat.com
Cc: Jiri Denemark jdene...@redhat.com
I was planning to reverse the logic of the compat init functions and
make pc_init_pci_1_3() enable kvm_mmu_op and then call pc_init_pci_1_4()
instead. But that would require changing pc_init_pci_no_kvmclock() and
pc_init_isa() as well. So to keep the changes simple, I am keeping the
pattern used when pc_init_pci_1_3() was introduced, making
pc_init_pci_1_4() disable kvm_mmu_op and then call pc_init_pci_1_3().
Changes v2:
- Coding style fix
- Removed redundant comments above machine init functions
---
hw/pc_piix.c | 9 -
target-i386/cpu.c | 9 +
target-i386/cpu.h | 1 +
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 99747a7..a32af6a 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -217,6 +217,7 @@ static void pc_init1(MemoryRegion *system_memory,
}
}
+/* machine init function for pc-0.14 - pc-1.2 */
static void pc_init_pci(QEMUMachineInitArgs *args)
{
ram_addr_t ram_size = args-ram_size;
@@ -238,6 +239,12 @@ static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
pc_init_pci(args);
}
+static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
+{
+disable_kvm_mmu_op();
+pc_init_pci_1_3(args);
+}
+
static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
{
ram_addr_t ram_size = args-ram_size;
@@ -285,7 +292,7 @@ static QEMUMachine pc_machine_v1_4 = {
.name = pc-1.4,
.alias = pc,
.desc = Standard PC,
-.init = pc_init_pci_1_3,
+.init = pc_init_pci_1_4,
.max_cpus = 255,
.is_default = 1,
};
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e6435da..c83a566 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -158,6 +158,15 @@ void enable_kvm_pv_eoi(void)
#endif
}
+void disable_kvm_mmu_op(void)
+{
+#ifdef CONFIG_KVM
No need for ifdef here too.
+if (kvm_enabled()) {
+kvm_default_features = ~(1UL KVM_FEATURE_MMU_OP);
clear_bit()
+}
+#endif
+}
+
void host_cpuid(uint32_t function, uint32_t count,
uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
{
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 1283537..27c8d0c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1219,5 +1219,6 @@ void do_smm_enter(CPUX86State *env1);
void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
void enable_kvm_pv_eoi(void);
+void disable_kvm_mmu_op(void);
#endif /* CPU_I386_H */
--
1.7.11.7
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 03/11] target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features
On Fri, Jan 04, 2013 at 08:01:04PM -0200, Eduardo Habkost wrote: The existing -cpu host code simply set every bit inside svm_features (initializing it to -1), and that makes it impossible to make the enforce/check options work properly when the user asks for SVM features explicitly in the command-line. So, instead of initializing svm_features to -1, use GET_SUPPORTED_CPUID to fill only the bits that are supported by the host (just like we do for all other CPUID feature words inside kvm_cpu_fill_host()). This will keep the existing behavior (as filter_features_for_kvm() already uses GET_SUPPORTED_CPUID to filter svm_features), but will allow us to properly check for KVM features inside kvm_check_features_against_host() later. For example, we will be able to make this: $ qemu-system-x86_64 -cpu ...,+pfthreshold,enforce refuse to start if the SVM pfthreshold feature is not supported by the host (after we fix kvm_check_features_against_host() to check SVM flags as well). Signed-off-by: Eduardo Habkost ehabk...@redhat.com Reviewed-by: Gleb Natapov g...@redhat.com --- Changes v2: - Coding style (indentation) fix Cc: Gleb Natapov g...@redhat.com Cc: Marcelo Tosatti mtosa...@redhat.com Cc: Joerg Roedel j...@8bytes.org Cc: k...@vger.kernel.org --- target-i386/cpu.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c83a566..c49a97c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -908,13 +908,10 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) } } -/* - * Every SVM feature requires emulation support in KVM - so we can't just - * read the host features here. KVM might even support SVM features not - * available on the host hardware. Just set all bits and mask out the - * unsupported ones later. - */ -x86_cpu_def-svm_features = -1; +/* Other KVM-specific feature fields: */ +x86_cpu_def-svm_features = +kvm_arch_get_supported_cpuid(s, 0x800A, 0, R_EDX); + #endif /* CONFIG_KVM */ } -- 1.7.11.7 -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 04/11] target-i386: kvm: Enable all supported KVM features for -cpu host
On Fri, Jan 04, 2013 at 08:01:05PM -0200, Eduardo Habkost wrote: When using -cpu host, we don't need to use the kvm_default_features variable, as the user is explicitly asking QEMU to enable all feature supported by the host. This changes the kvm_cpu_fill_host() code to use GET_SUPPORTED_CPUID to initialize the kvm_features field, so we get all host KVM features enabled. This will also allow use to properly check/enforce KVM features inside kvm_check_features_against_host() later. For example, we will be able to make this: $ qemu-system-x86_64 -cpu ...,+kvm_pv_eoi,enforce refuse to start if kvm_pv_eoi is not supported by the host (after we fix kvm_check_features_against_host() to check KVM flags as well). Signed-off-by: Eduardo Habkost ehabk...@redhat.com Reviewed-by: Gleb Natapov g...@redhat.com --- Changes v2: - Coding style (indentation) fix Cc: Gleb Natapov g...@redhat.com Cc: Michael S. Tsirkin m...@redhat.com Cc: Marcelo Tosatti mtosa...@redhat.com Cc: k...@vger.kernel.org --- target-i386/cpu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c49a97c..e916ae0 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -911,6 +911,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) /* Other KVM-specific feature fields: */ x86_cpu_def-svm_features = kvm_arch_get_supported_cpuid(s, 0x800A, 0, R_EDX); +x86_cpu_def-kvm_features = +kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX); #endif /* CONFIG_KVM */ } -- 1.7.11.7 -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages
On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost wrote:
The -cpu check/enforce warnings are printing incorrect information about the
missing flags. There are no feature flags on CPUID leaves 0 and 0x8000,
but
there were references to 0 and 0x8000 in the table at
kvm_check_features_against_host().
This changes the model_features_t struct to contain the register number as
well, so the error messages print the correct CPUID leaf+register information,
instead of wrong CPUID leaf numbers.
This also changes the format of the error messages, so they follow the
CPUID.leaf.register.name [bit offset] convention used on Intel
documentation. Example output:
$ qemu-system-x86_64 -machine pc-1.0,accel=kvm -cpu
Opteron_G4,+ia64,enforce
warning: host doesn't support requested feature: CPUID.01H:EDX.ia64 [bit
30]
warning: host doesn't support requested feature: CPUID.01H:ECX.xsave [bit
26]
warning: host doesn't support requested feature: CPUID.01H:ECX.avx [bit
28]
warning: host doesn't support requested feature: CPUID.8001H:ECX.abm
[bit 5]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.sse4a [bit 6]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.misalignsse [bit 7]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.3dnowprefetch [bit 8]
warning: host doesn't support requested feature: CPUID.8001H:ECX.xop
[bit 11]
warning: host doesn't support requested feature: CPUID.8001H:ECX.fma4
[bit 16]
Unable to find x86 CPU definition
$
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
Reviewed-by: Gleb Natapov g...@redhat.com
But see the question below.
---
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: k...@vger.kernel.org
Changes v2:
- Coding style fixes
- Add assert() for invalid register numbers on
unavailable_host_feature()
---
target-i386/cpu.c | 42 +-
target-i386/cpu.h | 3 +++
2 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e916ae0..c3e5db8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -124,6 +124,25 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
};
+const char *get_register_name_32(unsigned int reg)
+{
+static const char *reg_names[CPU_NB_REGS32] = {
+[R_EAX] = EAX,
+[R_ECX] = ECX,
+[R_EDX] = EDX,
+[R_EBX] = EBX,
+[R_ESP] = ESP,
+[R_EBP] = EBP,
+[R_ESI] = ESI,
+[R_EDI] = EDI,
+};
+
+if (reg CPU_NB_REGS32) {
+return NULL;
+}
+return reg_names[reg];
+}
+
/* collects per-function cpuid data
*/
typedef struct model_features_t {
@@ -132,7 +151,8 @@ typedef struct model_features_t {
uint32_t check_feat;
const char **flag_names;
uint32_t cpuid;
-} model_features_t;
+int reg;
+} model_features_t;
int check_cpuid = 0;
int enforce_cpuid = 0;
@@ -923,10 +943,13 @@ static int unavailable_host_feature(struct
model_features_t *f, uint32_t mask)
for (i = 0; i 32; ++i)
if (1 i mask) {
-fprintf(stderr, warning: host cpuid %04x_%04x lacks requested
- flag '%s' [0x%08x]\n,
-f-cpuid 16, f-cpuid 0x,
-f-flag_names[i] ? f-flag_names[i] : [reserved], mask);
+const char *reg = get_register_name_32(f-reg);
+assert(reg);
+fprintf(stderr, warning: host doesn't support requested
feature:
+CPUID.%02XH:%s%s%s [bit %d]\n,
+f-cpuid, reg,
+f-flag_names[i] ? . : ,
+f-flag_names[i] ? f-flag_names[i] : , i);
break;
}
return 0;
@@ -945,13 +968,14 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
int rv, i;
struct model_features_t ft[] = {
{guest_def-features, host_def.features,
-~0, feature_name, 0x},
+~0, feature_name, 0x0001, R_EDX},
{guest_def-ext_features, host_def.ext_features,
-~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001},
+~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001, R_ECX},
{guest_def-ext2_features, host_def.ext2_features,
-~PPRO_FEATURES, ext2_feature_name, 0x8000},
+~PPRO_FEATURES, ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
-~CPUID_EXT3_SVM, ext3_feature_name, 0x8001}};
+~CPUID_EXT3_SVM, ext3_feature_name, 0x8001, R_ECX}
Why do we exclude PPRO_FEATURES/CPUID_EXT3_SVM from been checked?
+};
assert(kvm_enabled());
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
Re: [libvirt] [Qemu-devel] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages
On Sun, Jan 06, 2013 at 04:12:54PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost wrote:
The -cpu check/enforce warnings are printing incorrect information about the
missing flags. There are no feature flags on CPUID leaves 0 and 0x8000,
but
there were references to 0 and 0x8000 in the table at
kvm_check_features_against_host().
This changes the model_features_t struct to contain the register number as
well, so the error messages print the correct CPUID leaf+register
information,
instead of wrong CPUID leaf numbers.
This also changes the format of the error messages, so they follow the
CPUID.leaf.register.name [bit offset] convention used on Intel
documentation. Example output:
$ qemu-system-x86_64 -machine pc-1.0,accel=kvm -cpu
Opteron_G4,+ia64,enforce
warning: host doesn't support requested feature: CPUID.01H:EDX.ia64
[bit 30]
warning: host doesn't support requested feature: CPUID.01H:ECX.xsave
[bit 26]
warning: host doesn't support requested feature: CPUID.01H:ECX.avx [bit
28]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.abm [bit 5]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.sse4a [bit 6]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.misalignsse [bit 7]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.3dnowprefetch [bit 8]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.xop [bit 11]
warning: host doesn't support requested feature:
CPUID.8001H:ECX.fma4 [bit 16]
Unable to find x86 CPU definition
$
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
Reviewed-by: Gleb Natapov g...@redhat.com
But see the question below.
Never mind. I found the answer in the following patches :)
---
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: k...@vger.kernel.org
Changes v2:
- Coding style fixes
- Add assert() for invalid register numbers on
unavailable_host_feature()
---
target-i386/cpu.c | 42 +-
target-i386/cpu.h | 3 +++
2 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e916ae0..c3e5db8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -124,6 +124,25 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
};
+const char *get_register_name_32(unsigned int reg)
+{
+static const char *reg_names[CPU_NB_REGS32] = {
+[R_EAX] = EAX,
+[R_ECX] = ECX,
+[R_EDX] = EDX,
+[R_EBX] = EBX,
+[R_ESP] = ESP,
+[R_EBP] = EBP,
+[R_ESI] = ESI,
+[R_EDI] = EDI,
+};
+
+if (reg CPU_NB_REGS32) {
+return NULL;
+}
+return reg_names[reg];
+}
+
/* collects per-function cpuid data
*/
typedef struct model_features_t {
@@ -132,7 +151,8 @@ typedef struct model_features_t {
uint32_t check_feat;
const char **flag_names;
uint32_t cpuid;
-} model_features_t;
+int reg;
+} model_features_t;
int check_cpuid = 0;
int enforce_cpuid = 0;
@@ -923,10 +943,13 @@ static int unavailable_host_feature(struct
model_features_t *f, uint32_t mask)
for (i = 0; i 32; ++i)
if (1 i mask) {
-fprintf(stderr, warning: host cpuid %04x_%04x lacks requested
- flag '%s' [0x%08x]\n,
-f-cpuid 16, f-cpuid 0x,
-f-flag_names[i] ? f-flag_names[i] : [reserved], mask);
+const char *reg = get_register_name_32(f-reg);
+assert(reg);
+fprintf(stderr, warning: host doesn't support requested
feature:
+CPUID.%02XH:%s%s%s [bit %d]\n,
+f-cpuid, reg,
+f-flag_names[i] ? . : ,
+f-flag_names[i] ? f-flag_names[i] : , i);
break;
}
return 0;
@@ -945,13 +968,14 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
int rv, i;
struct model_features_t ft[] = {
{guest_def-features, host_def.features,
-~0, feature_name, 0x},
+~0, feature_name, 0x0001, R_EDX},
{guest_def-ext_features, host_def.ext_features,
-~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001},
+~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001, R_ECX},
{guest_def-ext2_features, host_def.ext2_features,
-~PPRO_FEATURES, ext2_feature_name, 0x8000},
+~PPRO_FEATURES, ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
-~CPUID_EXT3_SVM
Re: [libvirt] [PATCH qom-cpu 06/11] target-i386: check/enforce: Do not ignore hypervisor flag
On Fri, Jan 04, 2013 at 08:01:07PM -0200, Eduardo Habkost wrote:
We don't need any hack to ignore CPUID_EXT_HYPERVISOR anymore, because
kvm_arch_get_supported_cpuid() now set CPUID_EXT_HYPERVISOR properly.
So, this shouldn't introduce any behavior change, but it makes the code
simpler.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
Reviewed-by: Gleb Natapov g...@redhat.com
---
My goal is to eliminate the check_feat field completely, as
kvm_arch_get_supported_cpuid() should now really return all the bits we
can set on all CPUID leaves.
---
target-i386/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c3e5db8..42c4c99 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -970,7 +970,7 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
{guest_def-features, host_def.features,
~0, feature_name, 0x0001, R_EDX},
{guest_def-ext_features, host_def.ext_features,
-~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x0001, R_ECX},
+~0, ext_feature_name, 0x0001, R_ECX},
{guest_def-ext2_features, host_def.ext2_features,
~PPRO_FEATURES, ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
--
1.7.11.7
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH qom-cpu 07/11] target-i386: check/enforce: Check all CPUID.80000001H.EDX bits
On Fri, Jan 04, 2013 at 08:01:08PM -0200, Eduardo Habkost wrote:
I have no idea why PPRO_FEATURES was being ignored on the check of the
CPUID.8001H.EDX bits. I believe it was a mistake, and it was
supposed to be ~(PPRO_FEATURES CPUID_EXT2_AMD_ALIASES) or just
~CPUID_EXT2_AMD_ALIASES, because some time ago kvm_cpu_fill_host() used
the CPUID instruction directly (instead of
kvm_arch_get_supported_cpuid()).
But now kvm_cpu_fill_host() use kvm_arch_get_supported_cpuid(), and
kvm_arch_get_supported_cpuid() returns all supported bits for
CPUID.8001H.EDX, even the AMD aliases (that are explicitly copied
from CPUID.01H.EDX), so we can make the code check/enforce all the
CPUID.8001H.EDX bits.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
Reviewed-by: Gleb Natapov g...@redhat.com
---
target-i386/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 42c4c99..ce64b98 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -972,7 +972,7 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
{guest_def-ext_features, host_def.ext_features,
~0, ext_feature_name, 0x0001, R_ECX},
{guest_def-ext2_features, host_def.ext2_features,
-~PPRO_FEATURES, ext2_feature_name, 0x8001, R_EDX},
+~0, ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
~CPUID_EXT3_SVM, ext3_feature_name, 0x8001, R_ECX}
};
--
1.7.11.7
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH qom-cpu 08/11] target-i386: check/enforce: Check SVM flag support as well
On Fri, Jan 04, 2013 at 08:01:09PM -0200, Eduardo Habkost wrote:
When nested SVM is supported, the kernel returns the SVM flag on
GET_SUPPORTED_CPUID[1], so we can check the SVM flag safely on
kvm_check_features_against_host().
I don't know why the original code ignored the SVM flag. Maybe it was
because kvm_cpu_fill_host() used the CPUID instruction directly instead
of GET_SUPPORTED_CPUID
[1] Older kernels (before v2.6.37) returned the SVM flag even if nested
SVM was _not_ supported. So the only cases where this patch should
change behavior is when SVM is being requested by the user or the
CPU model, but not supported by the host. And on these cases we
really want QEMU to abort if the enforce option is set.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
Reviewed-by: Gleb Natapov g...@redhat.com
---
Cc: Joerg Roedel j...@8bytes.org
Cc: k...@vger.kernel.org
Cc: libvir-list@redhat.com
Cc: Jiri Denemark jdene...@redhat.com
I'm CCing libvirt people in case having SVM enabled by default may cause
trouble when libvirt starts using the enforce flag. I don't know if
libvirt expects most of the QEMU CPU models to have nested SVM enabled.
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ce64b98..a9dd959 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -974,7 +974,7 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
{guest_def-ext2_features, host_def.ext2_features,
~0, ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
-~CPUID_EXT3_SVM, ext3_feature_name, 0x8001, R_ECX}
+~0, ext3_feature_name, 0x8001, R_ECX}
};
assert(kvm_enabled());
--
1.7.11.7
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH qom-cpu 09/11] target-i386: check/enforce: Eliminate check_feat field
On Fri, Jan 04, 2013 at 08:01:10PM -0200, Eduardo Habkost wrote:
Now that all entries have check_feat=~0 on
kvm_check_features_against_host(), we can eliminate check_feat entirely
and make the code check all bits.
This patch shouldn't introduce any behavior change, as check_feat is set
to ~0 on all entries.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
Reviewed-by: Gleb Natapov g...@redhat.com
---
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a9dd959..1c3c7e1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -148,7 +148,6 @@ const char *get_register_name_32(unsigned int reg)
typedef struct model_features_t {
uint32_t *guest_feat;
uint32_t *host_feat;
-uint32_t check_feat;
const char **flag_names;
uint32_t cpuid;
int reg;
@@ -956,8 +955,7 @@ static int unavailable_host_feature(struct
model_features_t *f, uint32_t mask)
}
/* best effort attempt to inform user requested cpu flags aren't making
- * their way to the guest. Note: ft[].check_feat ideally should be
- * specified via a guest_def field to suppress report of extraneous flags.
+ * their way to the guest.
*
* This function may be called only if KVM is enabled.
*/
@@ -968,13 +966,13 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
int rv, i;
struct model_features_t ft[] = {
{guest_def-features, host_def.features,
-~0, feature_name, 0x0001, R_EDX},
+feature_name, 0x0001, R_EDX},
{guest_def-ext_features, host_def.ext_features,
-~0, ext_feature_name, 0x0001, R_ECX},
+ext_feature_name, 0x0001, R_ECX},
{guest_def-ext2_features, host_def.ext2_features,
-~0, ext2_feature_name, 0x8001, R_EDX},
+ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
-~0, ext3_feature_name, 0x8001, R_ECX}
+ext3_feature_name, 0x8001, R_ECX}
};
assert(kvm_enabled());
@@ -982,7 +980,7 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
kvm_cpu_fill_host(host_def);
for (rv = 0, i = 0; i ARRAY_SIZE(ft); ++i)
for (mask = 1; mask; mask = 1)
-if (ft[i].check_feat mask *ft[i].guest_feat mask
+if (*ft[i].guest_feat mask
!(*ft[i].host_feat mask)) {
unavailable_host_feature(ft[i], mask);
rv = 1;
--
1.7.11.7
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set
On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
#endif /* CONFIG_KVM */
}
+#ifdef CONFIG_KVM
static int unavailable_host_feature(struct model_features_t *f, uint32_t
mask)
{
int i;
@@ -987,6 +988,7 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
}
return rv;
}
+#endif
static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
*opaque,
const char *name, Error **errp)
@@ -1410,10 +1412,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
x86_cpu_def-kvm_features = ~minus_kvm_features;
x86_cpu_def-svm_features = ~minus_svm_features;
x86_cpu_def-cpuid_7_0_ebx_features = ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
if (check_cpuid kvm_enabled()) {
if (kvm_check_features_against_host(x86_cpu_def) enforce_cpuid)
goto error;
}
+#endif
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.
return 0;
error:
--
1.7.11.7
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH qom-cpu 11/11] target-i386: check/enforce: Check all feature words
On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
This adds the following feature words to the list of flags to be checked
by kvm_check_features_against_host():
- cpuid_7_0_ebx_features
- ext4_features
- kvm_features
- svm_features
This will ensure the enforce flag works as it should: it won't allow
QEMU to be started unless every flag that was requested by the user or
defined in the CPU model is supported by the host.
This patch may cause existing configurations where enforce wasn't
preventing QEMU from being started to abort QEMU. But that's exactly the
point of this patch: if a flag was not supported by the host and QEMU
wasn't aborting, it was a bug in the enforce code.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: k...@vger.kernel.org
Cc: libvir-list@redhat.com
Cc: Jiri Denemark jdene...@redhat.com
CCing libvirt people, as this is directly related to the planned usage
of the enforce flag by libvirt.
The libvirt team probably has a problem in their hands: libvirt should
use enforce to make sure all requested flags are making their way into
the guest (so the resulting CPU is always the same, on any host), but
users may have existing working configurations where a flag is not
supported by the guest and the user really doesn't care about it. Those
configurations will necessarily break when libvirt starts using
enforce.
One example where it may cause trouble for common setups: pc-1.3 wants
the kvm_pv_eoi flag enabled by default (so enforce will make sure it
is enabled), but the user may have an existing VM running on a host
without pv_eoi support. That setup is unsafe today because
live-migration between different host kernel versions may enable/disable
pv_eoi silently (that's why we need the enforce flag to be used by
libvirt), but the user probably would like to be able to live-migrate
that VM anyway (and have libvirt to just do the right thing).
One possible solution to libvirt is to use enforce only on newer
machine-types, so existing machines with older machine-types will keep
the unsafe host-dependent-ABI behavior, but at least would keep
live-migration working in case the user is careful.
I really don't know what the libvirt team prefers, but that's the
situation today. The longer we take to make enforce strict as it
should and make libvirt finally use it, more users will have VMs with
migration-unsafe unpredictable guest ABIs.
Changes v2:
- Coding style fix
---
target-i386/cpu.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 876b0f6..52727ad 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -955,8 +955,9 @@ static int unavailable_host_feature(struct
model_features_t *f, uint32_t mask)
return 0;
}
-/* best effort attempt to inform user requested cpu flags aren't making
- * their way to the guest.
+/* Check if all requested cpu flags are making their way to the guest
+ *
+ * Returns 0 if all flags are supported by the host, non-zero otherwise.
*
* This function may be called only if KVM is enabled.
*/
@@ -973,7 +974,15 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
{guest_def-ext2_features, host_def.ext2_features,
ext2_feature_name, 0x8001, R_EDX},
{guest_def-ext3_features, host_def.ext3_features,
-ext3_feature_name, 0x8001, R_ECX}
+ext3_feature_name, 0x8001, R_ECX},
+{guest_def-ext4_features, host_def.ext4_features,
+NULL, 0xC001, R_EDX},
Since there is not name array for ext4_features they cannot be added or
removed on the command line hence no need to check them, no?
+{guest_def-cpuid_7_0_ebx_features,
host_def.cpuid_7_0_ebx_features,
+cpuid_7_0_ebx_feature_name, 7, R_EBX},
+{guest_def-svm_features, host_def.svm_features,
+svm_feature_name, 0x800A, R_EDX},
+{guest_def-kvm_features, host_def.kvm_features,
+kvm_feature_name, KVM_CPUID_FEATURES, R_EAX},
};
assert(kvm_enabled());
--
1.7.11.7
--
Gleb.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Mon, Mar 26, 2012 at 11:08:16AM +0200, Avi Kivity wrote: Exactly. The types are no different, so there's no reason to discriminate against types that happen to live in qemu-provided data files vs. qemu code. They aren't instantiated, so we lose nothing by creating the factories (just so long as the factories aren't mass-producing objects). At some point, I'd like to have type modules that are shared objects. I'd like QEMU to start with almost no builtin types and allow the user to configure which modules get loaded. In the long term, I'd like QEMU to be a small, robust core with the vast majority of code relegated to modules with the user ultimately in control of module loading. Yes, I'd want some module autoloading system but there should always be a way to launch QEMU without loading any modules and then load a very specific set of modules (as defined by the user). You can imagine this being useful for something like Common Criteria certifications. Okay. Modularised minimal QEMU may be a good thing, but how -nodefconfig helps here? Won't you have the same effect if QEMU will load modules on demand, only when they are actually needed (regardless of -nodefconfig). i.e virtio-blk is loaded only if there is -device virtio-blk somewhere in configuration. It's obviously defined for a given release, just not defined long term. If I see something like -nodefconfig, I assume it will create a bare bones guest that will not depend on any qemu defaults and will be stable across releases. That's not even close to what -nodefconfig is. That's pretty much what -nodefaults is but -nodefaults has also had a fluid definition historically. Okay. Let's just make sure to document -nodefconfig as version specific and -nodefaults as the stable way to create a bare bones guest (and define exactly what that means). What is the reason libvirt uses -nodefconfig instead of -nodefaults now? What the former does for them that the later doesn't? -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Mon, Mar 26, 2012 at 01:59:05PM +0200, Avi Kivity wrote: On 03/26/2012 01:24 PM, Jiri Denemark wrote: ... The command line becomes unstable if you use -nodefconfig. -no-user-config solves this but I fully expect libvirt would continue to use -nodefconfig. Libvirt uses -nodefaults -nodefconfig because it wants to fully control how the virtual machine will look like (mainly in terms of devices). In other words, we don't want any devices to just magically appear without libvirt knowing about them. -nodefaults gets rid of default devices that are built directly in qemu. Since users can set any devices or command line options (such as enable-kvm) into qemu configuration files in @SYSCONFDIR@, we need to avoid reading those files as well. Hence we use -nodefconfig. However, we would still like qemu to read CPU definitions, machine types, etc. once they become externally loaded configuration (or however we decide to call it). That said, when CPU definitions are moved into @DATADIR@, and -no-user-config is introduced, I don't see any reason for libvirt to keep using -nodefconfig. I actually like -no-user-config more than -nodefconfig -readconfig @DATADIR@/... since it would avoid additional magic to detect what files libvirt should explicitly pass to -readconfig but basically any approach that would allow us to do read files only from @DATADIR@ is much better than what we have with -nodefconfig now. That's how I see it as well. +1 except that instead of -no-user-config we can do what most other programs do. If config file is specified during invocation default one is not used. After implementing -no-user-config (or similar) we can drop -nodefconfig entirely since its only user will be gone it its semantics is not clear. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Thu, Mar 22, 2012 at 03:01:17PM -0500, Anthony Liguori wrote: On 03/22/2012 12:14 PM, Eduardo Habkost wrote: On Thu, Mar 22, 2012 at 11:37:39AM -0500, Anthony Liguori wrote: On 03/22/2012 04:32 AM, Gleb Natapov wrote: On Tue, Mar 13, 2012 at 11:53:19AM -0300, Eduardo Habkost wrote: So, this problem is solved if the defaults are easily found on /usr/share. What problem is solved and why are we mixing machine configuration files and cpu configuration files? They are different and should be treated differently. -nodefconfig exists only because there is not machine configuration files currently. With machine configuration files libvirt does not need -nodefconfig because it can create its own machine file and make QEMU use it. So specifying machine file on QEMU's command line implies -nodefconfig. The option itself loses its meaning and can be dropped. No, -nodefconfig means no default config. As with many projects, we can have *some* configuration required. The default configure should have a: [system] readconfig=@SYSCONFDIR@/cpu-models-x86_64.cfg Not @SYSCONFDIR@, but @DATADIR@. CPU models belong to /usr/share because they aren't meant to be changed by the user (I think I already explained why: because we have to be able to deploy fixes to them). Stanza by default. If libvirt wants to reuse this, they can use -readconfig if they use -nodefconfig. You are just repeating how you believe it should work based on the premise that cpudefs are configuration. We're discussing/questioning this exact premise, here, and I would really appreciate to hear why the model Gleb is proposing is not valid. More precisely, this part: cpu-models-x86.conf is not a configuration file. It is hardware description file. QEMU should not lose capability just because you run it with -nodefconfig. -nodefconfig means that QEMU does not create machine for you, but all parts needed to create a machine that would have been created without -nodefconfig are still present. Not been able to create Nehalem CPU after specifying -nodefconfig is the same as not been able to create virtio-net i.e the bug. And the related points Gleb mentioned further in this thread. Because the next patch series that would follow would be a -cpu-defs-path that would be a one-off hack with a global variable and a -no-cpu-defs. And it will be rejected since cpu models are not part of configuration, but QEMU internals stored in outside file. We have -L switch to tell qemu where such things should be loaded from and that's it. So let's avoid that and start by having a positive configuration mechanism that the user can use to change the path and exclude it. My suggestion eliminate the need for two future command line options. If cpu models are not part of configuration they should not be affected by configuration mechanism. You are just avoiding addressing the real question that if asked above. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Thu, Mar 22, 2012 at 12:50:18PM -0300, Eduardo Habkost wrote: On Thu, Mar 22, 2012 at 04:30:55PM +0200, Gleb Natapov wrote: On Thu, Mar 22, 2012 at 10:31:21AM -0300, Eduardo Habkost wrote: On Thu, Mar 22, 2012 at 11:32:44AM +0200, Gleb Natapov wrote: What does this mean? Will -nodefconfig disable loading of bios.bin, option roms, keymaps? Correcting myself: loading of _config_ files on /usr/share. ROM images are opaque data to be presented to the guest somehow, just like a disk image or kernel binary. But maybe keymaps will become configuration someday, I really don't know. Where do you draw the line between opaque data and configuration. CPU models are also something that is present to a guest somehow. Just the fact that it's in a structured key=value format that Qemu itself will interpret before exposing something to the guest. Yes, it's a bit arbitrary. If we could make everything configuration data, we would (or that's what I think Anthony is pushing for--I hope he will reply and clarify that). It is not a bit arbitrary it is completely arbitrary. Are you consider ROMs to be opaque data because they are binary and CPU models to be config just because it is ascii file? Not just ascii file, but structured (and relatively small) [section]key=value data. If BIOSes were not opaque binary code and could be converted to a small set of config sections and key=values just like cpudefs, one could argue that BIOSes could become configuration data, too. There is no argument I can make about it since there is no logic to refute to begin with :) Well may be except that when cpu model file will support configuring all couid leafs for each cpu model it will not be so small :) What if we pre-process CPU models into binary for QEMU to read will it magically stop being configuration? Doing the reverse (transforming simple [section]key=value data to a binary format) would just be silly and wouldn't gain us anything. The point here is that we (Qemu) seem to be moving towards a design where most things are structured configuration data that fits on a [section]key=value model, and Qemu just interprets that structured data to build a virtual machine. Nothing silly about it. You can move data parsing outside of QEMU and just mmap cpu definitions in QEMU. (That's why I said that perhaps keymaps could become configuration someday. Because maybe they can be converted to a key=value model relatively easily) Such whole sale approach is harmful since it starts to affect design decisions. So now if it seams logical to move something outside the code one can decide against it just because it will become configuration due to that design. Doing this would make it impossible to deploy fixes to users if we evern find out that the default configuration file had a serious bug. What if a bug in our default configuration file has a serious security implication? The answer to this is: if the broken templates/defaults are on /usr/share, it would be easy to deploy the fix. So, the compromise solution is: - We can move some configuration data (especially defaults/templates) to /usr/share (machine-types and CPU models could go there). This way we can easily deploy fixes to the defaults, if necessary. - To reuse Qemu models, or machine-types, and not define everything from scratch, libvirt will have to use something like: -nodefconfig -readconfig /usr/share/qemu/cpu-models-x86.conf cpu-models-x86.conf is not a configuration file. It is hardware description file. QEMU should not lose capability just because you run it with -nodefconfig. -nodefconfig means that QEMU does not create machine for you, but all parts needed to create a machine that would have been created without -nodefconfig are still present. Not been able to create Nehalem CPU after specifying -nodefconfig is the same as not been able to create virtio-net i.e the bug. The current design direction Qemu seems to be following is different from that: hardware description is also considered configuration just like actual machine configuration. Anthony, please correct me if I am wrong. That's a bug. Why trying to rationalize it now instead of fixing it. It's just a bug for you because you disagree with the current design. You can call it rationalization, yes; I am just accepting Anthony's proposal because it's equally good (to me) as what you are proposing. It was fixed in RHEL by the same person who introduced it in upstream in the first place. He just forgot to send the fix upstream. Does bug that is present for a long time is promoted to a feature? John didn't forget it, he knew that upstream could go in a different direction. The RHEL6 patch description has this: Note this is intended
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Sun, Mar 25, 2012 at 03:14:56PM +0200, Avi Kivity wrote: On 03/25/2012 03:12 PM, Anthony Liguori wrote: qemu -M pc Would effectively be short hand for -readconfig /usr/share/qemu/machines/pc.cfg In that case qemu -cpu westmere is shorthand for -readconfig /usr/share/qemu/cpus/westmere.cfg. This is not a bad suggestion, although it would make -cpu ? a bit awkward. Do you see an advantage to this over having /usr/share/qemu/target-x86_64-cpus.cfg that's read early on? Nope. As long as qemu -nodefconfig -cpu westmere works, I'm happy. As log as qemu -nodefconfig -cpu westmere -M pc1.1 can use different westmere definition than -M pc1.0 (by amending it according to qom properties in pc1.1 machine description or by reading /usr/share/qemu/cpus/westmere-pc1.1.cfg instead) I'm happy too. The reasoning is, loading target-x86_64-cpus.cfg does not alter the current instance's configuration, so reading it doesn't violate -nodefconfig. files be read by default or just treated as additional configuration files. If they're read as soon as they're referenced, what's the difference? I think the thread has reduced to: should /usr/share configuration I suspect libvirt would not be happy with reading configuration files on demand.. Why not? -- error compiling committee.c: too many arguments to function -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Tue, Mar 13, 2012 at 11:53:19AM -0300, Eduardo Habkost wrote: So, trying to summarize what was discussed in the call: On Mon, Mar 12, 2012 at 10:08:10AM -0300, Eduardo Habkost wrote: Let's say we moved CPU definitions to /usr/share/qemu/cpu-models.xml. Obviously, we'd want a command line option to be able to change that location so we'd introduce -cpu-models PATH. But we want all of our command line options to be settable by the global configuration file so we would have a cpu-model=PATH to the configuration file. But why hard code a path when we can just set the default path in the configuration file so let's avoid hard coding and just put cpu-models=/usr/share/qemu/cpu-models.xml in the default configuration file. We wouldn't do the above. -nodefconfig should disable the loading of files on /etc, but it shouldn't disable loading internal non-configurable data that we just happened to choose to store outside the qemu binary because it makes development easier. The statement above is the one not fulfilled by the compromise solution: -nodefconfig would really disable the loading of files on /usr/share. What does this mean? Will -nodefconfig disable loading of bios.bin, option roms, keymaps? Really, the requirement of a default configuration file is a problem by itself. Qemu should not require a default configuration file to work, and it shouldn't require users to copy the default configuration file to change options from the default. The statement above is only partly true. The default configuration file would be still needed, but if defaults are stored on /usr/share, I will be happy with it. My main problem was with the need to _copy_ or edit a non-trivial default config file. If the not-often-edited defaults/templates are easily found on /usr/share to be used with -readconfig, I will be happy with this solution, even if -nodefconfig disable the files on /usr/share. Doing this would make it impossible to deploy fixes to users if we evern find out that the default configuration file had a serious bug. What if a bug in our default configuration file has a serious security implication? The answer to this is: if the broken templates/defaults are on /usr/share, it would be easy to deploy the fix. So, the compromise solution is: - We can move some configuration data (especially defaults/templates) to /usr/share (machine-types and CPU models could go there). This way we can easily deploy fixes to the defaults, if necessary. - To reuse Qemu models, or machine-types, and not define everything from scratch, libvirt will have to use something like: -nodefconfig -readconfig /usr/share/qemu/cpu-models-x86.conf cpu-models-x86.conf is not a configuration file. It is hardware description file. QEMU should not lose capability just because you run it with -nodefconfig. -nodefconfig means that QEMU does not create machine for you, but all parts needed to create a machine that would have been created without -nodefconfig are still present. Not been able to create Nehalem CPU after specifying -nodefconfig is the same as not been able to create virtio-net i.e the bug. (the item below is not something discussed on the call, just something I want to add) To make this work better, we can allow users (humans or machines) to extend CPU models on the config file, instead of having to define everything from scratch. So, on /etc (or on a libvirt-generated config) we could have something like: = [cpu] base_cpudef = Nehalem add_features = vmx = Then, as long as /usr/share/cpu-models-x86.conf is loaded, the user will be able to reuse the Nehalem CPU model provided by Qemu. And if it will not be loaded? But now when libvirt uses -nodefconfig, those models go away. -nodefconfig means start QEMU in the most minimal state possible. You get what you pay for if you use it. We'll have the same problem with machine configuration files. At some point in time, -nodefconfig will make machine models disappear. It shouldn't. Machine-types are defaults to be used as base, they are not user-provided configuration. And the fact that we decided to store some data outside of the Qemu binary is orthogonal the design decisions in the Qemu command-line and configuration interface. So, this problem is solved if the defaults are easily found on /usr/share. What problem is solved and why are we mixing machine configuration files and cpu configuration files? They are different and should be treated differently. -nodefconfig exists only because there is not machine configuration files currently. With machine configuration files libvirt does not need -nodefconfig because it can create its own machine file and make QEMU use it. So specifying machine file on QEMU's command line implies -nodefconfig. The option itself loses its meaning and can
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Thu, Mar 22, 2012 at 10:31:21AM -0300, Eduardo Habkost wrote: On Thu, Mar 22, 2012 at 11:32:44AM +0200, Gleb Natapov wrote: On Tue, Mar 13, 2012 at 11:53:19AM -0300, Eduardo Habkost wrote: So, trying to summarize what was discussed in the call: On Mon, Mar 12, 2012 at 10:08:10AM -0300, Eduardo Habkost wrote: Let's say we moved CPU definitions to /usr/share/qemu/cpu-models.xml. Obviously, we'd want a command line option to be able to change that location so we'd introduce -cpu-models PATH. But we want all of our command line options to be settable by the global configuration file so we would have a cpu-model=PATH to the configuration file. But why hard code a path when we can just set the default path in the configuration file so let's avoid hard coding and just put cpu-models=/usr/share/qemu/cpu-models.xml in the default configuration file. We wouldn't do the above. -nodefconfig should disable the loading of files on /etc, but it shouldn't disable loading internal non-configurable data that we just happened to choose to store outside the qemu binary because it makes development easier. The statement above is the one not fulfilled by the compromise solution: -nodefconfig would really disable the loading of files on /usr/share. What does this mean? Will -nodefconfig disable loading of bios.bin, option roms, keymaps? Correcting myself: loading of _config_ files on /usr/share. ROM images are opaque data to be presented to the guest somehow, just like a disk image or kernel binary. But maybe keymaps will become configuration someday, I really don't know. Where do you draw the line between opaque data and configuration. CPU models are also something that is present to a guest somehow. Are you consider ROMs to be opaque data because they are binary and CPU models to be config just because it is ascii file? What if we pre-process CPU models into binary for QEMU to read will it magically stop being configuration? Doing this would make it impossible to deploy fixes to users if we evern find out that the default configuration file had a serious bug. What if a bug in our default configuration file has a serious security implication? The answer to this is: if the broken templates/defaults are on /usr/share, it would be easy to deploy the fix. So, the compromise solution is: - We can move some configuration data (especially defaults/templates) to /usr/share (machine-types and CPU models could go there). This way we can easily deploy fixes to the defaults, if necessary. - To reuse Qemu models, or machine-types, and not define everything from scratch, libvirt will have to use something like: -nodefconfig -readconfig /usr/share/qemu/cpu-models-x86.conf cpu-models-x86.conf is not a configuration file. It is hardware description file. QEMU should not lose capability just because you run it with -nodefconfig. -nodefconfig means that QEMU does not create machine for you, but all parts needed to create a machine that would have been created without -nodefconfig are still present. Not been able to create Nehalem CPU after specifying -nodefconfig is the same as not been able to create virtio-net i.e the bug. The current design direction Qemu seems to be following is different from that: hardware description is also considered configuration just like actual machine configuration. Anthony, please correct me if I am wrong. That's a bug. Why trying to rationalize it now instead of fixing it. It was fixed in RHEL by the same person who introduced it in upstream in the first place. He just forgot to send the fix upstream. Does bug that is present for a long time is promoted to a feature? What you propose is to have two levels of configuration (or descriptions, or whatever we call it): 1) Hardware descriptions (or templates, or models, whatever we call it), that are not editable by the user (and not disabled by -nodefconfig). This may include CPU models, hardware emulation implemented in another language, machine-types, and other stuff that is part of what Qemu always provides. 2) Actual machine configuration file, that is configurable and editable by the user, and normally loaded from /etc on from the command-line. The only problem is: the Qemu design simply doesn't have this distinction today (well, it _has_, the only difference is that today item (1) is almost completely coded inside tables in .c files). So if we want to go in that direction we have to agree this will be part of the Qemu design. I am not strongly inclined either way. Both approaches look good to me, we just have to decide where we are going, because we're are in this weird position todady because we never decided it explicitly, libvirt expected one thing, and we implemented something else
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Mon, Mar 12, 2012 at 01:04:19PM +, Daniel P. Berrange wrote: On Mon, Mar 12, 2012 at 09:52:27AM -0300, Eduardo Habkost wrote: On Sun, Mar 11, 2012 at 09:12:58AM -0500, Anthony Liguori wrote: On 03/11/2012 08:27 AM, Gleb Natapov wrote: On Sat, Mar 10, 2012 at 12:24:47PM -0600, Anthony Liguori wrote: Let's step back here. Why are you writing these patches? It's probably not because you have a desire to say -cpu Westmere when you run QEMU on your laptop. I'd wager to say that no human has ever done that or that if they had, they did so by accident because they read documentation and thought they had to. No, it's because libvirt doesn't handle all the tiny small details involved in specifying a CPU. All libvirty knows about are a set of CPU flag bits, but it knows nothing about 'level', 'family', and 'xlevel', but we would like to allow it to expose a Westmere-like CPU to the guest. This is easily fixable in libvirt - so for the point of going discussion, IMHO, we can assume libvirt will support level, family, xlevel, etc. And fill in all cpuid leafs by querying /dev/kvm when needed or, if TCG is used, replicating QEMU logic? And since QEMU should be usable without libvirt the same logic should be implemented in QEMU anyway. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Mon, Mar 12, 2012 at 10:32:21AM -0300, Eduardo Habkost wrote: On Mon, Mar 12, 2012 at 03:15:32PM +0200, Gleb Natapov wrote: On Mon, Mar 12, 2012 at 01:04:19PM +, Daniel P. Berrange wrote: On Mon, Mar 12, 2012 at 09:52:27AM -0300, Eduardo Habkost wrote: On Sun, Mar 11, 2012 at 09:12:58AM -0500, Anthony Liguori wrote: On 03/11/2012 08:27 AM, Gleb Natapov wrote: On Sat, Mar 10, 2012 at 12:24:47PM -0600, Anthony Liguori wrote: Let's step back here. Why are you writing these patches? It's probably not because you have a desire to say -cpu Westmere when you run QEMU on your laptop. I'd wager to say that no human has ever done that or that if they had, they did so by accident because they read documentation and thought they had to. No, it's because libvirt doesn't handle all the tiny small details involved in specifying a CPU. All libvirty knows about are a set of CPU flag bits, but it knows nothing about 'level', 'family', and 'xlevel', but we would like to allow it to expose a Westmere-like CPU to the guest. This is easily fixable in libvirt - so for the point of going discussion, IMHO, we can assume libvirt will support level, family, xlevel, etc. And fill in all cpuid leafs by querying /dev/kvm when needed or, if TCG is used, replicating QEMU logic? And since QEMU should be usable without libvirt the same logic should be implemented in QEMU anyway. To implement this properly, libvirt will need a proper probing interface to know what exactly is available and can be enabled. I plan to implement that. I am have no problem in giving to libvirt the power to shoot itself in the foot. I believe libvirt developers can handle that. I have a problem with requiring every user (human or machine) to handle a weapon that can shoot their foot (that means, requiring the user to write the CPU model definition from scratch, or requiring the user to blindly copypaste the default config file). You are dangerous person Eduardo! -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Mon, Mar 12, 2012 at 01:50:18PM +, Daniel P. Berrange wrote: On Mon, Mar 12, 2012 at 03:15:32PM +0200, Gleb Natapov wrote: On Mon, Mar 12, 2012 at 01:04:19PM +, Daniel P. Berrange wrote: On Mon, Mar 12, 2012 at 09:52:27AM -0300, Eduardo Habkost wrote: On Sun, Mar 11, 2012 at 09:12:58AM -0500, Anthony Liguori wrote: On 03/11/2012 08:27 AM, Gleb Natapov wrote: On Sat, Mar 10, 2012 at 12:24:47PM -0600, Anthony Liguori wrote: Let's step back here. Why are you writing these patches? It's probably not because you have a desire to say -cpu Westmere when you run QEMU on your laptop. I'd wager to say that no human has ever done that or that if they had, they did so by accident because they read documentation and thought they had to. No, it's because libvirt doesn't handle all the tiny small details involved in specifying a CPU. All libvirty knows about are a set of CPU flag bits, but it knows nothing about 'level', 'family', and 'xlevel', but we would like to allow it to expose a Westmere-like CPU to the guest. This is easily fixable in libvirt - so for the point of going discussion, IMHO, we can assume libvirt will support level, family, xlevel, etc. And fill in all cpuid leafs by querying /dev/kvm when needed or, if TCG is used, replicating QEMU logic? And since QEMU should be usable without libvirt the same logic should be implemented in QEMU anyway. I'm not refering to that. I'm saying that any data QEMU has in its config file (/etc/qemu/target-x86_64.conf) should be represented in the libvirt CPU XML. family, model, stepping, xlevel and model_id are currently in QEMU CPU configs, but not in libvirt XML, which is something we will fix. The other issues you mention are completely independant of that. Eduardo is going to extend what can be configured in /etc/qemu/target-x86_64.conf and make CPU models name per machine type. What QEMU has now is not good enough. I doubt libvirt goal is to be as bad as QEMU :) -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Mon, Mar 12, 2012 at 01:55:34PM +, Daniel P. Berrange wrote: On Mon, Mar 12, 2012 at 03:53:38PM +0200, Gleb Natapov wrote: On Mon, Mar 12, 2012 at 01:50:18PM +, Daniel P. Berrange wrote: On Mon, Mar 12, 2012 at 03:15:32PM +0200, Gleb Natapov wrote: On Mon, Mar 12, 2012 at 01:04:19PM +, Daniel P. Berrange wrote: On Mon, Mar 12, 2012 at 09:52:27AM -0300, Eduardo Habkost wrote: On Sun, Mar 11, 2012 at 09:12:58AM -0500, Anthony Liguori wrote: On 03/11/2012 08:27 AM, Gleb Natapov wrote: On Sat, Mar 10, 2012 at 12:24:47PM -0600, Anthony Liguori wrote: Let's step back here. Why are you writing these patches? It's probably not because you have a desire to say -cpu Westmere when you run QEMU on your laptop. I'd wager to say that no human has ever done that or that if they had, they did so by accident because they read documentation and thought they had to. No, it's because libvirt doesn't handle all the tiny small details involved in specifying a CPU. All libvirty knows about are a set of CPU flag bits, but it knows nothing about 'level', 'family', and 'xlevel', but we would like to allow it to expose a Westmere-like CPU to the guest. This is easily fixable in libvirt - so for the point of going discussion, IMHO, we can assume libvirt will support level, family, xlevel, etc. And fill in all cpuid leafs by querying /dev/kvm when needed or, if TCG is used, replicating QEMU logic? And since QEMU should be usable without libvirt the same logic should be implemented in QEMU anyway. I'm not refering to that. I'm saying that any data QEMU has in its config file (/etc/qemu/target-x86_64.conf) should be represented in the libvirt CPU XML. family, model, stepping, xlevel and model_id are currently in QEMU CPU configs, but not in libvirt XML, which is something we will fix. The other issues you mention are completely independant of that. Eduardo is going to extend what can be configured in /etc/qemu/target-x86_64.conf and make CPU models name per machine type. What QEMU has now is not good enough. I doubt libvirt goal is to be as bad as QEMU :) Of course not - libvirt will obviously be extended to cope with this too So you goal is to follow closely what QEMU does? Fine by me, but then QEMU design decisions in this ares should not rely on libvirt (as in this is libvirt job). -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Mon, Mar 12, 2012 at 06:53:27PM +0100, Andreas Färber wrote: Am 12.03.2012 18:47, schrieb Peter Maydell: On 12 March 2012 17:41, Andreas Färber afaer...@suse.de wrote: Also keep in mind linux-user. There's no concept of a machine there, but there's a cpu_copy() function used for forking that tries to re-create the CPU based on its model. Incidentally, do you know why the linux-user code calls cpu_reset on the newly copied CPU state but only for TARGET_I386/SPARC/PPC ? That looks very odd to me... Incidentally for i386 I do: cpu_reset() is intentionally not part of cpu_init() there because afterwards the machine or something sets whether this CPU is a bsp (Board Support Package? ;)) and only then Boot Strap Processor I guess :) resets it. For ppc and sparc I don't know but I'd be surprised if it's necessary for ppc... Alex? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Mon, Mar 12, 2012 at 06:41:06PM +0100, Andreas Färber wrote: Am 12.03.2012 17:50, schrieb Eduardo Habkost: On Mon, Mar 12, 2012 at 04:49:47PM +0100, Andreas Färber wrote: Am 11.03.2012 17:16, schrieb Gleb Natapov: On Sun, Mar 11, 2012 at 10:33:15AM -0500, Anthony Liguori wrote: On 03/11/2012 09:56 AM, Gleb Natapov wrote: On Sun, Mar 11, 2012 at 09:12:58AM -0500, Anthony Liguori wrote: -cpu best wouldn't solve this. You need a read/write configuration file where QEMU probes the available CPU and records it to be used for the lifetime of the VM. That what I thought too, but this shouldn't be the case (Avi's idea). We need two things: 1) CPU model config should be per machine type. 2) QEMU should refuse to start if it cannot create cpu exactly as specified by model config. This would either mean: A. pc-1.1 uses -cpu best with a fixed mask for 1.1 B. pc-1.1 hardcodes Westmere or some other family This would mean neither A nor B. May be it wasn't clear but I didn't talk about -cpu best above. I am talking about any CPU model with fixed meaning (not host or best which are host cpu dependant). Lets take Nehalem for example (just to move from Westmere :)). Currently it has level=2. Eduardo wants to fix it to be 11, but old guests, installed with -cpu Nehalem, should see the same CPU exactly. How do you do it? Have different Nehalem definition for pc-1.0 (which level=2) and pc-1.1 (with level=11). Lets get back to Westmere. It actually has level=11, but that's only expose another problem. Kernel 3.3 and qemu-1.1 combo will support architectural PMU which is exposed in cpuid leaf 10. We do not want guests installed with -cpu Westmere and qemu-1.0 to see architectural PMU after upgrade. How do you do it? Have different Westmere definitions for pc-1.0 (does not report PMU) and pc-1.1 (reports PMU). What happens if you'll try to run qemu-1.1 -cpu Westmere on Kernel 3.3 (without PMU support)? Qemu will fail to start. [...] IMO interpreting an explicit -cpu parameter depending on -M would be wrong. Changing the default CPU based on -M is fine with me. For an explicit argument we would need Westmere-1.0 analog to pc-1.0. Then the user gets what the user asks for, without unexpected magic. It is not unexpected magic. It would be a documented mechanism: -cpu Nehalem-1.0 and -cpu Nehalem-1.1 would have the same meaning every time, with any machine-type, but -cpu Nehalem would be an alias, whose meaning depends on the machine-type. Otherwise we would be stuck with a broken Nehalem model forever, and we don't want that. Not quite what I meant: In light of QOM we should be able to instantiate a CPU based on its name and optional parameters IMO. No dependency on the machine, please. An alias sure, but if the user explicitly says -cpu Nehalem then on 1.1 it should always be an alias to Nehalem-1.1 whether the machine is -M pc-0.15 or pc. If no -cpu was specified by the user, then choosing a default of Nehalem-1.0 for pc-1.0 is fine. Just trying to keep separate things separate here. Those things are not separate. If user will get Nehalem-1.1 with -M pc-0.15 on qemu-1.1 it will get broken VM. If user uses -M pc-0.15 it should get exactly same machine it gets by running qemu-0.15. Guest should not be able to tell the difference. This is the reason -M exists, anything else is a bug. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Modern CPU models cannot be used with libvirt
On Fri, Mar 09, 2012 at 03:15:26PM -0600, Anthony Liguori wrote: On 03/09/2012 03:04 PM, Daniel P. Berrange wrote: On Fri, Mar 09, 2012 at 05:56:52PM -0300, Eduardo Habkost wrote: Resurrecting an old thread: I didn't see any clear conclusion in this thread (this is why I am resurrecting it), except that many were arguing that libvirt should simply copy and/or generate the CPU model definitions from Qemu. I really don't think it's reasonable to expect that. On Thu, Dec 15, 2011 at 03:54:15PM +0100, Jiri Denemark wrote: Hi, Recently I realized that all modern CPU models defined in /etc/qemu/target-x86_64.conf are useless when qemu is used through libvirt. That's because we start qemu with -nodefconfig which results in qemu ignoring that file with CPU model definitions. We have a very good reason for using -nodefconfig because we need to control the ABI presented to a guest OS and we don't want any configuration file that can contain lots of things including device definitions to be read by qemu. However, we would really like the new CPU models to be understood by qemu even if used through libvirt. What would be the best way to solve this? I suspect this could have been already discussed in the past but obviously a workable solution was either not found or just not implemented. So, our problem today is basically: A) libvirt uses -nodefconfig; B) -nodefconfig makes Qemu not load the config file containing the CPU model definitions; and C) libvirt expects the full CPU model list from Qemu to be available. I could have sworn we had this discussion a year ago or so, and had decided that the default CPU models would be in something like /usr/share/qemu/cpu-x86_64.conf and loaded regardless of the -nodefconfig setting. /etc/qemu/target-x86_64.conf would be solely for end user configuration changes, not for QEMU builtin defaults. But looking at the code in QEMU, it doesn't seem we ever implemented this ? I don't remember that discussion and really don't think I agree with the conclusion. If libvirt wants to define CPU models on their own, they can. If It can't without knowing qemu/host cpu/host kernel capabilities and knowing the logic that qemu uses to combine them. libvirt wants to use the user's definitions, don't use -nodefconfig. CPU models aren't a QEMU concept. The reason it's in the I do not know what do you mean by that, but CPU capabilities (and CPU model is only a name for a group of them) are KVM/TCG concept and, by inclusion, are QEMU concept. If QEMU will not have built in support for CPU models (as a name for a group of CPU capabilities) then how do you start a guest without specifying full set of CPU capabilities on a command line? configuration file is to allow a user to add their own as they see fit. There is no right model names. It's strictly a policy. So you think it should be user's responsibility to check what his qemu/host cpu/host kernel combo can support? -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Sat, Mar 10, 2012 at 12:58:43PM -0300, Eduardo Habkost wrote: On Sat, Mar 10, 2012 at 12:42:46PM +, Daniel P. Berrange wrote: I could have sworn we had this discussion a year ago or so, and had decided that the default CPU models would be in something like /usr/share/qemu/cpu-x86_64.conf and loaded regardless of the -nodefconfig setting. /etc/qemu/target-x86_64.conf would be solely for end user configuration changes, not for QEMU builtin defaults. But looking at the code in QEMU, it doesn't seem we ever implemented this ? Arrrgggh. It seems this was implemented as a patch in RHEL-6 qemu RPMs but, contrary to our normal RHEL development practice, it was not based on a cherry-pick of an upstream patch :-( For sake of reference, I'm attaching the two patches from the RHEL6 source RPM that do what I'm describing NB, I'm not neccessarily advocating these patches for upstream. I still maintain that libvirt should write out a config file containing the exact CPU model description it desires and specify that with -readconfig. The end result would be identical from QEMU's POV and it would avoid playing games with QEMU's config loading code. I agree that libvirt should just write the config somewhere. The problem here is to define: 1) what information should be mandatory on that config data; 2) who should be responsible to test and maintain sane defaults (and where should they be maintained). The current cpudef definitions are simply too low-level to require it to be written from scratch. Lots of testing have to be done to make sure we have working combinations of CPUID bits defined, so they can be used as defaults or templates. Not facilitating reuse of those tested defauls/templates by libvirt is duplication of efforts. Really, if we expect libvirt to define all the CPU bits from scratch on a config file, we could as well just expect libvirt to open /dev/kvm itself and call the all CPUID setup ioctl()s itself. That's how low-level some of the cpudef bits are. s/some/all If libvirt assumes anything about what kvm actually supports it is working only by sheer luck. (Also, there are additional low-level bits that really have to be maintained somewhere, just to have sane defaults. Currently many CPUID leafs are exposed to the guest without letting the user control them, and worse: without keeping stability of guest-visible bits when upgrading Qemu or the host kernel. And that's what machine-types are for: to have sane defaults to be used as base.) Let me give you a practical example: I had a bug report about improper CPU topology information[1]. After investigating it, I have found out that the level cpudef field is too low; CPU core topology information is provided on CPUID leaf 4, and most of the Intel CPU models on Qemu have level=2 today (I don't know why). So, Qemu is responsible for exposing CPU topology information set using '-smp' to the guest OS, but libvirt would have to be responsible for choosing a proper level value that makes that information visible to the guest. We can _allow_ libvirt to fiddle with these low-level bits, of course, but requiring every management layer to build this low-level information from scratch is just a recipe to waste developer time. And QEMU become even less usable from a command line. One more point to kvm-tool I guess. (And I really hope that there's no plan to require all those low-level bits to appear as-is on the libvirt XML definitions. Because that would require users to read the Intel 64 and IA-32 Architectures Software Developer's Manual, or the AMD64 Architecture Programmer's Manual and BIOS and Kernel Developer's Guides, just to understand why something is not working on his Virtual Machine.) [1] https://bugzilla.redhat.com/show_bug.cgi?id=689665 -- Eduardo -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Sat, Mar 10, 2012 at 12:24:47PM -0600, Anthony Liguori wrote: Let's step back here. Why are you writing these patches? It's probably not because you have a desire to say -cpu Westmere when you run QEMU on your laptop. I'd wager to say that no human has ever done that or that if they had, they did so by accident because they read documentation and thought they had to. I'd be glad if QEMU will chose -cpu Westmere for me if it detects Westmere host CPU as a default. Humans probably do one of two things: 1) no cpu option or 2) -cpu host. And both are not optimal. Actually both are bad. First one because default cpu is very conservative and the second because there is no guaranty that guest will continue to work after qemu or kernel upgrade. Let me elaborate about the later. Suppose host CPU has kill_guest feature and at the time a guest was installed it was not implemented by kvm. Since it was not implemented by kvm it was not present in vcpu during installation and the guest didn't install workaround kill_guest module. Now unsuspecting user upgrades the kernel and tries to restart the guest and fails. He writes angry letter to qemu-devel and is asked to reinstall his guest and move along. So then why are you introducing -cpu Westmere? Because ovirt-engine has a concept of datacenters and the entire datacenter has to use a compatible CPU model to allow migration compatibility. Today, the interface that ovirt-engine exposes is based on CPU codenames. Presumably ovirt-engine wants to add a Westmere CPU group and as such have levied a requirement down the stack to QEMU. First of all this is not about live migration only. Guest visible vcpu should not change after guest reboot (or hibernate/resume) too. And second this concept exists with only your laptop and single guest on it too. There are three inputs into a CPU model module: 1) host cpu, 2) qemu capabilities, 3) kvm capabilities. With datacenters scenario all three can change, with your laptop only last two can change (first one can change too when you'll get new laptop) , but the net result is that guest visible cpuid can change and it shouldn't. This is the goal of introducing -cpu Westmere, to prevent it from happening. But there's no intrinsic reason why it uses CPU model names. VMware doesn't do this. It has a concept of compatibility groups[1]. As Andrew noted, not any more. There is no intrinsic reason, but people are more familiar with Intel terminology than random hypervisor terminology. oVirt could just as well define compatibility groups like GroupA, GroupB, GroupC, etc. and then the -cpu option we would be discussing would be -cpu GroupA. It could, but I can't see why this is less confusing. This is why it's a configuration option and not builtin to QEMU. It's a user interface as as such, should be defined at a higher level. This is not the only configuration that is builtin in QEMU. As it stands now QEMU does not even allow configuring cpuid enough to define those compatibility groups outside of QEMU. And after the work is done to allow enough configurability there is no much left to provide compatibility groups in QEMU itself. Perhaps it really should be VDSM that is providing the model info to libvirt? Then they can add whatever groups then want whenever they want as long as we have the appropriate feature bits. P.S. I spent 30 minutes the other day helping a user who was attempting to figure out whether his processor was a Conroe, Penryn, etc. Making this determination is fairly difficult and it makes me wonder whether having CPU code names is even the best interface for oVirt.. [1] http://kb.vmware.com/selfservice/microsites/search.do?language=en_UScmd=displayKCexternalId=1991 Regards, Anthony Liguori (Also, there are additional low-level bits that really have to be maintained somewhere, just to have sane defaults. Currently many CPUID leafs are exposed to the guest without letting the user control them, and worse: without keeping stability of guest-visible bits when upgrading Qemu or the host kernel. And that's what machine-types are for: to have sane defaults to be used as base.) Let me give you a practical example: I had a bug report about improper CPU topology information[1]. After investigating it, I have found out that the level cpudef field is too low; CPU core topology information is provided on CPUID leaf 4, and most of the Intel CPU models on Qemu have level=2 today (I don't know why). So, Qemu is responsible for exposing CPU topology information set using '-smp' to the guest OS, but libvirt would have to be responsible for choosing a proper level value that makes that information visible to the guest. We can _allow_ libvirt to fiddle with these low-level bits, of course, but requiring every management layer to build this low-level information from scratch is just a recipe to waste developer time. (And I really hope that there's no
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Sun, Mar 11, 2012 at 09:12:58AM -0500, Anthony Liguori wrote: On 03/11/2012 08:27 AM, Gleb Natapov wrote: On Sat, Mar 10, 2012 at 12:24:47PM -0600, Anthony Liguori wrote: Let's step back here. Why are you writing these patches? It's probably not because you have a desire to say -cpu Westmere when you run QEMU on your laptop. I'd wager to say that no human has ever done that or that if they had, they did so by accident because they read documentation and thought they had to. I'd be glad if QEMU will chose -cpu Westmere for me if it detects Westmere host CPU as a default. This is -cpu best that Alex proposed FWIW. I didn't look at exact implementation but I doubt it does exactly what we need because currently we do not have infrastructure for that. If qemu is upgraded with support for new cpuid bits and -cpu best will pass them to a guest on next boot then this is not the same. -cpu Westmere can mean different thing for different machine types with proper infrastructure in place. Humans probably do one of two things: 1) no cpu option or 2) -cpu host. And both are not optimal. Actually both are bad. First one because default cpu is very conservative and the second because there is no guaranty that guest will continue to work after qemu or kernel upgrade. Let me elaborate about the later. Suppose host CPU has kill_guest feature and at the time a guest was installed it was not implemented by kvm. Since it was not implemented by kvm it was not present in vcpu during installation and the guest didn't install workaround kill_guest module. Now unsuspecting user upgrades the kernel and tries to restart the guest and fails. He writes angry letter to qemu-devel and is asked to reinstall his guest and move along. -cpu best wouldn't solve this. You need a read/write configuration file where QEMU probes the available CPU and records it to be used for the lifetime of the VM. That what I thought too, but this shouldn't be the case (Avi's idea). We need two things: 1) CPU model config should be per machine type. 2) QEMU should refuse to start if it cannot create cpu exactly as specified by model config. With two conditions above if user creates VM with qemu 1.0 and cpu model Westmere which has no kill_guest feature he will still be able to run it in QEMU 1.1 (where kill_guest is added to Westmere model) and new kvm that support kill_guest by providing -M pc-1.0 flag (old definition of Westmere will be used). If user will try to create VM with QEMU 1.1 on a kernel that does not support kill_guest QEMU will refuse to start. So then why are you introducing -cpu Westmere? Because ovirt-engine has a concept of datacenters and the entire datacenter has to use a compatible CPU model to allow migration compatibility. Today, the interface that ovirt-engine exposes is based on CPU codenames. Presumably ovirt-engine wants to add a Westmere CPU group and as such have levied a requirement down the stack to QEMU. First of all this is not about live migration only. Guest visible vcpu should not change after guest reboot (or hibernate/resume) too. And second this concept exists with only your laptop and single guest on it too. There are three inputs into a CPU model module: 1) host cpu, 2) qemu capabilities, 3) kvm capabilities. With datacenters scenario all three can change, with your laptop only last two can change (first one can change too when you'll get new laptop) , but the net result is that guest visible cpuid can change and it shouldn't. This is the goal of introducing -cpu Westmere, to prevent it from happening. This discussion isn't about whether QEMU should have a Westmere processor definition. In fact, I think I already applied that patch. It's a discussion about how we handle this up and down the stack. The question is who should define and manage CPU compatibility. Right now QEMU does to a certain degree, libvirt discards this and does it's own thing, and VDSM/ovirt-engine assume that we're providing something and has built a UI around it. If we want QEMU to be usable without management layer then QEMU should provide stable CPU models. Stable in a sense that qemu, kernel or CPU upgrade does not change what guest sees. If libvirt wants to override QEMU we should have a way to allow that, but than compatibility becomes libvirt problem. Figuring out what minimal CPU model that can be used across a cluster of different machines should be ovirt task. What I'm proposing we consider: have VDSM manage CPU definitions in order to provide a specific user experience in ovirt-engine. We would continue to have Westmere/etc in QEMU exposed as part of the user configuration. But I don't think it makes a lot of sense to have to modify QEMU any time a new CPU comes out. If new cpu does not provide any new instruction set or capability that can be passed to a guest then there is no point creating CPU model for it in QEMU. If it does it is just
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Sun, Mar 11, 2012 at 09:16:49AM -0500, Anthony Liguori wrote: If libvirt assumes anything about what kvm actually supports it is working only by sheer luck. Well the simple answer for libvirt is don't use -nodefconfig and then it can reuse the CPU definitions (including any that the user adds). CPU models should be usable even with -nodefconfig. CPU model is more like device. By -cpu Nehalem I am saying I want Nehalem device in my machine. Really, what's the point of having a layer of management if we're saying that doing policy management is too complicated for that layer? What does that layer exist to provide then? I was always against libvirt configuring low level details of CPU. What it should do IMO is to chose best CPU model for host cpu (one can argue that fiddling with /proc/cpuinfo is not QEMU busyness). (Also, there are additional low-level bits that really have to be maintained somewhere, just to have sane defaults. Currently many CPUID leafs are exposed to the guest without letting the user control them, and worse: without keeping stability of guest-visible bits when upgrading Qemu or the host kernel. And that's what machine-types are for: to have sane defaults to be used as base.) Let me give you a practical example: I had a bug report about improper CPU topology information[1]. After investigating it, I have found out that the level cpudef field is too low; CPU core topology information is provided on CPUID leaf 4, and most of the Intel CPU models on Qemu have level=2 today (I don't know why). So, Qemu is responsible for exposing CPU topology information set using '-smp' to the guest OS, but libvirt would have to be responsible for choosing a proper level value that makes that information visible to the guest. We can _allow_ libvirt to fiddle with these low-level bits, of course, but requiring every management layer to build this low-level information from scratch is just a recipe to waste developer time. And QEMU become even less usable from a command line. One more point to kvm-tool I guess. I'm not sure what your point is. We're talking about an option that humans don't use. How is this a discussion about QEMU usability? If for a user to have stable guest environment we require libvirt use then QEMU by itself is less usable. We do have machine types in QEMU to expose stable machine to a guest. CPU models should be part of it. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Sun, Mar 11, 2012 at 10:33:15AM -0500, Anthony Liguori wrote: On 03/11/2012 09:56 AM, Gleb Natapov wrote: On Sun, Mar 11, 2012 at 09:12:58AM -0500, Anthony Liguori wrote: -cpu best wouldn't solve this. You need a read/write configuration file where QEMU probes the available CPU and records it to be used for the lifetime of the VM. That what I thought too, but this shouldn't be the case (Avi's idea). We need two things: 1) CPU model config should be per machine type. 2) QEMU should refuse to start if it cannot create cpu exactly as specified by model config. This would either mean: A. pc-1.1 uses -cpu best with a fixed mask for 1.1 B. pc-1.1 hardcodes Westmere or some other family This would mean neither A nor B. May be it wasn't clear but I didn't talk about -cpu best above. I am talking about any CPU model with fixed meaning (not host or best which are host cpu dependant). Lets take Nehalem for example (just to move from Westmere :)). Currently it has level=2. Eduardo wants to fix it to be 11, but old guests, installed with -cpu Nehalem, should see the same CPU exactly. How do you do it? Have different Nehalem definition for pc-1.0 (which level=2) and pc-1.1 (with level=11). Lets get back to Westmere. It actually has level=11, but that's only expose another problem. Kernel 3.3 and qemu-1.1 combo will support architectural PMU which is exposed in cpuid leaf 10. We do not want guests installed with -cpu Westmere and qemu-1.0 to see architectural PMU after upgrade. How do you do it? Have different Westmere definitions for pc-1.0 (does not report PMU) and pc-1.1 (reports PMU). What happens if you'll try to run qemu-1.1 -cpu Westmere on Kernel 3.3 (without PMU support)? Qemu will fail to start. (A) would imply a different CPU if you moved the machine from one system to another. I would think this would be very problematic from a user's perspective. (B) would imply that we had to choose the least common denominator which is essentially what we do today with qemu64. If you want to just switch qemu64 to Conroe, I don't think that's a huge difference from what we have today. It's a discussion about how we handle this up and down the stack. The question is who should define and manage CPU compatibility. Right now QEMU does to a certain degree, libvirt discards this and does it's own thing, and VDSM/ovirt-engine assume that we're providing something and has built a UI around it. If we want QEMU to be usable without management layer then QEMU should provide stable CPU models. Stable in a sense that qemu, kernel or CPU upgrade does not change what guest sees. We do this today by exposing -cpu qemu64 by default. If all you're advocating is doing -cpu Conroe by default, that's fine. I am not advocating that. I am saying we should be able to amend qemu64 definition without breaking older guests that use it. But I fail to see where this fits into the larger discussion here. The problem to solve is: I want to use the largest possible subset of CPU features available uniformly throughout my datacenter. QEMU and libvirt have single node views so they cannot solve this problem on their own. Whether that subset is a generic Westmere-like processor that never existed IRL or a specific Westmere processor seems like a decision that should be made by the datacenter level manager with the node level view. If I have a homogeneous environments of Xeon 7540, I would probably like to see a Xeon 7540 in my guest. Doesn't it make sense to enable the management tool to make this decision? Of course neither QEMU nor libvirt can't made a cluster wide decision. If QEMU provides sane CPU model definitions (usable even with -nodefconfig) it would be always possible to find the model that fits best. If the oldest CPU in data center is Nehalem then probably -cpu Nehalem will do. But our CPU model definitions have a lot of shortcomings and we were talking with Edurado how to fix them when he brought this thread back to life, so may be I stirred the discussion a little bit in the wrong direction, but I do think those things are connected. If QEMU CPU model definitions are not stable across upgrades how can we say to management that it is safe to use them? Instead they insist in reimplementing the same logic in mngmt layer and do it badly (because the lack of info). -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Sun, Mar 11, 2012 at 10:41:32AM -0500, Anthony Liguori wrote: On 03/11/2012 10:12 AM, Gleb Natapov wrote: On Sun, Mar 11, 2012 at 09:16:49AM -0500, Anthony Liguori wrote: If libvirt assumes anything about what kvm actually supports it is working only by sheer luck. Well the simple answer for libvirt is don't use -nodefconfig and then it can reuse the CPU definitions (including any that the user adds). CPU models should be usable even with -nodefconfig. CPU model is more like device. By -cpu Nehalem I am saying I want Nehalem device in my machine. Let's say we moved CPU definitions to /usr/share/qemu/cpu-models.xml. Obviously, we'd want a command line option to be able to change that location so we'd introduce -cpu-models PATH. But we want all of our command line options to be settable by the global configuration file so we would have a cpu-model=PATH to the configuration file. But why hard code a path when we can just set the default path in the configuration file so let's avoid hard coding and just put cpu-models=/usr/share/qemu/cpu-models.xml in the default configuration file. We have two places where we define cpu models: hardcoded in target-i386/cpuid.c and in target-x86_64.conf. We moved them out to conf file because this way it is easier to add, update, examine compare CPU models. But they still should be treated as essential part of qemu. Given this I do not see the step above as a logical one. CPU models are not part of machine config. -cpu Nehalem,-sse,level=3,model=5 is part of machine config. What if we introduce a way to write devices in LUA. Should -nodefconfig drop devices implemented as LUA scripts too? But now when libvirt uses -nodefconfig, those models go away. -nodefconfig means start QEMU in the most minimal state possible. You get what you pay for if you use it. We'll have the same problem with machine configuration files. At some point in time, -nodefconfig will make machine models disappear. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt
On Thu, Dec 15, 2011 at 03:42:44PM +, Daniel P. Berrange wrote: On Thu, Dec 15, 2011 at 03:54:15PM +0100, Jiri Denemark wrote: Hi, Recently I realized that all modern CPU models defined in /etc/qemu/target-x86_64.conf are useless when qemu is used through libvirt. That's because we start qemu with -nodefconfig which results in qemu ignoring that file with CPU model definitions. We have a very good reason for using -nodefconfig because we need to control the ABI presented to a guest OS and we don't want any configuration file that can contain lots of things including device definitions to be read by qemu. However, we would really like the new CPU models to be understood by qemu even if used through libvirt. What would be the best way to solve this? Ideally libvirt would just write out a config file with the CPU that was configured in libvirt and pass -readconfig /var/lib/libvirt/qemu/guestcpu.conf That way libvirt can configure CPU models without regard for what the particular QEMU version might have in its config. And how libvirt would know that particular QEMU/kvm version can create this CPU. Managing cpuid shouldn't be libvirt busyness. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] qemu and qemu.git - Migration + disk stress introduces qcow2 corruptions
On Mon, Nov 14, 2011 at 11:29:18AM +, Daniel P. Berrange wrote: Do you have some more details on the kind of errors? Missing files, permissions, something like this? Or rather something related to the actual content of an image file? Missing files due to wrong/missing NFS mounts, or incorrect SAN / iSCSI setup. Access permissions due to incorrect user / group setup, or read only mounts, or SELinux denials. Actual I/O errors are less common and are not so likely to cause QEMU to fail to start any, since QEMU is likely to just report them to the guest OS instead. If started correctly QEMU should not report them to the guest OS, but pause instead. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] smbios fixes
On Thu, Dec 02, 2010 at 08:12:23PM +, Daniel P. Berrange wrote: On Thu, Dec 02, 2010 at 10:04:59PM +0200, Gleb Natapov wrote: On Thu, Dec 02, 2010 at 07:56:03PM +, Daniel P. Berrange wrote: On Thu, Dec 02, 2010 at 09:44:47PM +0200, Gleb Natapov wrote: On Thu, Dec 02, 2010 at 10:36:59AM -0700, Eric Blake wrote: On 12/02/2010 05:47 AM, Daniel P. Berrange wrote: On Wed, Dec 01, 2010 at 05:12:08PM -0700, Eric Blake wrote: In testing smbios mode='host'/, I noticed a couple of problems. First, qemu rejected the command line we gave it (invalid UUID); ifixingthat showed that all other arguments were being given literal which then made the guest smbios differ from the host. Second, the qemu option -smbios type=1,family=string wasn't supported, which lead to a spurious difference between host and guest. Meanwhile, qemu has a bug where '-smbios type=1,uuid=uuid' must parse as a valid uuid, but is otherwise ignored. The current qemu.git code base _only_ sets smbios uuid from the '-uuid uuid' argument. I've filed a bug against the qemu folks asking whether this is intended (in which case we have to modify libvirt to change the -uuid argument it outputs when smbios is specified), or an oversight (hopefully the latter, since it's still nice to correlate /var/log/libvirt/qemu/log with the XML uuid even when that differs from the smbios uuid presented to the guest.) Hmm, I thought the UUID we were passing in was a host UUID, but on closer inspection the type=1 table is definitely intended to carry the guest UUID. As such it doesn't make sense to populate that with anything other than the 'uuid' from the guest XML. A host UUID should live elsewhere in the SMBIOS tables, likely in the type2 or type3 blocks What does that mean to our XML? Should we reject XML files where both domain/uuid and domain/sysinfo/system/entry/uuid exist, but differ? Both elements are optional, so it's feasible to see a guest uuid in either location. Meanwhile, I'm waiting on resolution to https://bugzilla.redhat.com/show_bug.cgi?id=659122 to see if qemu will be patched to let us stick the host's uuid in block 2 (base board) or block 3 (chassis), in which case, we'll need to expand our XML to support that notion. As I commented on the BZ use OEM strings type 11 smbios table to pass anything you want into a guest. I've also discovered that with current qemu, if both '-uuid uuid' and '-smbios type=1,uuid=uuid' are specified, then -uuid takes precedence; but either one of the two in isolation serves to set smbios block 1 with the guest's uuid. I am surprised that libvirt still uses -uuid. SMBIOS tables are only setup on x86 QEMU binaries, doing fullvirt. On non-x86, or QEMU used for Xen paravirt, the -uuid arg value would be used in other ways unrelated to SMBIOS. Thus replacing -uuid $UUID with -smbiios type=1,uuid=$UUID would be wrong. What non-x86 platforms libvirt supports? With Xen use -uuid or whatever they support. With KVM use only smbios type=1,uuid=$UUID. There is not valid reason that I see to use both. But if you use both of them for some strange reason please make sure you provide the same value for both. libvirt aims to support any and all QEMU target architectures not merely x86. There's no benefit to using -smbios type=1,uuid=$UUID with KVM, over -uuid $UUID. So why do you use them both? Even more interesting question why do you use them both with different $UUID? If you do, do not complain that there should be some kind of order between them. Changing it only for KVM would needlessly complicate the code. Doing things correctly is more important that simple code. And this is not related to KVM at all. You cannot expect qemu-sparc to work with qemu-x86 command line, so if you aim to support any and all qemu targets you should know how to build correct parameter list for any and all of them. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] smbios fixes
On Thu, Dec 02, 2010 at 10:36:59AM -0700, Eric Blake wrote: On 12/02/2010 05:47 AM, Daniel P. Berrange wrote: On Wed, Dec 01, 2010 at 05:12:08PM -0700, Eric Blake wrote: In testing smbios mode='host'/, I noticed a couple of problems. First, qemu rejected the command line we gave it (invalid UUID); ifixingthat showed that all other arguments were being given literal which then made the guest smbios differ from the host. Second, the qemu option -smbios type=1,family=string wasn't supported, which lead to a spurious difference between host and guest. Meanwhile, qemu has a bug where '-smbios type=1,uuid=uuid' must parse as a valid uuid, but is otherwise ignored. The current qemu.git code base _only_ sets smbios uuid from the '-uuid uuid' argument. I've filed a bug against the qemu folks asking whether this is intended (in which case we have to modify libvirt to change the -uuid argument it outputs when smbios is specified), or an oversight (hopefully the latter, since it's still nice to correlate /var/log/libvirt/qemu/log with the XML uuid even when that differs from the smbios uuid presented to the guest.) Hmm, I thought the UUID we were passing in was a host UUID, but on closer inspection the type=1 table is definitely intended to carry the guest UUID. As such it doesn't make sense to populate that with anything other than the 'uuid' from the guest XML. A host UUID should live elsewhere in the SMBIOS tables, likely in the type2 or type3 blocks What does that mean to our XML? Should we reject XML files where both domain/uuid and domain/sysinfo/system/entry/uuid exist, but differ? Both elements are optional, so it's feasible to see a guest uuid in either location. Meanwhile, I'm waiting on resolution to https://bugzilla.redhat.com/show_bug.cgi?id=659122 to see if qemu will be patched to let us stick the host's uuid in block 2 (base board) or block 3 (chassis), in which case, we'll need to expand our XML to support that notion. As I commented on the BZ use OEM strings type 11 smbios table to pass anything you want into a guest. I've also discovered that with current qemu, if both '-uuid uuid' and '-smbios type=1,uuid=uuid' are specified, then -uuid takes precedence; but either one of the two in isolation serves to set smbios block 1 with the guest's uuid. I am surprised that libvirt still uses -uuid. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] smbios fixes
On Thu, Dec 02, 2010 at 07:56:03PM +, Daniel P. Berrange wrote: On Thu, Dec 02, 2010 at 09:44:47PM +0200, Gleb Natapov wrote: On Thu, Dec 02, 2010 at 10:36:59AM -0700, Eric Blake wrote: On 12/02/2010 05:47 AM, Daniel P. Berrange wrote: On Wed, Dec 01, 2010 at 05:12:08PM -0700, Eric Blake wrote: In testing smbios mode='host'/, I noticed a couple of problems. First, qemu rejected the command line we gave it (invalid UUID); ifixingthat showed that all other arguments were being given literal which then made the guest smbios differ from the host. Second, the qemu option -smbios type=1,family=string wasn't supported, which lead to a spurious difference between host and guest. Meanwhile, qemu has a bug where '-smbios type=1,uuid=uuid' must parse as a valid uuid, but is otherwise ignored. The current qemu.git code base _only_ sets smbios uuid from the '-uuid uuid' argument. I've filed a bug against the qemu folks asking whether this is intended (in which case we have to modify libvirt to change the -uuid argument it outputs when smbios is specified), or an oversight (hopefully the latter, since it's still nice to correlate /var/log/libvirt/qemu/log with the XML uuid even when that differs from the smbios uuid presented to the guest.) Hmm, I thought the UUID we were passing in was a host UUID, but on closer inspection the type=1 table is definitely intended to carry the guest UUID. As such it doesn't make sense to populate that with anything other than the 'uuid' from the guest XML. A host UUID should live elsewhere in the SMBIOS tables, likely in the type2 or type3 blocks What does that mean to our XML? Should we reject XML files where both domain/uuid and domain/sysinfo/system/entry/uuid exist, but differ? Both elements are optional, so it's feasible to see a guest uuid in either location. Meanwhile, I'm waiting on resolution to https://bugzilla.redhat.com/show_bug.cgi?id=659122 to see if qemu will be patched to let us stick the host's uuid in block 2 (base board) or block 3 (chassis), in which case, we'll need to expand our XML to support that notion. As I commented on the BZ use OEM strings type 11 smbios table to pass anything you want into a guest. I've also discovered that with current qemu, if both '-uuid uuid' and '-smbios type=1,uuid=uuid' are specified, then -uuid takes precedence; but either one of the two in isolation serves to set smbios block 1 with the guest's uuid. I am surprised that libvirt still uses -uuid. SMBIOS tables are only setup on x86 QEMU binaries, doing fullvirt. On non-x86, or QEMU used for Xen paravirt, the -uuid arg value would be used in other ways unrelated to SMBIOS. Thus replacing -uuid $UUID with -smbiios type=1,uuid=$UUID would be wrong. What non-x86 platforms libvirt supports? With Xen use -uuid or whatever they support. With KVM use only smbios type=1,uuid=$UUID. There is not valid reason that I see to use both. But if you use both of them for some strange reason please make sure you provide the same value for both. -- Gleb. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
