Date:   Thu, 16 Mar 2023 09:17:25 +0100
From:   Stefano Garzarella <sgarzare@redhat.com>
To:     Jason Wang <jasowang@redhat.com>
Cc:     virtualization@lists.linux-foundation.org,
        Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>,
        eperezma@redhat.com, netdev@vger.kernel.org, stefanha@redhat.com,
        linux-kernel@vger.kernel.org,
        "Michael S. Tsirkin" <mst@redhat.com>, kvm@vger.kernel.org
Subject: Re: [PATCH v2 1/8] vdpa: add bind_mm/unbind_mm callbacks
Message-ID: <20230316081725.2gwfgptm3lkoptwt@sgarzare-redhat>
References: <20230302113421.174582-1-sgarzare@redhat.com>
 <20230302113421.174582-2-sgarzare@redhat.com>
 <CACGkMEv24Zw-OUbBBSne21pF7=4XCZ6JGj7Y_cC7cMFYTjbF1Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACGkMEv24Zw-OUbBBSne21pF7=4XCZ6JGj7Y_cC7cMFYTjbF1Q@mail.gmail.com>
Precedence: bulk

On Tue, Mar 14, 2023 at 11:39:42AM +0800, Jason Wang wrote:
>On Thu, Mar 2, 2023 at 7:34 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> These new optional callbacks is used to bind/unbind the device to
>> a specific address space so the vDPA framework can use VA when
>> these callbacks are implemented.
>>
>> Suggested-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>
>One thing that came into my mind is that after this commit:
>
>commit 5ce995f313ce56c0c62425c3ddc37c5c50fc33db
>Author: Jason Wang <jasowang@redhat.com>
>Date:   Fri May 29 16:02:59 2020 +0800
>
>    vhost: use mmgrab() instead of mmget() for non worker device
>
>    For the device that doesn't use vhost worker and use_mm(), mmget() is
>    too heavy weight and it may brings troubles for implementing mmap()
>    support for vDPA device.
>
>We don't hold the address space after this commit, so the userspace
>mapping could be invalid if the owner exits?

Thanks for mentioning it, I'll take a look at it!

In case maybe I should do a mmget (or get_task_mm) in vhost-vdpa before
calling the callback, or in the parent driver inside the callback, but
it seems duplicating code.

Thanks,
Stefano

>
>Thanks
>
>>
>> Notes:
>>     v2:
>>     - removed `struct task_struct *owner` param (unused for now, maybe
>>       useful to support cgroups) [Jason]
>>     - add unbind_mm callback [Jason]
>>
>>  include/linux/vdpa.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>> index 43f59ef10cc9..369c21394284 100644
>> --- a/include/linux/vdpa.h
>> +++ b/include/linux/vdpa.h
>> @@ -290,6 +290,14 @@ struct vdpa_map_file {
>>   *                             @vdev: vdpa device
>>   *                             @idx: virtqueue index
>>   *                             Returns pointer to structure device or error (NULL)
>> + * @bind_mm:                   Bind the device to a specific address space
>> + *                             so the vDPA framework can use VA when this
>> + *                             callback is implemented. (optional)
>> + *                             @vdev: vdpa device
>> + *                             @mm: address space to bind
>> + * @unbind_mm:                 Unbind the device from the address space
>> + *                             bound using the bind_mm callback. (optional)
>> + *                             @vdev: vdpa device
>>   * @free:                      Free resources that belongs to vDPA (optional)
>>   *                             @vdev: vdpa device
>>   */
>> @@ -351,6 +359,8 @@ struct vdpa_config_ops {
>>         int (*set_group_asid)(struct vdpa_device *vdev, unsigned int group,
>>                               unsigned int asid);
>>         struct device *(*get_vq_dma_dev)(struct vdpa_device *vdev, u16 idx);
>> +       int (*bind_mm)(struct vdpa_device *vdev, struct mm_struct *mm);
>> +       void (*unbind_mm)(struct vdpa_device *vdev);
>>
>>         /* Free device resources */
>>         void (*free)(struct vdpa_device *vdev);
>> --
>> 2.39.2
>>
>