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

[John Ferlan]

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:
>