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