Re: [libvirt] [PATCH 1/2] bhyve: add CPU topology support

On Mon, May 28, 2018 at 20:27:50 +0400, Roman Bogorodskiy wrote: > Recently, bhyve started supporting specifying guest CPU topology. > It looks this way: > > bhyve -c cpus=C,sockets=S,cores=C,threads=T ... > > The old behaviour with bhyve -c C, where C is a number of vCPUs, is > still supported. > > So if we have CPU topology in the domain XML, use the new syntax, > otherwise keeps the old behaviour. > > Signed-off-by: Roman Bogorodskiy <bogorodskiy(a)gmail.com&gt; > --- > src/bhyve/bhyve_capabilities.c | 7 +++-- > src/bhyve/bhyve_capabilities.h | 1 + > src/bhyve/bhyve_command.c | 17 +++++++++++- > .../bhyvexml2argv-cputopology.args | 9 +++++++ > .../bhyvexml2argv-cputopology.ldargs | 3 +++ > .../bhyvexml2argv-cputopology.xml | 26 +++++++++++++++++++ > tests/bhyvexml2argvtest.c | 7 ++++- > 7 files changed, 66 insertions(+), 4 deletions(-) > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml [...] > diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c > index e3f7ded7db..b319518520 100644 > --- a/src/bhyve/bhyve_command.c > +++ b/src/bhyve/bhyve_command.c > @@ -467,7 +467,22 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn, > > /* CPUs */ > virCommandAddArg(cmd, "-c"); > - virCommandAddArgFormat(cmd, "%d", virDomainDefGetVcpus(def)); > + if (def->cpu && def->cpu->sockets) { > + if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_CPUTOPOLOGY) != 0) { > + virCommandAddArgFormat(cmd, "cpus=%d,sockets=%d,cores=%d,threads=%d", > + virDomainDefGetVcpus(def), > + def->cpu->sockets, > + def->cpu->cores, > + def->cpu->threads); Note that libvirt XML->def conversion does not validate that def->nvcpus equals to def->cpu->sockets * def->cpu->cores * def->cpu->threads. This is a historic artefact since qemu did not do it either. They started doing it just recently. It might be worth adding that check to be sure in the future.

> + } else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Installed bhyve binary does not support " > + "defining CPU topology")); > + goto error; > + } The rest looks good to me, so ACK if you don't think the check for the topology<->vcpu count is important enough.