Re: [PATCH 6/8] IOMMU: log appropriate SBDF

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxx>

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

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.