Re: [PATCH v6 03/13] vdpa: Sync calls set/get config/status with cf_mutex

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 12/28/2021 7:36 AM, Eli Cohen wrote:
> On Thu, Dec 23, 2021 at 01:58:50PM -0800, Si-Wei Liu wrote:
>>
>> On 12/22/2021 9:46 PM, Eli Cohen wrote:
>>> On Wed, Dec 22, 2021 at 02:58:18PM -0800, Si-Wei Liu wrote:
>>>> On 12/22/2021 6:20 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.
>>>> Need to protect vdpa_reset() which is essentially vdpa_set_status(0)
>>> And what about vdpa_set_features()?
>> Oh, that was missed... Indeed, it also needs to take the lock.
>>
>>> Maybe it should be done as part of another patch to address this?
>> Isn't this patch the one adding the helpers for the locked API? I think it's
>> fine if you'd like specific ones for reset or set_features.
>>
>>> Shouldn't all these serializations requirements be handled by the caller?
>> If the caller remains in the vdpa core I think it should be fine.
> I meant usually qemu makes these calls. Isn't it the one that will
> maintain required serialzation?
Actually, I am not concerned much about QEMU as all the vDPA ioctl's 
have been serialized in vhost_vdpa_unlocked_ioctl() through 
vhost_dev`mutex. The cf_mutex is primarily used to serialize the getter 
and setter of the virtio config. For getter we need to ensure the 
integrity and consistency for the "vdpa dev config show" command output, 
while the updaters for the virtio config are the set_status(), reset(), 
set_config() and set_features() calls, which can be from either 
vhost_vdpa or virtio_vdpa. It's better to have vdpa core maintain the 
locking and individual bus driver just call the vdpa locked API routines 
with atomiticy guarantee as needed.

>
>> Or you
>> want the vdpa bus drivers to take the serialization requirement? Perhaps not
>> so good.
> Can you give an example (or two) of secnarios you're concerned with.
For e.g. in vhost_vdpa_set_status() the ops->reset() call is not 
cf_mutex protected. Theoretically it's possible for a concurrent 
vdpa_dev_config_fill() thread to get already-reset-to-zero features or 
invalid config data, even though the _S_FEATURES_OK check had been 
passed pveriously before the reset() call. virtio_vdpa_reset() suffers 
from the same problem, too.

For vdpa_set_features(), it's possible the feature negotiation is not 
complete yet, thus a concurrent vdpa_dev_config_fill() call may get 
premature config data. Since your next patch explicitly checks 
_S_FEATURES_OK, in reality it's probably fine to not take the cf_mutex 
lock - although the check only works with virtio 1.0 guest driver. If 
legacy support will be added in future, this check and the corresponding 
locking has to be revisited then.

Thanks,
-Siwei



>
>> Thanks,
>> -Siwei
>>
>>>> -Siwei
>>>>> Signed-off-by: Eli Cohen <elic@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);
>>>>>     	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);
>>>>>     	/*
>>>>>     	 * 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);
>>>>>     	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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization