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
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
> ---
> 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] [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 > >>> --- > >> 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 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 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] [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
> ---
> 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;
>
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 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
> > > > ---
> > > > 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 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
> > > > > > > ---
> > > > > > > Cc: k...@vger.kernel.org
> > > > > > > Cc: Michael S. Tsirkin
> > > > > > > Cc: Gleb Natapov
> > > > > > > Cc: Marcelo Tosatti
> > > > > > >
> > > > > > > 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:
> > >
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 > > > > > --- > > > > > Cc: Gleb Natapov > > > > > Cc: Marcelo Tosatti > > > > > Cc: k...@vger.kernel.org > > > > > Cc: libvir-list@redhat.com > > > > > Cc: Jiri Denemark > > > > > > > > > > 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 &
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
> > > > > ---
> > > > > Cc: k...@vger.kernel.org
> > > > > Cc: Michael S. Tsirkin
> > > > > Cc: Gleb Natapov
> > > > > Cc: Marcelo Tosatti
> > > > >
> > > > > 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: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
> > > ---
> > > Cc: Gleb Natapov
> > > Cc: Marcelo Tosatti
> > > Cc: k...@vger.kernel.org
> > > Cc: libvir-list@redhat.com
> > > Cc: Jiri Denemark
> > >
> > > 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 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
> > > ---
> > > Cc: k...@vger.kernel.org
> > > Cc: Michael S. Tsirkin
> > > Cc: Gleb Natapov
> > > Cc: Marcelo Tosatti
> > >
> > > 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 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
> ---
> Cc: Gleb Natapov
> Cc: Marcelo Tosatti
> Cc: k...@vger.kernel.org
> Cc: libvir-list@redhat.com
> Cc: Jiri Denemark
>
> 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] [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
> ---
> 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] [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
Reviewed-by: Gleb Natapov
> ---
> 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 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
Reviewed-by: Gleb Natapov
> ---
> Cc: Joerg Roedel
> Cc: k...@vger.kernel.org
> Cc: libvir-list@redhat.com
> Cc: Jiri Denemark
>
> 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 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
Reviewed-by: Gleb Natapov
> ---
> 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] [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
Reviewed-by: Gleb Natapov
> ---
> 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 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... [bit ]" 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
> Reviewed-by: Gleb Natapov
> But see the question below.
Never mind. I found the answer in the following patches :)
>
> > ---
> > Cc: Gleb Natapov
> > Cc: Marcelo Tosatti
> > 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
> &
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... [bit ]" 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
Reviewed-by: Gleb Natapov
But see the question below.
> ---
> Cc: Gleb Natapov
> Cc: Marcelo Tosatti
> 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},
> {&
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 Reviewed-by: Gleb Natapov > --- > Changes v2: > - Coding style (indentation) fix > > Cc: Gleb Natapov > Cc: Michael S. Tsirkin > Cc: Marcelo Tosatti > 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 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 Reviewed-by: Gleb Natapov > --- > Changes v2: > - Coding style (indentation) fix > > Cc: Gleb Natapov > Cc: Marcelo Tosatti > Cc: Joerg Roedel > 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 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
> ---
> Cc: k...@vger.kernel.org
> Cc: Michael S. Tsirkin
> Cc: Gleb Natapov
> Cc: Marcelo Tosatti
> Cc: libvir-list@redhat.com
> Cc: Jiri Denemark
>
> 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 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
> ---
> Cc: k...@vger.kernel.org
> Cc: Michael S. Tsirkin
> Cc: Gleb Natapov
> Cc: Marcelo Tosatti
>
> 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] [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 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 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 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 s
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 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 w
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 c
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] [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 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 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 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 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 copy&paste 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: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 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 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 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 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 us
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_US&cmd=displayKC&externalId=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 s
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] 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 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 Fri, Dec 03, 2010 at 03:15:11PM -0700, Eric Blake wrote: > On 12/03/2010 02:50 AM, Gleb Natapov wrote: > >>>>> I am surprised that libvirt still uses -uuid. > > Why? It still works, and it's backwards compatible even to older qemu > where -smbios wasn't supported. It's actually harder to omit -uuid and > use only -smbios, after first guaranteeing that -smbios is supported, > than it is blindly use -uuid. And since -uuid may have more > implications than just the -smbios table, whereas -smbios should not > affect anything except the smbios table, it makes sense to continue to > use -uuid for any of those other implications. > There is not any additional implication using -uuid. In fact -uuid and -smbios table1,uuid= are completely identical from qemu's code point of view since both of them set the same global variable qemu_uuid. Currently there is not problem using both of them (with the same uuid value of course) and I do not think this will change in the near feature, so fixing libvirt to use only -smbios may be not the most urgent bug to fix. -- 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 , 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 '' 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 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 , 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 '' 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
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 , 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 '' 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] Using vmchannel?
On Mon, Jan 19, 2009 at 12:15:14PM +, Richard W.M. Jones wrote: > I was just looking at the new vmchannel feature in QEMU and was > wondering if we could make this available in libvirt. > > Firstly, since there isn't much documentation, I should explain how > vmchannel works: > > [1] You pass an extra parameter on the qemu command line: > > qemu [...] -vmchannel : > > where is the TCP port number (see below) and is > a standard qemu device description. As an example: > > qemu [...] -vmchannel 600:unix:/some/path > > [2] A new character device appears in the host, eg. Unix domain > socket called "/some/path". > > [3] In the host, a userspace program should open a socket, bind(2) > it to /some/path and listen(2) and accept(2) on it. > unix:/some/path is just an example. It is possible to specify any qemu character device there with the same syntax as -serial option. So, for instance, you can specify unix:/some/path,server,nowait and then the host userspace program will need to use connect(2) only and qemu will do the server part. > [4] In the guest, any process may connect(2) to TCP 10.0.2.4:600 > (or whatever port was selected). Each connection in the guest > causes the listener in the host to accept(2). > Only one process can connect(2) to one vmchannel port. This restriction comes from the fact that only one process on the host can connect to qemu character device. > [5] This is only designed for low-bandwidth, low-performance > applications. > > [6] Multiple vmchannels are supported. > > [7] Host cannot initiate a connection. > > My plan to implement this would be to add a new network interface type > to the domain XML: > > > > > > > Only Unix domain socket paths would be allowed on the host side, and > the path would be expected to exist already with suitable permissions. > > Note that I think this would also allow guests to communicate with the > libvirtd on the host (not by default, of course, but if users wanted > to configure it that way), for example: > > > > > > > One problem is that it is qemu/kvm-only. It is built on top of > virtio, and virtio is meant to become a standard driver subsystem for > all full virtualization systems. > virtio is not required to use vmchannel. This is not final, but I hope it stays this way. -- Gleb. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
