Re: [PATCH v3 6/6] hisi_acc_vfio_pci: Add support for VFIO live migration

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Sep 15, 2021 at 10:50:37AM +0100, Shameer Kolothum wrote:
> +/*
> + * HiSilicon ACC VF dev MMIO space contains both the functional register
> + * space and the migration control register space. We hide the migration
> + * control space from the Guest. But to successfully complete the live
> + * migration, we still need access to the functional MMIO space assigned
> + * to the Guest. To avoid any potential security issues, we need to be
> + * careful not to access this region while the Guest vCPUs are running.
> + *
> + * Hence check the device state before we map the region.
> + */

The prior patch prevents mapping this area into the guest at all,
right?

So why the comment and logic? If the MMIO area isn't mapped then there
is nothing to do, right?

The only risk is P2P transactions from devices in the same IOMMU
group, and you might do well to mitigate that by asserting that the
device is in a singleton IOMMU group?

> +static int hisi_acc_vfio_pci_init(struct vfio_pci_core_device *vdev)
> +{
> +	struct acc_vf_migration *acc_vf_dev;
> +	struct pci_dev *pdev = vdev->pdev;
> +	struct pci_dev *pf_dev, *vf_dev;
> +	struct hisi_qm *pf_qm;
> +	int vf_id, ret;
> +
> +	pf_dev = pdev->physfn;
> +	vf_dev = pdev;
> +
> +	pf_qm = pci_get_drvdata(pf_dev);
> +	if (!pf_qm) {
> +		pr_err("HiSi ACC qm driver not loaded\n");
> +		return -EINVAL;
> +	}

Nope, this is locked wrong and has no lifetime management.


> +	if (pf_qm->ver < QM_HW_V3) {
> +		dev_err(&pdev->dev,
> +			"Migration not supported, hw version: 0x%x\n",
> +			 pf_qm->ver);
> +		return -ENODEV;
> +	}
> +
> +	vf_id = PCI_FUNC(vf_dev->devfn);
> +	acc_vf_dev = kzalloc(sizeof(*acc_vf_dev), GFP_KERNEL);
> +	if (!acc_vf_dev)
> +		return -ENOMEM;

Don't do the memory like this, the entire driver should have a global
struct, not one that is allocated/freed around open/close_device

struct hisi_acc_vfio_device {
      struct vfio_pci_core_device core_device;
      [put acc_vf_migration here]
      [put required state from mig_ctl here, don't allocate again]
      struct acc_vf_data mig_data; // Don't use wonky pointer maths
}

Then leave the releae function on the reg ops NULL and consistently
pass the hisi_acc_vfio_device everywhere instead of
acc_vf_migration. This way all the functions get all the needed
information, eg if they want to log or something.

The mlx5 driver that should be posted soon will show how to structure
most of this well and include several more patches you'll want to be
using here.

Jason