Stefano Garzarella
2021-Mar-02 14:15 UTC
[RFC PATCH 01/10] vdpa: add get_config_size callback in vdpa_config_ops
On Tue, Mar 02, 2021 at 12:14:13PM +0800, Jason Wang wrote:> >On 2021/2/16 5:44 ??, Stefano Garzarella wrote: >>This new callback is used to get the size of the configuration space >>of vDPA devices. >> >>Signed-off-by: Stefano Garzarella <sgarzare at redhat.com> >>--- >> include/linux/vdpa.h | 4 ++++ >> drivers/vdpa/ifcvf/ifcvf_main.c | 6 ++++++ >> drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++++++ >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 9 +++++++++ >> 4 files changed, 25 insertions(+) >> >>diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h >>index 4ab5494503a8..fddf42b17573 100644 >>--- a/include/linux/vdpa.h >>+++ b/include/linux/vdpa.h >>@@ -150,6 +150,9 @@ struct vdpa_iova_range { >> * @set_status: Set the device status >> * @vdev: vdpa device >> * @status: virtio device status >>+ * @get_config_size: Get the size of the configuration space >>+ * @vdev: vdpa device >>+ * Returns size_t: configuration size > > >Rethink about this, how much we could gain by introducing a dedicated >ops here? E.g would it be simpler if we simply introduce a >max_config_size to vdpa device?Mainly because in this way we don't have to add new parameters to the vdpa_alloc_device() function. We do the same for example for 'get_device_id', 'get_vendor_id', 'get_vq_num_max'. All of these are usually static, but we have ops. I think because it's easier to extend. I don't know if it's worth adding a new structure for these static values at this point, like 'struct vdpa_config_params'. Thanks, Stefano
Jason Wang
2021-Mar-04 08:34 UTC
[RFC PATCH 01/10] vdpa: add get_config_size callback in vdpa_config_ops
On 2021/3/2 10:15 ??, Stefano Garzarella wrote:> On Tue, Mar 02, 2021 at 12:14:13PM +0800, Jason Wang wrote: >> >> On 2021/2/16 5:44 ??, Stefano Garzarella wrote: >>> This new callback is used to get the size of the configuration space >>> of vDPA devices. >>> >>> Signed-off-by: Stefano Garzarella <sgarzare at redhat.com> >>> --- >>> ?include/linux/vdpa.h????????????? | 4 ++++ >>> ?drivers/vdpa/ifcvf/ifcvf_main.c?? | 6 ++++++ >>> ?drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++++++ >>> ?drivers/vdpa/vdpa_sim/vdpa_sim.c? | 9 +++++++++ >>> ?4 files changed, 25 insertions(+) >>> >>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h >>> index 4ab5494503a8..fddf42b17573 100644 >>> --- a/include/linux/vdpa.h >>> +++ b/include/linux/vdpa.h >>> @@ -150,6 +150,9 @@ struct vdpa_iova_range { >>> ? * @set_status:??????????? Set the device status >>> ? *??????????????? @vdev: vdpa device >>> ? *??????????????? @status: virtio device status >>> + * @get_config_size:??????? Get the size of the configuration space >>> + *??????????????? @vdev: vdpa device >>> + *??????????????? Returns size_t: configuration size >> >> >> Rethink about this, how much we could gain by introducing a dedicated >> ops here? E.g would it be simpler if we simply introduce a >> max_config_size to vdpa device? > > Mainly because in this way we don't have to add new parameters to the > vdpa_alloc_device() function. > > We do the same for example for 'get_device_id', 'get_vendor_id', > 'get_vq_num_max'. All of these are usually static, but we have ops. > I think because it's easier to extend. > > I don't know if it's worth adding a new structure for these static > values at this point, like 'struct vdpa_config_params'.Yes, that's the point. I think for any static values, it should be set during device allocation. I'm fine with both. Thanks> > Thanks, > Stefano >