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 1 commit
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': {'name': 'vhost', '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}}]
Copy link

@coderabbitai coderabbitai bot Oct 2, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Verify controller count and device placement.

The ASCII diagram in lines 103-107 suggests 4 pcie-root-ports are needed (one each for: virtio network interface, two VFIO interfaces, and virtio disk), but controller_dicts at line 112 only defines 3 root-ports (root_port1, root_port2, root_port3).

Additionally, this variant introduces:

  1. Mixed interface types in iface_dict (line 115): one virtio network and two VFIO hostdevs
  2. Reference to vf_pci_addr2 for the second VFIO interface

Please clarify:

  • Are 3 root-ports sufficient, or should a 4th be added for the disk?
  • Confirm that vf_pci_addr2 is properly defined in the test environment
  • Verify that the test code (parse_iface_dict() and related functions) correctly handles a list with mixed interface types (virtio network + hostdev)

Consider updating the comment or adding a 4th root-port to match the described topology:

         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}]
+        root_port4 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
+        controller_dicts = [${expander_bus}, ${root_port1}, ${root_port2}, ${root_port3}, ${root_port4}]
📝 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
- 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': {'name': 'vhost', '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}}]
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'}
root_port4 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
controller_dicts = [${expander_bus}, ${root_port1}, ${root_port2}, ${root_port3}, ${root_port4}]
🤖 Prompt for AI Agents 
libvirt/tests/cfg/sriov/vIOMMU/iommu_device_settings.cfg lines 98-115: the ASCII
topology implies four pcie-root-ports (virtio net, vfio1, vfio2, virtio disk)
but controller_dicts only includes three root_port entries; add a fourth
root_port definition (root_port4) and include it in controller_dicts so the disk
has its own root-port, ensure vf_pci_addr2 is declared in the test fixture or
config before use, and verify/adjust parse_iface_dict() and any related parsing
code to correctly accept a list with mixed interface dicts (handle 'network'
virtio entries and 'hostdev' pci entries in the same list), adding/adjusting
checks or unit tests to validate correct placement of each device on the
expected controllers.

13 changes: 10 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 @@ -58,9 +58,16 @@ 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):
for single_iface_dict in iface_dict:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", single_iface_dict)
else:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", iface_dict)
Comment on lines 68 to 80
Copy link

@coderabbitai coderabbitai bot Oct 2, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Consider error handling for multi-interface cleanup.

The current implementation iterates through interface dicts without error handling. If modify_vm_device fails on one interface, subsequent interfaces won't be cleaned up, potentially leaving the VM in an inconsistent state.

Consider wrapping the cleanup calls in a try-except block to ensure all interfaces are attempted:

         if cleanup_ifaces:
             # Handle both single dict and list of dicts
             if isinstance(iface_dict, list):
                 for single_iface_dict in iface_dict:
-                    libvirt_vmxml.modify_vm_device(
-                            vm_xml.VMXML.new_from_dumpxml(vm.name),
-                            "interface", single_iface_dict)
+                    try:
+                        libvirt_vmxml.modify_vm_device(
+                                vm_xml.VMXML.new_from_dumpxml(vm.name),
+                                "interface", single_iface_dict)
+                    except Exception as e:
+                        test.log.warning(f"Failed to cleanup interface: {e}")
             else:
                 libvirt_vmxml.modify_vm_device(
                         vm_xml.VMXML.new_from_dumpxml(vm.name),
                         "interface", iface_dict)

Additionally, verify that parse_iface_dict() validates list contents to prevent non-dict items from reaching this code.

🏁 Script executed:

#!/bin/bash
# Description: Verify parse_iface_dict() implementation and its validation logic

# Search for the parse_iface_dict method definition
ast-grep --pattern $'def parse_iface_dict($$$) {
  $$$
}'

# Also search for type validation or list handling in parse_iface_dict
rg -n -C5 'def parse_iface_dict' --type=py

Length of output: 4071

🏁 Script executed:

#!/bin/bash
sed -n '110,200p' provider/viommu/viommu_base.py

Length of output: 4517

Isolate interface cleanup failures and validate types
Wrap each modify_vm_device call in a try-except so one failure doesn’t block subsequent cleanups, and skip or error on non-dict entries in the list, e.g.:

     if cleanup_ifaces:
         # Handle both single dict and list of dicts
         if isinstance(iface_dict, list):
             for single_iface_dict in iface_dict:
+               if not isinstance(single_iface_dict, dict):
+                   test.log.warning(f"Skipping invalid iface entry: {single_iface_dict!r}")
+                   continue
+               try:
                     libvirt_vmxml.modify_vm_device(
                             vm_xml.VMXML.new_from_dumpxml(vm.name),
                             "interface", single_iface_dict)
+               except Exception as e:
+                   test.log.warning(f"Failed to cleanup interface: {e}")
         else:
             libvirt_vmxml.modify_vm_device(
                     vm_xml.VMXML.new_from_dumpxml(vm.name),
                     "interface", 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
# Handle both single dict and list of dicts
if isinstance(iface_dict, list):
for single_iface_dict in iface_dict:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", single_iface_dict)
else:
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):
for single_iface_dict in iface_dict:
if not isinstance(single_iface_dict, dict):
test.log.warning(f"Skipping invalid iface entry: {single_iface_dict!r}")
continue
try:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", single_iface_dict)
except Exception as e:
test.log.warning(f"Failed to cleanup interface: {e}")
else:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", iface_dict)
🤖 Prompt for AI Agents 
In libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py around lines 61 to
70, wrap each libvirt_vmxml.modify_vm_device call in a try/except so one failing
cleanup won’t stop others, and validate input types: when iterating a list,
check isinstance(single_iface_dict, dict) and skip/log a warning for non-dict
entries, and in the non-list branch ensure iface_dict is a dict before calling
modify_vm_device; on exception catch Exception as e, log the error with details
and continue to the next item.


test.log.info("TEST_STEP: Start the VM.")
vm.start()
Expand Down
22 changes: 19 additions & 3 deletions provider/viommu/viommu_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,30 @@ def __init__(self, vm, test, params, session=None):

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 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:
iface_bus = "%0#4x" % int(self.controller_dicts[-1].get("index"))
iface_attrs = {"bus": iface_bus}
Expand All @@ -129,7 +146,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 Down