On 28/01/2022 10:36, Jan Beulich wrote: > On 28.01.2022 10:28, Durrant, Paul wrote: >> On 27/01/2022 14:47, Jan Beulich wrote: >>> @@ -1457,24 +1462,24 @@ static int iommu_get_device_group( >>> if ( !is_iommu_enabled(d) || !ops->get_device_group_id ) >>> return 0; >>> >>> - group_id = ops->get_device_group_id(seg, bus, devfn); >>> + group_id = iommu_call(ops, get_device_group_id, seg, bus, devfn); >>> >>> pcidevs_lock(); >>> for_each_pdev( d, pdev ) >>> { >>> - if ( (pdev->seg != seg) || >>> - ((pdev->bus == bus) && (pdev->devfn == devfn)) ) >>> + unsigned int b = pdev->bus; >>> + unsigned int df = pdev->devfn; >>> + >>> + if ( (pdev->seg != seg) || ((b == bus) && (df == devfn)) ) >>> continue; >>> >>> - if ( xsm_get_device_group(XSM_HOOK, (seg << 16) | (pdev->bus << 8) >>> | pdev->devfn) ) >>> + if ( xsm_get_device_group(XSM_HOOK, (seg << 16) | (b << 8) | df) ) >>> continue; >>> >>> - sdev_id = ops->get_device_group_id(seg, pdev->bus, pdev->devfn); >>> + sdev_id = iommu_call(ops, get_device_group_id, seg, b, df); >>> if ( (sdev_id == group_id) && (i < max_sdevs) ) >>> { >>> - bdf = 0; >>> - bdf |= (pdev->bus & 0xff) << 16; >>> - bdf |= (pdev->devfn & 0xff) << 8; >>> + bdf = (b << 16) | (df << 8); >> Don't we have a macro for this now? Probably best to start using it >> whilst modifying the code. > We don't. And it would feel somewhat misleading to use PCI_BDF2(b, df) << 8 > here. The situation is even worse imo: Besides there not being a macro, I > also cannot seem to find any documentation on this non-standard layout (BDF > shifted left by 8). Yet then again I also can't spot any caller of > xc_get_device_group() ...
I'm sure I already did the archaeology. device groups were broken by a hypercall bounce buffering change 2 years before the only caller was dropped with Xend. This mess of a hypercall has demonstrably not been used in a decade. I firmly suggest dropping it, rather than wasting effort trying to unbreak an interface which needs deleting anyway as the first step to doing IOMMU groups. ~Andrew