* [PATCH v3] drm/amdgpu: Take IOMMU remapping into account for p2p checks
@ 2024-08-14 9:08 Rahul Jain
2024-08-14 15:17 ` Alex Deucher
0 siblings, 1 reply; 3+ messages in thread
From: Rahul Jain @ 2024-08-14 9:08 UTC (permalink / raw)
To: amd-gfx; +Cc: Rahul Jain
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
+ *
+ * @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)
+ /**
+ * 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
+ /**
+ * 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] drm/amdgpu: Take IOMMU remapping into account for p2p checks
2024-08-14 9:08 [PATCH v3] drm/amdgpu: Take IOMMU remapping into account for p2p checks Rahul Jain
@ 2024-08-14 15:17 ` Alex Deucher
2024-08-14 21:40 ` Felix Kuehling
0 siblings, 1 reply; 3+ messages in thread
From: Alex Deucher @ 2024-08-14 15:17 UTC (permalink / raw)
To: Rahul Jain, Ramesh Errabolu; +Cc: amd-gfx
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.
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] drm/amdgpu: Take IOMMU remapping into account for p2p checks
2024-08-14 15:17 ` Alex Deucher
@ 2024-08-14 21:40 ` Felix Kuehling
0 siblings, 0 replies; 3+ messages in thread
From: Felix Kuehling @ 2024-08-14 21:40 UTC (permalink / raw)
To: Alex Deucher, Rahul Jain, Ramesh Errabolu; +Cc: amd-gfx
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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-14 21:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-14 9:08 [PATCH v3] drm/amdgpu: Take IOMMU remapping into account for p2p checks Rahul Jain
2024-08-14 15:17 ` Alex Deucher
2024-08-14 21:40 ` Felix Kuehling
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox