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

[Eduardo Habkost]

On Tue, Jul 23, 2013 at 05:19:03PM +0100, Daniel P. Berrange wrote:
> 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.

The specific information libvirt requires here depend on KVM being
initialized, and QEMU code/interfaces currently assume that: 1) there's
only 1 machine being initialized, and it is initialized very early; 2)
there's only one accelerator being initialized, and it is initialized
very early.

I have no idea how long it will take for QEMU to provide a QMP interface
for late/multiple initialization of machines/accelerators. In the
meantime, the way libvirt queries for host CPU capabilities without
taking QEMU and kernel capabilities into account is a serious bug we
need to solve.

Maybe if we are lucky we can find a workaround in the meantime that
won't require running QEMU multiple times, but I am not sure. Maybe it
will be a hack that will be worse than simply running QEMU twice.

-- 
Eduardo