[PATCH] util: Fix secondary bus reset check for multi-root PCI systems
The virPCIDeviceReset() function was using a hardcoded check for bus != 0 to determine if a device is on a root bus before attempting a secondary bus reset. This breaks on multi-root systems like Intel Arrow Lake where devices can be on non-zero root buses (e.g., Bus 0x80 for the PCH root complex). Update the logic to use virPCIDeviceIsOnRootBus() which detects root bus attachment via sysfs topology, making it work correctly on any PCI-compliant system regardless of bus numbers. This ensures secondary bus reset is never attempted on ANY root bus (0x00, 0x80, etc.), only on devices behind PCI bridges where it's safe. Signed-off-by: Bruno Martins <ehanoc@protonmail.com> --- src/util/virpci.c | 106 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 7 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 2348a98003..f2ce24e6aa 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1081,6 +1081,81 @@ virPCIDeviceInit(virPCIDevice *dev, int cfgfd) return 0; } +/* + * Check if a PCI device is directly attached to a root bus (no parent bridge). + * + * Modern systems like Intel Arrow Lake (Core Ultra 200) can have multiple + * root buses (e.g., bus 0x00 for CPU devices and bus 0x80 for PCH devices). + * We detect root bus attachment by examining the sysfs topology: a device + * is on a root bus if its sysfs path is a direct child of a pciXXXX:YY + * directory (e.g., /sys/devices/pci0000:80/0000:80:14.0). + * + * Returns 1 if on root bus, 0 if not, -1 on error. + */ +static int +virPCIDeviceIsOnRootBus(virPCIDevice *dev) +{ + unsigned int domain, bus; + g_autofree char *devPath = NULL; + g_autofree char *realPath = NULL; + g_autofree char *parentDir = NULL; + const char *parentName; + + /* Get the sysfs symlink path for this device */ + devPath = g_strdup_printf(PCI_SYSFS "devices/%s", dev->name); + + /* Resolve the symlink to get the real path in /sys/devices/... */ + if (virFileResolveLink(devPath, &realPath) < 0) { + VIR_DEBUG("%s %s: failed to resolve sysfs path %s", + dev->id, dev->name, devPath); + return -1; + } + + parentDir = g_path_get_dirname(realPath); + if (!parentDir) + return -1; + + parentName = strrchr(parentDir, '/'); + if (!parentName) + return -1; + parentName++; // skip the '/' + + /* + * Detect root bus by parsing the parent directory name using sscanf. + * + * PCI root buses in sysfs follow the naming convention: + * pci<domain>:<bus> + * + * - <domain>: number (0000-ffff, hex) + * - <bus>: number within the domain (00-ff, hex) + * + * This format is defined in: + * https://www.kernel.org/doc/html/latest/PCI/sysfs-pci.html + * + * Examples from Intel Arrow Lake systems: + * - pci0000:00 (CPU root bus) + * - pci0000:80 (PCH root bus - the problematic case on Arrow Lake) + * + * sscanf pattern "pci%x:%x" matches: + * - "pci" : literal prefix + * - "%x" : hex domain (stops at ':') + * - ":" : literal separator + * - "%x" : hex bus (must reach end of string) + * + * Returns 2 if both domain and bus were successfully parsed, + * confirming the device is a direct child of a root bus. + */ + if (sscanf(parentName, "pci%x:%x", &domain, &bus) == 2) { + VIR_DEBUG("%s %s: device is on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 1; + } + + VIR_DEBUG("%s %s: device is not on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 0; +} + int virPCIDeviceReset(virPCIDevice *dev, virPCIDeviceList *activeDevs, @@ -1146,9 +1221,17 @@ virPCIDeviceReset(virPCIDevice *dev, if (dev->has_pm_reset) ret = virPCIDeviceTryPowerManagementReset(dev, fd); - /* Bus reset is not an option with the root bus */ - if (ret < 0 && dev->address.bus != 0) - ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + /* Bus reset is not an option with the root bus. On multi-root + * systems like Intel Arrow Lake, root buses can be non-zero (e.g., 0x80), + * so we use sysfs topology detection instead of checking bus == 0. + */ + if (ret < 0) { + int onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + goto cleanup; + if (!onRootBus) + ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + } if (ret < 0) { virErrorPtr err = virGetLastError(); @@ -2531,15 +2614,24 @@ static int virPCIDeviceIsBehindSwitchLackingACS(virPCIDevice *dev) { g_autoptr(virPCIDevice) parent = NULL; + int onRootBus; if (virPCIDeviceGetParent(dev, &parent) < 0) return -1; if (!parent) { - /* if we have no parent, and this is the root bus, ACS doesn't come - * into play since devices on the root bus can't P2P without going - * through the root IOMMU. + /* If we have no parent bridge, check if the device is directly + * attached to a root bus. Systems like Intel Arrow Lake can have + * multiple root buses (e.g., 0x00 and 0x80), so we can't just + * check for bus == 0. Instead, use sysfs topology to detect + * root bus attachment. + * + * ACS doesn't come into play for devices on root buses since + * they can't P2P without going through the root IOMMU. */ - if (dev->address.bus == 0) { + onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + return -1; + if (onRootBus) { return 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.47.3
On Wed, Feb 25, 2026 at 10:35:37AM +0000, ehanoc via Devel wrote:
The virPCIDeviceReset() function was using a hardcoded check for bus != 0 to determine if a device is on a root bus before attempting a secondary bus reset. This breaks on multi-root systems like Intel Arrow Lake where devices can be on non-zero root buses (e.g., Bus 0x80 for the PCH root complex).
Update the logic to use virPCIDeviceIsOnRootBus() which detects root bus attachment via sysfs topology, making it work correctly on any PCI-compliant system regardless of bus numbers.
This ensures secondary bus reset is never attempted on ANY root bus (0x00, 0x80, etc.), only on devices behind PCI bridges where it's safe.
Signed-off-by: Bruno Martins <ehanoc@protonmail.com>
Hi, Any (re)views on this patch? Is there something to change to get it in?
--- src/util/virpci.c | 106 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 7 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 2348a98003..f2ce24e6aa 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1081,6 +1081,81 @@ virPCIDeviceInit(virPCIDevice *dev, int cfgfd) return 0; }
+/* + * Check if a PCI device is directly attached to a root bus (no parent bridge). + * + * Modern systems like Intel Arrow Lake (Core Ultra 200) can have multiple + * root buses (e.g., bus 0x00 for CPU devices and bus 0x80 for PCH devices). + * We detect root bus attachment by examining the sysfs topology: a device + * is on a root bus if its sysfs path is a direct child of a pciXXXX:YY + * directory (e.g., /sys/devices/pci0000:80/0000:80:14.0). + * + * Returns 1 if on root bus, 0 if not, -1 on error. + */ +static int +virPCIDeviceIsOnRootBus(virPCIDevice *dev) +{ + unsigned int domain, bus; + g_autofree char *devPath = NULL; + g_autofree char *realPath = NULL; + g_autofree char *parentDir = NULL; + const char *parentName; + + /* Get the sysfs symlink path for this device */ + devPath = g_strdup_printf(PCI_SYSFS "devices/%s", dev->name); + + /* Resolve the symlink to get the real path in /sys/devices/... */ + if (virFileResolveLink(devPath, &realPath) < 0) { + VIR_DEBUG("%s %s: failed to resolve sysfs path %s", + dev->id, dev->name, devPath); + return -1; + } + + parentDir = g_path_get_dirname(realPath); + if (!parentDir) + return -1; + + parentName = strrchr(parentDir, '/'); + if (!parentName) + return -1; + parentName++; // skip the '/' + + /* + * Detect root bus by parsing the parent directory name using sscanf. + * + * PCI root buses in sysfs follow the naming convention: + * pci<domain>:<bus> + * + * - <domain>: number (0000-ffff, hex) + * - <bus>: number within the domain (00-ff, hex) + * + * This format is defined in: + * https://www.kernel.org/doc/html/latest/PCI/sysfs-pci.html + * + * Examples from Intel Arrow Lake systems: + * - pci0000:00 (CPU root bus) + * - pci0000:80 (PCH root bus - the problematic case on Arrow Lake) + * + * sscanf pattern "pci%x:%x" matches: + * - "pci" : literal prefix + * - "%x" : hex domain (stops at ':') + * - ":" : literal separator + * - "%x" : hex bus (must reach end of string) + * + * Returns 2 if both domain and bus were successfully parsed, + * confirming the device is a direct child of a root bus. + */ + if (sscanf(parentName, "pci%x:%x", &domain, &bus) == 2) { + VIR_DEBUG("%s %s: device is on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 1; + } + + VIR_DEBUG("%s %s: device is not on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 0; +} + int virPCIDeviceReset(virPCIDevice *dev, virPCIDeviceList *activeDevs, @@ -1146,9 +1221,17 @@ virPCIDeviceReset(virPCIDevice *dev, if (dev->has_pm_reset) ret = virPCIDeviceTryPowerManagementReset(dev, fd);
- /* Bus reset is not an option with the root bus */ - if (ret < 0 && dev->address.bus != 0) - ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + /* Bus reset is not an option with the root bus. On multi-root + * systems like Intel Arrow Lake, root buses can be non-zero (e.g., 0x80), + * so we use sysfs topology detection instead of checking bus == 0. + */ + if (ret < 0) { + int onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + goto cleanup; + if (!onRootBus) + ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + }
if (ret < 0) { virErrorPtr err = virGetLastError(); @@ -2531,15 +2614,24 @@ static int virPCIDeviceIsBehindSwitchLackingACS(virPCIDevice *dev) { g_autoptr(virPCIDevice) parent = NULL; + int onRootBus;
if (virPCIDeviceGetParent(dev, &parent) < 0) return -1; if (!parent) { - /* if we have no parent, and this is the root bus, ACS doesn't come - * into play since devices on the root bus can't P2P without going - * through the root IOMMU. + /* If we have no parent bridge, check if the device is directly + * attached to a root bus. Systems like Intel Arrow Lake can have + * multiple root buses (e.g., 0x00 and 0x80), so we can't just + * check for bus == 0. Instead, use sysfs topology to detect + * root bus attachment. + * + * ACS doesn't come into play for devices on root buses since + * they can't P2P without going through the root IOMMU. */ - if (dev->address.bus == 0) { + onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + return -1; + if (onRootBus) { return 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.47.3
-- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab
Sorry, I keep forgetting about this. I'll go through it in the morning and push it unless I find some problem. (I had a bit of a different idea for fixing it (replacing the horribly inefficient implement of virPCIDeviceGetParent, for starters), but it's not fair to compare an actual tested patch to vaporware :-P) Wasn't there an issue somewhere in someone's bug tracking system about this? It would be good to include a link to that in the commit log. On 4/21/26 10:10 AM, Marek Marczykowski-Górecki wrote:
On Wed, Feb 25, 2026 at 10:35:37AM +0000, ehanoc via Devel wrote:
The virPCIDeviceReset() function was using a hardcoded check for bus != 0 to determine if a device is on a root bus before attempting a secondary bus reset. This breaks on multi-root systems like Intel Arrow Lake where devices can be on non-zero root buses (e.g., Bus 0x80 for the PCH root complex).
Update the logic to use virPCIDeviceIsOnRootBus() which detects root bus attachment via sysfs topology, making it work correctly on any PCI-compliant system regardless of bus numbers.
This ensures secondary bus reset is never attempted on ANY root bus (0x00, 0x80, etc.), only on devices behind PCI bridges where it's safe.
Signed-off-by: Bruno Martins <ehanoc@protonmail.com>
Hi,
Any (re)views on this patch? Is there something to change to get it in?
--- src/util/virpci.c | 106 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 7 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 2348a98003..f2ce24e6aa 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1081,6 +1081,81 @@ virPCIDeviceInit(virPCIDevice *dev, int cfgfd) return 0; }
+/* + * Check if a PCI device is directly attached to a root bus (no parent bridge). + * + * Modern systems like Intel Arrow Lake (Core Ultra 200) can have multiple + * root buses (e.g., bus 0x00 for CPU devices and bus 0x80 for PCH devices). + * We detect root bus attachment by examining the sysfs topology: a device + * is on a root bus if its sysfs path is a direct child of a pciXXXX:YY + * directory (e.g., /sys/devices/pci0000:80/0000:80:14.0). + * + * Returns 1 if on root bus, 0 if not, -1 on error. + */ +static int +virPCIDeviceIsOnRootBus(virPCIDevice *dev) +{ + unsigned int domain, bus; + g_autofree char *devPath = NULL; + g_autofree char *realPath = NULL; + g_autofree char *parentDir = NULL; + const char *parentName; + + /* Get the sysfs symlink path for this device */ + devPath = g_strdup_printf(PCI_SYSFS "devices/%s", dev->name); + + /* Resolve the symlink to get the real path in /sys/devices/... */ + if (virFileResolveLink(devPath, &realPath) < 0) { + VIR_DEBUG("%s %s: failed to resolve sysfs path %s", + dev->id, dev->name, devPath); + return -1; + } + + parentDir = g_path_get_dirname(realPath); + if (!parentDir) + return -1; + + parentName = strrchr(parentDir, '/'); + if (!parentName) + return -1; + parentName++; // skip the '/' + + /* + * Detect root bus by parsing the parent directory name using sscanf. + * + * PCI root buses in sysfs follow the naming convention: + * pci<domain>:<bus> + * + * - <domain>: number (0000-ffff, hex) + * - <bus>: number within the domain (00-ff, hex) + * + * This format is defined in: + * https://www.kernel.org/doc/html/latest/PCI/sysfs-pci.html + * + * Examples from Intel Arrow Lake systems: + * - pci0000:00 (CPU root bus) + * - pci0000:80 (PCH root bus - the problematic case on Arrow Lake) + * + * sscanf pattern "pci%x:%x" matches: + * - "pci" : literal prefix + * - "%x" : hex domain (stops at ':') + * - ":" : literal separator + * - "%x" : hex bus (must reach end of string) + * + * Returns 2 if both domain and bus were successfully parsed, + * confirming the device is a direct child of a root bus. + */ + if (sscanf(parentName, "pci%x:%x", &domain, &bus) == 2) { + VIR_DEBUG("%s %s: device is on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 1; + } + + VIR_DEBUG("%s %s: device is not on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 0; +} + int virPCIDeviceReset(virPCIDevice *dev, virPCIDeviceList *activeDevs, @@ -1146,9 +1221,17 @@ virPCIDeviceReset(virPCIDevice *dev, if (dev->has_pm_reset) ret = virPCIDeviceTryPowerManagementReset(dev, fd);
- /* Bus reset is not an option with the root bus */ - if (ret < 0 && dev->address.bus != 0) - ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + /* Bus reset is not an option with the root bus. On multi-root + * systems like Intel Arrow Lake, root buses can be non-zero (e.g., 0x80), + * so we use sysfs topology detection instead of checking bus == 0. + */ + if (ret < 0) { + int onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + goto cleanup; + if (!onRootBus) + ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + }
if (ret < 0) { virErrorPtr err = virGetLastError(); @@ -2531,15 +2614,24 @@ static int virPCIDeviceIsBehindSwitchLackingACS(virPCIDevice *dev) { g_autoptr(virPCIDevice) parent = NULL; + int onRootBus;
if (virPCIDeviceGetParent(dev, &parent) < 0) return -1; if (!parent) { - /* if we have no parent, and this is the root bus, ACS doesn't come - * into play since devices on the root bus can't P2P without going - * through the root IOMMU. + /* If we have no parent bridge, check if the device is directly + * attached to a root bus. Systems like Intel Arrow Lake can have + * multiple root buses (e.g., 0x00 and 0x80), so we can't just + * check for bus == 0. Instead, use sysfs topology to detect + * root bus attachment. + * + * ACS doesn't come into play for devices on root buses since + * they can't P2P without going through the root IOMMU. */ - if (dev->address.bus == 0) { + onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + return -1; + if (onRootBus) { return 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.47.3
On Wed, Apr 22, 2026 at 02:36:30AM -0400, Laine Stump wrote:
Sorry, I keep forgetting about this. I'll go through it in the morning and push it unless I find some problem.
(I had a bit of a different idea for fixing it (replacing the horribly inefficient implement of virPCIDeviceGetParent, for starters), but it's not fair to compare an actual tested patch to vaporware :-P)
Wasn't there an issue somewhere in someone's bug tracking system about this? It would be good to include a link to that in the commit log.
Yes, here: https://github.com/QubesOS/qubes-issues/issues/10393
On 4/21/26 10:10 AM, Marek Marczykowski-Górecki wrote:
On Wed, Feb 25, 2026 at 10:35:37AM +0000, ehanoc via Devel wrote:
The virPCIDeviceReset() function was using a hardcoded check for bus != 0 to determine if a device is on a root bus before attempting a secondary bus reset. This breaks on multi-root systems like Intel Arrow Lake where devices can be on non-zero root buses (e.g., Bus 0x80 for the PCH root complex).
Update the logic to use virPCIDeviceIsOnRootBus() which detects root bus attachment via sysfs topology, making it work correctly on any PCI-compliant system regardless of bus numbers.
This ensures secondary bus reset is never attempted on ANY root bus (0x00, 0x80, etc.), only on devices behind PCI bridges where it's safe.
Signed-off-by: Bruno Martins <ehanoc@protonmail.com>
Hi,
Any (re)views on this patch? Is there something to change to get it in?
--- src/util/virpci.c | 106 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 7 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 2348a98003..f2ce24e6aa 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1081,6 +1081,81 @@ virPCIDeviceInit(virPCIDevice *dev, int cfgfd) return 0; } +/* + * Check if a PCI device is directly attached to a root bus (no parent bridge). + * + * Modern systems like Intel Arrow Lake (Core Ultra 200) can have multiple + * root buses (e.g., bus 0x00 for CPU devices and bus 0x80 for PCH devices). + * We detect root bus attachment by examining the sysfs topology: a device + * is on a root bus if its sysfs path is a direct child of a pciXXXX:YY + * directory (e.g., /sys/devices/pci0000:80/0000:80:14.0). + * + * Returns 1 if on root bus, 0 if not, -1 on error. + */ +static int +virPCIDeviceIsOnRootBus(virPCIDevice *dev) +{ + unsigned int domain, bus; + g_autofree char *devPath = NULL; + g_autofree char *realPath = NULL; + g_autofree char *parentDir = NULL; + const char *parentName; + + /* Get the sysfs symlink path for this device */ + devPath = g_strdup_printf(PCI_SYSFS "devices/%s", dev->name); + + /* Resolve the symlink to get the real path in /sys/devices/... */ + if (virFileResolveLink(devPath, &realPath) < 0) { + VIR_DEBUG("%s %s: failed to resolve sysfs path %s", + dev->id, dev->name, devPath); + return -1; + } + + parentDir = g_path_get_dirname(realPath); + if (!parentDir) + return -1; + + parentName = strrchr(parentDir, '/'); + if (!parentName) + return -1; + parentName++; // skip the '/' + + /* + * Detect root bus by parsing the parent directory name using sscanf. + * + * PCI root buses in sysfs follow the naming convention: + * pci<domain>:<bus> + * + * - <domain>: number (0000-ffff, hex) + * - <bus>: number within the domain (00-ff, hex) + * + * This format is defined in: + * https://www.kernel.org/doc/html/latest/PCI/sysfs-pci.html + * + * Examples from Intel Arrow Lake systems: + * - pci0000:00 (CPU root bus) + * - pci0000:80 (PCH root bus - the problematic case on Arrow Lake) + * + * sscanf pattern "pci%x:%x" matches: + * - "pci" : literal prefix + * - "%x" : hex domain (stops at ':') + * - ":" : literal separator + * - "%x" : hex bus (must reach end of string) + * + * Returns 2 if both domain and bus were successfully parsed, + * confirming the device is a direct child of a root bus. + */ + if (sscanf(parentName, "pci%x:%x", &domain, &bus) == 2) { + VIR_DEBUG("%s %s: device is on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 1; + } + + VIR_DEBUG("%s %s: device is not on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 0; +} + int virPCIDeviceReset(virPCIDevice *dev, virPCIDeviceList *activeDevs, @@ -1146,9 +1221,17 @@ virPCIDeviceReset(virPCIDevice *dev, if (dev->has_pm_reset) ret = virPCIDeviceTryPowerManagementReset(dev, fd); - /* Bus reset is not an option with the root bus */ - if (ret < 0 && dev->address.bus != 0) - ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + /* Bus reset is not an option with the root bus. On multi-root + * systems like Intel Arrow Lake, root buses can be non-zero (e.g., 0x80), + * so we use sysfs topology detection instead of checking bus == 0. + */ + if (ret < 0) { + int onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + goto cleanup; + if (!onRootBus) + ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + } if (ret < 0) { virErrorPtr err = virGetLastError(); @@ -2531,15 +2614,24 @@ static int virPCIDeviceIsBehindSwitchLackingACS(virPCIDevice *dev) { g_autoptr(virPCIDevice) parent = NULL; + int onRootBus; if (virPCIDeviceGetParent(dev, &parent) < 0) return -1; if (!parent) { - /* if we have no parent, and this is the root bus, ACS doesn't come - * into play since devices on the root bus can't P2P without going - * through the root IOMMU. + /* If we have no parent bridge, check if the device is directly + * attached to a root bus. Systems like Intel Arrow Lake can have + * multiple root buses (e.g., 0x00 and 0x80), so we can't just + * check for bus == 0. Instead, use sysfs topology to detect + * root bus attachment. + * + * ACS doesn't come into play for devices on root buses since + * they can't P2P without going through the root IOMMU. */ - if (dev->address.bus == 0) { + onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + return -1; + if (onRootBus) { return 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.47.3
-- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab
Hey Laine, Thanks for having a look at this. Happy to test any changes you deem worthy. This is the patch i'm running in my dom0 and was out of necessity to unblock what was preventing me from using QubesOS on my on Intel ArrowLake arch based workstation. It should be generic enough to cover single root bus or multi-root scenarios though. Let me know if you find anything. -- Best Bruno
On Wed, Feb 25, 2026 at 10:35:37AM +0000, ehanoc via Devel wrote:
The virPCIDeviceReset() function was using a hardcoded check for bus != 0 to determine if a device is on a root bus before attempting a secondary bus reset. This breaks on multi-root systems like Intel Arrow Lake where devices can be on non-zero root buses (e.g., Bus 0x80 for the PCH root complex).
Update the logic to use virPCIDeviceIsOnRootBus() which detects root bus attachment via sysfs topology, making it work correctly on any PCI-compliant system regardless of bus numbers.
This ensures secondary bus reset is never attempted on ANY root bus (0x00, 0x80, etc.), only on devices behind PCI bridges where it's safe.
Signed-off-by: Bruno Martins <ehanoc@protonmail.com> --- src/util/virpci.c | 106 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 7 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> (since Laine indicated intent to review it too, I won't push immediately)
On 2/25/26 5:35 AM, ehanoc via Devel wrote:
The virPCIDeviceReset() function was using a hardcoded check for bus != 0 to determine if a device is on a root bus before attempting a secondary bus reset. This breaks on multi-root systems like Intel Arrow Lake where devices can be on non-zero root buses (e.g., Bus 0x80 for the PCH root complex).
Update the logic to use virPCIDeviceIsOnRootBus() which detects root bus attachment via sysfs topology, making it work correctly on any PCI-compliant system regardless of bus numbers.
This ensures secondary bus reset is never attempted on ANY root bus (0x00, 0x80, etc.), only on devices behind PCI bridges where it's safe.
The patch also improves virPCIDeviceIsBehindSwitchLackingACS() in the same manner - that should also be listed in the commit log.
Signed-off-by: Bruno Martins <ehanoc@protonmail.com> --- src/util/virpci.c | 106 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 7 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 2348a98003..f2ce24e6aa 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1081,6 +1081,81 @@ virPCIDeviceInit(virPCIDevice *dev, int cfgfd) return 0; }
+/* + * Check if a PCI device is directly attached to a root bus (no parent bridge). + * + * Modern systems like Intel Arrow Lake (Core Ultra 200) can have multiple + * root buses (e.g., bus 0x00 for CPU devices and bus 0x80 for PCH devices). + * We detect root bus attachment by examining the sysfs topology: a device + * is on a root bus if its sysfs path is a direct child of a pciXXXX:YY + * directory (e.g., /sys/devices/pci0000:80/0000:80:14.0).
I reworded this comment and added an example of a device that *isn't* directly attached to a root bus. I'll send the proposed differences and if you approve I'll push that (there are also some changes to code in the error path).
+ * + * Returns 1 if on root bus, 0 if not, -1 on error. + */ +static int +virPCIDeviceIsOnRootBus(virPCIDevice *dev) +{ + unsigned int domain, bus; + g_autofree char *devPath = NULL; + g_autofree char *realPath = NULL; + g_autofree char *parentDir = NULL; + const char *parentName; + + /* Get the sysfs symlink path for this device */ + devPath = g_strdup_printf(PCI_SYSFS "devices/%s", dev->name); + + /* Resolve the symlink to get the real path in /sys/devices/... */ + if (virFileResolveLink(devPath, &realPath) < 0) { + VIR_DEBUG("%s %s: failed to resolve sysfs path %s", + dev->id, dev->name, devPath);
This needs to be logged as an error, otherwise this would result in the dreaded "failure with no error log" in the case that someone didn't have debug logging enabled.
+ return -1; + } + + parentDir = g_path_get_dirname(realPath); + if (!parentDir) + return -1;
Likewise for this one (and the next). Obviously they're both *extremely* unlikely to ever happen, but if they did then nobody would have any idea what caused the error (and no way to figure it out)
+ + parentName = strrchr(parentDir, '/'); + if (!parentName) + return -1; + parentName++; // skip the '/' + + /* + * Detect root bus by parsing the parent directory name using sscanf. + * + * PCI root buses in sysfs follow the naming convention: + * pci<domain>:<bus> + * + * - <domain>: number (0000-ffff, hex) + * - <bus>: number within the domain (00-ff, hex) + * + * This format is defined in: + * https://www.kernel.org/doc/html/latest/PCI/sysfs-pci.html + * + * Examples from Intel Arrow Lake systems: + * - pci0000:00 (CPU root bus) + * - pci0000:80 (PCH root bus - the problematic case on Arrow Lake) + * + * sscanf pattern "pci%x:%x" matches: + * - "pci" : literal prefix + * - "%x" : hex domain (stops at ':') + * - ":" : literal separator + * - "%x" : hex bus (must reach end of string) + *
I removed the description of how sscanf works, and the mention of the "problematic case on Arrow Lake" (since it is no longer a problem thanks to your patch :-), in favor of just giving the two examples of the naming convention and reference to its specification.
+ * Returns 2 if both domain and bus were successfully parsed, + * confirming the device is a direct child of a root bus. + */ + if (sscanf(parentName, "pci%x:%x", &domain, &bus) == 2) { + VIR_DEBUG("%s %s: device is on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 1; + } + + VIR_DEBUG("%s %s: device is not on root bus (parent=%s)", + dev->id, dev->name, parentName); + return 0; +} + int virPCIDeviceReset(virPCIDevice *dev, virPCIDeviceList *activeDevs, @@ -1146,9 +1221,17 @@ virPCIDeviceReset(virPCIDevice *dev, if (dev->has_pm_reset) ret = virPCIDeviceTryPowerManagementReset(dev, fd);
- /* Bus reset is not an option with the root bus */ - if (ret < 0 && dev->address.bus != 0) - ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + /* Bus reset is not an option with the root bus. On multi-root + * systems like Intel Arrow Lake, root buses can be non-zero (e.g., 0x80), + * so we use sysfs topology detection instead of checking bus == 0.
This extra verbiage is very useful for someone examining the change in the code made by this commit, but for someone in the future it will just be extra stuff they need to parse - they will have no idea that the code previously was so naive as to assume that only bus 0 is a root bus, and it will be obvious to them that "of course you determine if it's on a root bus by calling that function that has has 'IsOnRootBus' in its name" :-) (in other words, in my suggested change to the patch I've removed all the stuff about Arrow Lake and bus == 0, and just say "bus reset isn't an option on a root bus".
+ */ + if (ret < 0) { + int onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + goto cleanup;
(this is one of the paths where we would fail without logging an error)
+ if (!onRootBus) + ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); + }
if (ret < 0) { virErrorPtr err = virGetLastError(); @@ -2531,15 +2614,24 @@ static int virPCIDeviceIsBehindSwitchLackingACS(virPCIDevice *dev) { g_autoptr(virPCIDevice) parent = NULL; + int onRootBus;
if (virPCIDeviceGetParent(dev, &parent) < 0) return -1; if (!parent) { - /* if we have no parent, and this is the root bus, ACS doesn't come - * into play since devices on the root bus can't P2P without going - * through the root IOMMU. + /* If we have no parent bridge, check if the device is directly + * attached to a root bus. Systems like Intel Arrow Lake can have + * multiple root buses (e.g., 0x00 and 0x80), so we can't just + * check for bus == 0. Instead, use sysfs topology to detect + * root bus attachment.
Here too, this is useful information for the person looking at the patch, but in the future will be irrelevant.
+ * + * ACS doesn't come into play for devices on root buses since + * they can't P2P without going through the root IOMMU. */ - if (dev->address.bus == 0) { + onRootBus = virPCIDeviceIsOnRootBus(dev); + if (onRootBus < 0) + return -1;
(the other place where we would fail with no error log)
+ if (onRootBus) { return 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR,
Reviewed-by: Laine Stump <laine@redhat.com> assuming the minor changes I've outlined above are made (which I'm in the process of doing now, and will post for your approval as soon as I'm done)
Thanks Laine. I'm happy to make all or any of the above that will be remaining after yours. Best!
I made changes to your patch according to the notes in my review and posted. I've built it, but couldn't test, so please do! Thanks! On 4/23/26 1:06 PM, Bruno Martins via Devel wrote:
Thanks Laine.
I'm happy to make all or any of the above that will be remaining after yours.
Best!
participants (5)
-
Bruno Martins -
Daniel P. Berrange -
ehanoc
-
Laine Stump -
Marek Marczykowski-Górecki