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

[Jason Wang <jasowang@redhat.com>]

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
>