From: Simon Horman <simon.horman@corigine•com>
To: Brett Creeley <brett.creeley@amd•com>
Cc: kvm@vger•kernel.org, netdev@vger•kernel.org,
alex.williamson@redhat•com, jgg@nvidia•com, yishaih@nvidia•com,
shameerali.kolothum.thodi@huawei•com, kevin.tian@intel•com,
shannon.nelson@amd•com, drivers@pensando•io
Subject: Re: [PATCH v6 vfio 4/7] vfio/pds: Add VFIO live migration support
Date: Wed, 29 Mar 2023 14:24:40 +0200 [thread overview]
Message-ID: <ZCQuCPG864f0R+9s@corigine.com> (raw)
In-Reply-To: <20230327200553.13951-5-brett.creeley@amd.com>
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@amd•com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd•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;
> +}
...
next prev parent reply other threads:[~2023-03-29 12:25 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-27 20:05 [PATCH v6 vfio 0/7] pds_vfio driver Brett Creeley
2023-03-27 20:05 ` [PATCH v6 vfio 1/7] vfio: Commonize combine_ranges for use in other VFIO drivers Brett Creeley
2023-03-29 12:12 ` Simon Horman
2023-03-29 19:24 ` Brett Creeley
2023-03-27 20:05 ` [PATCH v6 vfio 2/7] vfio/pds: Initial support for pds_vfio VFIO driver Brett Creeley
2023-03-27 20:05 ` [PATCH v6 vfio 3/7] vfio/pds: register with the pds_core PF Brett Creeley
2023-03-27 20:05 ` [PATCH v6 vfio 4/7] vfio/pds: Add VFIO live migration support Brett Creeley
2023-03-29 12:24 ` Simon Horman [this message]
2023-03-27 20:05 ` [PATCH v6 vfio 5/7] vfio/pds: Add support for dirty page tracking Brett Creeley
2023-03-29 8:58 ` Simon Horman
2023-03-27 20:05 ` [PATCH v6 vfio 6/7] vfio/pds: Add support for firmware recovery Brett Creeley
2023-03-27 20:05 ` [PATCH v6 vfio 7/7] vfio/pds: Add Kconfig and documentation Brett Creeley
2023-03-29 17:49 ` Simon Horman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZCQuCPG864f0R+9s@corigine.com \
--to=simon.horman@corigine$(echo .)com \
--cc=alex.williamson@redhat$(echo .)com \
--cc=brett.creeley@amd$(echo .)com \
--cc=drivers@pensando$(echo .)io \
--cc=jgg@nvidia$(echo .)com \
--cc=kevin.tian@intel$(echo .)com \
--cc=kvm@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=shameerali.kolothum.thodi@huawei$(echo .)com \
--cc=shannon.nelson@amd$(echo .)com \
--cc=yishaih@nvidia$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox