Re: [libvirt] [PATCH 6/7] qemu: Probe QEMU binary for host CPU

On Tue, Jul 23, 2013 at 06:11:35PM +0200, Jiri Denemark wrote: > Since QEMU and kvm may filter some host CPU features or add efficiently > emulated features, asking QEMU binary for host CPU data provides > better results when we later use the data for building guest CPUs. > --- > src/qemu/qemu_capabilities.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > src/qemu/qemu_capabilities.h | 2 ++ > 2 files changed, 45 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 9440396..d46a059 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -253,6 +253,7 @@ struct _virQEMUCaps { > > size_t ncpuDefinitions; > char **cpuDefinitions; > + virCPUDefPtr hostCPU; > > size_t nmachineTypes; > char **machineTypes; > @@ -1757,6 +1758,9 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) > goto error; > } > > + if (!(ret->hostCPU = virCPUDefCopy(qemuCaps->hostCPU))) > + goto error; > + > if (VIR_ALLOC_N(ret->machineTypes, qemuCaps->nmachineTypes) < 0) > goto error; > if (VIR_ALLOC_N(ret->machineAliases, qemuCaps->nmachineTypes) < 0) > @@ -1796,6 +1800,7 @@ void virQEMUCapsDispose(void *obj) > VIR_FREE(qemuCaps->cpuDefinitions[i]); > } > VIR_FREE(qemuCaps->cpuDefinitions); > + virCPUDefFree(qemuCaps->hostCPU); > > virBitmapFree(qemuCaps->flags); > > @@ -2485,7 +2490,6 @@ virQEMUCapsInitQMPCommandNew(const char *binary, > "-no-user-config", > "-nodefaults", > "-nographic", > - "-M", "none", > "-qmp", monitor, > "-pidfile", pidfile, > "-daemonize", > @@ -2617,6 +2621,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, > > cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, monarg, pidfile, > runUid, runGid); > + virCommandAddArgList(cmd, "-M", "none", NULL); > > if ((ret = virQEMUCapsInitQMPCommandRun(cmd, qemuCaps->binary, pidfile, > &config, &mon, &pid)) < 0) { > @@ -2679,6 +2684,37 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, > if (virQEMUCapsProbeQMPCommandLine(qemuCaps, mon) < 0) > goto cleanup; > > + if ((qemuCaps->arch == VIR_ARCH_I686 || > + qemuCaps->arch == VIR_ARCH_X86_64) && > + (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) || > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_ENABLE_KVM)) && > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_HOST) && > + qemuCaps->nmachineTypes) { > + virQEMUCapsInitQMPCommandAbort(&cmd, &mon, &pid, pidfile); > + > + VIR_DEBUG("Checking host CPU data provided by %s", qemuCaps->binary); > + cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, monarg, pidfile, > + runUid, runGid); > + virCommandAddArgList(cmd, "-cpu", "host", NULL); > + /* -cpu host gives the same CPU for all machine types so we just > + * use the first one when probing > + */ > + virCommandAddArg(cmd, "-machine"); > + virCommandAddArgFormat(cmd, "%s,accel=kvm", > + qemuCaps->machineTypes[0]); > + > + if (virQEMUCapsInitQMPCommandRun(cmd, qemuCaps->binary, pidfile, > + &config, &mon, &pid) < 0) > + goto cleanup; > + > + qemuCaps->hostCPU = qemuMonitorGetCPU(mon, qemuCaps->arch); > + if (qemuCaps->hostCPU) { > + char *cpu = virCPUDefFormat(qemuCaps->hostCPU, 0); > + VIR_DEBUG("Host CPU reported by %s: %s", qemuCaps->binary, cpu); > + VIR_FREE(cpu); > + } > + } This code is causing us to invoke the QEMU binary multiple times, which is something we worked really hard to get away from. I really, really don't like this as an approach. QEMU needs to be able to give us the data we need here without multiple invocations. eg, by allowing the monitor command to specify 'kvm' vs 'qemu' when asking for data, so you can interrogate it without having to re-launch it with different accel=XXX args.