On Tue, Sep 15, 2026 at 13:46:18 +0000, David Vrabel wrote:
> Add speed/width target attributes for the generic PCIe root and switch
> ports to set the link speed/width capability.
>
> This useful when used with passthrough devices to make the in-guest
> apparent bandwidth match the hardware bandwidth. This allows in-guest
> software (such as NCCL) to make better decisions about whether to use
> P2P transfers etc.
>
> Signed-off-by: David Vrabel <[email protected]>
> ---
> docs/formatdomain.rst | 12 ++
> src/conf/domain_conf.c | 22 ++++
> src/conf/domain_conf.h | 3 +
> src/conf/schemas/domaincommon.rng | 10 ++
> src/qemu/qemu_capabilities.c | 114 +++++++++---------
> src/qemu/qemu_capabilities.h | 112 ++++++++---------
> src/qemu/qemu_command.c | 46 +++++++
> src/qemu/qemu_validate.c | 81 +++++++++++++
> .../caps_11.1.0_x86_64.replies | 4 +-
> .../caps_11.1.0_x86_64.xml | 2 +
> ...e-root-port-speed-width.x86_64-latest.args | 37 ++++++
> ...ie-root-port-speed-width.x86_64-latest.xml | 48 ++++++++
> .../pcie-root-port-speed-width.xml | 23 ++++
> ...ream-port-model-generic.x86_64-latest.args | 2 +-
> ...tream-port-model-generic.x86_64-latest.xml | 2 +-
> ...e-switch-downstream-port-model-generic.xml | 2 +-
> ...ream-port-model-generic.x86_64-latest.args | 2 +-
> ...tream-port-model-generic.x86_64-latest.xml | 1 +
> ...cie-switch-upstream-port-model-generic.xml | 1 +
> tests/qemuxmlconftest.c | 1 +
> 20 files changed, 411 insertions(+), 114 deletions(-)
> create mode 100644
> tests/qemuxmlconfdata/pcie-root-port-speed-width.x86_64-latest.args
> create mode 100644
> tests/qemuxmlconfdata/pcie-root-port-speed-width.x86_64-latest.xml
> create mode 100644 tests/qemuxmlconfdata/pcie-root-port-speed-width.xml
[...]
> @@ -9233,6 +9237,20 @@ virDomainControllerDefParseXML(virDomainXMLOption
> *xmlopt,
> def->opts.pciopts.port) < 0)
> return NULL;
>
> + speed = virXMLPropString(targetNodes[0], "speed");
> + if (speed &&
> + (virStrToDouble(speed, NULL, &def->opts.pciopts.speed) < 0 ||
Double? That's a bad idea ...
> + def->opts.pciopts.speed <= 0)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Invalid non-positive value for PCI
> controller link speed"));
> + return NULL;
> + }
[...]
> static struct virQEMUCapsDevicePropsFlags
> virQEMUCapsDevicePropsPCIeRootPort[] = {
> { "hotplug", QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, NULL },
> + { "speed", QEMU_CAPS_PCIE_ROOT_PORT_SPEED, NULL },
> + { "width", QEMU_CAPS_PCIE_ROOT_PORT_WIDTH, NULL },
> };
[...]
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index bcddb9c922..e3e4fe2538 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -182,6 +182,26 @@ qemuOnOffAuto(virTristateSwitch s)
> }
>
>
> +/* Map domain GT/s values to QEMU PCIELinkSpeed enum strings. */
> +static const char *
> +qemuPCIELinkSpeedString(double speed)
> +{
> + if (speed == 2.5)
> + return "2_5";
> + if (speed == 5.0)
> + return "5";
> + if (speed == 8.0)
> + return "8";
> + if (speed == 16.0)
> + return "16";
> + if (speed == 32.0)
> + return "32";
> + if (speed == 64.0)
> + return "64";
Comparing floating point values like this is a bad idea. It may work
here but can break if the mantissa doesn't round properly.
This clearly shows that you need an enum here.
> + return NULL;
> +}
> +
> +
> static int
> qemuBuildObjectCommandlineFromJSON(virCommand *cmd,
> virJSONValue *props)
> @@ -2790,6 +2810,32 @@ qemuBuildControllerPCIDevProps(virDomainControllerDef
> *def,
> return -1;
> }
>
> + /* Options common to all PCIe ports. */
> + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT ||
> + def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT
> ||
> + def->model ==
> VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT) {
> + if (pciopts->speed > 0) {
> + const char *speedstr = qemuPCIELinkSpeedString(pciopts->speed);
> +
> + if (!speedstr) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unexpected PCI controller link speed
> %1$g"),
> + pciopts->speed);
> + return -1;
> + }
> +
> + if (virJSONValueObjectAdd(&props, "S:speed", speedstr, NULL) < 0)
> + return -1;
> + }
> +
> + if (pciopts->width > 0) {
> + g_autofree char *widthstr = g_strdup_printf("%d",
> pciopts->width);
> +
> + if (virJSONValueObjectAdd(&props, "s:width", widthstr, NULL) < 0)
virJSONValueObjectAdd has formatters for numbers. Do not convert it to
string but use the appropriate converter. They can even omit the
argument if it's 0.
> + return -1;
> + }
> + }
> +
> *devprops = g_steal_pointer(&props);
> return 0;
> }
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 16beb3ca6a..562d6117af 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -4762,6 +4762,87 @@ qemuValidateDomainDeviceDefControllerPCI(const
> virDomainControllerDef *cont,
> return -1;
> }
>
> + /* speed and width */
> + switch ((virDomainControllerModelPCI) cont->model) {
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> + if (pciopts->speed != -1 &&
> + (pciopts->speed != 2.5 &&
> + pciopts->speed != 5.0 &&
> + pciopts->speed != 8.0 &&
> + pciopts->speed != 16.0 &&
> + pciopts->speed != 32.0 &&
> + pciopts->speed != 64.0)) {
> + virReportControllerInvalidValue(cont, model, modelName, "speed");
> + return -1;
> + }
> + if (pciopts->speed != -1) {
> + if (pciopts->modelName ==
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM ||
> + pciopts->modelName ==
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("setting the '%1$s' property on a '%2$s'
> device is not supported"),
> + "speed", modelName);
This breaks translatability rules
> + return -1;
> + }
> + if (pciopts->modelName ==
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_SPEED)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("setting the '%1$s' property on a '%2$s'
> device is not supported by this QEMU binary"),
> + "speed", modelName);
> + return -1;
> + }
> + }
> +
> + if (pciopts->width != -1 &&
> + pciopts->width != 1 &&
> + pciopts->width != 2 &&
> + pciopts->width != 4 &&
> + pciopts->width != 8 &&
> + pciopts->width != 16 &&
> + pciopts->width != 32) {
> + virReportControllerInvalidValue(cont, model, modelName, "width");
> + return -1;
> + }
> + if (pciopts->width != -1) {
> + if (pciopts->modelName ==
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM ||
> + pciopts->modelName ==
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("setting the '%1$s' property on a '%2$s'
> device is not supported"),
> + "width", modelName);
> + return -1;
> + }
> + if (pciopts->modelName ==
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_WIDTH)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("setting the '%1$s' property on a '%2$s'
> device is not supported by this QEMU binary"),
> + "width", modelName);
> + return -1;
> + }
> + }
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE:
> + if (pciopts->speed != -1) {
> + virReportControllerInvalidOption(cont, model, modelName,
> "speed");
> + return -1;
> + }
> + if (pciopts->width != -1) {
> + virReportControllerInvalidOption(cont, model, modelName,
> "width");
> + return -1;
> + }
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> + default:
> + virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> + }
> +
> /* hotplug */
> if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
> switch ((virDomainControllerModelPCI) cont->model) {
> diff --git a/tests/qemucapabilitiesdata/caps_11.1.0_x86_64.replies
> b/tests/qemucapabilitiesdata/caps_11.1.0_x86_64.replies
> index bead7636b1..84ddf0e6dd 100644
> --- a/tests/qemucapabilitiesdata/caps_11.1.0_x86_64.replies
> +++ b/tests/qemucapabilitiesdata/caps_11.1.0_x86_64.replies
> @@ -32371,7 +32371,7 @@
> },
> {
> "default-value": "32",
> - "name": "x-width",
> + "name": "width",
> "description": "1/2/4/8/12/16/32",
> "type": "PCIELinkWidth"
> },
> @@ -32387,7 +32387,7 @@
> },
> {
> "default-value": "16",
> - "name": "x-speed",
> + "name": "speed",
NACK. This dump was obtained from a real qemu. You can't change this.
Even in current upstream qemu this is still unstable (x-prefixed).
Until that changes this patch can't be accepted.
> "description": "2_5/5/8/16/32/64",
> "type": "PCIELinkSpeed"
> },
