diff mbox series

[6/8] IOMMU: log appropriate SBDF

Message ID e8780667-2307-fa7f-0768-753a83e00082@suse.com (mailing list archive)
State New, archived
Headers show
Series IOMMU: assorted follow-on to XSA-400 | expand

Commit Message

Jan Beulich April 11, 2022, 9:38 a.m. UTC 
To handle phantom devices, several functions are passed separate "devfn"
arguments besides a PCI device. In such cases we want to log the phantom
device's coordinates instead of the main one's. (Note that not all of
the instances being changed are fallout from the referenced commit.)

Fixes: 1ee1441835f4 ("print: introduce a format specifier for pci_sbdf_t")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Roger Pau Monné April 12, 2022, 10:05 a.m. UTC | #1 
On Mon, Apr 11, 2022 at 11:38:28AM +0200, Jan Beulich wrote:
> To handle phantom devices, several functions are passed separate "devfn"
> arguments besides a PCI device. In such cases we want to log the phantom
> device's coordinates instead of the main one's. (Note that not all of
> the instances being changed are fallout from the referenced commit.)
> 
> Fixes: 1ee1441835f4 ("print: introduce a format specifier for pci_sbdf_t")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I'm unsure it matters much to have the logs from failures to find an
IOMMU using pdev->sbdf or devfn, as the parent device should have been
added before attempting to add any phantom functions, and hence would
have already failed to find an IOMMU.

> 
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -291,7 +291,8 @@ void amd_iommu_flush_iotlb(u8 devfn, con
>  
>      if ( !iommu )
>      {
> -        AMD_IOMMU_WARN("can't find IOMMU for %pp\n", &pdev->sbdf);
> +        AMD_IOMMU_WARN("can't find IOMMU for %pp\n",
> +                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn));

Hm, the call to find_iommu_for_device() is explicitly using
pdev->devfn, so while the iommu of the phantom function is tied to
it's parent, it's unclear to me that logging the SBDF of the phantom
function will make this clearer for a user reading the logs.

>          return;
>      }
>  
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -461,7 +461,7 @@ static int cf_check reassign_device(
>      if ( !iommu )
>      {
>          AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be assigned to %pd\n",
> -                       &pdev->sbdf, target);
> +                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn), target);

IIRC we would first reassign the parent, and then the phantom
functions, so we would always hit this error first with devfn ==
pdev->sbdf.bdf.

Thanks, Roger.
Jan Beulich April 12, 2022, 10:39 a.m. UTC | #2 
On 12.04.2022 12:05, Roger Pau Monné wrote:
> On Mon, Apr 11, 2022 at 11:38:28AM +0200, Jan Beulich wrote:
>> To handle phantom devices, several functions are passed separate "devfn"
>> arguments besides a PCI device. In such cases we want to log the phantom
>> device's coordinates instead of the main one's. (Note that not all of
>> the instances being changed are fallout from the referenced commit.)
>>
>> Fixes: 1ee1441835f4 ("print: introduce a format specifier for pci_sbdf_t")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I'm unsure it matters much to have the logs from failures to find an
> IOMMU using pdev->sbdf or devfn, as the parent device should have been
> added before attempting to add any phantom functions, and hence would
> have already failed to find an IOMMU.

That's the expectation, unless something unexpected is going on. Hence
better have precise information in the log.

>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>> @@ -291,7 +291,8 @@ void amd_iommu_flush_iotlb(u8 devfn, con
>>  
>>      if ( !iommu )
>>      {
>> -        AMD_IOMMU_WARN("can't find IOMMU for %pp\n", &pdev->sbdf);
>> +        AMD_IOMMU_WARN("can't find IOMMU for %pp\n",
>> +                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn));
> 
> Hm, the call to find_iommu_for_device() is explicitly using
> pdev->devfn, so while the iommu of the phantom function is tied to
> it's parent, it's unclear to me that logging the SBDF of the phantom
> function will make this clearer for a user reading the logs.

The phantom function may not be possible to find an IOMMU for, so
using the base device for the lookup is unavoidable. For the message
here and ...

>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -461,7 +461,7 @@ static int cf_check reassign_device(
>>      if ( !iommu )
>>      {
>>          AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be assigned to %pd\n",
>> -                       &pdev->sbdf, target);
>> +                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn), target);
> 
> IIRC we would first reassign the parent, and then the phantom
> functions, so we would always hit this error first with devfn ==
> pdev->sbdf.bdf.

... here what I said further up applies.

Jan
diff mbox series

Patch

--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -291,7 +291,8 @@  void amd_iommu_flush_iotlb(u8 devfn, con
 
     if ( !iommu )
     {
-        AMD_IOMMU_WARN("can't find IOMMU for %pp\n", &pdev->sbdf);
+        AMD_IOMMU_WARN("can't find IOMMU for %pp\n",
+                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn));
         return;
     }
 
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -461,7 +461,7 @@  static int cf_check reassign_device(
     if ( !iommu )
     {
         AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be assigned to %pd\n",
-                       &pdev->sbdf, target);
+                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn), target);
         return -ENODEV;
     }
 
@@ -497,8 +497,8 @@  static int cf_check reassign_device(
             return rc;
     }
 
-    AMD_IOMMU_DEBUG("Re-assign %pp from dom%d to dom%d\n",
-                    &pdev->sbdf, source->domain_id, target->domain_id);
+    AMD_IOMMU_DEBUG("Re-assign %pp from %pd to %pd\n",
+                    &PCI_SBDF3(pdev->seg, pdev->bus, devfn), source, target);
 
     return 0;
 }
@@ -575,7 +575,7 @@  static int cf_check amd_iommu_add_device
         }
 
         AMD_IOMMU_WARN("no IOMMU for %pp; cannot be handed to %pd\n",
-                        &pdev->sbdf, pdev->domain);
+                        &PCI_SBDF3(pdev->seg, pdev->bus, devfn), pdev->domain);
         return -ENODEV;
     }
 
@@ -618,7 +618,7 @@  static int cf_check amd_iommu_add_device
              ivrs_mappings[ivrs_mappings[bdf].dte_requestor_id].unity_map,
              0) )
         AMD_IOMMU_WARN("%pd: unity mapping failed for %pp\n",
-                       pdev->domain, &pdev->sbdf);
+                       pdev->domain, &PCI_SBDF2(pdev->seg, bdf));
 
     if ( iommu_quarantine && pdev->arch.pseudo_domid == DOMID_INVALID )
     {
@@ -651,7 +651,7 @@  static int cf_check amd_iommu_remove_dev
     if ( !iommu )
     {
         AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be removed from %pd\n",
-                        &pdev->sbdf, pdev->domain);
+                        &PCI_SBDF3(pdev->seg, pdev->bus, devfn), pdev->domain);
         return -ENODEV;
     }
 
@@ -664,7 +664,7 @@  static int cf_check amd_iommu_remove_dev
              pdev->domain,
              ivrs_mappings[ivrs_mappings[bdf].dte_requestor_id].unity_map) )
         AMD_IOMMU_WARN("%pd: unity unmapping failed for %pp\n",
-                       pdev->domain, &pdev->sbdf);
+                       pdev->domain, &PCI_SBDF2(pdev->seg, bdf));
 
     amd_iommu_quarantine_teardown(pdev);
 
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1406,7 +1406,7 @@  static int iommu_add_device(struct pci_d
         rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
         if ( rc )
             printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
-                   &pdev->sbdf, rc);
+                   &PCI_SBDF3(pdev->seg, pdev->bus, devfn), rc);
     }
 }
 
@@ -1451,7 +1451,8 @@  static int iommu_remove_device(struct pc
         if ( !rc )
             continue;
 
-        printk(XENLOG_ERR "IOMMU: remove %pp failed (%d)\n", &pdev->sbdf, rc);
+        printk(XENLOG_ERR "IOMMU: remove %pp failed (%d)\n",
+               &PCI_SBDF3(pdev->seg, pdev->bus, devfn), rc);
         return rc;
     }