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

[Daniel P. Berrangé]

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 :|