netdev - Re: [PATCH v6 vfio 4/7] vfio/pds: Add VFIO live migration support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZCQuCPG864f0R+9s@corigine.com>
Date:   Wed, 29 Mar 2023 14:24:40 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Brett Creeley <brett.creeley@....com>
Cc:     kvm@...r.kernel.org, netdev@...r.kernel.org,
        alex.williamson@...hat.com, jgg@...dia.com, yishaih@...dia.com,
        shameerali.kolothum.thodi@...wei.com, kevin.tian@...el.com,
        shannon.nelson@....com, drivers@...sando.io
Subject: Re: [PATCH v6 vfio 4/7] vfio/pds: Add VFIO live migration support

On Mon, Mar 27, 2023 at 01:05:50PM -0700, Brett Creeley wrote:
> Add live migration support via the VFIO subsystem. The migration
> implementation aligns with the definition from uapi/vfio.h and uses
> the pds_core PF's adminq for device configuration.
> 
> The ability to suspend, resume, and transfer VF device state data is
> included along with the required admin queue command structures and
> implementations.
> 
> PDS_LM_CMD_SUSPEND and PDS_LM_CMD_SUSPEND_STATUS are added to support
> the VF device suspend operation.
> 
> PDS_LM_CMD_RESUME is added to support the VF device resume operation.
> 
> PDS_LM_CMD_STATUS is added to determine the exact size of the VF
> device state data.
> 
> PDS_LM_CMD_SAVE is added to get the VF device state data.
> 
> PDS_LM_CMD_RESTORE is added to restore the VF device with the
> previously saved data from PDS_LM_CMD_SAVE.
> 
> PDS_LM_CMD_HOST_VF_STATUS is added to notify the device when
> a migration is in/not-in progress from the host's perspective.
> 
> Signed-off-by: Brett Creeley <brett.creeley@....com>
> Signed-off-by: Shannon Nelson <shannon.nelson@....com>

Thanks Brett,

overall this looks clean to me.
I've provided some minor points inline which you may wish
to consider if you need to respin the series for some reason.

...

> diff --git a/drivers/vfio/pci/pds/cmds.c b/drivers/vfio/pci/pds/cmds.c

...

> +int
> +pds_vfio_get_lm_status_cmd(struct pds_vfio_pci_device *pds_vfio, u64 *size)
> +{
> +	struct pds_lm_status_cmd cmd = {
> +		.opcode = PDS_LM_CMD_STATUS,
> +		.vf_id = cpu_to_le16(pds_vfio->vf_id),
> +	};
> +	struct pds_lm_status_comp comp = {0};
> +	int err = 0;

nit: there is no need to initialise err here,
     as it is set below before it is used.

> +
> +	dev_dbg(&pds_vfio->pdev->dev, "vf%u: Get migration status\n",
> +		pds_vfio->vf_id);
> +
> +	err = pds_client_adminq_cmd(pds_vfio,
> +				    (union pds_core_adminq_cmd *)&cmd,
> +				    sizeof(cmd),
> +				    (union pds_core_adminq_comp *)&comp,
> +				    0);
> +	if (err)
> +		return err;
> +
> +	*size = le64_to_cpu(comp.size);
> +	return 0;
> +}
> +
> +static int
> +pds_vfio_dma_map_lm_file(struct device *dev, enum dma_data_direction dir,
> +			 struct pds_vfio_lm_file *lm_file)
> +{
> +	struct pds_lm_sg_elem *sgl, *sge;
> +	struct scatterlist *sg;
> +	int err = 0;

Ditto.

> +	int i;
> +
> +	if (!lm_file)
> +		return -EINVAL;
> +
> +	/* dma map file pages */
> +	err = dma_map_sgtable(dev, &lm_file->sg_table, dir, 0);
> +	if (err)
> +		goto err_dma_map_sg;

nit: 'return err;' might be simpler.

> +
> +	lm_file->num_sge = lm_file->sg_table.nents;
> +
> +	/* alloc sgl */
> +	sgl = dma_alloc_coherent(dev, lm_file->num_sge *
> +				 sizeof(struct pds_lm_sg_elem),
> +				 &lm_file->sgl_addr, GFP_KERNEL);
> +	if (!sgl) {
> +		err = -ENOMEM;
> +		goto err_alloc_sgl;
> +	}
> +
> +	lm_file->sgl = sgl;
> +
> +	/* fill sgl */
> +	sge = sgl;
> +	for_each_sgtable_dma_sg(&lm_file->sg_table, sg, i) {
> +		sge->addr = cpu_to_le64(sg_dma_address(sg));
> +		sge->len  = cpu_to_le32(sg_dma_len(sg));
> +		dev_dbg(dev, "addr = %llx, len = %u\n", sge->addr, sge->len);
> +		sge++;
> +	}
> +
> +	return 0;
> +
> +err_alloc_sgl:
> +	dma_unmap_sgtable(dev, &lm_file->sg_table, dir, 0);
> +err_dma_map_sg:
> +	return err;
> +}

...

> diff --git a/drivers/vfio/pci/pds/lm.c b/drivers/vfio/pci/pds/lm.c

...

> +static int
> +pds_vfio_get_save_file(struct pds_vfio_pci_device *pds_vfio)
> +{
> +	struct pci_dev *pdev = pds_vfio->pdev;
> +	struct pds_vfio_lm_file *lm_file;
> +	int err = 0;
> +	u64 size;
> +
> +	/* Get live migration state size in this state */
> +	err = pds_vfio_get_lm_status_cmd(pds_vfio, &size);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to get save status: %pe\n",
> +			ERR_PTR(err));
> +		goto err_get_lm_status;
> +	}
> +
> +	dev_dbg(&pdev->dev, "save status, size = %lld\n", size);
> +
> +	if (!size) {
> +		err = -EIO;
> +		dev_err(&pdev->dev, "invalid state size\n");
> +		goto err_get_lm_status;
> +	}
> +
> +	lm_file = pds_vfio_get_lm_file(PDS_VFIO_LM_FILENAME,
> +				       &pds_vfio_save_fops,
> +				       O_RDONLY, size);
> +	if (!lm_file) {
> +		err = -ENOENT;
> +		dev_err(&pdev->dev, "failed to create save file\n");
> +		goto err_get_lm_file;
> +	}
> +
> +	dev_dbg(&pdev->dev, "size = %lld, alloc_size = %lld, npages = %lld\n",
> +		lm_file->size, lm_file->alloc_size, lm_file->npages);
> +
> +	pds_vfio->save_file = lm_file;
> +
> +	return 0;
> +
> +err_get_lm_file:
> +err_get_lm_status:

nit: I don't think these labels are giving us much.
     If it was me I'd just use return rather than goto above.

> +	return err;
> +}

...