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@xxxxxxx>
> Signed-off-by: Shannon Nelson <shannon.nelson@xxxxxxx>
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);
> +}
...
