| 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
| ||
|
Message-ID: <259c5f0d-24bf-dfd4-a1c5-102944aecd4f@amd.com>
Date: Sat, 22 Jul 2023 00:17:34 -0700
From: Brett Creeley <bcreeley@....com>
To: "Tian, Kevin" <kevin.tian@...el.com>,
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
On 7/21/2023 2:15 AM, Tian, Kevin wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
>> 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.
I will take another look at this and see what makes the most sense.
>
>> --- 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.
I'd prefer to just leave it lm.o as I don't see a big benefit changing
it to migration.o.
>
>> +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.
IMO dev_warn() is good enough here. I don't plan on changing this.
>
>> +
>> +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?
There isn't device specific setup, but the other drivers were different
enough that it wasn't a straight forward task. I think it might be
possible to refactor the drivers to some common functionality here, but
IMO this seems like a task that can be further explored once this series
is merged.
Thanks for the review.
Brett
Powered by blists - more mailing lists