On 06/11/2024 0:47, Alex Williamson wrote:
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.
Setting the 'ops' as part of the probe() seems actually doable,
including calling virtiovf_set_migratable() following the virtiodev
allocation below.
The main issue with that approach will be the init part of the legacy IO
(i.e. virtiovf_init_legacy_io()) as part of virtiovf_pci_init_device().
Assuming that we don't want to repeat calling
virtiovf_support_legacy_io() as part of virtiovf_pci_init_device() to
know whether legacy IO is supported, we can consider calling
virtiovf_init_legacy_io() as part of the probe() as well, which IMO
doesn't look clean as it's actually seems to match the init flow.
Alternatively, we can consider checking inside
virtiovf_pci_init_device() whether the 'ops' actually equals the 'tran'
ones and then call it.
Something like the below.
static int virtiovf_pci_init_device(struct vfio_device *core_vdev)
{
...
#ifdef CONFIG_VIRTIO_PCI_ADMIN_LEGACY
if (core_vdev->ops == &virtiovf_vfio_pci_tran_lm_ops)
return virtiovf_init_legacy_io(virtvdev);
#endif
return 0;
}
Do you prefer the above approach rather than current V1 code which has a
single check as part of virtiovf_init_legacy_io() ?
I think the API to vfio core also suggests we shouldn't be modifying the
ops pointer after the core device is allocated.
Any pointer for that ?
Do we actually see a problem with replacing the 'ops' as part of the
init flow ?
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.
Sure, will change.
Thanks,
Yishai
