Re: [RFC v3 08/11] vduse: Introduce VDUSE - vDPA Device in Userspace

On 2021/1/28 下午2:03, Yongji Xie wrote:

quoted

quoted

quoted

quoted

+
+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.

[...]

quoted

quoted

quoted

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?

[...]

quoted

quoted

quoted

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.

[...]

quoted

quoted

quoted

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?

[...]

quoted

Something like swiotlb default value (64M)?

Do we need a module parameter to change it?

We can.

[...]

quoted

quoted

quoted

quoted

+     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