[PATCH 0/1] Implement device_attach for virtio gpu

[PATCH 0/1] Implement device_attach for virtio gpu

[Julia Zhang]

To realize dGPU prime feature for virtio gpu, we are trying let dGPU import
vram object of virtio gpu. But this feature would finally call function
virtio_dma_buf_ops.device_attach(), which was set as drm_gem_map_attach().
drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
implemented, or else return ENOSYS. But virtio gpu driver has not
implemented it for vram object and actually vram object does not require
it. So this add a new implementation of device_attach() to call
drm_gem_map_attach() for shmem object and return 0 for vram object as it
actually did before the requirement was added.

Julia Zhang (1):
  drm/virtio: Implement device_attach

 drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/1] drm/virtio: Implement device_attach

[Julia Zhang]

drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
implemented, or else return ENOSYS. Virtio has no get_sg_table
implemented for vram object. To fix this, add a new device_attach to
call drm_gem_map_attach() for shmem object and return 0 for vram object
instead of calling drm_gem_map_attach for both of these two kinds of
object.

Signed-off-by: Julia Zhang <julia.zhang@amd.com>
---
 drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
index 44425f20d91a..f0b0ff6f3813 100644
--- a/drivers/gpu/drm/virtio/virtgpu_prime.c
+++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
@@ -71,6 +71,18 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
 	drm_gem_unmap_dma_buf(attach, sgt, dir);
 }
 
+static int virtgpu_gem_device_attach(struct dma_buf *dma_buf,
+				     struct dma_buf_attachment *attach)
+{
+	struct drm_gem_object *obj = attach->dmabuf->priv;
+	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
+
+	if (virtio_gpu_is_vram(bo))
+		return 0;
+
+	return drm_gem_map_attach(dma_buf, attach);
+}
+
 static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
 	.ops = {
 		.cache_sgt_mapping = true,
@@ -83,7 +95,7 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
 		.vmap = drm_gem_dmabuf_vmap,
 		.vunmap = drm_gem_dmabuf_vunmap,
 	},
-	.device_attach = drm_gem_map_attach,
+	.device_attach = virtgpu_gem_device_attach,
 	.get_uuid = virtgpu_virtio_get_uuid,
 };
 
-- 
2.34.1

Re: [PATCH 1/1] drm/virtio: Implement device_attach

[Christian König]

Am 10.01.24 um 10:56 schrieb Julia Zhang:
> drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
> implemented, or else return ENOSYS. Virtio has no get_sg_table
> implemented for vram object. To fix this, add a new device_attach to
> call drm_gem_map_attach() for shmem object and return 0 for vram object
> instead of calling drm_gem_map_attach for both of these two kinds of
> object.

Well as far as I can see this is nonsense from the DMA-buf side of things.

SG tables are always needed as long as you don't re-import the same 
object into your driver and then you shouldn't end up in this function 
in the first place.

So that drm_gem_map_attach() requires get_sg_table to be implemented is 
intentional and should never be overridden like this.

Regards,
Christian.

>
> Signed-off-by: Julia Zhang <julia.zhang@amd.com>
> ---
>   drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index 44425f20d91a..f0b0ff6f3813 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -71,6 +71,18 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>   	drm_gem_unmap_dma_buf(attach, sgt, dir);
>   }
>   
> +static int virtgpu_gem_device_attach(struct dma_buf *dma_buf,
> +				     struct dma_buf_attachment *attach)
> +{
> +	struct drm_gem_object *obj = attach->dmabuf->priv;
> +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> +
> +	if (virtio_gpu_is_vram(bo))
> +		return 0;
> +
> +	return drm_gem_map_attach(dma_buf, attach);
> +}
> +
>   static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>   	.ops = {
>   		.cache_sgt_mapping = true,
> @@ -83,7 +95,7 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>   		.vmap = drm_gem_dmabuf_vmap,
>   		.vunmap = drm_gem_dmabuf_vunmap,
>   	},
> -	.device_attach = drm_gem_map_attach,
> +	.device_attach = virtgpu_gem_device_attach,
>   	.get_uuid = virtgpu_virtio_get_uuid,
>   };
>   

Re: [PATCH 1/1] drm/virtio: Implement device_attach

[Daniel Vetter]

On Wed, Jan 10, 2024 at 11:19:37AM +0100, Christian König wrote:
> Am 10.01.24 um 10:56 schrieb Julia Zhang:
> > drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
> > implemented, or else return ENOSYS. Virtio has no get_sg_table
> > implemented for vram object. To fix this, add a new device_attach to
> > call drm_gem_map_attach() for shmem object and return 0 for vram object
> > instead of calling drm_gem_map_attach for both of these two kinds of
> > object.
> 
> Well as far as I can see this is nonsense from the DMA-buf side of things.
> 
> SG tables are always needed as long as you don't re-import the same object
> into your driver and then you shouldn't end up in this function in the first
> place.
> 
> So that drm_gem_map_attach() requires get_sg_table to be implemented is
> intentional and should never be overridden like this.

See my reply, tldr; you're allowed to reject ->attach with -EBUSY to
handle exactly this case of non-shareable buffer types. But definitely
don't silently fail, that's a "we'll oops on map_attachment" kind of bug
:-)
-Sima

> 
> Regards,
> Christian.
> 
> > 
> > Signed-off-by: Julia Zhang <julia.zhang@amd.com>
> > ---
> >   drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
> > index 44425f20d91a..f0b0ff6f3813 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> > @@ -71,6 +71,18 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
> >   	drm_gem_unmap_dma_buf(attach, sgt, dir);
> >   }
> > +static int virtgpu_gem_device_attach(struct dma_buf *dma_buf,
> > +				     struct dma_buf_attachment *attach)
> > +{
> > +	struct drm_gem_object *obj = attach->dmabuf->priv;
> > +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> > +
> > +	if (virtio_gpu_is_vram(bo))
> > +		return 0;
> > +
> > +	return drm_gem_map_attach(dma_buf, attach);
> > +}
> > +
> >   static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
> >   	.ops = {
> >   		.cache_sgt_mapping = true,
> > @@ -83,7 +95,7 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
> >   		.vmap = drm_gem_dmabuf_vmap,
> >   		.vunmap = drm_gem_dmabuf_vunmap,
> >   	},
> > -	.device_attach = drm_gem_map_attach,
> > +	.device_attach = virtgpu_gem_device_attach,
> >   	.get_uuid = virtgpu_virtio_get_uuid,
> >   };
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/1] drm/virtio: Implement device_attach

[Christian König]

Am 10.01.24 um 11:22 schrieb Daniel Vetter:
> On Wed, Jan 10, 2024 at 11:19:37AM +0100, Christian König wrote:
>> Am 10.01.24 um 10:56 schrieb Julia Zhang:
>>> drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
>>> implemented, or else return ENOSYS. Virtio has no get_sg_table
>>> implemented for vram object. To fix this, add a new device_attach to
>>> call drm_gem_map_attach() for shmem object and return 0 for vram object
>>> instead of calling drm_gem_map_attach for both of these two kinds of
>>> object.
>> Well as far as I can see this is nonsense from the DMA-buf side of things.
>>
>> SG tables are always needed as long as you don't re-import the same object
>> into your driver and then you shouldn't end up in this function in the first
>> place.
>>
>> So that drm_gem_map_attach() requires get_sg_table to be implemented is
>> intentional and should never be overridden like this.
> See my reply, tldr; you're allowed to reject ->attach with -EBUSY to
> handle exactly this case of non-shareable buffer types. But definitely
> don't silently fail, that's a "we'll oops on map_attachment" kind of bug
> :-)

Ah, yes that makes much more sense!

So basically just the "return 0;" needs to be "return -EBUSY;".

Regards,
Christian.

> -Sima
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Julia Zhang <julia.zhang@amd.com>
>>> ---
>>>    drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
>>>    1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
>>> index 44425f20d91a..f0b0ff6f3813 100644
>>> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
>>> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
>>> @@ -71,6 +71,18 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>>>    	drm_gem_unmap_dma_buf(attach, sgt, dir);
>>>    }
>>> +static int virtgpu_gem_device_attach(struct dma_buf *dma_buf,
>>> +				     struct dma_buf_attachment *attach)
>>> +{
>>> +	struct drm_gem_object *obj = attach->dmabuf->priv;
>>> +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
>>> +
>>> +	if (virtio_gpu_is_vram(bo))
>>> +		return 0;
>>> +
>>> +	return drm_gem_map_attach(dma_buf, attach);
>>> +}
>>> +
>>>    static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>>>    	.ops = {
>>>    		.cache_sgt_mapping = true,
>>> @@ -83,7 +95,7 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>>>    		.vmap = drm_gem_dmabuf_vmap,
>>>    		.vunmap = drm_gem_dmabuf_vunmap,
>>>    	},
>>> -	.device_attach = drm_gem_map_attach,
>>> +	.device_attach = virtgpu_gem_device_attach,
>>>    	.get_uuid = virtgpu_virtio_get_uuid,
>>>    };

Re: [PATCH 1/1] drm/virtio: Implement device_attach

[Daniel Vetter]

On Wed, Jan 10, 2024 at 11:46:35AM +0100, Christian König wrote:
> Am 10.01.24 um 11:22 schrieb Daniel Vetter:
> > On Wed, Jan 10, 2024 at 11:19:37AM +0100, Christian König wrote:
> > > Am 10.01.24 um 10:56 schrieb Julia Zhang:
> > > > drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
> > > > implemented, or else return ENOSYS. Virtio has no get_sg_table
> > > > implemented for vram object. To fix this, add a new device_attach to
> > > > call drm_gem_map_attach() for shmem object and return 0 for vram object
> > > > instead of calling drm_gem_map_attach for both of these two kinds of
> > > > object.
> > > Well as far as I can see this is nonsense from the DMA-buf side of things.
> > > 
> > > SG tables are always needed as long as you don't re-import the same object
> > > into your driver and then you shouldn't end up in this function in the first
> > > place.
> > > 
> > > So that drm_gem_map_attach() requires get_sg_table to be implemented is
> > > intentional and should never be overridden like this.
> > See my reply, tldr; you're allowed to reject ->attach with -EBUSY to
> > handle exactly this case of non-shareable buffer types. But definitely
> > don't silently fail, that's a "we'll oops on map_attachment" kind of bug
> > :-)
> 
> Ah, yes that makes much more sense!
> 
> So basically just the "return 0;" needs to be "return -EBUSY;".

Well plus 2nd patch to polish the virtio_dma_buf docs a bit, that would be
nice :-D
-Sima

> 
> Regards,
> Christian.
> 
> > -Sima
> > 
> > > Regards,
> > > Christian.
> > > 
> > > > Signed-off-by: Julia Zhang <julia.zhang@amd.com>
> > > > ---
> > > >    drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
> > > >    1 file changed, 13 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
> > > > index 44425f20d91a..f0b0ff6f3813 100644
> > > > --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> > > > +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> > > > @@ -71,6 +71,18 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
> > > >    	drm_gem_unmap_dma_buf(attach, sgt, dir);
> > > >    }
> > > > +static int virtgpu_gem_device_attach(struct dma_buf *dma_buf,
> > > > +				     struct dma_buf_attachment *attach)
> > > > +{
> > > > +	struct drm_gem_object *obj = attach->dmabuf->priv;
> > > > +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> > > > +
> > > > +	if (virtio_gpu_is_vram(bo))
> > > > +		return 0;
> > > > +
> > > > +	return drm_gem_map_attach(dma_buf, attach);
> > > > +}
> > > > +
> > > >    static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
> > > >    	.ops = {
> > > >    		.cache_sgt_mapping = true,
> > > > @@ -83,7 +95,7 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
> > > >    		.vmap = drm_gem_dmabuf_vmap,
> > > >    		.vunmap = drm_gem_dmabuf_vunmap,
> > > >    	},
> > > > -	.device_attach = drm_gem_map_attach,
> > > > +	.device_attach = virtgpu_gem_device_attach,
> > > >    	.get_uuid = virtgpu_virtio_get_uuid,
> > > >    };
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/1] drm/virtio: Implement device_attach

[Daniel Vetter]

On Wed, Jan 10, 2024 at 05:56:28PM +0800, Julia Zhang wrote:
> drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
> implemented, or else return ENOSYS. Virtio has no get_sg_table
> implemented for vram object. To fix this, add a new device_attach to
> call drm_gem_map_attach() for shmem object and return 0 for vram object
> instead of calling drm_gem_map_attach for both of these two kinds of
> object.
> 
> Signed-off-by: Julia Zhang <julia.zhang@amd.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index 44425f20d91a..f0b0ff6f3813 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -71,6 +71,18 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>  	drm_gem_unmap_dma_buf(attach, sgt, dir);
>  }
>  
> +static int virtgpu_gem_device_attach(struct dma_buf *dma_buf,
> +				     struct dma_buf_attachment *attach)
> +{
> +	struct drm_gem_object *obj = attach->dmabuf->priv;
> +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> +
> +	if (virtio_gpu_is_vram(bo))
> +		return 0;

You need to reject attach here because these vram buffer objects cannot be
used by any other driver. In that case dma_buf_attach _must_ fail, not
silently succeed.

Because if it silently succeeds then the subsequent dma_buf_map_attachment
will blow up because you don't have the ->get_sg_table hook implemented.

Per the documentation the error code for this case must be -EBUSY, see the
section for the attach hook here:

https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html#c.dma_buf_ops

Since you're looking into this area, please make sure there's not other
similar mistake in virtio code.

Also can you please make a kerneldoc patch for struct virtio_dma_buf_ops
to improve the documentation there? I think it would be good to move those
to the inline style and then at least put a kernel-doc hyperlink to struct
dma_buf_ops.attach and mention that attach must fail for non-shareable
buffers.

In general the virtio_dma_buf kerneldoc seems to be on the "too minimal,
explains nothing" side of things :-/

Cheers, Sima

> +
> +	return drm_gem_map_attach(dma_buf, attach);
> +}
> +
>  static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>  	.ops = {
>  		.cache_sgt_mapping = true,
> @@ -83,7 +95,7 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>  		.vmap = drm_gem_dmabuf_vmap,
>  		.vunmap = drm_gem_dmabuf_vunmap,
>  	},
> -	.device_attach = drm_gem_map_attach,
> +	.device_attach = virtgpu_gem_device_attach,
>  	.get_uuid = virtgpu_virtio_get_uuid,
>  };
>  
> -- 
> 2.34.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/1] drm/virtio: Implement device_attach

[Zhang, Julia]

On 2024/1/10 18:21, Daniel Vetter wrote:
> On Wed, Jan 10, 2024 at 05:56:28PM +0800, Julia Zhang wrote:
>> drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
>> implemented, or else return ENOSYS. Virtio has no get_sg_table
>> implemented for vram object. To fix this, add a new device_attach to
>> call drm_gem_map_attach() for shmem object and return 0 for vram object
>> instead of calling drm_gem_map_attach for both of these two kinds of
>> object.
>>
>> Signed-off-by: Julia Zhang <julia.zhang@amd.com>
>> ---
>>  drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
>> index 44425f20d91a..f0b0ff6f3813 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
>> @@ -71,6 +71,18 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>>  	drm_gem_unmap_dma_buf(attach, sgt, dir);
>>  }
>>  
>> +static int virtgpu_gem_device_attach(struct dma_buf *dma_buf,
>> +				     struct dma_buf_attachment *attach)
>> +{
>> +	struct drm_gem_object *obj = attach->dmabuf->priv;
>> +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
>> +
>> +	if (virtio_gpu_is_vram(bo))
>> +		return 0;
> 
> You need to reject attach here because these vram buffer objects cannot be
> used by any other driver. In that case dma_buf_attach _must_ fail, not
> silently succeed.
> 

Do you mean these vram buffer objects should not be imported by other drivers?

> Because if it silently succeeds then the subsequent dma_buf_map_attachment
> will blow up because you don't have the ->get_sg_table hook implemented.
> 

I saw only this call stack would call ->get_sg_table:

#0 ->get_sg_table
#1 drm_gem_map_dma_buf
#2 virtgpu_gem_map_dma_buf
#3 __map_dma_buf
#4 dma_buf_dynamic_attach
#5 amdgpu_gem_prime_import

this stack is for shmem object and it requires ->get_sg_table get implemented.


But for vram object, __map_dma_buf will call like this:

#0 sg_alloc_table
#1 virtio_gpu_vram_map_dma_buf
#2 virtgpu_gem_map_dma_buf
#3 __map_dma_buf
#4 dma_buf_dynamic_attach
#5 amdgpu_gem_prime_import

which will not call ->get_sg_table but alloc a sg table instead and fill it from the vram object.

Before __map_dma_buf, the call stack of virtgpu_gem_device_attach is:

#0 virtgpu_gem_device_attach
#1 virtio_dma_buf_attach
#2 dma_buf_dynamic_attach
#3 amdgpu_gem_prime_import

So my problem is that to realize dGPU prime feature on VM, I actually want dma_buf_attach succeed
for vram object so that passthrough dGPU can import these vram objects and blit data to it. 
If here return -EBUSY, then amdgpu_gem_prime_import will fail and the whole feature will fail.

> Per the documentation the error code for this case must be -EBUSY, see the
> section for the attach hook here:
> 
> https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html#c.dma_buf_ops
> 
> Since you're looking into this area, please make sure there's not other
> similar mistake in virtio code.
> 
> Also can you please make a kerneldoc patch for struct virtio_dma_buf_ops
> to improve the documentation there? I think it would be good to move those
> to the inline style and then at least put a kernel-doc hyperlink to struct
> dma_buf_ops.attach and mention that attach must fail for non-shareable
> buffers.
> 
> In general the virtio_dma_buf kerneldoc seems to be on the "too minimal,
> explains nothing" side of things :-/

OK, let me take a look and try to do it.

Regards,
Julia

> 
> Cheers, Sima
> 
>> +
>> +	return drm_gem_map_attach(dma_buf, attach);
>> +}
>> +
>>  static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>>  	.ops = {
>>  		.cache_sgt_mapping = true,
>> @@ -83,7 +95,7 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>>  		.vmap = drm_gem_dmabuf_vmap,
>>  		.vunmap = drm_gem_dmabuf_vunmap,
>>  	},
>> -	.device_attach = drm_gem_map_attach,
>> +	.device_attach = virtgpu_gem_device_attach,
>>  	.get_uuid = virtgpu_virtio_get_uuid,
>>  };
>>  
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 1/1] drm/virtio: Implement device_attach

[Christian König]

Am 11.01.24 um 09:52 schrieb Zhang, Julia:
>
> On 2024/1/10 18:21, Daniel Vetter wrote:
>> On Wed, Jan 10, 2024 at 05:56:28PM +0800, Julia Zhang wrote:
>>> drm_gem_map_attach() requires drm_gem_object_funcs.get_sg_table to be
>>> implemented, or else return ENOSYS. Virtio has no get_sg_table
>>> implemented for vram object. To fix this, add a new device_attach to
>>> call drm_gem_map_attach() for shmem object and return 0 for vram object
>>> instead of calling drm_gem_map_attach for both of these two kinds of
>>> object.
>>>
>>> Signed-off-by: Julia Zhang <julia.zhang@amd.com>
>>> ---
>>>   drivers/gpu/drm/virtio/virtgpu_prime.c | 14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
>>> index 44425f20d91a..f0b0ff6f3813 100644
>>> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
>>> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
>>> @@ -71,6 +71,18 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>>>   	drm_gem_unmap_dma_buf(attach, sgt, dir);
>>>   }
>>>   
>>> +static int virtgpu_gem_device_attach(struct dma_buf *dma_buf,
>>> +				     struct dma_buf_attachment *attach)
>>> +{
>>> +	struct drm_gem_object *obj = attach->dmabuf->priv;
>>> +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
>>> +
>>> +	if (virtio_gpu_is_vram(bo))
>>> +		return 0;
>> You need to reject attach here because these vram buffer objects cannot be
>> used by any other driver. In that case dma_buf_attach _must_ fail, not
>> silently succeed.
>>
> Do you mean these vram buffer objects should not be imported by other drivers?
>
>> Because if it silently succeeds then the subsequent dma_buf_map_attachment
>> will blow up because you don't have the ->get_sg_table hook implemented.
>>
> I saw only this call stack would call ->get_sg_table:
>
> #0 ->get_sg_table
> #1 drm_gem_map_dma_buf
> #2 virtgpu_gem_map_dma_buf
> #3 __map_dma_buf
> #4 dma_buf_dynamic_attach
> #5 amdgpu_gem_prime_import
>
> this stack is for shmem object and it requires ->get_sg_table get implemented.
>
>
> But for vram object, __map_dma_buf will call like this:
>
> #0 sg_alloc_table
> #1 virtio_gpu_vram_map_dma_buf
> #2 virtgpu_gem_map_dma_buf
> #3 __map_dma_buf
> #4 dma_buf_dynamic_attach
> #5 amdgpu_gem_prime_import
>
> which will not call ->get_sg_table but alloc a sg table instead and fill it from the vram object.

Yeah and exactly that won't work for this use case.

The VRAM of amdgpu is exposed through an MMIO BAR the guest can't access.

> Before __map_dma_buf, the call stack of virtgpu_gem_device_attach is:
>
> #0 virtgpu_gem_device_attach
> #1 virtio_dma_buf_attach
> #2 dma_buf_dynamic_attach
> #3 amdgpu_gem_prime_import
>
> So my problem is that to realize dGPU prime feature on VM, I actually want dma_buf_attach succeed
> for vram object so that passthrough dGPU can import these vram objects and blit data to it.

That is completely futile effort, the dGPU physically can't access that 
stuff or otherwise we have a major security hole in the VM.

> If here return -EBUSY, then amdgpu_gem_prime_import will fail and the whole feature will fail.

Yeah and that is completely intentional. Let's discuss that use case AMD 
internally first.

Regards,
Christian.

>
>> Per the documentation the error code for this case must be -EBUSY, see the
>> section for the attach hook here:
>>
>> https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html#c.dma_buf_ops
>>
>> Since you're looking into this area, please make sure there's not other
>> similar mistake in virtio code.
>>
>> Also can you please make a kerneldoc patch for struct virtio_dma_buf_ops
>> to improve the documentation there? I think it would be good to move those
>> to the inline style and then at least put a kernel-doc hyperlink to struct
>> dma_buf_ops.attach and mention that attach must fail for non-shareable
>> buffers.
>>
>> In general the virtio_dma_buf kerneldoc seems to be on the "too minimal,
>> explains nothing" side of things :-/
> OK, let me take a look and try to do it.
>
> Regards,
> Julia
>
>> Cheers, Sima
>>
>>> +
>>> +	return drm_gem_map_attach(dma_buf, attach);
>>> +}
>>> +
>>>   static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>>>   	.ops = {
>>>   		.cache_sgt_mapping = true,
>>> @@ -83,7 +95,7 @@ static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops =  {
>>>   		.vmap = drm_gem_dmabuf_vmap,
>>>   		.vunmap = drm_gem_dmabuf_vunmap,
>>>   	},
>>> -	.device_attach = drm_gem_map_attach,
>>> +	.device_attach = virtgpu_gem_device_attach,
>>>   	.get_uuid = virtgpu_virtio_get_uuid,
>>>   };
>>>   
>>> -- 
>>> 2.34.1
>>>