Jason Wang
2022-Jan-07 05:08 UTC
[PATCH v7 03/14] vdpa: Sync calls set/get config/status with cf_mutex
? 2022/1/7 ??8:33, Si-Wei Liu ??:> > > On 1/5/2022 3:46 AM, Eli Cohen wrote: >> Add wrappers to get/set status and protect these operations with >> cf_mutex to serialize these operations with respect to get/set config >> operations. >> >> Signed-off-by: Eli Cohen <elic at nvidia.com> >> --- >> ? drivers/vdpa/vdpa.c????????? | 19 +++++++++++++++++++ >> ? drivers/vhost/vdpa.c???????? |? 7 +++---- >> ? drivers/virtio/virtio_vdpa.c |? 3 +-- >> ? include/linux/vdpa.h???????? |? 3 +++ >> ? 4 files changed, 26 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >> index 42d71d60d5dc..5134c83c4a22 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head); >> ? static DEFINE_MUTEX(vdpa_dev_mutex); >> ? static DEFINE_IDA(vdpa_index_ida); >> ? +u8 vdpa_get_status(struct vdpa_device *vdev) >> +{ >> +??? u8 status; >> + >> +??? mutex_lock(&vdev->cf_mutex); >> +??? status = vdev->config->get_status(vdev); >> +??? mutex_unlock(&vdev->cf_mutex); >> +??? return status; >> +} >> +EXPORT_SYMBOL(vdpa_get_status); >> + >> +void vdpa_set_status(struct vdpa_device *vdev, u8 status) >> +{ >> +??? mutex_lock(&vdev->cf_mutex); >> +??? vdev->config->set_status(vdev, status); >> +??? mutex_unlock(&vdev->cf_mutex); >> +} >> +EXPORT_SYMBOL(vdpa_set_status); >> + >> ? static struct genl_family vdpa_nl_family; >> ? ? static int vdpa_dev_probe(struct device *d) >> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >> index ebaa373e9b82..d9d499465e2e 100644 >> --- a/drivers/vhost/vdpa.c >> +++ b/drivers/vhost/vdpa.c >> @@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct >> vhost_vdpa *v, u8 __user *argp) >> ? static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user >> *statusp) >> ? { >> ????? struct vdpa_device *vdpa = v->vdpa; >> -??? const struct vdpa_config_ops *ops = vdpa->config; >> ????? u8 status; >> ? -??? status = ops->get_status(vdpa); >> +??? status = vdpa_get_status(vdpa); > Not sure why we need to take cf_mutex here. Appears to me only setters > (set_status and reset) need to take the lock in this function.You may be right but it doesn't harm and it is guaranteed to be correct if we protect it with mutex here. Thanks> >> ? ????? if (copy_to_user(statusp, &status, sizeof(status))) >> ????????? return -EFAULT; >> @@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct >> vhost_vdpa *v, u8 __user *statusp) >> ????? if (copy_from_user(&status, statusp, sizeof(status))) >> ????????? return -EFAULT; >> ? -??? status_old = ops->get_status(vdpa); >> +??? status_old = vdpa_get_status(vdpa); > Ditto. > >> ? ????? /* >> ?????? * Userspace shouldn't remove status bits unless reset the >> @@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct >> vhost_vdpa *v, u8 __user *statusp) >> ????????? if (ret) >> ????????????? return ret; >> ????? } else >> -??????? ops->set_status(vdpa, status); >> +??????? vdpa_set_status(vdpa, status); > The reset() call in the if branch above needs to take the cf_mutex, > the same way as that for set_status(). The reset() is effectively > set_status(vdpa, 0). > > Thanks, > -Siwei > >> ? ????? if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & >> VIRTIO_CONFIG_S_DRIVER_OK)) >> ????????? for (i = 0; i < nvqs; i++) >> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c >> index a84b04ba3195..76504559bc25 100644 >> --- a/drivers/virtio/virtio_vdpa.c >> +++ b/drivers/virtio/virtio_vdpa.c >> @@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct >> virtio_device *vdev) >> ? static void virtio_vdpa_set_status(struct virtio_device *vdev, u8 >> status) >> ? { >> ????? struct vdpa_device *vdpa = vd_get_vdpa(vdev); >> -??? const struct vdpa_config_ops *ops = vdpa->config; >> ? -??? return ops->set_status(vdpa, status); >> +??? return vdpa_set_status(vdpa, status); >> ? } >> ? ? static void virtio_vdpa_reset(struct virtio_device *vdev) >> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h >> index 9cc4291a79b3..ae047fae2603 100644 >> --- a/include/linux/vdpa.h >> +++ b/include/linux/vdpa.h >> @@ -408,6 +408,9 @@ 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, >> ?????????????? const void *buf, unsigned int length); >> +u8 vdpa_get_status(struct vdpa_device *vdev); >> +void vdpa_set_status(struct vdpa_device *vdev, u8 status); >> + >> ? /** >> ?? * struct vdpa_mgmtdev_ops - vdpa device ops >> ?? * @dev_add: Add a vdpa device using alloc and register >
Si-Wei Liu
2022-Jan-08 01:23 UTC
[PATCH v7 03/14] vdpa: Sync calls set/get config/status with cf_mutex
On 1/6/2022 9:08 PM, Jason Wang wrote:> > ? 2022/1/7 ??8:33, Si-Wei Liu ??: >> >> >> On 1/5/2022 3:46 AM, Eli Cohen wrote: >>> Add wrappers to get/set status and protect these operations with >>> cf_mutex to serialize these operations with respect to get/set config >>> operations. >>> >>> Signed-off-by: Eli Cohen <elic at nvidia.com> >>> --- >>> ? drivers/vdpa/vdpa.c????????? | 19 +++++++++++++++++++ >>> ? drivers/vhost/vdpa.c???????? |? 7 +++---- >>> ? drivers/virtio/virtio_vdpa.c |? 3 +-- >>> ? include/linux/vdpa.h???????? |? 3 +++ >>> ? 4 files changed, 26 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >>> index 42d71d60d5dc..5134c83c4a22 100644 >>> --- a/drivers/vdpa/vdpa.c >>> +++ b/drivers/vdpa/vdpa.c >>> @@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head); >>> ? static DEFINE_MUTEX(vdpa_dev_mutex); >>> ? static DEFINE_IDA(vdpa_index_ida); >>> ? +u8 vdpa_get_status(struct vdpa_device *vdev) >>> +{ >>> +??? u8 status; >>> + >>> +??? mutex_lock(&vdev->cf_mutex); >>> +??? status = vdev->config->get_status(vdev); >>> +??? mutex_unlock(&vdev->cf_mutex); >>> +??? return status; >>> +} >>> +EXPORT_SYMBOL(vdpa_get_status); >>> + >>> +void vdpa_set_status(struct vdpa_device *vdev, u8 status) >>> +{ >>> +??? mutex_lock(&vdev->cf_mutex); >>> +??? vdev->config->set_status(vdev, status); >>> +??? mutex_unlock(&vdev->cf_mutex); >>> +} >>> +EXPORT_SYMBOL(vdpa_set_status); >>> + >>> ? static struct genl_family vdpa_nl_family; >>> ? ? static int vdpa_dev_probe(struct device *d) >>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>> index ebaa373e9b82..d9d499465e2e 100644 >>> --- a/drivers/vhost/vdpa.c >>> +++ b/drivers/vhost/vdpa.c >>> @@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct >>> vhost_vdpa *v, u8 __user *argp) >>> ? static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user >>> *statusp) >>> ? { >>> ????? struct vdpa_device *vdpa = v->vdpa; >>> -??? const struct vdpa_config_ops *ops = vdpa->config; >>> ????? u8 status; >>> ? -??? status = ops->get_status(vdpa); >>> +??? status = vdpa_get_status(vdpa); >> Not sure why we need to take cf_mutex here. Appears to me only >> setters (set_status and reset) need to take the lock in this function. > > > You may be right but it doesn't harm and it is guaranteed to be > correct if we protect it with mutex here.Is it more for future proof? Ok, but IMHO it might be better to get some comment here, otherwise it's quite confusing why the lock needs to be held. vhost_vdpa had done its own serialization in vhost_vdpa_unlocked_ioctl() through vhost_dev`mutex. -Siwei> > Thanks > > >> >>> ? ????? if (copy_to_user(statusp, &status, sizeof(status))) >>> ????????? return -EFAULT; >>> @@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct >>> vhost_vdpa *v, u8 __user *statusp) >>> ????? if (copy_from_user(&status, statusp, sizeof(status))) >>> ????????? return -EFAULT; >>> ? -??? status_old = ops->get_status(vdpa); >>> +??? status_old = vdpa_get_status(vdpa); >> Ditto. >> >>> ? ????? /* >>> ?????? * Userspace shouldn't remove status bits unless reset the >>> @@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct >>> vhost_vdpa *v, u8 __user *statusp) >>> ????????? if (ret) >>> ????????????? return ret; >>> ????? } else >>> -??????? ops->set_status(vdpa, status); >>> +??????? vdpa_set_status(vdpa, status); >> The reset() call in the if branch above needs to take the cf_mutex, >> the same way as that for set_status(). The reset() is effectively >> set_status(vdpa, 0). >> >> Thanks, >> -Siwei >> >>> ? ????? if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & >>> VIRTIO_CONFIG_S_DRIVER_OK)) >>> ????????? for (i = 0; i < nvqs; i++) >>> diff --git a/drivers/virtio/virtio_vdpa.c >>> b/drivers/virtio/virtio_vdpa.c >>> index a84b04ba3195..76504559bc25 100644 >>> --- a/drivers/virtio/virtio_vdpa.c >>> +++ b/drivers/virtio/virtio_vdpa.c >>> @@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct >>> virtio_device *vdev) >>> ? static void virtio_vdpa_set_status(struct virtio_device *vdev, u8 >>> status) >>> ? { >>> ????? struct vdpa_device *vdpa = vd_get_vdpa(vdev); >>> -??? const struct vdpa_config_ops *ops = vdpa->config; >>> ? -??? return ops->set_status(vdpa, status); >>> +??? return vdpa_set_status(vdpa, status); >>> ? } >>> ? ? static void virtio_vdpa_reset(struct virtio_device *vdev) >>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h >>> index 9cc4291a79b3..ae047fae2603 100644 >>> --- a/include/linux/vdpa.h >>> +++ b/include/linux/vdpa.h >>> @@ -408,6 +408,9 @@ 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, >>> ?????????????? const void *buf, unsigned int length); >>> +u8 vdpa_get_status(struct vdpa_device *vdev); >>> +void vdpa_set_status(struct vdpa_device *vdev, u8 status); >>> + >>> ? /** >>> ?? * struct vdpa_mgmtdev_ops - vdpa device ops >>> ?? * @dev_add: Add a vdpa device using alloc and register >> >