Re: [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2025 09:34, dmkhn@xxxxxxxxx wrote:
> On Mon, Apr 14, 2025 at 08:19:16PM +0100, Andrii Sultanov wrote:
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -43,7 +43,7 @@ struct amd_iommu *find_iommu_for_device(int seg, int bdf)
>>      {
>>          unsigned int bd0 = bdf & ~PCI_FUNC(~0);
>>
>> -        if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != 
>> bdf )
>> +        if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->sbdf.bdf 
>> != bdf )
>>          {
>>              struct ivrs_mappings tmp = ivrs_mappings[bd0];
>>
>> @@ -121,7 +121,7 @@ static bool use_ats(
>>  {
>>      return !ivrs_dev->block_ats &&
>>             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
>> -           pci_ats_device(iommu->seg, pdev->bus, pdev->devfn);
>> +           pci_ats_device(iommu->sbdf.seg, pdev->bus, pdev->devfn);
> 
> Same idea about updating signature to take pci_sbdf_t.

Perhaps both this and ...

>> @@ -147,17 +147,17 @@ static int __must_check amd_iommu_setup_domain_device(
>>      if ( rc )
>>          return rc;
>>
>> -    req_id = get_dma_requestor_id(iommu->seg, pdev->sbdf.bdf);
>> -    ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
>> +    req_id = get_dma_requestor_id(iommu->sbdf.seg, pdev->sbdf.bdf);
>> +    ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id];
>>      sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain)
>>                  ? 0 : SET_ROOT_VALID)
>>                 | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0);
>>
>>      /* get device-table entry */
>> -    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn));
>> +    req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(bus, devfn));
>>      table = iommu->dev_table.buffer;
>>      dte = &table[req_id];
>> -    ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
>> +    ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id];
>>
>>      if ( domain != dom_io )
>>      {
>> @@ -275,7 +275,7 @@ static int __must_check amd_iommu_setup_domain_device(
>>      ASSERT(pcidevs_locked());
>>
>>      if ( use_ats(pdev, iommu, ivrs_dev) &&
>> -         !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
>> +         !pci_ats_enabled(iommu->sbdf.seg, bus, pdev->devfn) )
> 
> ... and same for pci_ats_enabled()

... this would best take a pointer to a const pdev (if that works out).

And yes, there's a lot of similar cleanup to be done in this area. I don't
think we can demand Andrii to do it all in one go.

Jan