On Mon, 21 Feb 2022 11:40:42 +0000
Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx> wrote:
> @@ -159,23 +1110,46 @@ static long hisi_acc_vfio_pci_ioctl(struct vfio_device *core_vdev, unsigned int
>
> static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
> {
> - struct vfio_pci_core_device *vdev =
> - container_of(core_vdev, struct vfio_pci_core_device, vdev);
> + struct hisi_acc_vf_core_device *hisi_acc_vdev = container_of(core_vdev,
> + struct hisi_acc_vf_core_device, core_device.vdev);
> + struct vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device;
> int ret;
>
> ret = vfio_pci_core_enable(vdev);
> if (ret)
> return ret;
>
> - vfio_pci_core_finish_enable(vdev);
> + if (core_vdev->migration_flags != VFIO_MIGRATION_STOP_COPY) {
This looks like a minor synchronization issue with
hisi_acc_vfio_pci_migrn_init(), I think it might be cleaner to test
core_vdev->ops against the migration enabled set.
> + vfio_pci_core_finish_enable(vdev);
> + return 0;
> + }
> +
> + ret = hisi_acc_vf_qm_init(hisi_acc_vdev);
> + if (ret) {
> + vfio_pci_core_disable(vdev);
> + return ret;
> + }
>
> + hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
Change the polarity of the if() above and encompass this all within
that branch scope so we can use the finish/return below for both cases?
> +
> + vfio_pci_core_finish_enable(vdev);
> return 0;
> }
>
> +static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev)
> +{
> + struct hisi_acc_vf_core_device *hisi_acc_vdev = container_of(core_vdev,
> + struct hisi_acc_vf_core_device, core_device.vdev);
> + struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm;
> +
> + iounmap(vf_qm->io_base);
> + vfio_pci_core_close_device(core_vdev);
> +}
> +
> static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
> .name = "hisi-acc-vfio-pci",
> .open_device = hisi_acc_vfio_pci_open_device,
> - .close_device = vfio_pci_core_close_device,
> + .close_device = hisi_acc_vfio_pci_close_device,
> .ioctl = hisi_acc_vfio_pci_ioctl,
> .device_feature = vfio_pci_core_ioctl_feature,
> .read = hisi_acc_vfio_pci_read,
> @@ -183,6 +1157,8 @@ static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
> .mmap = hisi_acc_vfio_pci_mmap,
> .request = vfio_pci_core_request,
> .match = vfio_pci_core_match,
> + .migration_set_state = hisi_acc_vfio_pci_set_device_state,
> + .migration_get_state = hisi_acc_vfio_pci_get_device_state,
> };
>
> static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> @@ -198,38 +1174,71 @@ static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> .match = vfio_pci_core_match,
> };
>
> +static int
> +hisi_acc_vfio_pci_migrn_init(struct hisi_acc_vf_core_device *hisi_acc_vdev,
> + struct pci_dev *pdev, struct hisi_qm *pf_qm)
> +{
> + int vf_id;
> +
> + vf_id = pci_iov_vf_id(pdev);
> + if (vf_id < 0)
> + return vf_id;
> +
> + hisi_acc_vdev->vf_id = vf_id + 1;
> + hisi_acc_vdev->core_device.vdev.migration_flags =
> + VFIO_MIGRATION_STOP_COPY;
> + hisi_acc_vdev->pf_qm = pf_qm;
> + hisi_acc_vdev->vf_dev = pdev;
> + mutex_init(&hisi_acc_vdev->state_mutex);
> +
> + return 0;
> +}
> +
> static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> - struct vfio_pci_core_device *vdev;
> + struct hisi_acc_vf_core_device *hisi_acc_vdev;
> + struct hisi_qm *pf_qm;
> int ret;
>
> - vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> - if (!vdev)
> + hisi_acc_vdev = kzalloc(sizeof(*hisi_acc_vdev), GFP_KERNEL);
> + if (!hisi_acc_vdev)
> return -ENOMEM;
>
> - vfio_pci_core_init_device(vdev, pdev, &hisi_acc_vfio_pci_ops);
> + pf_qm = hisi_acc_get_pf_qm(pdev);
> + if (pf_qm && pf_qm->ver >= QM_HW_V3) {
> + ret = hisi_acc_vfio_pci_migrn_init(hisi_acc_vdev, pdev, pf_qm);
> + if (ret < 0) {
> + kfree(hisi_acc_vdev);
> + return ret;
> + }
This error path can only occur if the VF ID lookup fails, but should we
fall through to the non-migration ops, maybe with a dev_warn()? Thanks,
Alex
> +
> + vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev,
> + &hisi_acc_vfio_pci_migrn_ops);
> + } else {
> + vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev,
> + &hisi_acc_vfio_pci_ops);
> + }
