[libvirt] [PATCH 40/41] qemu: Update guest CPU def in live XML

[John Ferlan]

On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> Storing the updated CPU definition in the live domain definition saves
> us from having to update it over and over when we need it. Not to
> mention that we will soon further update the CPU definition according to
> QEMU once it's started.
> 
> A highly wanted side effect of this patch, libvirt will pass all CPU
> features explicitly specified in domain XML to QEMU, even those that are
> already included in the host model.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/qemu/qemu_command.c                            | 161 +++++++------------
>  src/qemu/qemu_domain.c                             |  10 +-
>  src/qemu/qemu_process.c                            | 178 +++++++++------------
>  .../qemuxml2argv-cpu-Haswell3.args                 |   2 +-
>  .../qemuxml2argv-cpu-exact2-nofallback.args        |   3 +-
>  .../qemuxml2argvdata/qemuxml2argv-cpu-exact2.args  |   3 +-
>  .../qemuxml2argv-cpu-fallback.args                 |   2 +-
>  .../qemuxml2argv-cpu-host-model-cmt.args           |   2 +-
>  .../qemuxml2argv-cpu-host-model-fallback.args      |   2 +-
>  .../qemuxml2argv-cpu-minimum2.args                 |   2 +-
>  .../qemuxml2argvdata/qemuxml2argv-cpu-strict1.args |   3 +-
>  11 files changed, 147 insertions(+), 221 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 985b628..ebeeebe 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6432,62 +6432,18 @@ static int
>  qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
>                          const virDomainDef *def,
>                          virBufferPtr buf,
> -                        virQEMUCapsPtr qemuCaps,
> -                        bool *hasHwVirt,
> -                        bool migrating)
> +                        virQEMUCapsPtr qemuCaps)

This is a nice reduction!  So much more understandable.

>  {

[...]

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index decbdd0..0b052ce 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3267,14 +3267,8 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
>      /* Update guest CPU requirements according to host CPU */
>      if ((flags & VIR_DOMAIN_XML_UPDATE_CPU) &&
>          def->cpu &&
> -        (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) {
> -        if (!caps->host.cpu ||
> -            !caps->host.cpu->model) {
> -            virReportError(VIR_ERR_OPERATION_FAILED,
> -                           "%s", _("cannot get host CPU capabilities"));
> -            goto cleanup;
> -        }
> -
> +        (def->cpu->mode != VIR_CPU_MODE_CUSTOM ||
> +         def->cpu->model)) {

It would seem this could be related to an earlier patch - that is the
cpuUpdate patch...  It can stay here, but it just feels like it could move.

>          if (virCPUUpdate(def->os.arch, def->cpu, caps->host.cpu) < 0)
>              goto cleanup;
>      }
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d4269db..943704f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4397,107 +4397,6 @@ qemuProcessStartValidateXML(virQEMUDriverPtr driver,
>  }
>  

[...]

>  
> +static int
> +qemuProcessUpdateGuestCPU(virDomainDefPtr def,
> +                          virQEMUCapsPtr qemuCaps,
> +                          virCapsPtr caps,
> +                          unsigned int flags)
> +{
> +    int ret = -1;
> +    size_t nmodels = 0;
> +    char **models = NULL;
> +
> +    if (!def->cpu)
> +        return 0;
> +
> +    /* nothing to do if only topology part of CPU def is used */
> +    if (def->cpu->mode == VIR_CPU_MODE_CUSTOM && !def->cpu->model)
> +        return 0;
> +
> +    /* Old libvirt added host CPU model to host-model CPUs for migrations,
> +     * while new libvirt just turns host-model into custom mode. We need
> +     * to fix the mode to maintain backward compatibility and to avoid
> +     * the CPU model to be replaced in virCPUUpdate.
> +     */
> +    if (!(flags & VIR_QEMU_PROCESS_START_NEW) &&
> +        ARCH_IS_X86(def->os.arch) &&
> +        def->cpu->mode == VIR_CPU_MODE_HOST_MODEL &&
> +        def->cpu->model) {
> +        def->cpu->mode = VIR_CPU_MODE_CUSTOM;
> +    }
> +
> +    if (!virQEMUCapsIsCPUModeSupported(qemuCaps, caps, def->virtType,
> +                                       def->cpu->mode)) {

So this will fail for mode == VIR_CPU_MODE_HOST_MODEL if ->cpuModel
wasn't filled in, but it's just not clear enough to me (after 40
patches) whether the following checks could return NULL for the
GetHostModel. IIRC none of the failures are cores, just reported errors,
which is good and I supposed would be expected by this point in time!

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("CPU mode '%s' for %s %s domain on %s host is not "
> +                         "supported by hypervisor"),
> +                       virCPUModeTypeToString(def->cpu->mode),
> +                       virArchToString(def->os.arch),
> +                       virDomainVirtTypeToString(def->virtType),
> +                       virArchToString(caps->host.arch));
> +        return -1;
> +    }
> +
> +    /* nothing to update for host-passthrough */
> +    if (def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH)
> +        return 0;
> +
> +    /* custom CPUs in TCG mode don't need to be compared to host CPU */
> +    if (def->virtType != VIR_DOMAIN_VIRT_QEMU ||
> +        def->cpu->mode != VIR_CPU_MODE_CUSTOM) {
> +        if (virCPUCompare(caps->host.arch, virQEMUCapsGetHostModel(qemuCaps),
                                              ^^^^^^^^^^^^^^^^^^^^^^^
This could legally be NULL and thus cause failures in the various
compare function callbacks.

> +                          def->cpu, true) < 0)
> +            return -1;
> +    }
> +
> +    if (virCPUUpdate(def->os.arch, def->cpu,
> +                     virQEMUCapsGetHostModel(qemuCaps)) < 0)

Same here for update callbacks

> +        goto cleanup;
> +
> +    if (virQEMUCapsGetCPUDefinitions(qemuCaps, &models, &nmodels) < 0 ||
> +        virCPUTranslate(def->os.arch, def->cpu, models, nmodels) < 0)
> +        goto cleanup;
> +
> +    def->cpu->fallback = VIR_CPU_FALLBACK_FORBID;
> +    ret = 0;
> +
> + cleanup:
> +    virStringFreeListCount(models, nmodels);
> +    return ret;
> +}
> +
> +

[...]