Date:   Thu, 28 Jan 2021 14:14:33 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Yongji Xie <xieyongji@...edance.com>
Cc:     "Michael S. Tsirkin" <mst@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Parav Pandit <parav@...dia.com>, Bob Liu <bob.liu@...cle.com>,
        Christoph Hellwig <hch@...radead.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Matthew Wilcox <willy@...radead.org>, viro@...iv.linux.org.uk,
        Jens Axboe <axboe@...nel.dk>, bcrl@...ck.org,
        Jonathan Corbet <corbet@....net>,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        kvm@...r.kernel.org, linux-aio@...ck.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: [RFC v3 08/11] vduse: Introduce VDUSE - vDPA Device in Userspace


On 2021/1/28 下午2:03, Yongji Xie wrote:
>>>>> +
>>>>> +static const struct file_operations vduse_domain_fops = {
>>>>> +     .mmap = vduse_domain_mmap,
>>>>> +     .release = vduse_domain_release,
>>>>> +};
>>>> It's better to explain the reason for introducing a dedicated file for
>>>> mmap() here.
>>>>
>>> To make the implementation of iova_domain independent with vduse_dev.
>> My understanding is that, the only usage for this is to:
>>
>> 1) support different type of iova mappings
>> 2) or switch between iova domain mappings
>>
>> But I can't think of a need for this.
>>
> For example, share one iova_domain between several vduse devices.


Interesting.


>
> And it will be helpful if we want to split this patch into iova domain
> part and vduse device part. Because the page fault handler should be
> paired with dma_map/dma_unmap.


Ok.

[...]


>
>>>> This looks not safe, let's use idr here.
>>>>
>>> Could you give more details? Looks like idr should not used in this
>>> case which can not tolerate failure. And using a list to store the msg
>>> is better than using idr when the msg needs to be re-inserted in some
>>> cases.
>> My understanding is the "unique" (probably need a better name) is a
>> token that is used to uniquely identify a message. The reply from
>> userspace is required to write with exact the same token(unique). IDR
>> seems better but consider we can hardly hit 64bit overflow, atomic might
>> be OK as well.
>>
>> Btw, under what case do we need to do "re-inserted"?
>>
> When userspace daemon receive the message but doesn't reply it before crash.


Do we have code to do this?

[...]


>
>>>> So we had multiple types of requests/responses, is this better to
>>>> introduce a queue based admin interface other than ioctl?
>>>>
>>> Sorry, I didn't get your point. What do you mean by queue-based admin
>>> interface? Virtqueue-based?
>> Yes, a queue(virtqueue). The commands could be passed through the queue.
>> (Just an idea, not sure it's worth)
>>
> I considered it before. But I found it still needs some extra works
> (setup eventfd, set vring base and so on) to setup the admin virtqueue
> before using it for communication. So I turn to use this simple way.


Yes. We might consider it in the future.

[...]


>
>>>> Any reason for such IOTLB invalidation here?
>>>>
>>> As I mentioned before, this is used to notify userspace to update the
>>> IOTLB. Mainly for virtio-vdpa case.
>> So the question is, usually, there could be several times of status
>> setting during driver initialization. Do we really need to update IOTLB
>> every time?
>>
> I think we can check whether there are some changes after the last
> IOTLB updating here.


So the question still, except reset (write 0), any other status that can 
affect IOTLB?

[...]

>
>> Something like swiotlb default value (64M)?
>>
> Do we need a module parameter to change it?


We can.

[...]

>
>>>>> +     union {
>>>>> +             struct vduse_vq_num vq_num; /* virtqueue num */
>>>>> +             struct vduse_vq_addr vq_addr; /* virtqueue address */
>>>>> +             struct vduse_vq_ready vq_ready; /* virtqueue ready status */
>>>>> +             struct vduse_vq_state vq_state; /* virtqueue state */
>>>>> +             struct vduse_dev_config_data config; /* virtio device config space */
>>>>> +             struct vduse_iova_range iova; /* iova range for updating */
>>>>> +             __u64 features; /* virtio features */
>>>>> +             __u8 status; /* device status */
>>>> Let's add some padding for future extensions.
>>>>
>>> Is sizeof(vduse_dev_config_data) ok? Or char[1024]?
>> 1024 seems too large, 128 or 256 looks better.
>>
> If so, sizeof(vduse_dev_config_data) is enough.


Ok if we don't need a message more than that in the future.

Thanks

Powered by blists - more mailing lists