Re: [PATCH RFC 02/12] vdpa: split vdpasim to core and net modules

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2020/11/18 下午9:14, Stefano Garzarella wrote:
Hi Jason,
I just discovered that I missed the other questions in this email,
sorry for that!


No problem :)



On Mon, Nov 16, 2020 at 12:00:11PM +0800, Jason Wang wrote:

On 2020/11/13 下午9:47, Stefano Garzarella wrote:
From: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>

Introduce new vdpa_sim_net and vdpa_sim (core) drivers. This is a
preparation for adding a vdpa simulator module for block devices.

Signed-off-by: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
---
v1:
- Removed unused headers
- Removed empty module_init() module_exit()
- Moved vdpasim_is_little_endian() in vdpa_sim.h
- Moved vdpasim16_to_cpu/cpu_to_vdpasim16() in vdpa_sim.h
- Added vdpasim*_to_cpu/cpu_to_vdpasim*() also for 32 and 64
- Replaced 'select VDPA_SIM' with 'depends on VDPA_SIM' since selected
  option can not depend on other [Jason]


If possible, I would suggest to split this patch further:

1) convert to use void *config, and an attribute for setting config size during allocation
2) introduce supported_features
3) other attributes (#vqs)
4) rename config ops (more generic one)
5) introduce ops for set|get_config, set_get_features
6) real split



[...]

-static const struct vdpa_config_ops vdpasim_net_config_ops;
-static const struct vdpa_config_ops vdpasim_net_batch_config_ops;
+static const struct vdpa_config_ops vdpasim_config_ops;
+static const struct vdpa_config_ops vdpasim_batch_config_ops;
-static struct vdpasim *vdpasim_create(void)
+struct vdpasim *vdpasim_create(struct vdpasim_init_attr *attr)
 {
     const struct vdpa_config_ops *ops;
     struct vdpasim *vdpasim;
+    u32 device_id;
     struct device *dev;
-    int ret = -ENOMEM;
+    int i, size, ret = -ENOMEM;
-    if (batch_mapping)
-        ops = &vdpasim_net_batch_config_ops;
+    device_id = attr->device_id;
+    /* Currently, we only accept the network and block devices. */
+    if (device_id != VIRTIO_ID_NET && device_id != VIRTIO_ID_BLOCK)
+        return ERR_PTR(-EOPNOTSUPP);
+
+    if (attr->batch_mapping)
+        ops = &vdpasim_batch_config_ops;
     else
-        ops = &vdpasim_net_config_ops;
+        ops = &vdpasim_config_ops;
     vdpasim = vdpa_alloc_device(struct vdpasim, vdpa, NULL, ops, VDPASIM_VQ_NUM);
     if (!vdpasim)
         goto err_alloc;
-    INIT_WORK(&vdpasim->work, vdpasim_work);
+    if (device_id == VIRTIO_ID_NET)
+        size = sizeof(struct virtio_net_config);
+    else
+        size = sizeof(struct virtio_blk_config);


It's better to avoid such if/else consider we may introduce more type of devices.

Can we have an attribute of config size instead?

Yes, I'll move the patch 7 before this.

About config size and set/get_config ops, I'm not sure if it is better to hidden everything under the new set/get_config ops, allocating the config structure in each device, or leave the allocation in the core and update it like now.


I think we'd better to avoid having any type specific codes in generic sim codes.


[...]


+config VDPA_SIM_NET
+    tristate "vDPA simulator for networking device"
+    depends on VDPA_SIM
+    default n


I remember somebody told me that if we don't enable a module it was disabled by default.

So, should I remove "default n" from vdpa_sim* entries?


Yes, but please do that in another patch.

Thanks



Thanks,
Stefano


_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux