Re: [PATCH vfio 2/2] vfio: Split migration ops from main device ops

[Yishai Hadas <yishaih@nvidia.com>]

On 13/06/2022 10:13, Yishai Hadas wrote:
> On 10/06/2022 6:32, Tian, Kevin wrote:
>>> From: Yishai Hadas <yishaih@nvidia.com>
>>> Sent: Monday, June 6, 2022 4:56 PM
>>>
>>> vfio core checks whether the driver sets some migration op (e.g.
>>> set_state/get_state) and accordingly calls its op.
>>>
>>> However, currently mlx5 driver sets the above ops without regards to 
>>> its
>>> migration caps.
>>>
>>> This might lead to unexpected usage/Oops if user space may call to the
>>> above ops even if the driver doesn't support migration. As for example,
>>> the migration state_mutex is not initialized in that case.
>>>
>>> The cleanest way to manage that seems to split the migration ops from
>>> the main device ops, this will let the driver setting them separately
>>> from the main ops when it's applicable.
>>>
>>> As part of that, changed HISI driver to match this scheme.
>>>
>>> This scheme may enable down the road to come with some extra group of
>>> ops (e.g. DMA log) that can be set without regards to the other options
>>> based on driver caps.
>>>
>>> Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 
>>> devices")
>>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one nit:
>
> Thanks Kevin, please see below.
>
>>
>>> @@ -1534,8 +1534,8 @@ vfio_ioctl_device_feature_mig_device_state(struct
>>> vfio_device *device,
>>>       struct file *filp = NULL;
>>>       int ret;
>>>
>>> -    if (!device->ops->migration_set_state ||
>>> -        !device->ops->migration_get_state)
>>> +    if (!device->mig_ops->migration_set_state ||
>>> +        !device->mig_ops->migration_get_state)
>>>           return -ENOTTY;
>> ...
>>
>>> @@ -1582,8 +1583,8 @@ static int
>>> vfio_ioctl_device_feature_migration(struct vfio_device *device,
>>>       };
>>>       int ret;
>>>
>>> -    if (!device->ops->migration_set_state ||
>>> -        !device->ops->migration_get_state)
>>> +    if (!device->mig_ops->migration_set_state ||
>>> +        !device->mig_ops->migration_get_state)
>>>           return -ENOTTY;
>>>
>> Above checks can be done once when the device is registered then
>> here replaced with a single check on device->mig_ops.
>>
> I agree, it may look as of below.
>
> Theoretically, this could be done even before this patch upon device 
> registration.
>
> We could check that both 'ops' were set and *not* only one of and 
> later check for the specific 'op' upon the feature request.
>
> Alex,
>
> Do you prefer to switch to the below as part of V2 or stay as of 
> current submission and I'll just add Kevin as Reviewed-by ?
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c 
> b/drivers/vfio/pci/vfio_pci_core.c
> index a0d69ddaf90d..f42102a03851 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1855,6 +1855,11 @@ int vfio_pci_core_register_device(struct 
> vfio_pci_core_device *vdev)
>         if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
>                 return -EINVAL;
>
> +       if (vdev->vdev.mig_ops &&
> +          !(vdev->vdev.mig_ops->migration_get_state &&
> +            vdev->vdev.mig_ops->migration_get_state))
> +               return -EINVAL;
> +
>
> Yishai
>
Hi Alex,

Did you have the chance to review the above note ?

I would like to send V2 with Kevin's Reviewed-by tag for both patches, 
just wonder about the nit.

Yishai