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

[Jason Wang]

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