Skip to content

LIBVIRTAT-20967: Add multiple VFIO interfaces in vIOMMU test #6590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

LIBVIRTAT-20967: Add multiple VFIO interfaces in vIOMMU test #6590

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bdce0d5
LIBVIRTAT-20967: Add support for multiple VFIO interfaces in vIOMMU t…
hholoubk Oct 2, 2025
213aba1
LIBVIRTAT-20967: Fix hostdev interface handling and improve IOMMU ver…
hholoubk Oct 2, 2025
7f94c7e
Fix multiple VFIO interface handling and console session management
hholoubk Oct 2, 2025
8c2b566
Fix multiple VFIO interface handling with correct API usage
hholoubk Oct 2, 2025
19b1adb
better logging
hholoubk Oct 2, 2025
31b0139
debug logging
hholoubk Oct 3, 2025
869b90c
use busNr as ref
hholoubk Oct 3, 2025
c771b29
resolve issue
hholoubk Oct 3, 2025
a5be1ec
Refactor pre_controller lookup logic in viommu_base.py
hholoubk Oct 6, 2025
23db7a5
partial
hholoubk Oct 7, 2025
75e1f85
partial
hholoubk Oct 8, 2025
ecf63c8
Update vIOMMU device addressing logic
hholoubk Oct 8, 2025
9e1b307
update viommu base
hholoubk Oct 8, 2025
5f587be
update viommu base
hholoubk Oct 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion libvirt/tests/cfg/sriov/vIOMMU/iommu_device_settings.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
only q35
iface_model = 'rtl8139'
iface_dict = {'type_name': 'network', 'model': '${iface_model}', 'source': {'network': 'default'}}
- virtio_muti_devices:
- virtio_multi_devices:
disk_dict = {'target': {'dev': 'vda', 'bus': 'virtio'}, 'device': 'disk', 'driver': ${disk_driver}}
video_dict = {'primary': 'yes', 'model_heads': '1', 'model_type': 'virtio', 'driver': {'iommu': 'on'}}
test_devices = ["Eth", "block", "gpu"]
Expand Down Expand Up @@ -83,3 +83,33 @@
downstream_port = {'type': 'pci', 'model': 'pcie-switch-downstream-port', 'pre_controller': 'pcie-switch-upstream-port'}
root_port = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
controller_dicts = [{'type': 'pci', 'model': 'pcie-expander-bus', 'target': {'busNr': '250'}, 'pre_controller': 'pcie-root'}, ${root_port}, ${upstream_port}, ${upstream_port}, ${downstream_port}, ${downstream_port}]
- multiple_vfio_pcie_root_port:
only virtio
need_sriov = yes
ping_dest = ''
test_devices = ["Eth"]
# Multiple VFIO interfaces under pcie-root-port
# pcie-root <-- pcie-root-port <--- disk with iommu=on
# <-- pcie-root-port <- vfio interface
# <-- pcie-root-port <- vfio interface
controller_dicts = [{'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-root'}, {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-root'}, {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-root'}]
disk_dict = {'target': {'dev': 'vda', 'bus': 'virtio'}, 'device': 'disk', 'driver': ${disk_driver}}
iface_dict = [{'managed': 'yes', 'type_name': 'hostdev', 'hostdev_address': {'type_name': 'pci', 'attrs': vf_pci_addr}}, {'managed': 'yes', 'type_name': 'hostdev', 'hostdev_address': {'type_name': 'pci', 'attrs': vf_pci_addr2}}]
- multiple_vfio_pcie_expander_bus:
only virtio
need_sriov = yes
ping_dest = ''
test_devices = ["Eth"]
# Multiple VFIO interfaces with pcie-expander-bus
# pcie-root <--- pcie-expander-bus <-- pcie-root-port <--- virtio interface with iommu=on
# I |<----------------- pcie-root-port <--- vfio interface
# | |<------------------pcie-root-port <--- vfio interface
# | <----------pcie-root-port < ---virtio disk with iommu=on
expander_bus = {'type': 'pci', 'model': 'pcie-expander-bus', 'target': {'busNr': '251'}, 'pre_controller': 'pcie-root'}
root_port1 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
root_port2 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
root_port3 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
controller_dicts = [${expander_bus}, ${root_port1}, ${root_port2}, ${root_port3}]
disk_dict = {'target': {'dev': 'vda', 'bus': 'virtio'}, 'device': 'disk', 'driver': ${disk_driver}}
# Mixed interfaces: one virtio network with iommu=on and two VFIO interfaces
iface_dict = [{'type_name': 'network', 'model': 'virtio', 'driver': {'driver_attr': {'iommu': 'on'}}, 'source': {'network': 'default'}}, {'managed': 'yes', 'type_name': 'hostdev', 'hostdev_address': {'type_name': 'pci', 'attrs': vf_pci_addr}}, {'managed': 'yes', 'type_name': 'hostdev', 'hostdev_address': {'type_name': 'pci', 'attrs': vf_pci_addr2}}]
24 changes: 21 additions & 3 deletions libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from virttest.libvirt_xml import vm_xml
from virttest.utils_libvirt import libvirt_vmxml
from virttest.utils_test import libvirt

from provider.sriov import sriov_base
from provider.viommu import viommu_base
Expand Down Expand Up @@ -39,6 +40,7 @@ def run(test, params, env):
vm_session = vm.wait_for_serial_login(
timeout=int(params.get('login_timeout')))
pre_devices = viommu_base.get_devices_pci(vm_session, test_devices)
vm_session.close()
vm.destroy()

for dev in ["disk", "video"]:
Expand All @@ -58,12 +60,24 @@ def run(test, params, env):
iface_dict = test_obj.parse_iface_dict()

if cleanup_ifaces:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", iface_dict)
# Handle both single dict and list of dicts
if isinstance(iface_dict, list):
libvirt_vmxml.remove_vm_devices_by_type(vm, 'interface')
for single_iface_dict in iface_dict:
dev_obj = libvirt_vmxml.create_vm_device_by_type("interface", single_iface_dict)
test.log.debug(f"XML of interface device is:\n{dev_obj}")
libvirt.add_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
dev_obj)
else:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", iface_dict)

test.log.info("TEST_STEP: Start the VM.")
vm.start()
vm.cleanup_serial_console()
vm.create_serial_console()
vm_session = vm.wait_for_serial_login(
timeout=int(params.get('login_timeout')))
test.log.debug(vm_xml.VMXML.new_from_dumpxml(vm.name))
Expand All @@ -87,4 +101,8 @@ def run(test, params, env):
if s:
test.fail("Failed to ping %s! status: %s, output: %s." % (ping_dest, s, o))
finally:
if 'vm_session' in locals():
test.log.debug("Closing vm_session")
vm_session.close()
vm.cleanup_serial_console()
test_obj.teardown_iommu_test()
69 changes: 56 additions & 13 deletions provider/viommu/viommu_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,35 @@ def __init__(self, vm, test, params, session=None):
libvirt_version.is_libvirt_feature_supported(self.params)
new_xml = vm_xml.VMXML.new_from_inactive_dumpxml(self.vm.name)
self.orig_config_xml = new_xml.copy()
self.test.log.debug("Original VM configuration backed up for restore")

def parse_iface_dict(self):
"""
Parse iface_dict from params
Parse iface_dict from params, supporting both single dict and list of dicts

:return: The updated iface_dict
:return: The updated iface_dict or list of iface_dicts based on type of input
"""
# generate mac address, if it is needed in iface_dict
mac_addr = utils_net.generate_mac_address_simple()
iface_dict = eval(self.params.get("iface_dict", "{}"))

if self.controller_dicts and iface_dict:
if isinstance(iface_dict, list):
result = []
for single_iface_dict in iface_dict:
self._update_iface_dict_with_controller_info(single_iface_dict)
result.append(single_iface_dict)
return result
else:
self._update_iface_dict_with_controller_info(iface_dict)
return iface_dict
Comment on lines +123 to +131
Copy link

@coderabbitai coderabbitai bot Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add input validation and error handling.

The code assumes iface_dict is either a list or a dict but doesn't validate this. If parsing yields an unexpected type (e.g., string, int, None with truthy check bypassed), subsequent operations may fail with unclear errors.

Consider adding validation:

     iface_dict = literal_eval(self.params.get("iface_dict", "{}"))
+    
+    if not isinstance(iface_dict, (dict, list)):
+        raise ValueError(f"iface_dict must be dict or list, got {type(iface_dict).__name__}")

     if isinstance(iface_dict, list):
+        if not all(isinstance(item, dict) for item in iface_dict):
+            raise ValueError("All items in iface_dict list must be dicts")
         result = []
         for single_iface_dict in iface_dict:
             self._update_iface_dict_with_controller_info(single_iface_dict)
             result.append(single_iface_dict)
         return result
     else:
         self._update_iface_dict_with_controller_info(iface_dict)
         return iface_dict
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if isinstance(iface_dict, list):
result = []
for single_iface_dict in iface_dict:
self._update_iface_dict_with_controller_info(single_iface_dict)
result.append(single_iface_dict)
return result
else:
self._update_iface_dict_with_controller_info(iface_dict)
return iface_dict
iface_dict = literal_eval(self.params.get("iface_dict", "{}"))
if not isinstance(iface_dict, (dict, list)):
raise ValueError(f"iface_dict must be dict or list, got {type(iface_dict).__name__}")
if isinstance(iface_dict, list):
if not all(isinstance(item, dict) for item in iface_dict):
raise ValueError("All items in iface_dict list must be dicts")
result = []
for single_iface_dict in iface_dict:
self._update_iface_dict_with_controller_info(single_iface_dict)
result.append(single_iface_dict)
return result
else:
self._update_iface_dict_with_controller_info(iface_dict)
return iface_dict


def _update_iface_dict_with_controller_info(self, iface_dict):
"""
Update interface dictionary with controller addressing information

:param iface_dict: Interface dictionary to update with controller info
"""
if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev':
Copy link

@coderabbitai coderabbitai bot Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix conditional check for empty dict.

The condition iface_dict will evaluate to False for an empty dict {}, potentially skipping valid updates. If an empty dict is intentionally meant to skip processing, this should be explicit.

Apply this diff to be explicit:

-    if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev':
+    if self.controller_dicts and iface_dict is not None and iface_dict.get('type_name') != 'hostdev':

Or, if empty dicts should skip processing:

-    if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev':
+    if self.controller_dicts and iface_dict and len(iface_dict) > 0 and iface_dict.get('type_name') != 'hostdev':
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev':
if self.controller_dicts and iface_dict is not None and iface_dict.get('type_name') != 'hostdev':
🤖 Prompt for AI Agents 
provider/viommu/viommu_base.py around line 139: the current conditional relies
on the truthiness of iface_dict so an empty dict will be treated as false and
unintentionally skipped; replace the truthiness check with an explicit test—if
empty dicts should be processed, change the condition to explicitly check that
iface_dict is not None before checking its type_name; if empty dicts should be
skipped, make that explicit by testing that iface_dict is not an empty dict (or
explicitly checking for existence of type_name) before comparing to 'hostdev'.

iface_bus = "%0#4x" % int(self.controller_dicts[-1].get("index"))
iface_attrs = {"bus": iface_bus}
if self.controller_dicts[-1].get("model") == "pcie-to-pci-bridge":
Expand All @@ -129,7 +147,6 @@ def parse_iface_dict(self):

iface_dict.update({"address": {"attrs": iface_attrs}})
self.test.log.debug("iface_dict: %s.", iface_dict)
return iface_dict

def prepare_controller(self):
"""
Expand All @@ -143,17 +160,30 @@ def prepare_controller(self):
contr_dict = self.controller_dicts[i]
pre_controller = contr_dict.get("pre_controller")
if pre_controller:
pre_contrs = list(
filter(None, [c.get("index") for c in self.controller_dicts
if c["type"] == contr_dict["type"] and
c["model"] == pre_controller]))
if pre_contrs:
pre_idx = pre_contrs[0]
self.test.log.debug(f"Processing controller {i}: {contr_dict}")
self.test.log.debug(f"Looking for pre_controller: {pre_controller}")
# Get parent controller index and busNr
parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr"))
for c in self.controller_dicts
if c["type"] == contr_dict["type"] and c["model"] == pre_controller]
Copy link

@coderabbitai coderabbitai bot Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add safe access for controller attributes.

The list comprehension accesses c["type"] and c["model"] which could raise KeyError if these keys are missing from a controller dict. Use .get() for safer access.

Apply this diff to add safe access:

-                parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr")) 
-                               for c in self.controller_dicts
-                               if c["type"] == contr_dict["type"] and c["model"] == pre_controller]
+                parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr")) 
+                               for c in self.controller_dicts
+                               if c.get("type") == contr_dict.get("type") and c.get("model") == pre_controller]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr"))
for c in self.controller_dicts
if c["type"] == contr_dict["type"] and c["model"] == pre_controller]
parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr"))
for c in self.controller_dicts
if c.get("type") == contr_dict.get("type") and c.get("model") == pre_controller]
🤖 Prompt for AI Agents 
In provider/viommu/viommu_base.py around lines 166 to 168, the list
comprehension uses c["type"] and c["model"] which can raise KeyError; change
those to c.get("type") and c.get("model") so missing keys return None and the
comparison is safe, preserving the existing c.get("index") and c.get("target",
{}).get("busNr") accesses; ensure the logic still filters controllers by
comparing c.get("type") == contr_dict.get("type") and c.get("model") ==
pre_controller (or contr_dict.get("model") if that was intended).


if parent_contrs:
parent_idx, parent_busnr = parent_contrs[0]
# Use target busNr if it exists, otherwise use assigned index
if parent_busnr:
pre_idx = int(parent_busnr)
self.test.log.debug(f"Using parent target busNr {pre_idx} for {pre_controller}")
else:
pre_idx = parent_idx
self.test.log.debug(f"Using parent index {pre_idx} for {pre_controller}")
else:
# Fall back to existing logic if parent not found
pre_idx = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], pre_controller)[-1]
self.test.log.debug(f"Using fallback index {pre_idx} for {pre_controller}")
pre_contr_bus = "%0#4x" % int(pre_idx)
self.test.log.debug(f"Parent controller index: {pre_idx}, bus: {pre_contr_bus}")
contr_attrs = {"bus": pre_contr_bus}
contr_slot = libvirt_pcicontr.get_free_pci_slot(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
Expand All @@ -178,12 +208,24 @@ def prepare_controller(self):

contr_dict.update({"address": {"attrs": contr_attrs}})
contr_dict.pop("pre_controller")
self.test.log.debug(f"Final controller config: {contr_dict}")
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(self.vm.name), "controller",
contr_dict, 100)
contr_dict["index"] = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], contr_dict["model"])[-1]
# Only assign index if it wasn't already assigned
if "index" not in contr_dict:
# Check if target busNr is specified and use it as index
if contr_dict.get("target", {}).get("busNr"):
target_bus = int(contr_dict["target"]["busNr"])
contr_dict["index"] = target_bus
self.test.log.debug(f"Using target busNr {target_bus} as index for controller {i}: {contr_dict['model']}")
else:
contr_dict["index"] = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], contr_dict["model"])[-1]
self.test.log.debug(f"Assigned index {contr_dict['index']} to controller {i}: {contr_dict['model']}")
else:
self.test.log.debug(f"Index already assigned {contr_dict['index']} for controller {i}: {contr_dict['model']}")
Comment on lines 282 to 289
Copy link

@coderabbitai coderabbitai bot Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix truthy check that incorrectly rejects busNr=0.

Line 199 uses a truthy check on busNr, which evaluates to False when busNr is 0. Since 0 is a valid bus number, this check incorrectly skips the target busNr logic and falls through to the else branch.

Apply this diff to check explicitly for None:

-            if contr_dict.get("target", {}).get("busNr"):
+            if contr_dict.get("target", {}).get("busNr") is not None:
                 target_bus = int(contr_dict["target"]["busNr"])
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Only assign index if it wasn't already assigned
if "index" not in contr_dict:
# Check if target busNr is specified and use it as index
if contr_dict.get("target", {}).get("busNr"):
target_bus = int(contr_dict["target"]["busNr"])
contr_dict["index"] = target_bus
self.test.log.debug(f"Using target busNr {target_bus} as index for controller {i}: {contr_dict['model']}")
else:
contr_dict["index"] = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], contr_dict["model"])[-1]
self.test.log.debug(f"Assigned index {contr_dict['index']} to controller {i}: {contr_dict['model']}")
else:
self.test.log.debug(f"Index already assigned {contr_dict['index']} for controller {i}: {contr_dict['model']}")
# Only assign index if it wasn't already assigned
if "index" not in contr_dict:
# Check if target busNr is specified and use it as index
if contr_dict.get("target", {}).get("busNr") is not None:
target_bus = int(contr_dict["target"]["busNr"])
contr_dict["index"] = target_bus
self.test.log.debug(f"Using target busNr {target_bus} as index for controller {i}: {contr_dict['model']}")
else:
contr_dict["index"] = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], contr_dict["model"])[-1]
self.test.log.debug(f"Assigned index {contr_dict['index']} to controller {i}: {contr_dict['model']}")
else:
self.test.log.debug(f"Index already assigned {contr_dict['index']} for controller {i}: {contr_dict['model']}")
🤖 Prompt for AI Agents 
In provider/viommu/viommu_base.py around lines 196 to 209, the truthy check on
contr_dict.get("target", {}).get("busNr") incorrectly treats busNr==0 as absent;
change the logic to explicitly test for None (e.g., bus_nr =
contr_dict.get("target", {}).get("busNr"); if bus_nr is not None: target_bus =
int(bus_nr); contr_dict["index"] = target_bus; ...) so zero is accepted while
still handling missing values, and keep the debug log and remaining branches
unchanged.

self.controller_dicts[i] = contr_dict

self.test.log.debug(f"Prepared controllers according to {self.controller_dicts}")
Expand Down Expand Up @@ -243,3 +285,4 @@ def teardown_iommu_test(self, **dargs):
if self.vm.is_alive():
self.vm.destroy(gracefully=False)
self.orig_config_xml.sync()
self.test.log.debug("VM configuration restored to original state")
Loading