On Wed, Jul 17, 2019 at 10:03:25 -0400, Collin Walling wrote:
> This command is hooked into the virsh hypervisor-cpu-baseline command.
> As such, the CPU models provided in the XML sent to the command will be
> baselined with the CPU contained in the QEMU capabilities file for the
> appropriate QEMU binary (for s390x, this CPU definition can be observed
> via virsh domcapabilities).
This is not how the baseline API works. If a user wants the current host
CPU model to be taken in the baseline computation, they need to provide
it in the array of input CPUs by themselves.
>
> Signed-off-by: Collin Walling <walling@xxxxxxxxxxxxx>
> Reviewed-by: Daniel Henrique Barboza <danielh413@xxxxxxxxx>
> ---
> src/qemu/qemu_capabilities.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_capabilities.h | 7 ++++
> src/qemu/qemu_driver.c | 27 ++++++++++++++
> 3 files changed, 118 insertions(+)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index b898113..ab337fa 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -5574,3 +5574,87 @@ virQEMUCapsStripMachineAliases(virQEMUCapsPtr qemuCaps)
> for (i = 0; i < qemuCaps->nmachineTypes; i++)
> VIR_FREE(qemuCaps->machineTypes[i].alias);
> }
> +
> +
> +static int
> +virQEMUCapsStealCPUModelFromInfo(virCPUDefPtr dst,
> + qemuMonitorCPUModelInfoPtr *src)
> +{
> + qemuMonitorCPUModelInfoPtr info = *src;
> + size_t i;
> +
> + virCPUDefFreeModel(dst);
> +
> + VIR_STEAL_PTR(dst->model, info->name);
> +
> + for (i = 0; i < info->nprops; i++) {
> + char *name = info->props[i].name;
> + int policy = VIR_CPU_FEATURE_REQUIRE;
> +
> + if (info->props[i].type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN &&
> + !info->props[i].value.boolean)
> + policy = VIR_CPU_FEATURE_DISABLE;
> +
> + if (virCPUDefAddFeature(dst, name, policy) < 0)
> + goto error;
> + }
> +
> + qemuMonitorCPUModelInfoFree(info);
> + *src = NULL;
> + return 0;
> +
> + error:
> + virCPUDefFree(dst);
> + return -1;
> +}
> +
> +
> +virCPUDefPtr
> +virQEMUCapsCPUModelBaseline(virQEMUCapsPtr qemuCaps,
> + const char *libDir,
> + uid_t runUid,
> + gid_t runGid,
> + int ncpus,
> + virCPUDefPtr *cpus)
This function does not belong to qemu_capabilities.c, it should be in
qemu_driver.c and with a different prefix, of course. And this applies
to the helper above too.
> +{
> + qemuMonitorCPUModelInfoPtr result = NULL;
> + qemuProcessQMPPtr proc = NULL;
> + virCPUDefPtr cpu = NULL;
> + virCPUDefPtr baseline = NULL;
> + size_t i;
> +
> + if (VIR_ALLOC(cpu) < 0)
> + goto cleanup;
> +
> + if (virCPUDefCopyModel(cpu, cpus[0], false))
> + goto cleanup;
> +
> + if (!(proc = qemuProcessQMPNew(qemuCaps->binary, libDir,
> + runUid, runGid, false)))
> + goto cleanup;
> +
> + if (qemuProcessQMPStart(proc) < 0)
> + goto cleanup;
> +
> + for (i = 1; i < ncpus; i++) {
> + if (qemuMonitorGetCPUModelBaseline(proc->mon, cpu->model,
> + cpu->nfeatures, cpu->features,
> + cpus[i]->model, cpus[i]->nfeatures,
> + cpus[i]->features, &result) < 0)
See my comment to 2/8. Wouldn't this be much better as
if (qemuMonitorGetCPUModelBaseline(proc->mon, cpu, cpus[i],
&result) < 0)
> + goto cleanup;
> +
> + if (virQEMUCapsStealCPUModelFromInfo(cpu, &result) < 0)
> + goto cleanup;
> + }
> +
> + VIR_STEAL_PTR(baseline, cpu);
> +
> + cleanup:
> + if (!baseline)
> + virQEMUCapsLogProbeFailure(qemuCaps->binary);
There's no probing here. It's just an implementation of the API invoked
by libvirt client. No need to log anything here.
> +
> + qemuMonitorCPUModelInfoFree(result);
As Boris already noticed, result cannot be non-NULL here. In fact, you
could even make the result variable local to the for loop.
> + qemuProcessQMPFree(proc);
> + virCPUDefFree(cpu);
> + return baseline;
> +}
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 8dde759..b49c0b9 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -665,3 +665,10 @@ virQEMUCapsGetSEVCapabilities(virQEMUCapsPtr qemuCaps);
>
> virArch virQEMUCapsArchFromString(const char *arch);
> const char *virQEMUCapsArchToString(virArch arch);
> +
> +virCPUDefPtr virQEMUCapsCPUModelBaseline(virQEMUCapsPtr qemuCaps,
> + const char *libDir,
> + uid_t runUid,
> + gid_t runGid,
> + int ncpus,
> + virCPUDefPtr *cpus);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d3a144a..37b9c75 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13553,6 +13553,7 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
> unsigned int flags)
> {
> virQEMUDriverPtr driver = conn->privateData;
> + virQEMUDriverConfigPtr config = driver->config;
You should use
VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
instead.
> virCPUDefPtr *cpus = NULL;
> virQEMUCapsPtr qemuCaps = NULL;
> virArch arch;
> @@ -13607,6 +13608,32 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
> if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels,
> (const char **)features, migratable)))
> goto cleanup;
> + } else if (ARCH_IS_S390(arch) &&
> + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE)) {
> + /* Add a copy of the hypervisor CPU to the list */
> + virCPUDefPtr hvCPU, tmp;
> + size_t allocated = ncpus + 1;
> +
> + hvCPU = virQEMUCapsGetHostModel(qemuCaps, virttype,
> + VIR_QEMU_CAPS_HOST_CPU_REPORTED);
> + if (VIR_ALLOC(tmp) < 0)
> + goto cleanup;
> +
> + if (virCPUDefCopyModel(tmp, hvCPU, false))
> + goto cleanup;
> +
> + if (VIR_INSERT_ELEMENT(cpus, ncpus, allocated, tmp) < 0)
> + goto cleanup;
> +
> + ncpus++;
All the code in this if statement down to here should go away.
> +
> + if (!(cpu = virQEMUCapsCPUModelBaseline(qemuCaps, config->libDir,
> + config->user, config->group,
> + ncpus, cpus)))
> + goto cleanup;
> +
> + if (!(flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES))
> + virCPUDefFreeFeatures(cpu);
This seems to be wrong.
> } else {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("computing baseline hypervisor CPU is not supported "
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
