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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

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