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.
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.
---
xen/drivers/passthrough/pci.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 28ed8ea8172a..71072efceb7a 100644
--- 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;
+ /* Keep iterating even if the iommu call failed. */
}
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
--
2.43.0
