Jason Wang
2021-Jan-28 06:14 UTC
[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