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 5/7] vfio/pds: Add support for dirty page tracking
Date: Wed, 29 Mar 2023 10:58:51 +0200 [thread overview]
Message-ID: <ZCP9y/cmfB4acIOV@corigine.com> (raw)
In-Reply-To: <20230327200553.13951-6-brett.creeley@amd.com>
On Mon, Mar 27, 2023 at 01:05:51PM -0700, Brett Creeley wrote:
> In order to support dirty page tracking, the driver has to implement
> the VFIO subsystem's vfio_log_ops. This includes log_start, log_stop,
> and log_read_and_clear.
>
> All of the tracker resources are allocated and dirty tracking on the
> device is started during log_start. The resources are cleaned up and
> dirty tracking on the device is stopped during log_stop. The dirty
> pages are determined and reported during log_read_and_clear.
>
> In order to support these callbacks admin queue commands are used.
> All of the adminq queue command structures and implementations
> are included as part of this patch.
>
> PDS_LM_CMD_DIRTY_STATUS is added to query the current status of
> dirty tracking on the device. This includes if it's enabled (i.e.
> number of regions being tracked from the device's perspective) and
> the maximum number of regions supported from the device's perspective.
>
> PDS_LM_CMD_DIRTY_ENABLE is added to enable dirty tracking on the
> specified number of regions and their iova ranges.
>
> PDS_LM_CMD_DIRTY_DISABLE is added to disable dirty tracking for all
> regions on the device.
>
> PDS_LM_CMD_READ_SEQ and PDS_LM_CMD_DIRTY_WRITE_ACK are added to
> support reading and acknowledging the currently dirtied pages.
>
> Signed-off-by: Brett Creeley <brett.creeley@amd•com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd•com>
Hi Brett,
overall this patch looks clean to me.
I've made a minor comment inline, which you may wish to consider
if you need to respin the series for some other reason.
> diff --git a/drivers/vfio/pci/pds/dirty.c b/drivers/vfio/pci/pds/dirty.c
...
> +static void
> +pds_vfio_print_guest_region_info(struct pds_vfio_pci_device *pds_vfio,
> + u8 max_regions)
> +{
> + int len = max_regions * sizeof(struct pds_lm_dirty_region_info);
> + struct pds_lm_dirty_region_info *region_info;
> + struct pci_dev *pdev = pds_vfio->pdev;
> + dma_addr_t regions_dma;
> + u8 num_regions;
> + int err;
> +
> + region_info = kcalloc(max_regions,
> + sizeof(struct pds_lm_dirty_region_info),
> + GFP_KERNEL);
> + if (!region_info)
> + return;
> +
> + regions_dma = dma_map_single(pds_vfio->coredev, region_info, len,
> + DMA_FROM_DEVICE);
> + if (dma_mapping_error(pds_vfio->coredev, regions_dma)) {
> + kfree(region_info);
> + return;
nit: I think it would be more idiomatic to use a goto label here, say:
goto err_out;
> + }
> +
> + err = pds_vfio_dirty_status_cmd(pds_vfio, regions_dma,
> + &max_regions, &num_regions);
> + dma_unmap_single(pds_vfio->coredev, regions_dma, len, DMA_FROM_DEVICE);
and here:
if (err)
goto err_out;
> +
> + if (!err) {
And move this out of a conditional, into the main block of the function.
> + int i;
> +
> + for (i = 0; i < num_regions; i++)
The scope of i can handily be limited, now that the kernel has moved to C99.
And it might be slightly nicer to use an unsigned type for it,
I don't think it can ever be negative.
for (unsigned i = 0; i < num_regions; i++)
> + dev_dbg(&pdev->dev, "region_info[%d]: dma_base 0x%llx page_count %u page_size_log2 %u\n",
> + i, le64_to_cpu(region_info[i].dma_base),
> + le32_to_cpu(region_info[i].page_count),
> + region_info[i].page_size_log2);
> + }
> +
err_out:
> + kfree(region_info);
> +}
...
next prev parent reply other threads:[~2023-03-29 9:00 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
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 [this message]
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=ZCP9y/cmfB4acIOV@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