[PATCH 1/1] set vm physical bits(phys_bits)

[Michal Privoznik]

On 3/25/21 2:24 AM, Paul Schlacter wrote:
> Set the vm phys_bits through the phys and hostphysbits in XML
> 
> <phys bits='43' /> corresponds to "-cpu-phys-bits=42"
> 
> <hostphysbits /> corresponds to "host-phys-bits=on"
> 
> 
> <cpu mode='host-passthrough' check='none'>
> 
> <phys bits='43' />
> 
> <hostphysbits />
> 
> </cpu>
> 
> ---
> 
> docs/schemas/cputypes.rng | 20 ++++++++++++++++++++
> 
> src/conf/cpu_conf.c | 34 ++++++++++++++++++++++++++++++++++
> 
> src/conf/cpu_conf.h |2 ++
> 
> src/qemu/qemu_command.c |6 ++++++
> 
> 4 files changed, 62 insertions(+)

I'm sorry, but it appears that your MTA is broken, because this patch 
doesn't apply on the top of master. Either git-send-email or git-publish 
are known to work well.

https://github.com/stefanha/git-publish

Also, if this is a v2 it's perfectly okay to start a new thread and 
state that it is a v2 of $url. For instance like this:

https://listman.redhat.com/archives/libvir-list/2021-March/msg00607.html

Anyway, what's still missing is documentation - how should users know 
that: a) it is possible to change this, b) what version this was 
introduced in? For instance like this:

https://listman.redhat.com/archives/libvir-list/2021-March/msg00930.html

Also, a test case would be nice. For both qemuxml2argvtest and 
qemuxml2xmltest - so that we know that XML parser/formatter works and 
cmd line generator works too. As a bonus, once added to git 
virschematest will check if XML matches schema.

> 
> 
> diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
> 
> index 77c8fa783b..fb8a14ddea 100644
> 
> --- a/docs/schemas/cputypes.rng
> 
> +++ b/docs/schemas/cputypes.rng
> 
> @@ -300,6 +300,20 @@
> 
> </element>
> 
> </define>
> 
> +<define name="cpuPhysBits">
> 
> +<element name="phys">
> 
> +<attribute name="bits">
> 
> +<ref name="positiveInteger"/>
> 
> +</attribute>
> 
> +</element>
> 
> +</define>
> 
> +
> 
> +<define name="cpuHostPhysBits">
> 
> +<element name="hostphysbits">
> 
> +<empty/>
> 
> +</element>
> 
> +</define>
> 
> +
> 
> <define name="hostcpu">
> 
> <element name="cpu">
> 
> <element name="arch">
> 
> @@ -414,6 +428,12 @@
> 
> <optional>
> 
> <ref name="cpuCache"/>
> 
> </optional>
> 
> +<optional>
> 
> +<ref name="cpuPhysBits"/>
> 
> +</optional>
> 
> +<optional>
> 
> +<ref name="cpuHostPhysBits"/>
> 
> +</optional>
> 
> </interleave>
> 
> </element>
> 
> </define>
> 
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> 
> index 380a74691d..24b0fa67ef 100644
> 
> --- a/src/conf/cpu_conf.c
> 
> +++ b/src/conf/cpu_conf.c
> 
> @@ -158,6 +158,8 @@ virCPUDefCopyModelFilter(virCPUDefPtr dst,
> 
> dst->model = g_strdup(src->model);
> 
> dst->vendor = g_strdup(src->vendor);
> 
> dst->vendor_id = g_strdup(src->vendor_id);
> 
> +dst->phys_bits = src->phys_bits;
> 
> +dst->host_phys_bits = src->host_phys_bits;
> 
> dst->microcodeVersion = src->microcodeVersion;
> 
> dst->nfeatures_max = src->nfeatures;
> 
> dst->nfeatures = 0;
> 

Doesn't virCPUDefStealModel() need the same treatment?

> @@ -540,6 +542,18 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
> 
> return -1;
> 
> }
> 
> +if (virXPathNode("./phys[1]", ctxt)) {
> 
> +unsigned long phys_bits;
> 
> +if (virXPathUInt("string(./phys[1]/@bits)",
> 
> +ctxt, &phys_bits) >=0 ) {
> 
> +def->phys_bits = (unsigned int) phys_bits;
> 
> +}
> 
> +}
> 
> +
> 
> +if (virXPathNode("./hostphysbits[1]", ctxt)) {
> 
> +def->host_phys_bits = true;
> 
> +}
> 
> +
> 
> if (virXPathNode("./topology[1]", ctxt)) {
> 
> unsigned long ul;
> 
> @@ -811,6 +825,12 @@ virCPUDefFormatBuf(virBufferPtr buf,
> 
> virBufferAddLit(buf, "/>\n");
> 
> }
> 
> +if (def->phys_bits > 0)
> 
> +virBufferAsprintf(buf, "<phys bits='%u' />\n", def->phys_bits);
> 
> +
> 
> +if (def->host_phys_bits)
> 
> +virBufferAddLit(buf, "<hostphysbits />\n");
> 
> +
> 
> if (def->sockets && def->dies && def->cores && def->threads) {
> 
> virBufferAddLit(buf, "<topology");
> 
> virBufferAsprintf(buf, " sockets='%u'", def->sockets);
> 
> @@ -1067,6 +1087,20 @@ virCPUDefIsEqual(virCPUDefPtr src,
> 
> return false;
> 
> }
> 
> +if (src->phys_bits != dst->phys_bits) {
> 
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> 
> + _("Target CPU phys_bits %d does not match source %d"),
> 
> + dst->phys_bits, src->phys_bits);
> 
> +goto cleanup;
> 
> +}
> 
> +
> 
> +if (src->host_phys_bits != dst->host_phys_bits) {
> 
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> 
> + _("Target CPU host_phys_bits %d does not match source %d"),
> 
> + dst->host_phys_bits, src->host_phys_bits);
> 
> +goto cleanup;
> 
> +}
> 
> +
> 
> if (src->sockets != dst->sockets) {
> 
> MISMATCH(_("Target CPU sockets %d does not match source %d"),
> 
> dst->sockets, src->sockets);
> 
> diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
> 
> index 7ab198d370..f2a23ad41e 100644
> 
> --- a/src/conf/cpu_conf.h
> 
> +++ b/src/conf/cpu_conf.h
> 
> @@ -132,6 +132,8 @@ struct _virCPUDef {
> 
> char *vendor_id;/* vendor id returned by CPUID in the guest */
> 
> int fallback; /* enum virCPUFallback */
> 
> char *vendor;
> 
> +unsigned int phys_bits;
> 
> +bool host_phys_bits;
> 
> unsigned int microcodeVersion;
> 
> unsigned int sockets;
> 
> unsigned int dies;
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> 
> index 1b4fa77867..d9bf3d5ce8 100644
> 
> --- a/src/qemu/qemu_command.c
> 
> +++ b/src/qemu/qemu_command.c
> 
> @@ -6729,6 +6729,12 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
> 
> virBufferAddLit(&buf, ",l3-cache=off");
> 
> }
> 
> +if (def->cpu && def->cpu->phys_bits > 0)
> 
> +virBufferAsprintf(&buf, ",phys-bits=%u", def->cpu->phys_bits);
> 
> +
> 
> +if (def->cpu && def->cpu->host_phys_bits)
> 
> +virBufferAddLit(&buf, ",host-phys-bits=on");


It's not that simple. phys-bits property was introduced to QEMU in
v2.7.0-rc0~7^2~26 and host-phys-bits a few commits later in 
v2.7.0-rc0~7^2~21. The minimal version of QEMU we currently support is 
1.5.0 (see QEMU_MIN_{MAJOR,MINOR,MICRO} in qemu_capabilities.c). 
Therefore, it is possible that we're constructing a command line for 
QEMU that doesn't have the properties. The way we detect this is so 
called QEMU capabilities. It is a bitmap where each bit has a name and 
is flipped on or off depending whether QEMU supports corresponding 
feature. And I think we will need one and produce an error message if 
QEMU doesn't have the property.

I think we can safely assume that either QEMU has both properties or 
none. I don't think there is a downstream maintainer that would backport 
only one of the properties.

>
> +
> 
> cpu = virBufferContentAndReset(&cpu_buf);
> 
> cpu_flags = virBufferContentAndReset(&buf);
> 
> --
> 

Looking forward to v3.

Michal