Messages in this thread | | | Subject | Re: [PATCH RFC 02/12] vdpa: split vdpasim to core and net modules | From | Jason Wang <> | Date | Thu, 19 Nov 2020 14:16:06 +0800 |
| |
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@nvidia.com> >>> >>> 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@nvidia.com> >>> [sgarzare: various cleanups/fixes] >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>> --- >>> 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 >
| |