Re: [libvirt] [PATCH 40/41] qemu: Update guest CPU def in live XML
On Tue, Aug 30, 2016 at 17:58:56 -0400, John Ferlan wrote:
...
> @@ -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.
Perhaps it could, although the old code was very fragile and sometimes I
had to postpone some changes to avoid breaking tests. I'm not sure if it
was this case or not, but I wanted to change only the virCPUUpdate APIs
in that patch and keep unnecessary changes in callers for a later patch.
> if (virCPUUpdate(def->os.arch, def->cpu,
caps->host.cpu) < 0)
> goto cleanup;
> }
...
> + 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!
Not knowing the host CPU model is a normal and expected situation (ARM
never sets it and even on x86 we may be unable to detect the host CPU in
some cases) and the code should be able to deal with it by reporting an
error only when we really need the host CPU model (e.g., for host-model
CPUs, minimum match CPUs, optional features, etc.).
> + 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.
Indeed.
> + def->cpu, true) < 0)
> + return -1;
> + }
> +
> + if (virCPUUpdate(def->os.arch, def->cpu,
> + virQEMUCapsGetHostModel(qemuCaps)) < 0)
Same here for update callbacks
Right.
Jirka