[Linuxarm] Re: [PATCH v12 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver

-----Original Message----- From: liulongfang liulongfang@huawei.com Sent: Tuesday, November 5, 2024 3:53 AM To: alex.williamson@redhat.com; jgg@nvidia.com; Shameerali Kolothum Thodi shameerali.kolothum.thodi@huawei.com; Jonathan Cameron jonathan.cameron@huawei.com Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; linuxarm@openeuler.org; liulongfang liulongfang@huawei.com Subject: [PATCH v12 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver

Hi,

Few minor comments below. Please don't re-spin just for these yet. Please wait for others to review as well.

Thanks, Shameer

diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c index a8c53952d82e..7728c9745b9d 100644 --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c @@ -627,15 +627,30 @@ static void hisi_acc_vf_disable_fd(struct hisi_acc_vf_migration_file *migf) mutex_unlock(&migf->lock); }

+static void hisi_acc_debug_migf_copy(struct hisi_acc_vf_core_device *hisi_acc_vdev,

• struct hisi_acc_vf_migration_file *src_migf)

Alignment should match open parenthesis here.

+{

• struct hisi_acc_vf_migration_file *dst_migf = hisi_acc_vdev-

...

debug_migf;

• if (!dst_migf)

• return;
  

• dst_migf->total_length = src_migf->total_length;
• memcpy(&dst_migf->vf_data, &src_migf->vf_data,

• sizeof(struct acc_vf_data));
  

Here too, alignment not correct. It is better to run, ./scripts/checkpatch --strict on these patches if you haven't done already.

+}

static void hisi_acc_vf_disable_fds(struct hisi_acc_vf_core_device *hisi_acc_vdev) { if (hisi_acc_vdev->resuming_migf) {

• hisi_acc_debug_migf_copy(hisi_acc_vdev, hisi_acc_vdev-
  

...

resuming_migf);

hisi_acc_vf_disable_fd(hisi_acc_vdev->resuming_migf);
fput(hisi_acc_vdev->resuming_migf->filp);
hisi_acc_vdev->resuming_migf = NULL;

}

if (hisi_acc_vdev->saving_migf) {

• hisi_acc_debug_migf_copy(hisi_acc_vdev, hisi_acc_vdev-
  

...

saving_migf);

hisi_acc_vf_disable_fd(hisi_acc_vdev->saving_migf);
fput(hisi_acc_vdev->saving_migf->filp);
hisi_acc_vdev->saving_migf = NULL;

@@ -1294,6 +1309,129 @@ static long hisi_acc_vfio_pci_ioctl(struct vfio_device *core_vdev, unsigned int return vfio_pci_core_ioctl(core_vdev, cmd, arg); }

+static int hisi_acc_vf_debug_check(struct seq_file *seq, struct vfio_device *vdev) +{

• struct hisi_acc_vf_core_device *hisi_acc_vdev =

hisi_acc_get_vf_dev(vdev);

• struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm;
• int ret;
• lockdep_assert_held(&hisi_acc_vdev->open_mutex);
• /*

• * When the device is not opened, the io_base is not mapped.
  

• * The driver cannot perform device read and write operations.
  

• */
  

• if (!hisi_acc_vdev->dev_opened) {

• seq_printf(seq, "device not opened!\n");
  

• return -EINVAL;
  

• }
• ret = qm_wait_dev_not_ready(vf_qm);
• if (ret) {

• seq_printf(seq, "VF device not ready!\n");
  

• return -EBUSY;
  

• }
• return 0;

+}

+static int hisi_acc_vf_debug_cmd(struct seq_file *seq, void *data) +{

• struct device *vf_dev = seq->private;
• struct vfio_pci_core_device *core_device =

dev_get_drvdata(vf_dev);

• struct vfio_device *vdev = &core_device->vdev;
• struct hisi_acc_vf_core_device *hisi_acc_vdev =

hisi_acc_get_vf_dev(vdev);

• struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm;
• u64 value;
• int ret;
• mutex_lock(&hisi_acc_vdev->open_mutex);
• ret = hisi_acc_vf_debug_check(seq, vdev);
• if (ret) {

• mutex_unlock(&hisi_acc_vdev->open_mutex);
  

• return ret;
  

• }
• value = readl(vf_qm->io_base + QM_MB_CMD_SEND_BASE);
• if (value == QM_MB_CMD_NOT_READY) {

• mutex_unlock(&hisi_acc_vdev->open_mutex);
  

• seq_printf(seq, "mailbox cmd channel not ready!\n");
  

• return -EINVAL;
  

• }
• mutex_unlock(&hisi_acc_vdev->open_mutex);
• seq_printf(seq, "mailbox cmd channel ready!\n");
• return 0;

+}

+static int hisi_acc_vf_dev_read(struct seq_file *seq, void *data) +{

• struct device *vf_dev = seq->private;
• struct vfio_pci_core_device *core_device =

dev_get_drvdata(vf_dev);

• struct vfio_device *vdev = &core_device->vdev;
• struct hisi_acc_vf_core_device *hisi_acc_vdev =

hisi_acc_get_vf_dev(vdev);

• size_t vf_data_sz = offsetofend(struct acc_vf_data, padding);
• struct acc_vf_data *vf_data;
• int ret;
• mutex_lock(&hisi_acc_vdev->open_mutex);
• ret = hisi_acc_vf_debug_check(seq, vdev);
• if (ret) {

• mutex_unlock(&hisi_acc_vdev->open_mutex);
  

• return ret;
  

• }
• mutex_lock(&hisi_acc_vdev->state_mutex);
• vf_data = kzalloc(sizeof(struct acc_vf_data), GFP_KERNEL);
• if (!vf_data) {

• ret = -ENOMEM;
  

• goto mutex_release;
  

• }
• vf_data->vf_qm_state = hisi_acc_vdev->vf_qm_state;
• ret = vf_qm_read_data(&hisi_acc_vdev->vf_qm, vf_data);
• if (ret)

• goto migf_err;
  

• seq_hex_dump(seq, "Dev Data:", DUMP_PREFIX_OFFSET, 16, 1,

•      (const void *)vf_data, vf_data_sz, false);
  

• seq_printf(seq,

•    "guest driver load: %u\n"
  

•    "data size: %lu\n",
  

•    hisi_acc_vdev->vf_qm_state,
  

•    sizeof(struct acc_vf_data));
  

There was a suggestion to add a comment here to describe vf_qm_state better. May be something like,

vf_qm_state here indicates whether the Guest has loaded the driver for the ACC VF device or not.

+migf_err:

• kfree(vf_data);

+mutex_release:

• mutex_unlock(&hisi_acc_vdev->state_mutex);
• mutex_unlock(&hisi_acc_vdev->open_mutex);
• return ret;

+}

+static int hisi_acc_vf_migf_read(struct seq_file *seq, void *data) +{

• struct device *vf_dev = seq->private;
• struct vfio_pci_core_device *core_device =

dev_get_drvdata(vf_dev);

• struct vfio_device *vdev = &core_device->vdev;
• struct hisi_acc_vf_core_device *hisi_acc_vdev =

hisi_acc_get_vf_dev(vdev);

• size_t vf_data_sz = offsetofend(struct acc_vf_data, padding);
• struct hisi_acc_vf_migration_file *debug_migf = hisi_acc_vdev-

...

debug_migf;

• /* Check whether the live migration operation has been performed

*/

• if (debug_migf->total_length < QM_MATCH_SIZE) {

• seq_printf(seq, "device not migrated!\n");
  

• return -EAGAIN;
  

• }
• seq_hex_dump(seq, "Mig Data:", DUMP_PREFIX_OFFSET, 16, 1,

•      (const void *)&debug_migf->vf_data, vf_data_sz, false);
  

• seq_printf(seq, "migrate data length: %lu\n", debug_migf-

...

total_length);

• return 0;

+}

static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev) { struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_get_vf_dev(core_vdev); @@ -1305,12 +1443,16 @@ static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev) return ret;

if (core_vdev->mig_ops) {

• mutex_lock(&hisi_acc_vdev->open_mutex);
  

  ret = hisi_acc_vf_qm_init(hisi_acc_vdev); if (ret) {

• 	mutex_unlock(&hisi_acc_vdev->open_mutex);
  vfio_pci_core_disable(vdev);
  return ret;
  

  } hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;

• hisi_acc_vdev->dev_opened = true;
  

• mutex_unlock(&hisi_acc_vdev->open_mutex);
  

  }

  vfio_pci_core_finish_enable(vdev);

@@ -1322,7 +1464,10 @@ static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev) struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_get_vf_dev(core_vdev); struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm;

• mutex_lock(&hisi_acc_vdev->open_mutex);
• hisi_acc_vdev->dev_opened = false; iounmap(vf_qm->io_base);
• mutex_unlock(&hisi_acc_vdev->open_mutex); vfio_pci_core_close_device(core_vdev);

}

@@ -1342,6 +1487,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev) hisi_acc_vdev->pf_qm = pf_qm; hisi_acc_vdev->vf_dev = pdev; mutex_init(&hisi_acc_vdev->state_mutex);

• mutex_init(&hisi_acc_vdev->open_mutex);

  core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY |

VFIO_MIGRATION_PRE_COPY; core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops; @@ -1387,6 +1533,48 @@ static const struct vfio_device_ops hisi_acc_vfio_pci_ops = { .detach_ioas = vfio_iommufd_physical_detach_ioas, };

+static void hisi_acc_vfio_debug_init(struct hisi_acc_vf_core_device *hisi_acc_vdev) +{

• struct vfio_device *vdev = &hisi_acc_vdev->core_device.vdev;
• struct dentry *vfio_dev_migration = NULL;
• struct dentry *vfio_hisi_acc = NULL;
• struct device *dev = vdev->dev;
• void *migf = NULL;
• if (!debugfs_initialized() ||

•    !IS_ENABLED(CONFIG_VFIO_DEBUGFS))
  

• return;
  

• if (vdev->ops != &hisi_acc_vfio_pci_migrn_ops)

• return;
  

• vfio_dev_migration = debugfs_lookup("migration", vdev-

...

debug_root);

• if (!vfio_dev_migration) {

• dev_err(dev, "failed to lookup migration debugfs file!\n");
  

• return;
  

• }
• migf = kzalloc(sizeof(struct hisi_acc_vf_migration_file),

GFP_KERNEL);

• if (!migf)

• return;
  

• hisi_acc_vdev->debug_migf = migf;
• vfio_hisi_acc = debugfs_create_dir("hisi_acc", vfio_dev_migration);
• debugfs_create_devm_seqfile(dev, "dev_data", vfio_hisi_acc,

• 		    hisi_acc_vf_dev_read);
  

• debugfs_create_devm_seqfile(dev, "migf_data", vfio_hisi_acc,

• 		    hisi_acc_vf_migf_read);
  

• debugfs_create_devm_seqfile(dev, "cmd_state", vfio_hisi_acc,

• 		    hisi_acc_vf_debug_cmd);
  

+}

+static void hisi_acc_vf_debugfs_exit(struct hisi_acc_vf_core_device *hisi_acc_vdev) +{

• /* If migrn_ops is not used, kfree(NULL) is valid */

The above comment is not required. Please remove.

• kfree(hisi_acc_vdev->debug_migf);
• hisi_acc_vdev->debug_migf = NULL;

+}