On Mon, 4 Nov 2024 12:21:29 +0200
Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
> diff --git a/drivers/vfio/pci/virtio/main.c b/drivers/vfio/pci/virtio/main.c
> index b5d3a8c5bbc9..e2cdf2d48200 100644
> --- a/drivers/vfio/pci/virtio/main.c
> +++ b/drivers/vfio/pci/virtio/main.c
...
> @@ -485,16 +478,66 @@ static bool virtiovf_bar0_exists(struct pci_dev *pdev)
> return res->flags;
> }
>
> +static int virtiovf_pci_init_device(struct vfio_device *core_vdev)
> +{
> + struct virtiovf_pci_core_device *virtvdev = container_of(core_vdev,
> + struct virtiovf_pci_core_device, core_device.vdev);
> + struct pci_dev *pdev;
> + bool sup_legacy_io;
> + bool sup_lm;
> + int ret;
> +
> + ret = vfio_pci_core_init_dev(core_vdev);
> + if (ret)
> + return ret;
> +
> + pdev = virtvdev->core_device.pdev;
> + sup_legacy_io = virtio_pci_admin_has_legacy_io(pdev) &&
> + !virtiovf_bar0_exists(pdev);
> + sup_lm = virtio_pci_admin_has_dev_parts(pdev);
> +
> + /*
> + * If the device is not capable to this driver functionality, fallback
> + * to the default vfio-pci ops
> + */
> + if (!sup_legacy_io && !sup_lm) {
> + core_vdev->ops = &virtiovf_vfio_pci_ops;
> + return 0;
> + }
> +
> + if (sup_legacy_io) {
> + ret = virtiovf_read_notify_info(virtvdev);
> + if (ret)
> + return ret;
> +
> + virtvdev->bar0_virtual_buf_size = VIRTIO_PCI_CONFIG_OFF(true) +
> + virtiovf_get_device_config_size(pdev->device);
> + BUILD_BUG_ON(!is_power_of_2(virtvdev->bar0_virtual_buf_size));
> + virtvdev->bar0_virtual_buf = kzalloc(virtvdev->bar0_virtual_buf_size,
> + GFP_KERNEL);
> + if (!virtvdev->bar0_virtual_buf)
> + return -ENOMEM;
> + mutex_init(&virtvdev->bar_mutex);
> + }
> +
> + if (sup_lm)
> + virtiovf_set_migratable(virtvdev);
> +
> + if (sup_lm && !sup_legacy_io)
> + core_vdev->ops = &virtiovf_vfio_pci_lm_ops;
> +
> + return 0;
> +}
> +
> static int virtiovf_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
> {
> - const struct vfio_device_ops *ops = &virtiovf_vfio_pci_ops;
> struct virtiovf_pci_core_device *virtvdev;
> + const struct vfio_device_ops *ops;
> int ret;
>
> - if (pdev->is_virtfn && virtio_pci_admin_has_legacy_io(pdev) &&
> - !virtiovf_bar0_exists(pdev))
> - ops = &virtiovf_vfio_pci_tran_ops;
> + ops = (pdev->is_virtfn) ? &virtiovf_vfio_pci_tran_lm_ops :
> + &virtiovf_vfio_pci_ops;
I can't figure out why we moved the more thorough ops setup to the
.init() callback of the ops themselves. Clearly we can do the legacy
IO and BAR0 test here and the dev parts test uses the same mechanisms
as the legacy IO test, so it seems we could know sup_legacy_io and
sup_lm here. I think we can even do virtiovf_set_migratable() here
after virtvdev is allocated below.
I think the API to vfio core also suggests we shouldn't be modifying the
ops pointer after the core device is allocated.
>
> virtvdev = vfio_alloc_device(virtiovf_pci_core_device, core_device.vdev,
> &pdev->dev, ops);
> @@ -532,6 +575,7 @@ static void virtiovf_pci_aer_reset_done(struct pci_dev *pdev)
> struct virtiovf_pci_core_device *virtvdev = dev_get_drvdata(&pdev->dev);
>
> virtvdev->pci_cmd = 0;
> + virtiovf_migration_reset_done(pdev);
> }
>
> static const struct pci_error_handlers virtiovf_err_handlers = {
> diff --git a/drivers/vfio/pci/virtio/migrate.c b/drivers/vfio/pci/virtio/migrate.c
> new file mode 100644
> index 000000000000..2a9614c2ef07
> --- /dev/null
> +++ b/drivers/vfio/pci/virtio/migrate.c
...
> +static int virtiovf_pci_get_data_size(struct vfio_device *vdev,
> + unsigned long *stop_copy_length)
> +{
> + struct virtiovf_pci_core_device *virtvdev = container_of(
> + vdev, struct virtiovf_pci_core_device, core_device.vdev);
> + bool obj_id_exists;
> + u32 res_size;
> + u32 obj_id;
> + int ret;
> +
> + mutex_lock(&virtvdev->state_mutex);
> + obj_id_exists = virtvdev->saving_migf && virtvdev->saving_migf->has_obj_id;
> + if (!obj_id_exists) {
> + ret = virtiovf_pci_alloc_obj_id(virtvdev,
> + VIRTIO_RESOURCE_OBJ_DEV_PARTS_TYPE_GET,
> + &obj_id);
> + if (ret)
> + goto end;
> + } else {
> + obj_id = virtvdev->saving_migf->obj_id;
> + }
> +
> + ret = virtio_pci_admin_dev_parts_metadata_get(virtvdev->core_device.pdev,
> + VIRTIO_RESOURCE_OBJ_DEV_PARTS, obj_id,
> + VIRTIO_ADMIN_CMD_DEV_PARTS_METADATA_TYPE_SIZE,
> + &res_size);
> + if (!ret)
> + *stop_copy_length = res_size;
> +
> + /* We can't leave this obj_id alive if didn't exist before, otherwise, it might
> + * stay alive, even without an active migration flow (e.g. migration was cancelled)
> + */
Nit, multi-line comment style.
Thanks,
Alex
