Re: [PATCH 1/8] qemu: Add support for unavailable-features

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 02, 2016 at 10:22:30AM +0100, Jiri Denemark wrote:
> QEMU 2.8.0 adds support for unavailable-features in
> query-cpu-definitions reply. The unavailable-features array lists CPU
> features which prevent a corresponding CPU model from being usable on
> current host. It can only be used when all the unavailable features are
> disabled. Empty array means the CPU model can be used without
> modifications.
> 
> We can use unavailable-features for providing CPU model usability info
> in domain capabilities XML:
> 
>     <domainCapabilities>
>       ...
>       <cpu>
>         <mode name='host-passthrough' supported='yes'/>
>         <mode name='host-model' supported='yes'>
>           <model fallback='allow'>Skylake-Client</model>
>           ...
>         </mode>
>         <mode name='custom' supported='yes'>
>           <model usable='yes'>qemu64</model>
>           <model usable='yes'>qemu32</model>
>           <model usable='no'>phenom</model>
>           <model usable='yes'>pentium3</model>
>           <model usable='yes'>pentium2</model>
>           <model usable='yes'>pentium</model>
>           <model usable='yes'>n270</model>
>           <model usable='yes'>kvm64</model>
>           <model usable='yes'>kvm32</model>
>           <model usable='yes'>coreduo</model>
>           <model usable='yes'>core2duo</model>
>           <model usable='no'>athlon</model>
>           <model usable='yes'>Westmere</model>
>           <model usable='yes'>Skylake-Client</model>
>           ...
>         </mode>
>       </cpu>
>       ...
>     </domainCapabilities>
> 
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
>  src/qemu/qemu_capabilities.c | 53 ++++++++++++++++++++++++++++++++++----------
>  src/qemu/qemu_capabilities.h |  6 +++--
>  src/qemu/qemu_monitor.h      |  1 +
>  src/qemu/qemu_monitor_json.c | 27 ++++++++++++++++------
>  src/qemu/qemu_process.c      |  2 +-
>  tests/qemumonitorjsontest.c  | 25 +++++++++++++++++----
>  tests/qemuxml2argvtest.c     | 12 ++++++----
>  7 files changed, 96 insertions(+), 30 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 7a8202a..9fce7a6 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2306,7 +2306,8 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps)
>  int
>  virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
>                               const char **name,
> -                             size_t count)
> +                             size_t count,
> +                             virDomainCapsCPUUsable usable)
>  {
>      size_t i;
>  
> @@ -2316,7 +2317,7 @@ virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
>  
>      for (i = 0; i < count; i++) {
>          if (virDomainCapsCPUModelsAdd(qemuCaps->cpuDefinitions, name[i], -1,
> -                                      VIR_DOMCAPS_CPU_USABLE_UNKNOWN) < 0)
> +                                      usable) < 0)
>              return -1;
>      }
>  
> @@ -2327,10 +2328,12 @@ virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
>  int
>  virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
>                               char ***names,
> -                             size_t *count)
> +                             size_t *count,
> +                             bool usable)
>  {
>      size_t i;
>      char **models = NULL;
> +    size_t n = 0;
>  
>      *count = 0;
>      if (names)
> @@ -2344,17 +2347,21 @@ virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
>  
>      for (i = 0; i < qemuCaps->cpuDefinitions->nmodels; i++) {
>          virDomainCapsCPUModelPtr cpu = qemuCaps->cpuDefinitions->models + i;
> -        if (models && VIR_STRDUP(models[i], cpu->name) < 0)
> -            goto error;
> +        if (!usable ||
> +            cpu->usable == VIR_DOMCAPS_CPU_USABLE_YES) {
> +            if (models && VIR_STRDUP(models[n], cpu->name) < 0)
> +                goto error;
> +            n++;
> +        }

This function is called only on one place and the argument *usable* is always
false and it's not used in any of the following patches in this series so there
is no need to modify this function at all.

>      }
>  
>      if (names)
>          *names = models;
> -    *count = qemuCaps->cpuDefinitions->nmodels;
> +    *count = n;
>      return 0;
>  
>   error:
> -    virStringFreeListCount(models, i);
> +    virStringFreeListCount(models, n);
>      return -1;
>  }
>  
> @@ -2713,9 +2720,15 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps,
>          goto cleanup;
>  
>      for (i = 0; i < ncpus; i++) {
> +        virDomainCapsCPUUsable usable = VIR_DOMCAPS_CPU_USABLE_UNKNOWN;
> +
> +        if (cpus[i]->usable == VIR_TRISTATE_BOOL_YES)
> +            usable = VIR_DOMCAPS_CPU_USABLE_YES;
> +        else if (cpus[i]->usable == VIR_TRISTATE_BOOL_NO)
> +            usable = VIR_DOMCAPS_CPU_USABLE_NO;
> +
>          if (virDomainCapsCPUModelsAddSteal(qemuCaps->cpuDefinitions,
> -                                           &cpus[i]->name,
> -                                           VIR_DOMCAPS_CPU_USABLE_UNKNOWN) < 0)
> +                                           &cpus[i]->name, usable) < 0)
>              goto cleanup;
>      }
>  
> @@ -3128,6 +3141,17 @@ virQEMUCapsLoadCache(virCapsPtr caps,
>              goto cleanup;
>  
>          for (i = 0; i < n; i++) {
> +            int usable = VIR_DOMCAPS_CPU_USABLE_UNKNOWN;
> +
> +            if ((str = virXMLPropString(nodes[i], "usable")) &&
> +                (usable = virDomainCapsCPUUsableTypeFromString(str)) < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("unknown value '%s' in attribute 'usable'"),
> +                               str);
> +                goto cleanup;
> +            }
> +            VIR_FREE(str);
> +
>              if (!(str = virXMLPropString(nodes[i], "name"))) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                 _("missing cpu name in QEMU capabilities cache"));
> @@ -3135,8 +3159,7 @@ virQEMUCapsLoadCache(virCapsPtr caps,
>              }
>  
>              if (virDomainCapsCPUModelsAddSteal(qemuCaps->cpuDefinitions,
> -                                               &str,
> -                                               VIR_DOMCAPS_CPU_USABLE_UNKNOWN) < 0)
> +                                               &str, usable) < 0)
>                  goto cleanup;
>          }
>      }
> @@ -3301,7 +3324,13 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps,
>      if (qemuCaps->cpuDefinitions) {
>          for (i = 0; i < qemuCaps->cpuDefinitions->nmodels; i++) {
>              virDomainCapsCPUModelPtr cpu = qemuCaps->cpuDefinitions->models + i;
> -            virBufferEscapeString(&buf, "<cpu name='%s'/>\n", cpu->name);
> +            virBufferEscapeString(&buf, "<cpu name='%s'", cpu->name);
> +            if (cpu->usable) {
> +                const char *usable;
> +                usable = virDomainCapsCPUUsableTypeToString(cpu->usable);
> +                virBufferAsprintf(&buf, " usable='%s'", usable);
> +            }
> +            virBufferAddLit(&buf, "/>\n");
>          }
>      }
>  
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 6e7a855..c77ba13 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -428,10 +428,12 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps);
>  unsigned int virQEMUCapsGetKVMVersion(virQEMUCapsPtr qemuCaps);
>  int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
>                                   const char **name,
> -                                 size_t count);
> +                                 size_t count,
> +                                 virDomainCapsCPUUsable usable);
>  int virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
>                                   char ***names,
> -                                 size_t *count);
> +                                 size_t *count,
> +                                 bool usable);
>  virCPUDefPtr virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps);
>  bool virQEMUCapsIsCPUModeSupported(virQEMUCapsPtr qemuCaps,
>                                     virCapsPtr caps,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index c3133c4..f81de68 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -913,6 +913,7 @@ typedef struct _qemuMonitorCPUDefInfo qemuMonitorCPUDefInfo;
>  typedef qemuMonitorCPUDefInfo *qemuMonitorCPUDefInfoPtr;
>  
>  struct _qemuMonitorCPUDefInfo {
> +    virTristateBool usable;
>      char *name;
>  };
>  
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 6c13832..e2dfb34 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4925,13 +4925,8 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>      if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>          goto cleanup;
>  
> -    if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("query-cpu-definitions reply was missing return data"));
> -        goto cleanup;
> -    }
> -
> -    if ((n = virJSONValueArraySize(data)) < 0) {
> +    if (!(data = virJSONValueObjectGetArray(reply, "return")) ||
> +        (n = virJSONValueArraySize(data)) < 0) {

This change is unrelated, it would be probably better to put in into separate
patch.

>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("query-cpu-definitions reply data was not an array"));
>          goto cleanup;
> @@ -4958,6 +4953,24 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>  
>          if (VIR_STRDUP(cpu->name, tmp) < 0)
>              goto cleanup;
> +
> +        if (virJSONValueObjectHasKey(child, "unavailable-features")) {
> +            virJSONValuePtr blockers;
> +
> +            blockers = virJSONValueObjectGetArray(child,
> +                                                  "unavailable-features");
> +            if (!blockers) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("unavailable-features in query-cpu-definitions "
> +                                 "reply data was not an array"));
> +                goto cleanup;
> +            }
> +
> +            if (virJSONValueArraySize(blockers) > 0)
> +                cpu->usable = VIR_TRISTATE_BOOL_NO;
> +            else
> +                cpu->usable = VIR_TRISTATE_BOOL_YES;
> +        }
>      }
>  
>      ret = n;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1b67aee..50fb3de 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5057,7 +5057,7 @@ qemuProcessUpdateGuestCPU(virDomainDefPtr def,
>                       virQEMUCapsGetHostModel(qemuCaps)) < 0)
>          goto cleanup;
>  
> -    if (virQEMUCapsGetCPUDefinitions(qemuCaps, &models, &nmodels) < 0 ||
> +    if (virQEMUCapsGetCPUDefinitions(qemuCaps, &models, &nmodels, false) < 0 ||
>          virCPUTranslate(def->os.arch, def->cpu, models, nmodels) < 0)
>          goto cleanup;
>  
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index d8fd958..7111bc8 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -432,10 +432,12 @@ testQemuMonitorJSONGetCPUDefinitions(const void *data)
>                                 "     \"name\": \"qemu64\" "
>                                 "   }, "
>                                 "   { "
> -                               "     \"name\": \"Opteron_G4\" "
> +                               "     \"name\": \"Opteron_G4\", "
> +                               "     \"unavailable-features\": [\"vme\"]"
>                                 "   }, "
>                                 "   { "
> -                               "     \"name\": \"Westmere\" "
> +                               "     \"name\": \"Westmere\", "
> +                               "     \"unavailable-features\": []"
>                                 "   } "
>                                 "  ]"
>                                 "}") < 0)
> @@ -452,6 +454,13 @@ testQemuMonitorJSONGetCPUDefinitions(const void *data)
>      }
>  
>  #define CHECK(i, wantname)                                              \
> +    CHECK_FULL(i, wantname, VIR_TRISTATE_BOOL_ABSENT)
> +
> +#define CHECK_USABLE(i, wantname, usable)                               \
> +    CHECK_FULL(i, wantname,                                             \
> +               usable ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO)
> +

The ordering of macros is strange.  The above macros uses CHECK_FULL so I would
rather move them after the CHECK_FULL macro.

ACK with the issues fixed.

Pavel

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]