Re: [libvirt] [PATCH 1/5] conf: add support for specifying CPU "dies" parameter

On Thu, Dec 19, 2019 at 12:42:04 +0000, Daniel P. Berrangé wrote: > Recently CPU hardware vendors have started to support a new level of > inside the CPU package topology known as a "die". Thus the hierarchy Level of what? Looks like a missing word between "level of" and "inside".

> @@ -1675,10 +1675,10 @@ > <dd>The <code>topology</code> element specifies requested topology of > virtual CPU provided to the guest. Three non-zero values have to be > given for <code>sockets</code>, <code>cores</code>, and > - <code>threads</code>: total number of CPU sockets, number of cores per > - socket, and number of threads per core, respectively. Hypervisors may > - require that the maximum number of vCPUs specified by the > - <code>cpus</code> element equals to the number of vcpus resulting > + <code>threads</code>: total number of CPU sockets, dies per socket, > + number of cores per die, and number of threads per core, respectively. > + Hypervisors may require that the maximum number of vCPUs specified by > + the <code>cpus</code> element equals to the number of vcpus resulting > from the topology.</dd> This needs to be rewritten from scratch, I believe. The current version doesn't work because the first part talks about three attributes, while the rest documents four attributes. And just adding the dies attribute there wouldn't fix it as sockets, cores, and threads are required, but dies is optional.

> <dt><code>feature</code></dt> ... > diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c > index 7490d6bf73..c874c47354 100644 > --- a/src/conf/cpu_conf.c > +++ b/src/conf/cpu_conf.c ... > @@ -535,6 +536,12 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt, > } > def->sockets = (unsigned int) ul; > > + if (virXPathULong("string(./topology[1]/@dies)", ctxt, &ul) < 0) { > + def->dies = 1; > + } else { > + def->dies = (unsigned int) ul; > + } > + I don't think you want to silently ignore dies='-5' or dies='foo' and use 1 instead. You should report an error if dies is specified, but it contains an incorrect value.

> if (virXPathULong("string(./topology[1]/@cores)", ctxt, &ul) < 0) { > virReportError(VIR_ERR_XML_ERROR, "%s", > _("Missing 'cores' attribute in CPU topology")); ... Jirka