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

[Jiri Denemark]

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".

> is now:
> 
>   sockets > dies > cores > threads
> 
> This adds support for "dies" in the XML parser, with the value
> defaulting to 1 if not specified for backwards compatibility.
> 
> For example a system with 64 logical CPUs might report
> 
>    <topology sockets="4" dies="2" cores="4" threads="2"/>
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
...
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index dd04a05f09..1433ff7043 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
...
> @@ -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