Re: [libvirt] [PATCH 1/2] Don't verify CPU features with host-passthrough

On 09/29/2014 10:27 AM, Ján Tomko wrote: > Commit fba6bc4 introduced the non-migratable invtsc feature, > breaking save/migration with host-model and host-passthrough. > > On hosts with this feature present it was automatically included > in the CPU definition, regardless of QEMU support. > > Commit de0aeaf stopped including it by default for host-model, > but failed to fix host-passthrough. > > This commit ignores checking of CPU features with host-passthrough, > since we don't pass them to QEMU (only -cpu host is passed), > allowing domains using host-passthrough that were saved with > the broken version of libvirtd to be restored. > > https://bugzilla.redhat.com/show_bug.cgi?id=1147584 > --- > src/qemu/qemu_migration.c | 22 ++++++++++++---------- > src/qemu/qemu_process.c | 5 +++++ > 2 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 6b38592..284cd5a 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -1714,18 +1714,20 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, virDomainObjPtr vm, > return false; > } > > - for (i = 0; def->cpu && i < def->cpu->nfeatures; i++) { > - virCPUFeatureDefPtr feature = &def->cpu->features[i]; > + if (def->cpu && def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { Only real change (if viewed thru -w) and this looks safe with the extra check. > + for (i = 0; i < def->cpu->nfeatures; i++) { > + virCPUFeatureDefPtr feature = &def->cpu->features[i]; What is interesting from what I read in the formatdomain description for host-passthrough mode: "Thus, if you hit any bugs, you are on your own. Neither model nor feature elements are allowed in this mode." So since this for loop is looping on nfeatures, but we state in the docs for mode that neither model or feature is allowed - one wonders why they'd be added to the def->cpu->features[] if using passthrough? The question being, are we fixing the root cause or the side effect of allowing/having features? Almost seems as though if features wasn't populated or "free'd" and nfeatures set to 0 when we find host-passthrough, then we would hit this (or some other yet to be found issue).

Although perhaps for *this* release this is safer. > > - if (feature->policy != VIR_CPU_FEATURE_REQUIRE) > - continue; > + if (feature->policy != VIR_CPU_FEATURE_REQUIRE) > + continue; > > - /* QEMU blocks migration and save with invariant TSC enabled */ > - if (STREQ(feature->name, "invtsc")) { > - virReportError(VIR_ERR_OPERATION_INVALID, > - _("domain has CPU feature: %s"), > - feature->name); > - return false; > + /* QEMU blocks migration and save with invariant TSC enabled */ > + if (STREQ(feature->name, "invtsc")) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("domain has CPU feature: %s"), > + feature->name); > + return false; > + } > } > } > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 1b8931e..ca78aac 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3787,6 +3787,11 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, > bool ret = false; > size_t i; > > + /* no features are passed to QEMU with -cpu host > + * so it makes no sense to verify them */ > + if (def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) Ran this through my Coverity test... A few lines down there's a: for (i = 0; def->cpu && i < def->cpu->nfeatures; i++) { because the def->cpu is not checked here, there's a REVERSE_INULL issue

ACK, although I'm not that familiar with all the cpu fields and expectations, but this does seem reasonable otherwise and since it fixes a bug noted in/for rc1 it probably should be pushed. Your call (or perhaps with Jirka) regarding the note above... John > + return true; > + > switch (arch) { > case VIR_ARCH_I686: > case VIR_ARCH_X86_64: > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list