| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fa42f92c-67c6-a448-d51f-8a2e4652918c@nvidia.com>
Date: Sun, 8 May 2022 16:04:40 +0300
From: Yishai Hadas <yishaih@...dia.com>
To: Alex Williamson <alex.williamson@...hat.com>
CC: <jgg@...dia.com>, <saeedm@...dia.com>, <kvm@...r.kernel.org>,
<netdev@...r.kernel.org>, <kuba@...nel.org>, <leonro@...dia.com>,
<maorg@...dia.com>, <cohuck@...hat.com>
Subject: Re: [PATCH mlx5-next 3/5] vfio/mlx5: Manage the VF attach/detach
callback from the PF
On 04/05/2022 23:34, Alex Williamson wrote:
> On Wed, 27 Apr 2022 12:31:18 +0300
> Yishai Hadas <yishaih@...dia.com> wrote:
>
>> Manage the VF attach/detach callback from the PF.
>>
>> This lets the driver to enable parallel VFs migration as will be
>> introduced in the next patch.
>>
>> Signed-off-by: Yishai Hadas <yishaih@...dia.com>
>> Signed-off-by: Leon Romanovsky <leonro@...dia.com>
>> ---
>> drivers/vfio/pci/mlx5/cmd.c | 59 +++++++++++++++++++++++++++++++++---
>> drivers/vfio/pci/mlx5/cmd.h | 23 +++++++++++++-
>> drivers/vfio/pci/mlx5/main.c | 25 ++++-----------
>> 3 files changed, 82 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
>> index d608b8167f58..1f84d7b9b9e5 100644
>> --- a/drivers/vfio/pci/mlx5/cmd.c
>> +++ b/drivers/vfio/pci/mlx5/cmd.c
>> @@ -71,21 +71,70 @@ int mlx5vf_cmd_query_vhca_migration_state(struct pci_dev *pdev, u16 vhca_id,
>> return ret;
>> }
>>
>> -bool mlx5vf_cmd_is_migratable(struct pci_dev *pdev)
>> +static int mlx5fv_vf_event(struct notifier_block *nb,
>> + unsigned long event, void *data)
>> {
>> - struct mlx5_core_dev *mdev = mlx5_vf_get_core_dev(pdev);
>> + struct mlx5vf_pci_core_device *mvdev =
>> + container_of(nb, struct mlx5vf_pci_core_device, nb);
>> +
>> + mutex_lock(&mvdev->state_mutex);
>> + switch (event) {
>> + case MLX5_PF_NOTIFY_ENABLE_VF:
>> + mvdev->mdev_detach = false;
>> + break;
>> + case MLX5_PF_NOTIFY_DISABLE_VF:
>> + mvdev->mdev_detach = true;
>> + break;
>> + default:
>> + break;
>> + }
>> + mlx5vf_state_mutex_unlock(mvdev);
>> + return 0;
>> +}
>> +
>> +void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev)
>> +{
>> + mlx5_sriov_blocking_notifier_unregister(mvdev->mdev, mvdev->vf_id,
>> + &mvdev->nb);
>> +}
>> +
>> +bool mlx5vf_cmd_is_migratable(struct mlx5vf_pci_core_device *mvdev)
> Why did the original implementation take a pdev knowing we're going to
> gut it in the next patch to use an mvdev? The diff would be easier to
> read.
Agree, in V1 I just combined this patch with the changes from patch #1.
>
> There's also quite a lot of setup here now, it's no longer a simple
> test whether the device supports migration which makes the name
> misleading. This looks like a "setup migration" function that should
> return 0/-errno.
Thanks, makes sense.
>> +{
>> + struct pci_dev *pdev = mvdev->core_device.pdev;
>> bool migratable = false;
>> + int ret;
>>
>> - if (!mdev)
>> + mvdev->mdev = mlx5_vf_get_core_dev(pdev);
>> + if (!mvdev->mdev)
>> return false;
>> + if (!MLX5_CAP_GEN(mvdev->mdev, migration))
>> + goto end;
>> + mvdev->vf_id = pci_iov_vf_id(pdev);
>> + if (mvdev->vf_id < 0)
>> + goto end;
>>
>> - if (!MLX5_CAP_GEN(mdev, migration))
>> + mutex_init(&mvdev->state_mutex);
>> + spin_lock_init(&mvdev->reset_lock);
>> + mvdev->nb.notifier_call = mlx5fv_vf_event;
>> + ret = mlx5_sriov_blocking_notifier_register(mvdev->mdev, mvdev->vf_id,
>> + &mvdev->nb);
>> + if (ret)
>> goto end;
>>
>> + mutex_lock(&mvdev->state_mutex);
>> + if (mvdev->mdev_detach)
>> + goto unreg;
>> +
>> + mlx5vf_state_mutex_unlock(mvdev);
>> migratable = true;
>> + goto end;
>>
>> +unreg:
>> + mlx5vf_state_mutex_unlock(mvdev);
>> + mlx5_sriov_blocking_notifier_unregister(mvdev->mdev, mvdev->vf_id,
>> + &mvdev->nb);
>> end:
>> - mlx5_vf_put_core_dev(mdev);
>> + mlx5_vf_put_core_dev(mvdev->mdev);
>> return migratable;
>> }
>>
>> diff --git a/drivers/vfio/pci/mlx5/cmd.h b/drivers/vfio/pci/mlx5/cmd.h
>> index 2da6a1c0ec5c..f47174eab4b8 100644
>> --- a/drivers/vfio/pci/mlx5/cmd.h
>> +++ b/drivers/vfio/pci/mlx5/cmd.h
>> @@ -7,6 +7,7 @@
>> #define MLX5_VFIO_CMD_H
>>
>> #include <linux/kernel.h>
>> +#include <linux/vfio_pci_core.h>
>> #include <linux/mlx5/driver.h>
>>
>> struct mlx5_vf_migration_file {
>> @@ -24,14 +25,34 @@ struct mlx5_vf_migration_file {
>> unsigned long last_offset;
>> };
>>
>> +struct mlx5vf_pci_core_device {
>> + struct vfio_pci_core_device core_device;
>> + int vf_id;
>> + u16 vhca_id;
>> + u8 migrate_cap:1;
>> + u8 deferred_reset:1;
>> + /* protect migration state */
>> + struct mutex state_mutex;
>> + enum vfio_device_mig_state mig_state;
>> + /* protect the reset_done flow */
>> + spinlock_t reset_lock;
>> + struct mlx5_vf_migration_file *resuming_migf;
>> + struct mlx5_vf_migration_file *saving_migf;
>> + struct notifier_block nb;
>> + struct mlx5_core_dev *mdev;
>> + u8 mdev_detach:1;
>> +};
>> +
>> int mlx5vf_cmd_suspend_vhca(struct pci_dev *pdev, u16 vhca_id, u16 op_mod);
>> int mlx5vf_cmd_resume_vhca(struct pci_dev *pdev, u16 vhca_id, u16 op_mod);
>> int mlx5vf_cmd_query_vhca_migration_state(struct pci_dev *pdev, u16 vhca_id,
>> size_t *state_size);
>> int mlx5vf_cmd_get_vhca_id(struct pci_dev *pdev, u16 function_id, u16 *vhca_id);
>> -bool mlx5vf_cmd_is_migratable(struct pci_dev *pdev);
>> +bool mlx5vf_cmd_is_migratable(struct mlx5vf_pci_core_device *mvdev);
>> +void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev);
>> int mlx5vf_cmd_save_vhca_state(struct pci_dev *pdev, u16 vhca_id,
>> struct mlx5_vf_migration_file *migf);
>> int mlx5vf_cmd_load_vhca_state(struct pci_dev *pdev, u16 vhca_id,
>> struct mlx5_vf_migration_file *migf);
>> +void mlx5vf_state_mutex_unlock(struct mlx5vf_pci_core_device *mvdev);
>> #endif /* MLX5_VFIO_CMD_H */
>> diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
>> index 2578f61eaeae..445c516d38d9 100644
>> --- a/drivers/vfio/pci/mlx5/main.c
>> +++ b/drivers/vfio/pci/mlx5/main.c
>> @@ -17,7 +17,6 @@
>> #include <linux/uaccess.h>
>> #include <linux/vfio.h>
>> #include <linux/sched/mm.h>
>> -#include <linux/vfio_pci_core.h>
>> #include <linux/anon_inodes.h>
>>
>> #include "cmd.h"
>> @@ -25,20 +24,6 @@
>> /* Arbitrary to prevent userspace from consuming endless memory */
>> #define MAX_MIGRATION_SIZE (512*1024*1024)
>>
>> -struct mlx5vf_pci_core_device {
>> - struct vfio_pci_core_device core_device;
>> - u16 vhca_id;
>> - u8 migrate_cap:1;
>> - u8 deferred_reset:1;
>> - /* protect migration state */
>> - struct mutex state_mutex;
>> - enum vfio_device_mig_state mig_state;
>> - /* protect the reset_done flow */
>> - spinlock_t reset_lock;
>> - struct mlx5_vf_migration_file *resuming_migf;
>> - struct mlx5_vf_migration_file *saving_migf;
>> -};
>> -
>> static struct page *
>> mlx5vf_get_migration_page(struct mlx5_vf_migration_file *migf,
>> unsigned long offset)
>> @@ -444,7 +429,7 @@ mlx5vf_pci_step_device_state_locked(struct mlx5vf_pci_core_device *mvdev,
>> * This function is called in all state_mutex unlock cases to
>> * handle a 'deferred_reset' if exists.
>> */
>> -static void mlx5vf_state_mutex_unlock(struct mlx5vf_pci_core_device *mvdev)
>> +void mlx5vf_state_mutex_unlock(struct mlx5vf_pci_core_device *mvdev)
>> {
>> again:
>> spin_lock(&mvdev->reset_lock);
>> @@ -597,13 +582,11 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
>> return -ENOMEM;
>> vfio_pci_core_init_device(&mvdev->core_device, pdev, &mlx5vf_pci_ops);
>>
>> - if (pdev->is_virtfn && mlx5vf_cmd_is_migratable(pdev)) {
>> + if (pdev->is_virtfn && mlx5vf_cmd_is_migratable(mvdev)) {
>> mvdev->migrate_cap = 1;
>> mvdev->core_device.vdev.migration_flags =
>> VFIO_MIGRATION_STOP_COPY |
>> VFIO_MIGRATION_P2P;
> Why do these aspects of setting up migration remain here? Do we even
> need this new function to have a return value? It looks like all of
> this and testing whether the pdev->is_virtfn could be pushed into the
> new function, which could then return void. Thanks,
Makes sense, will be part of V1, thanks.
> Alex
>
>> - mutex_init(&mvdev->state_mutex);
>> - spin_lock_init(&mvdev->reset_lock);
>> }
>>
>> ret = vfio_pci_core_register_device(&mvdev->core_device);
>> @@ -614,6 +597,8 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
>> return 0;
>>
>> out_free:
>> + if (mvdev->migrate_cap)
>> + mlx5vf_cmd_remove_migratable(mvdev);
>> vfio_pci_core_uninit_device(&mvdev->core_device);
>> kfree(mvdev);
>> return ret;
>> @@ -624,6 +609,8 @@ static void mlx5vf_pci_remove(struct pci_dev *pdev)
>> struct mlx5vf_pci_core_device *mvdev = dev_get_drvdata(&pdev->dev);
>>
>> vfio_pci_core_unregister_device(&mvdev->core_device);
>> + if (mvdev->migrate_cap)
>> + mlx5vf_cmd_remove_migratable(mvdev);
>> vfio_pci_core_uninit_device(&mvdev->core_device);
>> kfree(mvdev);
>> }
Powered by blists - more mailing lists