Re: [PATCH v3] drm/amdgpu: Take IOMMU remapping into account for p2p checks

[Felix Kuehling <felix.kuehling@amd.com>]

On 2024-08-14 11:17, Alex Deucher wrote:
> On Wed, Aug 14, 2024 at 5:15 AM Rahul Jain <Rahul.Jain@amd.com> wrote:
>> when trying to enable p2p the amdgpu_device_is_peer_accessible()
>> checks the condition where address_mask overlaps the aper_base
>> and hence returns 0, due to which the p2p disables for this platform
>>
>> IOMMU should remap the BAR addresses so the device can access
>> them. Hence check if peer_adev is remapping DMA
>>
>> v3:
>> - remove iommu_remap variable
>>
>> v2: (Alex)
>> - Fix as per review comments
>> - add new function amdgpu_device_check_iommu_remap to check if iommu
>>    remap
>>
>> Signed-off-by: Rahul Jain <Rahul.Jain@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 46 +++++++++++++++++++---
>>   1 file changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a6b8d0ba4758..040c75c491cd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3952,6 +3952,25 @@ static void amdgpu_device_check_iommu_direct_map(struct amdgpu_device *adev)
>>                  adev->ram_is_direct_mapped = true;
>>   }
>>
>> +/**
>> + * amdgpu_device_check_iommu_remap - check if iommu remaped BAR
> change this to:
> Check if DMA remapping is enabled.
>
> since it's not just the BAR, all system address space accesses will be remapped.
>
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * return if IOMMU remapping bar address
>> + */
>> +static bool amdgpu_device_check_iommu_remap(struct amdgpu_device *adev)
>> +{
>> +       struct iommu_domain *domain;
>> +
>> +       domain = iommu_get_domain_for_dev(adev->dev);
>> +       if (domain && (domain->type == IOMMU_DOMAIN_DMA ||
>> +               domain->type == IOMMU_DOMAIN_DMA_FQ))
>> +               return true;
>> +
>> +       return false;
>> +}
>> +
>>   static const struct attribute *amdgpu_dev_attributes[] = {
>>          &dev_attr_pcie_replay_count.attr,
>>          NULL
>> @@ -6127,6 +6146,8 @@ bool amdgpu_device_is_peer_accessible(struct amdgpu_device *adev,
>>                                        struct amdgpu_device *peer_adev)
>>   {
>>   #ifdef CONFIG_HSA_AMD_P2P
>> +       bool peer_remap = amdgpu_device_check_iommu_remap(peer_adev);
>> +
>>          uint64_t address_mask = peer_adev->dev->dma_mask ?
>>                  ~*peer_adev->dev->dma_mask : ~((1ULL << 32) - 1);
>>          resource_size_t aper_limit =
>> @@ -6135,13 +6156,26 @@ bool amdgpu_device_is_peer_accessible(struct amdgpu_device *adev,
>>                  !adev->gmc.xgmi.connected_to_cpu &&
>>                  !(pci_p2pdma_distance(adev->pdev, peer_adev->dev, false) < 0);
>>
>> -       return pcie_p2p && p2p_access && (adev->gmc.visible_vram_size &&
>> -               adev->gmc.real_vram_size == adev->gmc.visible_vram_size &&
>> -               !(adev->gmc.aper_base & address_mask ||
>> -                 aper_limit & address_mask));
>> -#else
>> -       return false;
>> +       if (peer_remap)
>> +               /**
> These don't need to be kerneldoc comments.  Replace /** with /*
>
>> +                * IOMMU is remapping DMA for peer_adev so all accesses
>> +                * should be within peer_adev's DMA mask
>> +                */
>> +               return pcie_p2p && p2p_access &&
>> +                       (adev->gmc.visible_vram_size &&
>> +                        adev->gmc.real_vram_size == adev->gmc.visible_vram_size);
>> +       else
>> +               /**
> Same here.
>
> With those fixed, it looks good to me, but it would be good if Ramesh
> took a look as well as he wrote this code originally.

Looks reasonable to me. But it could be refactored to avoid duplicating 
a bunch of the condition. Maybe something like:

	bool is_large_bar = adev->gmc.visible_vram_size &&
		adev->gmc.real_vram_size == adev->gmc.visible_vram_size;
	bool p2p_access =
		!adev->gmc.xgmi.connected_to_cpu &&
		!(pci_p2pdma_distance(adev->pdev, peer_adev->dev, false) < 0);
	bool p2p_addressable = amdgpu_device_check_iommu_remap(peer_adev);

	if (!p2p_addressable) {
		uint64_t address_mask = peer_adev->dev->dma_mask ?
			~*peer_adev->dev->dma_mask : ~((1ULL << 32) - 1);
		resource_size_t aper_limit =
			adev->gmc.aper_base + adev->gmc.aper_size - 1;

		peer_addressable = !(adev->gmc.aper_base & address_mask ||
				     aper_limit & address_mask);
	}
	return is_large_bar && p2p_access && p2p_addressable;

Regards,
   Felix


>
> Alex
>
>
>> +                * No IOMMU remapping so make sure the adev's aperture
>> +                * fits into peer_adev's dma mask
>> +                */
>> +               return pcie_p2p && p2p_access &&
>> +                       (adev->gmc.visible_vram_size &&
>> +                       adev->gmc.real_vram_size == adev->gmc.visible_vram_size &&
>> +                       !(adev->gmc.aper_base & address_mask ||
>> +                       aper_limit & address_mask));
>>   #endif
>> +       return false;
>>   }
>>
>>   int amdgpu_device_baco_enter(struct drm_device *dev)
>> --
>> 2.34.1
>>