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 <[email protected]> > > > > 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
signature.asc
Description: PGP signature
