Re: [PATCH v8 vfio 4/7] vfio/pds: Add VFIO live migration support

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Apr 04, 2023 at 12:01:38PM -0700, Brett Creeley wrote:
> +	union pds_core_adminq_cmd cmd = { 0 };

These should all be = {}, adding the 0 is a subtly different thing in

> +int
> +pds_vfio_suspend_device_cmd(struct pds_vfio_pci_device *pds_vfio)
> +{
> +	struct pds_lm_suspend_cmd cmd = {
> +		.opcode = PDS_LM_CMD_SUSPEND,
> +		.vf_id = cpu_to_le16(pds_vfio->vf_id),
> +	};
> +	struct pds_lm_suspend_comp comp = {0};
> +	struct pci_dev *pdev = pds_vfio->pdev;
> +	int err;
> +
> +	dev_dbg(&pdev->dev, "vf%u: Suspend device\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,
> +				    PDS_AQ_FLAG_FASTPOLL);

These casts to a union are really weird, why isn't the union the type
on the stack?

Jason

> +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;
> +	int i;
> +
> +	if (!lm_file)
> +		return -EINVAL;
> +
> +	/* dma map file pages */
> +	err = dma_map_sgtable(dev, &lm_file->sg_table, dir, 0);
> +	if (err)
> +		return err;
> +
> +	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);

Do you really need a coherent allocation for this?

> diff --git a/drivers/vfio/pci/pds/lm.c b/drivers/vfio/pci/pds/lm.c
> new file mode 100644
> index 000000000000..855f5da9b99a
> --- /dev/null
> +++ b/drivers/vfio/pci/pds/lm.c
> @@ -0,0 +1,479 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2023 Advanced Micro Devices, Inc. */
> +
> +#include <linux/anon_inodes.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/highmem.h>
> +#include <linux/vfio.h>
> +#include <linux/vfio_pci_core.h>
> +
> +#include "vfio_dev.h"
> +#include "cmds.h"
> +
> +#define PDS_VFIO_LM_FILENAME	"pds_vfio_lm"

This doesn't need a define, it is typical to write the pseudo filename
in the only anon_inode_getfile()

> +static struct pds_vfio_lm_file *
> +pds_vfio_get_lm_file(const char *name, const struct file_operations *fops,
> +		     int flags, u64 size)
> +{
> +	struct pds_vfio_lm_file *lm_file = NULL;
> +	unsigned long long npages;
> +	struct page **pages;
> +	int err = 0;
> +
> +	if (!size)
> +		return NULL;
> +
> +	/* Alloc file structure */
> +	lm_file = kzalloc(sizeof(*lm_file), GFP_KERNEL);
> +	if (!lm_file)
> +		return NULL;
> +
> +	/* Create file */
> +	lm_file->filep = anon_inode_getfile(name, fops, lm_file, flags);
> +	if (!lm_file->filep)
> +		goto err_get_file;
> +
> +	stream_open(lm_file->filep->f_inode, lm_file->filep);
> +	mutex_init(&lm_file->lock);
> +
> +	lm_file->size = size;
> +
> +	/* Allocate memory for file pages */
> +	npages = DIV_ROUND_UP_ULL(lm_file->size, PAGE_SIZE);
> +
> +	pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);
> +	if (!pages)
> +		goto err_alloc_pages;
> +
> +	for (unsigned long long i = 0; i < npages; i++) {
> +		pages[i] = alloc_page(GFP_KERNEL);
> +		if (!pages[i])
> +			goto err_alloc_page;
> +	}
> +
> +	lm_file->pages = pages;
> +	lm_file->npages = npages;
> +	lm_file->alloc_size = npages * PAGE_SIZE;
> +
> +	/* Create scatterlist of file pages to use for DMA mapping later */
> +	err = sg_alloc_table_from_pages(&lm_file->sg_table, pages, npages,
> +					0, size, GFP_KERNEL);
> +	if (err)
> +		goto err_alloc_sg_table;

This is the same basic thing the mlx5 driver does, you should move the
mlx5 code into some common place and just re-use it here.

> diff --git a/drivers/vfio/pci/pds/vfio_dev.h b/drivers/vfio/pci/pds/vfio_dev.h
> index 10557e8dc829..3f55861ffc7c 100644
> --- a/drivers/vfio/pci/pds/vfio_dev.h
> +++ b/drivers/vfio/pci/pds/vfio_dev.h
> @@ -7,10 +7,20 @@
>  #include <linux/pci.h>
>  #include <linux/vfio_pci_core.h>
>  
> +#include "lm.h"
> +
>  struct pds_vfio_pci_device {
>  	struct vfio_pci_core_device vfio_coredev;
>  	struct pci_dev *pdev;
>  	void *pdsc;
> +	struct device *coredev;

Why? If this is just &pdev->dev it it doesn't need to be in the struct

And pdev is just vfio_coredev->pdev, don't need to duplicate it either

Jason