On 12/11/23 11:21, Jan Beulich wrote:
> On 11.12.2023 16:05, Stewart Hildebrand wrote:
>> Currently if an iommu_call() for a phantom function fails, there is no
>> indication of the failure. Propagate (but don't return) the error code
>> from the most recently failed iommu_call() and emit a warning. While
>> here, add a comment to clarify that the loop keeps iterating even when
>> failure is encountered.
>>
>> Fixes: cd7dedad8209 ("passthrough: simplify locking and logging")
>> Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com>
>> ---
>> Unlike assign_device(), deassign_device() breaks out of the phantom loop
>> when a failure is encountered and returns the error code. I'm curious
>> why assign_device() and deassign_device() behave differently? It looks
>> like it's been that way since
>> 4e9950dc1bd2 ("IOMMU: add phantom function support"). I was initially
>> inclined to break out of the loop and return the error code in
>> assign_device(), but adhering to the principle of Chesterton's fence,
>> I'd first like to understand why this is not currently being done.
>
> It's been a long time, but seeing the same pattern for add/remove I think
> the idea was that a device may still work with its phantom functions not
> properly mapped in the IOMMU, whereas failure to unmap means a domain may
> retain (partial) access to a device.
Thanks for the info. I'll add something about this to the in-code comment (see
below).
>
>> I'm aware that I could have avoided introducing a tmp local variable by
>> using the conditional operator with omitted middle operand:
>>
>> rc_nonfatal = iommu_call(...) ?: rc_nonfatal;
>>
>> However, I explicitly chose not to do this to avoid relying on a GNU
>> extension in yet another place.
>
> Introducing a helper variable is certainly okay, if you think that's
> better. However, in cases like ...
>
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1407,7 +1407,7 @@ static int assign_device(struct domain *d, u16 seg, u8
>> bus, u8 devfn, u32 flag)
>> {
>> const struct domain_iommu *hd = dom_iommu(d);
>> struct pci_dev *pdev;
>> - int rc = 0;
>> + int rc = 0, rc_nonfatal = 0;
>>
>> if ( !is_iommu_enabled(d) )
>> return 0;
>> @@ -1443,21 +1443,28 @@ static int assign_device(struct domain *d, u16 seg,
>> u8 bus, u8 devfn, u32 flag)
>> pci_to_dev(pdev), flag)) )
>> goto done;
>>
>> - for ( ; pdev->phantom_stride; rc = 0 )
>> + while ( pdev->phantom_stride )
>> {
>> + int tmp;
>> +
>> devfn += pdev->phantom_stride;
>> if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>> + {
>> + devfn -= pdev->phantom_stride; /* Adjust for printing */
>> break;
>> + }
>> - rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
>> - pci_to_dev(pdev), flag);
>> + tmp = iommu_call(hd->platform_ops, assign_device, d, devfn,
>> + pci_to_dev(pdev), flag);
>> + rc_nonfatal = tmp ? tmp : rc_nonfatal;
>
> ... this, I'm relatively certain most maintainers would agree that using
> the extension will yield easier to read code. Plus there's no reason to
> avoid extensions we use widely anyway, as long as there's no (reasonably
> neat) way to express the same without using extensions.
OK, I'll use the extension.
>
> Jan
>
>> + /* Keep iterating even if the iommu call failed. */
I'll change this in-code comment to:
/*
* Keep going in case of iommu_call failure for phantom functions. The
* device may still be usable without phantom functions mapped in the
* IOMMU.
*/
>> }
>>
>> done:
>> - if ( rc )
>> + if ( rc || rc_nonfatal )
>> printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
>> - d, &PCI_SBDF(seg, bus, devfn), rc);
>> + d, &PCI_SBDF(seg, bus, devfn), rc ? rc : rc_nonfatal);
>> /* The device is assigned to dom_io so mark it as quarantined */
>> - else if ( d == dom_io )
>> + if ( !rc && d == dom_io )
>> pdev->quarantine = true;
>>
>> return rc;
>>
>> base-commit: 1403131596fa77663708f6baa0fee8bf7b95eb5a
>
