[PATCH] drm/amdkfd: Check cgroup when returning DMABuf info

[Joshi, Mukul]

[AMD Official Use Only - General]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Wu,
> David
> Sent: Wednesday, March 20, 2024 8:02 PM
> To: amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdkfd: Check cgroup when returning DMABuf info
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 3/20/2024 4:21 PM, Felix Kuehling wrote:
> > On 2024-03-18 16:12, Felix Kuehling wrote:
> >>
> >> On 2024-03-15 14:17, Mukul Joshi wrote:
> >>> Check cgroup permissions when returning DMA-buf info and based on
> >>> cgroup check return the id of the GPU that has access to the BO.
> >>>
> >>> Signed-off-by: Mukul Joshi <mukul.joshi at amd.com>
> >>> ---
> >>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> >>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> >>> index dfa8c69532d4..f9631f4b1a02 100644
> >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> >>> @@ -1523,7 +1523,7 @@ static int kfd_ioctl_get_dmabuf_info(struct
> >>> file *filep,
> >>>         /* Find a KFD GPU device that supports the get_dmabuf_info
> >>> query */
> >>>       for (i = 0; kfd_topology_enum_kfd_devices(i, &dev) == 0; i++)
> >>> -        if (dev)
> >>> +        if (dev && !kfd_devcgroup_check_permission(dev))
> >>>               break;
> >>>       if (!dev)
> >>>           return -EINVAL;
> >>> @@ -1545,7 +1545,7 @@ static int kfd_ioctl_get_dmabuf_info(struct
> >>> file *filep,
> >>>       if (xcp_id >= 0)
> >>>           args->gpu_id = dmabuf_adev->kfd.dev->nodes[xcp_id]->id;
> >>>       else
> >>> -        args->gpu_id = dmabuf_adev->kfd.dev->nodes[0]->id;
> >>> +        args->gpu_id = dev->id;
> >>
> >> If I remember correctly, this was meant as a fallback in case for GTT
> >> BOs where the exporting partition wasn't known and the application
> >> didn't have access to the first partition. I think the way you wrote
> >> this, it could also change the behaviour (report the wrong GPU ID) on
> >> single-partition GPUs, which is probably not intended.
> >
> > Never mind. I double checked: On single-partition GPUs, bo->xcp_id
> > always seems to be 0. So your code won't change the behaviour here.
> > The patch is
> should bo->xcp_id be >= 0 for all cases? if yes then I think all tests can be
> moved. (like below)
> args->gpu_id = dmabuf_adev->kfd.dev->nodes[xcp_id]->id;
>
No it can be -1 on GPUs that support spatial partitioning, especially for GTT BOs.

Regards,
Mukul

> David
> >
> > Reviewed-by: Felix Kuehling <felix.kuehling at amd.com>
> >
> >
> >
> >> Maybe this would preserve the behaviour for that case:
> >>
> >>     ...
> >> -    else
> >> +    else if
> >> (!kfd_devcgroup_check_permission(dmabuf_adev->kfd.dev->nodes[0]))
> >>          args->gpu_id = dmabuf_adev->kfd.dev->nodes[0]->id;
> >> +    else
> >> +        args->gpu_id = dev->id;
> >>
> >> Or maybe a more general solution would make DMABuf import work when
> >> the exporter is really unknown or not even a GPU. This came up not so
> >> long ago in the context of interop with 3rd-party devices. This may
> >> require user mode changes as well.
> >>
> >> Regards,
> >>   Felix
> >>
> >>
> >>>       args->flags = flags;
> >>>         /* Copy metadata buffer to user mode */