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

[Ján Tomko]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140930/6683150e/attachment-0001.sig>