RE: [PATCH linux-next v2 03/14] vdpa: Introduce and use vdpa device get,set config, features helpers

[Parav Pandit <parav@nvidia.com>]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, April 7, 2021 8:48 AM
[..]
> > +/**
> > + * vdpa_set_features - Set vDPA device features
> > + * @vdev: vdpa device whose features to set
> > + * @features: features of the vdpa device to enable/disable
> > + *
> > + * Return: Returns 0 on success or error code.
> > + */
> > +int vdpa_set_features(struct vdpa_device *vdev, u64 features) {
> > +	const struct vdpa_config_ops *ops = vdev->config;
> > +
> > +	vdev->features_valid = true;
> > +	return ops->set_features(vdev, features); }
> > +EXPORT_SYMBOL_GPL(vdpa_set_features);
> > +
> 
> 
> Let's add a doc for this function?
Kdoc comment is added above the function in this patch.

> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> 
> 
> We probably need to convert drivers/virtio/vdpa.c as well.
Yes, will do.

> 
> 
> > @@ -236,7 +236,6 @@ static long vhost_vdpa_set_config(struct
> vhost_vdpa *v,
> >   				  struct vhost_vdpa_config __user *c)
> >   {
> >   	struct vdpa_device *vdpa = v->vdpa;
> > -	const struct vdpa_config_ops *ops = vdpa->config;
> >   	struct vhost_vdpa_config config;
> >   	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
> >   	u8 *buf;
> > @@ -250,7 +249,7 @@ static long vhost_vdpa_set_config(struct
> vhost_vdpa *v,
> >   	if (IS_ERR(buf))
> >   		return PTR_ERR(buf);
> >
> > -	ops->set_config(vdpa, config.off, buf, config.len);
> > +	vdpa_set_config(vdpa, config.off, buf, config.len);
> >
> >   	kvfree(buf);
> >   	return 0;
> > @@ -259,10 +258,9 @@ static long vhost_vdpa_set_config(struct
> vhost_vdpa *v,
> >   static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user
> *featurep)
> >   {
> >   	struct vdpa_device *vdpa = v->vdpa;
> > -	const struct vdpa_config_ops *ops = vdpa->config;
> >   	u64 features;
> >
> > -	features = ops->get_features(vdpa);
> > +	features = vdpa_get_features(vdpa);
> >
> >   	if (copy_to_user(featurep, &features, sizeof(features)))
> >   		return -EFAULT;
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h index
> > 37b65ca940cf..62e68ccc4c96 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -320,28 +320,12 @@ static inline void vdpa_reset(struct vdpa_device
> *vdev)
> >           ops->set_status(vdev, 0);
> >   }
> >
> > -static inline int vdpa_set_features(struct vdpa_device *vdev, u64
> > features) -{
> > -        const struct vdpa_config_ops *ops = vdev->config;
> > -
> > -	vdev->features_valid = true;
> > -        return ops->set_features(vdev, features);
> > -}
> > -
> > -
> > -static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned
> offset,
> > -				   void *buf, unsigned int len)
> > -{
> > -        const struct vdpa_config_ops *ops = vdev->config;
> > -
> > -	/*
> > -	 * Config accesses aren't supposed to trigger before features are set.
> > -	 * If it does happen we assume a legacy guest.
> > -	 */
> > -	if (!vdev->features_valid)
> > -		vdpa_set_features(vdev, 0);
> > -	ops->get_config(vdev, offset, buf, len);
> > -}
> > +u64 vdpa_get_features(struct vdpa_device *vdev); int
> > +vdpa_set_features(struct vdpa_device *vdev, u64 features); void
> > +vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
> > +		     void *buf, unsigned int len);
> > +void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
> > +		     void *buf, unsigned int length);
> 
> 
> I think it's better to use a separate patch for this moving.
> 
Ok. will have prepatch to this one for movement.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization