Re: [PATCH v6 vfio 5/7] vfio/pds: Add support for dirty page tracking

[Simon Horman <simon.horman@corigine.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);
> +}

...