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@xxxxxxx>
> > > ---
> > >   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