On Wed, Jan 08, 2020 at 03:34:34PM +0100, Jiri Denemark wrote:
> 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".
Really it was just saying 'a new level inside the CPU'.
> > @@ -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.
Sigh, yes. Next time i'll actually read it.
> > <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.
This is a bit of a long standing design flaw in our virXPathULong
usage in many places in fact.
>
> > if (virXPathULong("string(./topology[1]/@cores)", ctxt, &ul) < 0) {
> > virReportError(VIR_ERR_XML_ERROR, "%s",
> > _("Missing 'cores' attribute in CPU topology"));
> ...
>
> Jirka
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
