On Tue, Aug 19, 2025 at 18:22:18 +0200, Andrea Bolognani via Devel wrote:
> This centralizes the knowledge about which network interface
> models are PCI devices and thus need to have a PCI address
> allocated by libvirt, and expands said knowledge to include
> the fact that models such as spapr-vlan and smc91c111 are
> not PCI devices.
>
> Signed-off-by: Andrea Bolognani <abolo...@redhat.com>
> ---
> src/qemu/qemu_domain_address.c | 62 +++++++++++++++++++++++++++++++---
> 1 file changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 96a9ca9b14..07d6366b1b 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -499,6 +499,58 @@
> qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCaps *qemuCaps,
> }
>
>
> +static bool
> +qemuDomainNetIsPCI(const virDomainNetDef *net)
> +{
> + switch ((virDomainNetModelType)net->model) {
> + case VIR_DOMAIN_NET_MODEL_USB_NET:
> + case VIR_DOMAIN_NET_MODEL_SPAPR_VLAN:
> + case VIR_DOMAIN_NET_MODEL_LAN9118:
> + case VIR_DOMAIN_NET_MODEL_SMC91C111:
> + /* The model above are not PCI devices */
> + return false;
This returns false and no error ...
> +
> + case VIR_DOMAIN_NET_MODEL_RTL8139:
> + case VIR_DOMAIN_NET_MODEL_VIRTIO:
> + case VIR_DOMAIN_NET_MODEL_E1000:
> + case VIR_DOMAIN_NET_MODEL_E1000E:
> + case VIR_DOMAIN_NET_MODEL_IGB:
> + case VIR_DOMAIN_NET_MODEL_VIRTIO_TRANSITIONAL:
> + case VIR_DOMAIN_NET_MODEL_VIRTIO_NON_TRANSITIONAL:
> + case VIR_DOMAIN_NET_MODEL_VMXNET3:
> + /* The models above are PCI devices */
> + return true;
> +
> + case VIR_DOMAIN_NET_MODEL_NETFRONT:
> + case VIR_DOMAIN_NET_MODEL_VMXNET:
> + case VIR_DOMAIN_NET_MODEL_VMXNET2:
> + case VIR_DOMAIN_NET_MODEL_VLANCE:
> + case VIR_DOMAIN_NET_MODEL_AM79C970A:
> + case VIR_DOMAIN_NET_MODEL_AM79C973:
> + case VIR_DOMAIN_NET_MODEL_82540EM:
> + case VIR_DOMAIN_NET_MODEL_82545EM:
> + case VIR_DOMAIN_NET_MODEL_82543GC:
> + case VIR_DOMAIN_NET_MODEL_UNKNOWN:
> + /* The models above are probably not PCI devices, and in fact
> + * some of them are not even relevant to the QEMU driver, but
> + * historically we've defaulted to considering all network
> + * interfaces to be PCI so we preserve that behavior here */
> + return true;
> +
> + case VIR_DOMAIN_NET_MODEL_LAST:
> + default:
> + virReportEnumRangeError(virDomainNetModelType, net->model);
> + return false;
And this returns false and raises an error. The caller can't know
whether there is an error to raise or not.
> + }
> +
> + /* Due to historical reasons, model names for network interfaces
> + * are not validated as strictly as other devices. When in doubt,
> + * assume that network interfaces are PCI devices, as that has
> + * the highest chance of working correctly */
> + return true;
Also this is dead code based on the fact that you have 'default' above.
> +}
> +
> +
> /**
> * qemuDomainDeviceCalculatePCIConnectFlags:
> *
> @@ -669,10 +721,11 @@
> qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
> * address is assigned when we're assigning the
> * addresses for other hostdev devices.
> */
> - if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
> - net->model == VIR_DOMAIN_NET_MODEL_USB_NET) {
> + if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)
> + return 0;
> +
> + if (!qemuDomainNetIsPCI(net))
> return 0;
> - }
>
> if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO ||
> net->model == VIR_DOMAIN_NET_MODEL_VIRTIO_NON_TRANSITIONAL)
> @@ -2110,9 +2163,8 @@ qemuDomainAssignDevicePCISlots(virDomainDef *def,
> for (i = 0; i < def->nnets; i++) {
> virDomainNetDef *net = def->nets[i];
>
> - if (net->model == VIR_DOMAIN_NET_MODEL_USB_NET) {
> + if (!qemuDomainNetIsPCI(net))
> continue;
> - }
>
> /* type='hostdev' network devices might be USB, and are also
> * in hostdevs list anyway, so handle them with other hostdevs
This part looks reasonable but as you can see callers ignore the error
raised by virReportEnumRangeError.
I suggest you remove the error reported and also merge the 'default'
case with the return at the end of the function with the comment
explaining why you return true.
Reviewed-by: Peter Krempa <pkre...@redhat.com>
