[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 12/13] Implement CPU selection in QEMU driver



On Wed, Dec 16, 2009 at 12:04:09AM +0100, Jiri Denemark wrote:
> 
> Signed-off-by: Jiri Denemark <jdenemar redhat com>
> ---
>  src/qemu/qemu_conf.c   |  401 +++++++++++++++++++++++++++++++++++++++++++++---
>  src/qemu/qemu_conf.h   |    7 +
>  src/qemu/qemu_driver.c |   43 ++++--
>  3 files changed, 421 insertions(+), 30 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> +
> +int
> +qemudProbeCPUModels(const char *qemu,
> +                    const char *arch,
> +                    unsigned int *count,
> +                    const char ***cpus)
> +{
> +    const char *const qemuarg[] = { qemu, "-cpu", "?", NULL };
> +    const char *const qemuenv[] = { "LC_ALL=C", NULL };
> +    enum { MAX_MACHINES_OUTPUT_SIZE = 1024*4 };
> +    char *output = NULL;
> +    int newstdout = -1;
> +    int ret = -1;
> +    pid_t child;
> +    int status;
> +    int len;
> +    qemudParseCPUModels parse;
> +
> +    if (count)
> +        *count = 0;
> +    if (cpus)
> +        *cpus = NULL;
> +
> +    if (STREQ(arch, "i686") || STREQ(arch, "x86_64"))
> +        parse = qemudParseX86Models;
> +    else {
> +        VIR_DEBUG(_("don't know how to parse %s CPU models"), arch);
> +        return 0;
> +    }
> +
> +    if (virExec(NULL, qemuarg, qemuenv, NULL,
> +                &child, -1, &newstdout, NULL, VIR_EXEC_CLEAR_CAPS) < 0)
> +        return -1;
> +
> +    len = virFileReadLimFD(newstdout, MAX_MACHINES_OUTPUT_SIZE, &output);
> +    if (len < 0) {
> +        virReportSystemError(NULL, errno, "%s",
> +                             _("Unable to read QEMU supported CPU models"));
> +        goto cleanup;
> +    }
> +
> +    if (parse(output, count, cpus) < 0) {

Can you put this in a nice namespace,  qemuParseCPUs or something
along those lines.

> @@ -808,6 +960,49 @@ qemudCapsInitGuest(virCapsPtr caps,
>      return 0;
>  }
>  
> +
> +static int
> +qemudCapsInitCPU(virCapsPtr caps,
> +                 const char *arch)
> +{
> +    virCPUDefPtr cpu = NULL;
> +    union cpuData *data = NULL;
> +    virNodeInfo nodeinfo;
> +    int ret = -1;
> +
> +    if (VIR_ALLOC(cpu) < 0
> +        || !(cpu->arch = strdup(arch))) {
> +        virReportOOMError(NULL);
> +        goto error;
> +    }
> +
> +    if (nodeGetInfo(NULL, &nodeinfo))
> +        goto error;
> +
> +    cpu->type = VIR_CPU_TYPE_HOST;
> +    cpu->sockets = nodeinfo.sockets * nodeinfo.nodes;

I'm not sure this is the right thing todo here, since it means
this data for 'sockets' differs from that shown by the nodeinfo
command. If an app needs the total number of sockets, they can
already calculate that either from the nodeinfo API, or from
the NUMA topology data in the cpabilities, so we don't need to
do it here too. Just make it match nodeinfo.sockets.

> +    cpu->cores = nodeinfo.cores;
> +    cpu->threads = nodeinfo.threads;
> +
> +    if (!(data = cpuNodeData(NULL, arch))
> +        || cpuDecode(NULL, cpu, data, 0, NULL) < 0)
> +        goto error;
> +
> +    caps->host.cpu = cpu;
> +
> +    ret = 0;
> +
> +cleanup:
> +    cpuDataFree(NULL, arch, data);
> +
> +    return ret;
> +
> +error:
> +    virCPUDefFree(cpu);
> +    goto cleanup;
> +}
> +
> +
>  virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
>      struct utsname utsname;
>      virCapsPtr caps;
> @@ -832,8 +1027,17 @@ virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
>          VIR_WARN0("Failed to query host NUMA topology, disabling NUMA capabilities");
>      }
>  
> +    if (old_caps == NULL || old_caps->host.cpu == NULL) {
> +        if (qemudCapsInitCPU(caps, utsname.machine) < 0)
> +            VIR_WARN0("Failed to get host CPU");
> +    }
> +    else {
> +        caps->host.cpu = old_caps->host.cpu;
> +        old_caps->host.cpu = NULL;
> +    }
> +
>      virCapabilitiesAddHostMigrateTransport(caps,
> -                                           "tcp");
> +                                           "Tcp");

Why this change ? The migrate transport is the uri prefix, so this is
changing the semantics.

>  
>      /* First the pure HVM guests */
>      for (i = 0 ; i < ARRAY_CARDINALITY(arch_info_hvm) ; i++)
> @@ -1563,6 +1767,99 @@ static void qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev,
>      }
>  }
>  
> +
> +static int
> +qemudBuildCommandLineCPU(virConnectPtr conn,
> +                         const struct qemud_driver *driver,
> +                         const virDomainDefPtr def,
> +                         const char *emulator,
> +                         const struct utsname *ut,
> +                         char **opt)
> +{
> +    const virCPUDefPtr host = driver->caps->host.cpu;
> +    virCPUDefPtr guest = NULL;
> +    unsigned int ncpus;
> +    const char **cpus = NULL;
> +    union cpuData *data = NULL;
> +    int ret = -1;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int i;
> +
> +    if (qemudProbeCPUModels(emulator, ut->machine, &ncpus, &cpus) < 0)
> +        goto cleanup;
> +
> +    if (ncpus > 0 && host && def->cpu && def->cpu->model) {
> +        virCPUCompareResult cmp;
> +
> +        cmp = cpuGuestData(conn, host, def->cpu, &data);
> +        switch (cmp) {
> +        case VIR_CPU_COMPARE_INCOMPATIBLE:
> +            qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                    "%s", _("guest CPU is not compatible with host CPU"));
> +            /* fall through */
> +        case VIR_CPU_COMPARE_ERROR:
> +            goto cleanup;
> +
> +        default:
> +            break;
> +        }
> +
> +        if (VIR_ALLOC(guest) < 0 || !(guest->arch = strdup(ut->machine)))
> +            goto no_memory;
> +
> +        if (qemudProbeCPUModels(emulator, ut->machine, &ncpus, &cpus) < 0
> +            || cpuDecode(conn, guest, data, ncpus, cpus) < 0)
> +            goto cleanup;

Can't we avoid calling Probe again here, since we alrady did this 20
lines earlier. This second time will leak memory  from the first time.

> +
> +        virBufferVSprintf(&buf, "%s", guest->model);
> +        for (i = 0; i < guest->nfeatures; i++)
> +            virBufferVSprintf(&buf, ",+%s", guest->features[i].name);
> +    }
> +    else {
> +        /*
> +         * Need to force a 32-bit guest CPU type if
> +         *
> +         *  1. guest OS is i686
> +         *  2. host OS is x86_64
> +         *  3. emulator is qemu-kvm or kvm
> +         *
> +         * Or
> +         *
> +         *  1. guest OS is i686
> +         *  2. emulator is qemu-system-x86_64
> +         */
> +        if (STREQ(def->os.arch, "i686") &&
> +            ((STREQ(ut->machine, "x86_64") &&
> +              strstr(emulator, "kvm")) ||
> +             strstr(emulator, "x86_64")))
> +            virBufferAddLit(&buf, "qemu32");
> +    }


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]