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
