netdev - RE: [PATCH v12 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]

Date: Fri, 21 Jul 2023 09:15:13 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Brett Creeley <brett.creeley@....com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"alex.williamson@...hat.com" <alex.williamson@...hat.com>, "jgg@...dia.com"
	<jgg@...dia.com>, "yishaih@...dia.com" <yishaih@...dia.com>,
	"shameerali.kolothum.thodi@...wei.com" <shameerali.kolothum.thodi@...wei.com>
CC: "shannon.nelson@....com" <shannon.nelson@....com>
Subject: RE: [PATCH v12 vfio 4/7] vfio/pds: Add VFIO live migration support

> From: Brett Creeley <brett.creeley@....com>
> Sent: Thursday, July 20, 2023 6:35 AM
> 
> PDS_LM_CMD_STATUS is added to determine the exact size of the VF
> device state data.

based on the description PDS_LM_CMD_STATE_SIZE is clearer.

> --- a/drivers/vfio/pci/pds/Makefile
> +++ b/drivers/vfio/pci/pds/Makefile
> @@ -5,5 +5,6 @@ obj-$(CONFIG_PDS_VFIO_PCI) += pds-vfio-pci.o
> 
>  pds-vfio-pci-y := \
>  	cmds.o		\
> +	lm.o		\

nit. "migration.o" is more readable.

> +static int pds_vfio_client_adminq_cmd(struct pds_vfio_pci_device *pds_vfio,
> +				      union pds_core_adminq_cmd *req,
> +				      union pds_core_adminq_comp *resp,
> +				      bool fast_poll)
> +{
> +	union pds_core_adminq_cmd cmd = {};
> +	int err;
> +
> +	/* Wrap the client request */
> +	cmd.client_request.opcode = PDS_AQ_CMD_CLIENT_CMD;
> +	cmd.client_request.client_id = cpu_to_le16(pds_vfio->client_id);
> +	memcpy(cmd.client_request.client_cmd, req,
> +	       sizeof(cmd.client_request.client_cmd));
> +
> +	err = pdsc_adminq_post(pds_vfio->pdsc, &cmd, resp, fast_poll);
> +	if (err && err != -EAGAIN)
> +		dev_info(pds_vfio_to_dev(pds_vfio),
> +			 "client admin cmd failed: %pe\n", ERR_PTR(err));

dev_err()

> +void pds_vfio_send_host_vf_lm_status_cmd(struct pds_vfio_pci_device
> *pds_vfio,
> +					 enum pds_lm_host_vf_status
> vf_status)
> +{
> +	union pds_core_adminq_cmd cmd = {
> +		.lm_host_vf_status = {
> +			.opcode = PDS_LM_CMD_HOST_VF_STATUS,
> +			.vf_id = cpu_to_le16(pds_vfio->vf_id),
> +			.status = vf_status,
> +		},
> +	};
> +	struct device *dev = pds_vfio_to_dev(pds_vfio);
> +	union pds_core_adminq_comp comp = {};
> +	int err;
> +
> +	dev_dbg(dev, "vf%u: Set host VF LM status: %u", pds_vfio->vf_id,
> +		vf_status);
> +	if (vf_status != PDS_LM_STA_IN_PROGRESS &&
> +	    vf_status != PDS_LM_STA_NONE) {
> +		dev_warn(dev, "Invalid host VF migration status, %d\n",
> +			 vf_status);
> +		return;
> +	}

WARN_ON() as it's a driver bug if passing in unsupported status code.

> +
> +static struct pds_vfio_lm_file *
> +pds_vfio_get_lm_file(const struct file_operations *fops, int flags, u64 size)
> +{
> +	struct pds_vfio_lm_file *lm_file = NULL;
> +	unsigned long long npages;
> +	struct page **pages;
> +	void *page_mem;
> +	const void *p;
> +
> +	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("pds_vfio_lm", fops, lm_file, flags);
> +	if (!lm_file->filep)
> +		goto out_free_file;
> +
> +	stream_open(lm_file->filep->f_inode, lm_file->filep);
> +	mutex_init(&lm_file->lock);
> +
> +	/* prevent file from being released before we are done with it */
> +	get_file(lm_file->filep);
> +
> +	/* Allocate memory for file pages */
> +	npages = DIV_ROUND_UP_ULL(size, PAGE_SIZE);
> +	pages = kmalloc_array(npages, sizeof(*pages), GFP_KERNEL);
> +	if (!pages)
> +		goto out_put_file;
> +
> +	page_mem = kvzalloc(ALIGN(size, PAGE_SIZE), GFP_KERNEL);
> +	if (!page_mem)
> +		goto out_free_pages_array;
> +
> +	p = page_mem - offset_in_page(page_mem);
> +	for (unsigned long long i = 0; i < npages; i++) {
> +		if (is_vmalloc_addr(p))
> +			pages[i] = vmalloc_to_page(p);
> +		else
> +			pages[i] = kmap_to_page((void *)p);
> +		if (!pages[i])
> +			goto out_free_page_mem;
> +
> +		p += PAGE_SIZE;
> +	}
> +
> +	/* Create scatterlist of file pages to use for DMA mapping later */
> +	if (sg_alloc_table_from_pages(&lm_file->sg_table, pages, npages, 0,
> +				      size, GFP_KERNEL))
> +		goto out_free_page_mem;
> +
> +	lm_file->size = size;
> +	lm_file->pages = pages;
> +	lm_file->npages = npages;
> +	lm_file->page_mem = page_mem;
> +	lm_file->alloc_size = npages * PAGE_SIZE;
> +
> +	return lm_file;
> +
> +out_free_page_mem:
> +	kvfree(page_mem);
> +out_free_pages_array:
> +	kfree(pages);
> +out_put_file:
> +	fput(lm_file->filep);
> +	mutex_destroy(&lm_file->lock);
> +out_free_file:
> +	kfree(lm_file);
> +
> +	return NULL;
> +}

I wonder whether the logic about migration file can be generalized.
It's not very maintainable to have every migration driver implementing
their own code for similar functions.

Did I overlook any device specific setup required here?