Messages in this thread Patch in this message | | | Subject | Re: [PATCH v7 3/5] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver | From | liulongfang <> | Date | Wed, 1 Feb 2023 11:24:31 +0800 |
| |
On 2023/1/27 3:28, Alex Williamson wrote: > On Fri, 20 Jan 2023 11:29:28 +0800 > Longfang Liu <liulongfang@huawei.com> wrote: > >> On the debugfs framework of VFIO, if the CONFIG_DEBUG_FS macro is >> enabled, the debug function is registered for the live migration driver >> of the HiSilicon accelerator device. >> >> On the basis of the original public debug function, a private debug >> function is added: >> >> mig_data file: used to get the migration data of the live migration device >> mig_attr file: used to get device attributes of the live migration device >> debug file: used to test for acquiring and writing device state data >> for VF device. >> >> Signed-off-by: Longfang Liu <liulongfang@huawei.com> >> --- >> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 282 ++++++++++++++++++ >> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 11 + >> 2 files changed, 293 insertions(+) >> >> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c >> index 6a219b01c965..96e2a021a956 100644 >> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c >> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c >> @@ -15,6 +15,7 @@ >> #include <linux/anon_inodes.h> >> >> #include "hisi_acc_vfio_pci.h" >> +#include "../vfio_pci_debugfs.h" >> >> /* Return 0 on VM acc device ready, -ETIMEDOUT hardware timeout */ >> static int qm_wait_dev_not_ready(struct hisi_qm *qm) >> @@ -606,6 +607,18 @@ hisi_acc_check_int_state(struct hisi_acc_vf_core_device *hisi_acc_vdev) >> } >> } >> >> +static void hisi_acc_vf_migf_save(struct hisi_acc_vf_migration_file *src_migf, >> + struct hisi_acc_vf_migration_file *dst_migf) >> +{ >> + if (!dst_migf) >> + return; >> + >> + dst_migf->disabled = false; >> + dst_migf->total_length = src_migf->total_length; >> + memcpy(&dst_migf->vf_data, &src_migf->vf_data, >> + sizeof(struct acc_vf_data)); >> +} >> + >> static void hisi_acc_vf_disable_fd(struct hisi_acc_vf_migration_file *migf) >> { >> mutex_lock(&migf->lock); >> @@ -618,12 +631,16 @@ static void hisi_acc_vf_disable_fd(struct hisi_acc_vf_migration_file *migf) >> static void hisi_acc_vf_disable_fds(struct hisi_acc_vf_core_device *hisi_acc_vdev) >> { >> if (hisi_acc_vdev->resuming_migf) { >> + hisi_acc_vf_migf_save(hisi_acc_vdev->resuming_migf, >> + hisi_acc_vdev->debug_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_vf_migf_save(hisi_acc_vdev->saving_migf, >> + hisi_acc_vdev->debug_migf); >> hisi_acc_vf_disable_fd(hisi_acc_vdev->saving_migf); >> fput(hisi_acc_vdev->saving_migf->filp); >> hisi_acc_vdev->saving_migf = NULL; >> @@ -1303,6 +1320,265 @@ 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_io(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; >> + struct device *dev = vdev->dev; >> + u64 data; >> + int ret; >> + >> + ret = qm_wait_dev_not_ready(vf_qm); >> + if (ret) >> + dev_err(dev, "VF device not ready!\n"); >> + >> + data = readl(vf_qm->io_base + QM_MB_CMD_SEND_BASE); >> + dev_info(dev, "debug mailbox val: 0x%llx\n", data); >> + >> + return ret; >> +} >> + >> +static int hisi_acc_vf_debug_resume(struct vfio_device *vdev) >> +{ >> + struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_get_vf_dev(vdev); >> + struct hisi_acc_vf_migration_file *migf = hisi_acc_vdev->debug_migf; >> + struct device *dev = vdev->dev; >> + int ret; >> + >> + ret = vf_qm_state_save(hisi_acc_vdev, migf); >> + if (ret) { >> + dev_err(dev, "failed to save device data!\n"); >> + return -EINVAL; >> + } >> + >> + ret = vf_qm_check_match(hisi_acc_vdev, migf); >> + if (ret) { >> + dev_err(dev, "failed to match the VF!\n"); >> + return -EINVAL; >> + } >> + >> + ret = vf_qm_load_data(hisi_acc_vdev, migf); >> + if (ret) { >> + dev_err(dev, "failed to recover the VF!\n"); >> + return -EINVAL; >> + } >> + >> + vf_qm_fun_reset(&hisi_acc_vdev->vf_qm); >> + dev_info(dev, "successful to resume device data!\n"); >> + >> + return 0; >> +} >> + >> +static int hisi_acc_vf_debug_save(struct vfio_device *vdev) >> +{ >> + struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_get_vf_dev(vdev); >> + struct hisi_acc_vf_migration_file *migf = hisi_acc_vdev->debug_migf; >> + struct device *dev = vdev->dev; >> + int ret; >> + >> + ret = vf_qm_state_save(hisi_acc_vdev, migf); >> + if (ret) { >> + dev_err(dev, "failed to save device data!\n"); >> + return -EINVAL; >> + } >> + dev_info(dev, "successful to save device data!\n"); >> + >> + return 0; >> +} >> + >> +static int >> +hisi_acc_vf_debug_operate(struct vfio_device *vdev, unsigned int cmd) >> +{ >> + int ret; >> + >> + switch (cmd) { >> + case STATE_SAVE: >> + ret = hisi_acc_vf_debug_save(vdev); >> + if (ret) >> + return ret; >> + break; >> + case STATE_RESUME: >> + ret = hisi_acc_vf_debug_resume(vdev); >> + if (ret) >> + return ret; >> + break; >> + case RW_IO_TEST: >> + ret = hisi_acc_vf_debug_io(vdev); >> + if (ret) >> + return ret; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int hisi_acc_vf_debug_check(struct vfio_device *vdev) >> +{ >> + struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_get_vf_dev(vdev); >> + struct hisi_acc_vf_migration_file *migf = hisi_acc_vdev->debug_migf; >> + enum vfio_device_mig_state state; >> + >> + if (!vdev->mig_ops || !migf) { >> + dev_err(vdev->dev, "device not support debugfs!\n"); >> + return -EINVAL; >> + } >> + >> + /* If device not opened, the debugfs operation will trigger calltrace */ >> + (void)vdev->mig_ops->migration_get_state(vdev, &state); >> + if (state == VFIO_DEVICE_STATE_ERROR || >> + state == VFIO_DEVICE_STATE_STOP) { >> + dev_err(vdev->dev, "device not opened!\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static ssize_t hisi_acc_vf_debug_write(struct file *filp, const char __user *buffer, >> + size_t count, loff_t *pos) >> +{ >> + struct vfio_device *vdev = filp->private_data; >> + char tbuf[VFIO_DEV_DBG_LEN]; >> + unsigned long cmd; >> + int len, ret; >> + >> + if (*pos) >> + return 0; >> + >> + if (count >= VFIO_DEV_DBG_LEN) >> + return -ENOSPC; >> + >> + ret = hisi_acc_vf_debug_check(vdev); >> + if (ret) >> + return -EINVAL; >> + >> + len = simple_write_to_buffer(tbuf, VFIO_DEV_DBG_LEN - 1, >> + pos, buffer, count); >> + if (len < 0 || len > VFIO_DEV_DBG_LEN - 1) >> + return -EINVAL; >> + tbuf[len] = '\0'; >> + if (kstrtoul(tbuf, 0, &cmd)) >> + return -EFAULT; >> + >> + ret = hisi_acc_vf_debug_operate(vdev, cmd); >> + if (ret) { >> + dev_err(vdev->dev, "device debug test failed!\n"); >> + return -EINVAL; >> + } >> + >> + return count; >> +} >> + >> +static const struct file_operations hisi_acc_vf_debug_fops = { >> + .owner = THIS_MODULE, >> + .open = simple_open, >> + .write = hisi_acc_vf_debug_write, >> +}; >> + >> +static void seq_print_hex_data(struct seq_file *seq, const void *buf, size_t len) >> +{ >> +#define HEX_LINE_SIZE 131 >> +#define HEX_ROW_SIZE 16 >> + unsigned char linebuf[HEX_LINE_SIZE]; >> + int i, linelen, remaining = len; >> + const u8 *ptr = buf; >> + >> + for (i = 0; i < len; i += HEX_ROW_SIZE) { >> + linelen = min(remaining, HEX_ROW_SIZE); >> + remaining -= HEX_ROW_SIZE; >> + >> + hex_dump_to_buffer(ptr + i, linelen, HEX_ROW_SIZE, 1, >> + linebuf, sizeof(linebuf), false); >> + >> + seq_printf(seq, "%s%.8x: %s\n", "Mig Data:", i, linebuf); >> + } >> +} >> + >> +static int hisi_acc_vf_data_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); >> + struct hisi_acc_vf_migration_file *debug_migf = hisi_acc_vdev->debug_migf; >> + size_t vf_data_sz = offsetofend(struct acc_vf_data, padding); >> + >> + if (debug_migf && debug_migf->total_length) >> + seq_print_hex_data(seq, (unsigned char *)&debug_migf->vf_data, >> + vf_data_sz); >> + else >> + seq_printf(seq, "%s\n", "device not migrated!"); >> + >> + return 0; >> +} >> + >> +static int hisi_acc_vf_attr_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); >> + struct hisi_acc_vf_migration_file *debug_migf = hisi_acc_vdev->debug_migf; >> + >> + if (debug_migf && debug_migf->total_length) { >> + seq_printf(seq, >> + "acc device:\n" >> + "device state: %d\n" >> + "device ready: %u\n" >> + "data valid: %d\n" >> + "data size: %lu\n", >> + hisi_acc_vdev->mig_state, >> + hisi_acc_vdev->vf_qm_state, >> + debug_migf->disabled, >> + debug_migf->total_length); >> + } else { >> + seq_printf(seq, "%s\n", "device not migrated!"); >> + } >> + >> + return 0; >> +} >> + >> +static int 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 device *dev = vdev->dev; >> + void *migf = NULL; >> + >> + if (!debugfs_initialized()) >> + return 0; >> + >> + vfio_pci_vf_debugfs_init(vdev); >> + >> + migf = kzalloc(sizeof(struct hisi_acc_vf_migration_file), GFP_KERNEL); >> + if (!migf) >> + return -ENOMEM; >> + >> + hisi_acc_vdev->debug_migf = migf; >> + >> + debugfs_create_devm_seqfile(dev, "mig_data", vdev->debug_root, >> + hisi_acc_vf_data_read); >> + debugfs_create_devm_seqfile(dev, "mig_attr", vdev->debug_root, >> + hisi_acc_vf_attr_read); >> + >> + debugfs_create_file("debug", 0200, vdev->debug_root, >> + vdev, &hisi_acc_vf_debug_fops); > > Naming is rather haphazard. I think this results in vfio_mig/state, > vfio_mig/mig_data, vfio_mig/mig_attr, vfio_mig/debug. If would be much > cleaner if we had either {state,data,attr,debug} under vfio/migration/ > or migration_{state,data,attr,debug} under vfio/ >
Following the previous instructions, I will change these to vfio-pci/migration_{state,data,attr,debug}.
>> + >> + return 0; >> +} >> + >> +static void hisi_acc_vf_debugfs_exit(struct hisi_acc_vf_core_device *hisi_acc_vdev) >> +{ >> + struct vfio_device *vdev = &hisi_acc_vdev->core_device.vdev; >> + >> + if (!debugfs_initialized()) >> + return; >> + >> + kfree(hisi_acc_vdev->debug_migf); >> + vfio_pci_vf_debugfs_exit(vdev); > > Ordering is suspect here, we're freeing data used by the debugfs > entries before removing the entries themselves, which suggests a > use-after-free issue. Thanks, >
Yes, this is a problem, I will fix it in the next version.
> Alex >
Thanks, Longfang.
>> +} >> + >> 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); >> @@ -1410,6 +1686,9 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device >> if (IS_ERR(hisi_acc_vdev)) >> return PTR_ERR(hisi_acc_vdev); >> >> + if (ops == &hisi_acc_vfio_pci_migrn_ops) >> + hisi_acc_vfio_debug_init(hisi_acc_vdev); >> + >> dev_set_drvdata(&pdev->dev, &hisi_acc_vdev->core_device); >> ret = vfio_pci_core_register_device(&hisi_acc_vdev->core_device); >> if (ret) >> @@ -1417,6 +1696,8 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device >> return 0; >> >> out_put_vdev: >> + if (ops == &hisi_acc_vfio_pci_migrn_ops) >> + hisi_acc_vf_debugfs_exit(hisi_acc_vdev); >> vfio_put_device(&hisi_acc_vdev->core_device.vdev); >> return ret; >> } >> @@ -1425,6 +1706,7 @@ static void hisi_acc_vfio_pci_remove(struct pci_dev *pdev) >> { >> struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev); >> >> + hisi_acc_vf_debugfs_exit(hisi_acc_vdev); >> vfio_pci_core_unregister_device(&hisi_acc_vdev->core_device); >> vfio_put_device(&hisi_acc_vdev->core_device.vdev); >> } >> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h >> index dcabfeec6ca1..ef50b12f018d 100644 >> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h >> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h >> @@ -49,6 +49,14 @@ >> #define QM_EQC_DW0 0X8000 >> #define QM_AEQC_DW0 0X8020 >> >> +#define VFIO_DEV_DBG_LEN 256 >> + >> +enum mig_debug_cmd { >> + STATE_SAVE, >> + STATE_RESUME, >> + RW_IO_TEST, >> +}; >> + >> struct acc_vf_data { >> #define QM_MATCH_SIZE offsetofend(struct acc_vf_data, qm_rsv_state) >> /* QM match information */ >> @@ -113,5 +121,8 @@ struct hisi_acc_vf_core_device { >> spinlock_t reset_lock; >> struct hisi_acc_vf_migration_file *resuming_migf; >> struct hisi_acc_vf_migration_file *saving_migf; >> + >> + /* For debugfs */ >> + struct hisi_acc_vf_migration_file *debug_migf; >> }; >> #endif /* HISI_ACC_VFIO_PCI_H */ > > . >
| |