On 09/30/2014 01:09 AM, John Ferlan wrote:

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).

The features aren't really allowed. We just fill the cpu def with host-passthrough with what we think QEMU will do to inform the user, but the features aren't passed to QEMU. So this would be the side effect. This patch fixes the already saved files where we added invtsc even though QEMU didn't and I think the second one should be enough to fix it for new saves.

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

Thanks for catching that. Jan

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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 09/29/2014 10:27 AM, Ján Tomko wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1147584

Could this be expanded a bit - so one doesn't have to chase into the bz in order to understand what is/was being fixed?

--- src/cpu/cpu_x86.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a98a847..57f343c 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2068,7 +2068,8 @@ x86UpdateCustom(virCPUDefPtr guest,

static int x86UpdateHostModel(virCPUDefPtr guest, - const virCPUDef *host) + const virCPUDef *host, + bool passthrough) { virCPUDefPtr oldguest = NULL; const struct x86_map *map; @@ -2076,8 +2077,6 @@ x86UpdateHostModel(virCPUDefPtr guest, size_t i; int ret = -1;

- guest->match = VIR_CPU_MATCH_EXACT; - if (!(map = virCPUx86GetMap())) goto cleanup;

@@ -2100,8 +2099,7 @@ x86UpdateHostModel(virCPUDefPtr guest, } } }

So if I understand correctly the above loop removes not migrate-able features - from the passed in migrated cpu and the loop below is updating features for anything left. So given my question from patch 1/2 - it seems if features weren't allowed for host-passthrough, well then neither of these would be necessary. In any case - even if we added something that removed features, the incoming, saved/restore, etc. guest with host-passthrough could still have it so this would be necessary - which I guess means it's an ACK (typing and thinking). John

- - for (i = 0; i < oldguest->nfeatures; i++) { + for (i = 0; !passthrough && i < oldguest->nfeatures; i++) { if (virCPUDefUpdateFeature(guest, oldguest->features[i].name, oldguest->features[i].policy) < 0) @@ -2125,12 +2123,12 @@ x86Update(virCPUDefPtr guest, return x86UpdateCustom(guest, host);

case VIR_CPU_MODE_HOST_MODEL: - return x86UpdateHostModel(guest, host); + guest->match = VIR_CPU_MATCH_EXACT; + return x86UpdateHostModel(guest, host, false);

case VIR_CPU_MODE_HOST_PASSTHROUGH: guest->match = VIR_CPU_MATCH_MINIMUM; - virCPUDefFreeModel(guest); - return virCPUDefCopyModel(guest, host, true); + return x86UpdateHostModel(guest, host, true);

case VIR_CPU_MODE_LAST: break;