diff mbox series

[2/3] drm/etnaviv: Map and unmap the GPU VA range with respect to its user size

Message ID 20241004194207.1013744-3-sui.jingfeng@linux.dev (mailing list archive)
State New, archived
Headers show
Series Fix GPU virtual address collosion when CPU page size != GPU page size | expand

Commit Message

Sui Jingfeng Oct. 4, 2024, 7:42 p.m. UTC 
Since the GPU VA space is compact in terms of 4KiB unit, map and/or unmap
the area that doesn't belong to a context breaks the philosophy of PPAS.
That results in severe errors: GPU hang and MMU fault (page not present)
and such.

Shrink the usuable size of etnaviv GEM buffer object to its user size,
instead of the original physical size of its backing memory.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 28 +++++++++------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

Comments

Lucas Stach Oct. 7, 2024, 10:17 a.m. UTC | #1 
Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
> Since the GPU VA space is compact in terms of 4KiB unit, map and/or unmap
> the area that doesn't belong to a context breaks the philosophy of PPAS.
> That results in severe errors: GPU hang and MMU fault (page not present)
> and such.
> 
> Shrink the usuable size of etnaviv GEM buffer object to its user size,
> instead of the original physical size of its backing memory.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 28 +++++++++------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> index 6fbc62772d85..a52ec5eb0e3d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> @@ -70,8 +70,10 @@ static int etnaviv_context_map(struct etnaviv_iommu_context *context,
>  }
>  
>  static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
> +			     unsigned int user_len,
>  			     struct sg_table *sgt, int prot)
> -{	struct scatterlist *sg;
> +{
> +	struct scatterlist *sg;
>  	unsigned int da = iova;
>  	unsigned int i;
>  	int ret;
> @@ -81,7 +83,8 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>  
>  	for_each_sgtable_dma_sg(sgt, sg, i) {
>  		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
> -		size_t bytes = sg_dma_len(sg) + sg->offset;
> +		unsigned int phys_len = sg_dma_len(sg) + sg->offset;
> +		size_t bytes = MIN(phys_len, user_len);
>  
>  		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
>  
> @@ -89,6 +92,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>  		if (ret)
>  			goto fail;
>  
> +		user_len -= bytes;
>  		da += bytes;
>  	}

Since the MIN(phys_len, user_len) may limit the mapped amount in the
wrong direction, I would think it would be good to add a
WARN_ON(user_len != 0) after the dma SG iteration.

>  
> @@ -104,21 +108,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>  static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
>  				struct sg_table *sgt, unsigned len)
>  {
> -	struct scatterlist *sg;
> -	unsigned int da = iova;
> -	int i;
> -
> -	for_each_sgtable_dma_sg(sgt, sg, i) {
> -		size_t bytes = sg_dma_len(sg) + sg->offset;
> -
> -		etnaviv_context_unmap(context, da, bytes);
> -
> -		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
> -
> -		BUG_ON(!PAGE_ALIGNED(bytes));
> -
> -		da += bytes;
> -	}
> +	etnaviv_context_unmap(context, iova, len);

This drops some sanity checks, but I have only ever seen them fire when
we had other kernel memory corruption issues, so I'm fine with the
simplification you did here.

Regards,
Lucas

>  
>  	context->flush_seq++;
>  }
> @@ -131,7 +121,7 @@ static void etnaviv_iommu_remove_mapping(struct etnaviv_iommu_context *context,
>  	lockdep_assert_held(&context->lock);
>  
>  	etnaviv_iommu_unmap(context, mapping->vram_node.start,
> -			    etnaviv_obj->sgt, etnaviv_obj->base.size);
> +			    etnaviv_obj->sgt, etnaviv_obj->user_size);
>  	drm_mm_remove_node(&mapping->vram_node);
>  }
>  
> @@ -314,7 +304,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>  		goto unlock;
>  
>  	mapping->iova = node->start;
> -	ret = etnaviv_iommu_map(context, node->start, sgt,
> +	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,
>  				ETNAVIV_PROT_READ | ETNAVIV_PROT_WRITE);
>  
>  	if (ret < 0) {
Sui Jingfeng Oct. 25, 2024, 5:19 p.m. UTC | #2 
Hi,


On 2024/10/7 18:17, Lucas Stach wrote:
> Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
>> Since the GPU VA space is compact in terms of 4KiB unit, map and/or unmap
>> the area that doesn't belong to a context breaks the philosophy of PPAS.
>> That results in severe errors: GPU hang and MMU fault (page not present)
>> and such.
>>
>> Shrink the usuable size of etnaviv GEM buffer object to its user size,
>> instead of the original physical size of its backing memory.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 28 +++++++++------------------
>>   1 file changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> index 6fbc62772d85..a52ec5eb0e3d 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> @@ -70,8 +70,10 @@ static int etnaviv_context_map(struct etnaviv_iommu_context *context,
>>   }
>>   
>>   static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>> +			     unsigned int user_len,
>>   			     struct sg_table *sgt, int prot)
>> -{	struct scatterlist *sg;
>> +{
>> +	struct scatterlist *sg;
>>   	unsigned int da = iova;
>>   	unsigned int i;
>>   	int ret;
>> @@ -81,7 +83,8 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>>   
>>   	for_each_sgtable_dma_sg(sgt, sg, i) {
>>   		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
>> -		size_t bytes = sg_dma_len(sg) + sg->offset;
>> +		unsigned int phys_len = sg_dma_len(sg) + sg->offset;
>> +		size_t bytes = MIN(phys_len, user_len);
>>   
>>   		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
>>   
>> @@ -89,6 +92,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>>   		if (ret)
>>   			goto fail;
>>   
>> +		user_len -= bytes;
>>   		da += bytes;
>>   	}
> Since the MIN(phys_len, user_len) may limit the mapped amount in the
> wrong direction,

I was thinking that if this will could really happen.

'if (phys_len <= user_len)' is true, the 'bytes' is a number of multiple
of PAGE_SIZE. Since our sg table is created by drm_prime_pages_to_sg(),
so the program still works exactly some as before.

It only differs from previous when 'if (phys_len > user_len)' is true,
but then, it is the tail SG entry that the size of it is not a multiple
of PAGE_SIZE. The 'bytes' will be *exactly* the size of remain GPUVA
range we should map.

> I would think it would be good to add a
> WARN_ON(user_len != 0) after the dma SG iteration.

So the program here seems nearly always correct, no?

Or are you means that when the CPU PAGE size < 4KiB cases ?

I never ever have a CPU has < 4 KiB page configuration.

Regards,
Sui


>>   
>> @@ -104,21 +108,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>>   static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
>>   				struct sg_table *sgt, unsigned len)
>>   {
>> -	struct scatterlist *sg;
>> -	unsigned int da = iova;
>> -	int i;
>> -
>> -	for_each_sgtable_dma_sg(sgt, sg, i) {
>> -		size_t bytes = sg_dma_len(sg) + sg->offset;
>> -
>> -		etnaviv_context_unmap(context, da, bytes);
>> -
>> -		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
>> -
>> -		BUG_ON(!PAGE_ALIGNED(bytes));
>> -
>> -		da += bytes;
>> -	}
>> +	etnaviv_context_unmap(context, iova, len);
> This drops some sanity checks, but I have only ever seen them fire when
> we had other kernel memory corruption issues, so I'm fine with the
> simplification you did here.
>
> Regards,
> Lucas
>
>>   
>>   	context->flush_seq++;
>>   }
>> @@ -131,7 +121,7 @@ static void etnaviv_iommu_remove_mapping(struct etnaviv_iommu_context *context,
>>   	lockdep_assert_held(&context->lock);
>>   
>>   	etnaviv_iommu_unmap(context, mapping->vram_node.start,
>> -			    etnaviv_obj->sgt, etnaviv_obj->base.size);
>> +			    etnaviv_obj->sgt, etnaviv_obj->user_size);
>>   	drm_mm_remove_node(&mapping->vram_node);
>>   }
>>   
>> @@ -314,7 +304,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>>   		goto unlock;
>>   
>>   	mapping->iova = node->start;
>> -	ret = etnaviv_iommu_map(context, node->start, sgt,
>> +	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,
>>   				ETNAVIV_PROT_READ | ETNAVIV_PROT_WRITE);
>>   
>>   	if (ret < 0) {
Sui Jingfeng Oct. 26, 2024, 5:55 a.m. UTC | #3 
Hi,

On 2024/10/7 18:17, Lucas Stach wrote:
>>   
>> @@ -104,21 +108,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>>   static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
>>   				struct sg_table *sgt, unsigned len)
>>   {
>> -	struct scatterlist *sg;
>> -	unsigned int da = iova;
>> -	int i;
>> -
>> -	for_each_sgtable_dma_sg(sgt, sg, i) {
>> -		size_t bytes = sg_dma_len(sg) + sg->offset;


Why the length of a single SG segment is `sg_dma_len(sg) + sg->offset` here?


>> -		etnaviv_context_unmap(context, da, bytes);
>> -
>> -		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
>> -
>> -		BUG_ON(!PAGE_ALIGNED(bytes));
>> -
>> -		da += bytes;
>> -	}
>> +	etnaviv_context_unmap(context, iova, len);
> This drops some sanity checks, but I have only ever seen them fire when
> we had other kernel memory corruption issues, so I'm fine with the
> simplification you did here.


Isn't that 'sg_dma_len(sg)' itself is the length of its backing memory ?


> Regards,
> Lucas
diff mbox series

Patch

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 6fbc62772d85..a52ec5eb0e3d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -70,8 +70,10 @@  static int etnaviv_context_map(struct etnaviv_iommu_context *context,
 }
 
 static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
+			     unsigned int user_len,
 			     struct sg_table *sgt, int prot)
-{	struct scatterlist *sg;
+{
+	struct scatterlist *sg;
 	unsigned int da = iova;
 	unsigned int i;
 	int ret;
@@ -81,7 +83,8 @@  static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 
 	for_each_sgtable_dma_sg(sgt, sg, i) {
 		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
-		size_t bytes = sg_dma_len(sg) + sg->offset;
+		unsigned int phys_len = sg_dma_len(sg) + sg->offset;
+		size_t bytes = MIN(phys_len, user_len);
 
 		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
 
@@ -89,6 +92,7 @@  static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 		if (ret)
 			goto fail;
 
+		user_len -= bytes;
 		da += bytes;
 	}
 
@@ -104,21 +108,7 @@  static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
 				struct sg_table *sgt, unsigned len)
 {
-	struct scatterlist *sg;
-	unsigned int da = iova;
-	int i;
-
-	for_each_sgtable_dma_sg(sgt, sg, i) {
-		size_t bytes = sg_dma_len(sg) + sg->offset;
-
-		etnaviv_context_unmap(context, da, bytes);
-
-		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
-
-		BUG_ON(!PAGE_ALIGNED(bytes));
-
-		da += bytes;
-	}
+	etnaviv_context_unmap(context, iova, len);
 
 	context->flush_seq++;
 }
@@ -131,7 +121,7 @@  static void etnaviv_iommu_remove_mapping(struct etnaviv_iommu_context *context,
 	lockdep_assert_held(&context->lock);
 
 	etnaviv_iommu_unmap(context, mapping->vram_node.start,
-			    etnaviv_obj->sgt, etnaviv_obj->base.size);
+			    etnaviv_obj->sgt, etnaviv_obj->user_size);
 	drm_mm_remove_node(&mapping->vram_node);
 }
 
@@ -314,7 +304,7 @@  int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
 		goto unlock;
 
 	mapping->iova = node->start;
-	ret = etnaviv_iommu_map(context, node->start, sgt,
+	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,
 				ETNAVIV_PROT_READ | ETNAVIV_PROT_WRITE);
 
 	if (ret < 0) {