Re: [PATCH] xen/pci: fix phantom error path in assign_device()

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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.hildebrand@xxxxxxx>
>> ---
>> 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
>