Add a set of debugfs for the hisilicon accelerator live migration
driver. This debugfs is used to test the functions of the qemu
tools, driver software and accelerator devices involved in the live
migration step by step.
Get software data or accelerator devices data in the event of
a live migration failure.
Longfang Liu (2):
hisi_acc_vfio_pci: Add debugfs to migration driver
Documentation: Add debugfs for hisi_acc_vfio_pci
.../ABI/testing/debugfs-hisi-migration | 16 +
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 296 ++++++++++++++++++
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 14 +
3 files changed, 326 insertions(+)
create mode 100644 Documentation/ABI/testing/debugfs-hisi-migration
--
2.24.0
Add a debugfs document description file to help users understand
how to use the accelerator live migration driver's debugfs.
Signed-off-by: Longfang Liu <[email protected]>
---
Documentation/ABI/testing/debugfs-hisi-migration | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
create mode 100644 Documentation/ABI/testing/debugfs-hisi-migration
diff --git a/Documentation/ABI/testing/debugfs-hisi-migration b/Documentation/ABI/testing/debugfs-hisi-migration
new file mode 100644
index 000000000000..36f0882c3a52
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-hisi-migration
@@ -0,0 +1,16 @@
+What: /sys/kernel/debug/hisi_vfio_acc/<bdf>/state
+Date: Sep 2022
+Contact: [email protected]
+Description: Cat the status of the live migration of the current VF device.
+ The status of these live migrations includes:
+ Error, RUNNING, STOP, STOP and COPYING, RESUMING.
+
+What: /sys/kernel/debug/hisi_vfio_acc/<bdf>/debug
+Date: Sep 2022
+Contact: [email protected]
+Description: This debug file supports "cat" read operations and "echo"
+ write operations. The read debug file operation will return the
+ command description of the write operation. By writing different
+ commands, different test functions can be performed.
+ The specific operation method can be obtained through the
+ "cat debug" command.
--
2.24.0
There are multiple devices, software and operational steps involved
in the process of live migration. An error occurred on any node may
cause the live migration operation to fail.
This complex process makes it very difficult to locate and analyze
the cause when the function fails.
In order to quickly locate the cause of the problem when the
live migration fails, I added a set of debugfs to the accelerator
live migration driver.
+-----------------------------------+
| |
| QEMU |
| |
+---+--^--------------------+--^----+
| | | |
| | | |
+---v--+----+ +---v--+----+
| | | |
| src VF | | dest VF |
| | | |
+---+--^----+ +---+--^----+
| | | |
| | | |
+---v--+----+ +---v--+----+
| Debugfs | | Debugfs |
+-----+-----+ +-----+-----+
|state|debug| |state|debug|
+-----+-----+ +-----+-----+
This set of debugfs will create two files for each VF device:
a state file and a debug file.
The migration status of the current VF device can be obtained by
reading the status file.
The live migration function of the current device can be tested by
operating the debug file, and the functional status of the equipment
and software at each stage can be tested step by step without
performing the complete live migration function. And after the live
migration is performed, the migration device data of the live migration
can be obtained through the debug file.
Signed-off-by: Longfang Liu <[email protected]>
---
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 296 ++++++++++++++++++
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 14 +
2 files changed, 310 insertions(+)
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
index 39eeca18a0f7..bed54e8e3a64 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
@@ -3,6 +3,7 @@
* Copyright (c) 2021, HiSilicon Ltd.
*/
+#include <linux/anon_inodes.h>
#include <linux/device.h>
#include <linux/eventfd.h>
#include <linux/file.h>
@@ -16,6 +17,9 @@
#include "hisi_acc_vfio_pci.h"
+static struct dentry *hisi_acc_debugfs_root;
+static atomic_t hisi_acc_root_ref;
+
/* Return 0 on VM acc device ready, -ETIMEDOUT hardware timeout */
static int qm_wait_dev_not_ready(struct hisi_qm *qm)
{
@@ -609,6 +613,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);
@@ -621,12 +637,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;
@@ -1176,6 +1196,277 @@ 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_create(struct hisi_acc_vf_core_device *hisi_acc_vdev)
+{
+ struct hisi_acc_vf_migration_file *migf;
+
+ migf = kzalloc(sizeof(*migf), GFP_KERNEL);
+ if (!migf)
+ return -ENOMEM;
+
+ migf->disabled = false;
+ hisi_acc_vdev->debug_migf = migf;
+ mutex_init(&migf->lock);
+
+ return 0;
+}
+
+static void hisi_acc_vf_debug_release(struct hisi_acc_vf_migration_file *migf)
+{
+ migf->disabled = true;
+ migf->total_length = 0;
+ mutex_destroy(&migf->lock);
+ kfree(migf);
+}
+
+static int hisi_acc_vf_debug_test(struct hisi_acc_vf_core_device *hisi_acc_vdev)
+{
+ struct device *dev = &hisi_acc_vdev->vf_dev->dev;
+ struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm;
+ u64 data;
+ int ret;
+
+ data = readl(vf_qm->io_base + QM_MB_CMD_SEND_BASE);
+ dev_info(dev, "debug mailbox val: 0x%llx\n", data);
+
+ ret = qm_wait_dev_not_ready(vf_qm);
+ if (ret)
+ dev_err(dev, "VF device not ready!\n");
+
+ return ret;
+}
+
+static ssize_t hisi_acc_vf_debug_read(struct file *filp, char __user *buffer,
+ size_t count, loff_t *pos)
+{
+ char buf[VFIO_DEV_DBG_LEN];
+ int len;
+
+ len = scnprintf(buf, VFIO_DEV_DBG_LEN, "%s\n",
+ "echo 0: test vf state save\n"
+ "echo 1: test vf state resume\n"
+ "echo 2: test vf send mailbox\n"
+ "echo 3: dump vf config data\n"
+ "echo 4: dump vf migration state\n");
+
+ return simple_read_from_buffer(buffer, count, pos, buf, len);
+}
+
+static void hisi_acc_vf_dev_data_dump(struct hisi_acc_vf_core_device *hisi_acc_vdev)
+{
+ size_t vf_data_sz = offsetofend(struct acc_vf_data, padding);
+ struct device *dev = &hisi_acc_vdev->vf_dev->dev;
+
+ if (hisi_acc_vdev->debug_migf &&
+ hisi_acc_vdev->debug_migf->total_length) {
+ print_hex_dump(KERN_INFO, "dev mig data:",
+ DUMP_PREFIX_OFFSET, 16, 1,
+ (u8 *)&hisi_acc_vdev->debug_migf->vf_data,
+ vf_data_sz, false);
+ } else {
+ dev_info(dev, "device not migrated!\n");
+ }
+}
+
+static void hisi_acc_vf_dev_attr_show(struct hisi_acc_vf_core_device *hisi_acc_vdev)
+{
+ struct device *dev = &hisi_acc_vdev->vf_dev->dev;
+
+ if (hisi_acc_vdev->debug_migf &&
+ hisi_acc_vdev->debug_migf->total_length) {
+ dev_info(dev, "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,
+ hisi_acc_vdev->debug_migf->disabled,
+ hisi_acc_vdev->debug_migf->total_length);
+ } else {
+ dev_info(dev, "device not migrated!\n");
+ }
+}
+
+static int hisi_acc_vf_debug_resume(struct hisi_acc_vf_core_device *hisi_acc_vdev)
+{
+ struct hisi_acc_vf_migration_file *migf = hisi_acc_vdev->debug_migf;
+ struct device *dev = &hisi_acc_vdev->vf_dev->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 hisi_acc_vf_core_device *hisi_acc_vdev)
+{
+ struct hisi_acc_vf_migration_file *migf = hisi_acc_vdev->debug_migf;
+ struct device *dev = &hisi_acc_vdev->vf_dev->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 ssize_t hisi_acc_vf_debug_write(struct file *filp, const char __user *buffer,
+ size_t count, loff_t *pos)
+{
+ struct hisi_acc_vf_core_device *hisi_acc_vdev = filp->private_data;
+ char tbuf[VFIO_DEV_DBG_LEN];
+ unsigned long val;
+ int len, ret;
+
+ if (*pos)
+ return 0;
+
+ if (count >= VFIO_DEV_DBG_LEN)
+ return -ENOSPC;
+
+ 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, &val))
+ return -EFAULT;
+
+ switch (val) {
+ case STATE_SAVE:
+ ret = hisi_acc_vf_debug_save(hisi_acc_vdev);
+ if (ret)
+ return ret;
+ break;
+ case STATE_RESUME:
+ ret = hisi_acc_vf_debug_resume(hisi_acc_vdev);
+ if (ret)
+ return ret;
+ break;
+ case MB_TEST:
+ ret = hisi_acc_vf_debug_test(hisi_acc_vdev);
+ if (ret)
+ return -EINVAL;
+ break;
+ case MIG_DATA_DUMP:
+ hisi_acc_vf_dev_data_dump(hisi_acc_vdev);
+ break;
+ case MIG_DEV_SHOW:
+ hisi_acc_vf_dev_attr_show(hisi_acc_vdev);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+static const struct file_operations hisi_acc_vf_debug_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = hisi_acc_vf_debug_read,
+ .write = hisi_acc_vf_debug_write,
+};
+
+static ssize_t hisi_acc_vf_state_read(struct file *filp, char __user *buffer,
+ size_t count, loff_t *pos)
+{
+ struct hisi_acc_vf_core_device *hisi_acc_vdev = filp->private_data;
+ char buf[VFIO_DEV_DBG_LEN];
+ u32 state;
+ int len;
+
+ state = hisi_acc_vdev->mig_state;
+ switch (state) {
+ case VFIO_DEVICE_STATE_RUNNING:
+ len = scnprintf(buf, VFIO_DEV_DBG_LEN, "%s\n",
+ "RUNNING\n");
+ break;
+ case VFIO_DEVICE_STATE_STOP_COPY:
+ len = scnprintf(buf, VFIO_DEV_DBG_LEN, "%s\n",
+ "STOP and COPYING\n");
+ break;
+ case VFIO_DEVICE_STATE_STOP:
+ len = scnprintf(buf, VFIO_DEV_DBG_LEN, "%s\n",
+ "STOP\n");
+ break;
+ case VFIO_DEVICE_STATE_RESUMING:
+ len = scnprintf(buf, VFIO_DEV_DBG_LEN, "%s\n",
+ "RESUMING\n");
+ break;
+ default:
+ len = scnprintf(buf, VFIO_DEV_DBG_LEN, "%s\n",
+ "Error\n");
+ }
+
+ return simple_read_from_buffer(buffer, count, pos, buf, len);
+}
+
+static const struct file_operations hisi_acc_vf_state_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = hisi_acc_vf_state_read,
+};
+
+static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
+{
+ struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
+ struct device *dev = &vf_pdev->dev;
+ int ret;
+
+ if (!atomic_read(&hisi_acc_root_ref))
+ hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc", NULL);
+ atomic_inc(&hisi_acc_root_ref);
+
+ hisi_acc_vdev->debug_root = debugfs_create_dir(dev_name(dev), hisi_acc_debugfs_root);
+ debugfs_create_file("state", 0444, hisi_acc_vdev->debug_root,
+ hisi_acc_vdev, &hisi_acc_vf_state_fops);
+
+ ret = hisi_acc_vf_debug_create(hisi_acc_vdev);
+ if (ret) {
+ dev_err(dev, "failed to alloc migration debug node\n");
+ return;
+ }
+ debugfs_create_file("debug", 0644, hisi_acc_vdev->debug_root,
+ hisi_acc_vdev, &hisi_acc_vf_debug_fops);
+}
+
+static void hisi_acc_vf_debugfs_exit(struct hisi_acc_vf_core_device *hisi_acc_vdev)
+{
+ if (hisi_acc_vdev->debug_migf)
+ hisi_acc_vf_debug_release(hisi_acc_vdev->debug_migf);
+
+ debugfs_remove_recursive(hisi_acc_vdev->debug_root);
+
+ atomic_dec(&hisi_acc_root_ref);
+ if (!atomic_read(&hisi_acc_root_ref))
+ debugfs_remove_recursive(hisi_acc_debugfs_root);
+}
+
static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
{
struct hisi_acc_vf_core_device *hisi_acc_vdev = container_of(core_vdev,
@@ -1194,6 +1485,8 @@ static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
return ret;
}
hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
+
+ hisi_acc_vf_debugfs_init(hisi_acc_vdev);
}
vfio_pci_core_finish_enable(vdev);
@@ -1206,6 +1499,9 @@ static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev)
struct hisi_acc_vf_core_device, core_device.vdev);
struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm;
+ if (core_vdev->mig_ops)
+ hisi_acc_vf_debugfs_exit(hisi_acc_vdev);
+
iounmap(vf_qm->io_base);
vfio_pci_core_close_device(core_vdev);
}
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
index 67343325b320..ae2a686c2e4d 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
@@ -4,8 +4,11 @@
#ifndef HISI_ACC_VFIO_PCI_H
#define HISI_ACC_VFIO_PCI_H
+#include <linux/debugfs.h>
#include <linux/hisi_acc_qm.h>
+#define VFIO_DEV_DBG_LEN 256
+
#define MB_POLL_PERIOD_US 10
#define MB_POLL_TIMEOUT_US 1000
#define QM_CACHE_WB_START 0x204
@@ -49,6 +52,14 @@
#define QM_EQC_DW0 0X8000
#define QM_AEQC_DW0 0X8020
+enum mig_debug_cmd {
+ STATE_SAVE,
+ STATE_RESUME,
+ MB_TEST,
+ MIG_DATA_DUMP,
+ MIG_DEV_SHOW,
+};
+
struct acc_vf_data {
#define QM_MATCH_SIZE offsetofend(struct acc_vf_data, qm_rsv_state)
/* QM match information */
@@ -111,5 +122,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 dentry *debug_root;
+ struct hisi_acc_vf_migration_file *debug_migf;
};
#endif /* HISI_ACC_VFIO_PCI_H */
--
2.24.0
On 14/10/2022 03:57, Longfang Liu wrote:
> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> +{
> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
> + struct device *dev = &vf_pdev->dev;
> + int ret;
> +
> + if (!atomic_read(&hisi_acc_root_ref))
> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc", NULL);
> + atomic_inc(&hisi_acc_root_ref);
> +
This looks totally racy, such that I wonder why even bother using an
atomic for hisi_acc_root_ref. Indeed, why is hisi_acc_debugfs_root not
created in the driver module init?
Thanks,
John
On 2022/10/14 17:20, John Garry wrote:
> On 14/10/2022 03:57, Longfang Liu wrote:
>> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
>> +{
>> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
>> + struct device *dev = &vf_pdev->dev;
>> + int ret;
>> +
>> + if (!atomic_read(&hisi_acc_root_ref))
>> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc", NULL);
>> + atomic_inc(&hisi_acc_root_ref);
>> +
>
> This looks totally racy, such that I wonder why even bother using an atomic for hisi_acc_root_ref.
When enabling VF, it is possible for multiple VMs to enable VF at the same time. The atomic variable
is used to ensure that only one "hisi_vfio_acc" is created. When other VFs are enabled,
it will not be created again, but will be used directly.
Indeed, why is hisi_acc_debugfs_root not created in the driver module init?
>
Because the normal function of VF is to perform encryption and decryption services, the live migration
function is an auxiliary function, which no need to be used in scenarios where only encryption and
decryption services are performed.
During module init, it can register ops(hisi_acc_vfio_pci_ops) that only perform encryption and
decryption services, and can also register with live migration function ops(hisi_acc_vfio_pci_migrn_ops),
and this debugfs only needs to register it when the the ops is hisi_acc_vfio_pci_migrn_ops.
> Thanks,
> John
> .
>
On Mon, Oct 17, 2022 at 05:20:34PM +0800, liulongfang wrote:
> On 2022/10/14 17:20, John Garry wrote:
> > On 14/10/2022 03:57, Longfang Liu wrote:
> >> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> >> +{
> >> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
> >> + struct device *dev = &vf_pdev->dev;
> >> + int ret;
> >> +
> >> + if (!atomic_read(&hisi_acc_root_ref))
> >> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc", NULL);
> >> + atomic_inc(&hisi_acc_root_ref);
> >> +
> >
> > This looks totally racy, such that I wonder why even bother using an atomic for hisi_acc_root_ref.
>
>
> When enabling VF, it is possible for multiple VMs to enable VF at the same time. The atomic variable
> is used to ensure that only one "hisi_vfio_acc" is created. When other VFs are enabled,
> it will not be created again, but will be used directly.
It is still completely racy. Use a lock
Jason
> -----Original Message-----
> From: liulongfang
> Sent: 17 October 2022 10:21
> To: John Garry <[email protected]>; [email protected];
> [email protected]; Shameerali Kolothum Thodi
> <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [Linuxarm] [PATCH 1/2] hisi_acc_vfio_pci: Add debugfs to
> migration driver
>
> On 2022/10/14 17:20, John Garry wrote:
> > On 14/10/2022 03:57, Longfang Liu wrote:
> >> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device
> *hisi_acc_vdev)
> >> +{
> >> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
> >> + struct device *dev = &vf_pdev->dev;
> >> + int ret;
> >> +
> >> + if (!atomic_read(&hisi_acc_root_ref))
> >> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc",
> NULL);
> >> + atomic_inc(&hisi_acc_root_ref);
> >> +
> >
> > This looks totally racy, such that I wonder why even bother using an atomic
> for hisi_acc_root_ref.
>
>
> When enabling VF, it is possible for multiple VMs to enable VF at the same
> time. The atomic variable
> is used to ensure that only one "hisi_vfio_acc" is created. When other VFs
> are enabled,
> it will not be created again, but will be used directly.
>
> Indeed, why is hisi_acc_debugfs_root not created in the driver module init?
> >
> Because the normal function of VF is to perform encryption and decryption
> services, the live migration
> function is an auxiliary function, which no need to be used in scenarios
> where only encryption and
> decryption services are performed.
>
> During module init, it can register ops(hisi_acc_vfio_pci_ops) that only
> perform encryption and
> decryption services, and can also register with live migration function
> ops(hisi_acc_vfio_pci_migrn_ops),
> and this debugfs only needs to register it when the the ops is
> hisi_acc_vfio_pci_migrn_ops.
Isn't the ops registration happens at probe()? In any case, I think you can move
the hisi_acc_debugfs_root creation to module_init() as suggested above
and avoid the race.
Thanks,
Shameer
On 2022/10/17 21:57, Shameerali Kolothum Thodi Wrote:
>
>
>> -----Original Message-----
>> From: liulongfang
>> Sent: 17 October 2022 10:21
>> To: John Garry <[email protected]>; [email protected];
>> [email protected]; Shameerali Kolothum Thodi
>> <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [Linuxarm] [PATCH 1/2] hisi_acc_vfio_pci: Add debugfs to
>> migration driver
>>
>> On 2022/10/14 17:20, John Garry wrote:
>>> On 14/10/2022 03:57, Longfang Liu wrote:
>>>> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device
>> *hisi_acc_vdev)
>>>> +{
>>>> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
>>>> + struct device *dev = &vf_pdev->dev;
>>>> + int ret;
>>>> +
>>>> + if (!atomic_read(&hisi_acc_root_ref))
>>>> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc",
>> NULL);
>>>> + atomic_inc(&hisi_acc_root_ref);
>>>> +
>>>
>>> This looks totally racy, such that I wonder why even bother using an atomic
>> for hisi_acc_root_ref.
>>
>>
>> When enabling VF, it is possible for multiple VMs to enable VF at the same
>> time. The atomic variable
>> is used to ensure that only one "hisi_vfio_acc" is created. When other VFs
>> are enabled,
>> it will not be created again, but will be used directly.
>>
>> Indeed, why is hisi_acc_debugfs_root not created in the driver module init?
>>>
>> Because the normal function of VF is to perform encryption and decryption
>> services, the live migration
>> function is an auxiliary function, which no need to be used in scenarios
>> where only encryption and
>> decryption services are performed.
>>
>> During module init, it can register ops(hisi_acc_vfio_pci_ops) that only
>> perform encryption and
>> decryption services, and can also register with live migration function
>> ops(hisi_acc_vfio_pci_migrn_ops),
>> and this debugfs only needs to register it when the the ops is
>> hisi_acc_vfio_pci_migrn_ops.
>
> Isn't the ops registration happens at probe()? In any case, I think you can move
> the hisi_acc_debugfs_root creation to module_init() as suggested above
> and avoid the race.
>
mig_root_ref uses atomic to prevent race, and another function is for reference counting.
Even if this hisi_acc_debugfs_root() is moved to module_init(), this mig_root_ref is still
needed, because the driver needs to ensure that the debugfs file is created when the first
VF is enabled, destroy it when the last VF have unloaded.
> Thanks,
> Shameer
>
Thanks,
Longfang.
On 2022/10/17 20:11, Jason Gunthorpe Wrote:
> On Mon, Oct 17, 2022 at 05:20:34PM +0800, liulongfang wrote:
>> On 2022/10/14 17:20, John Garry wrote:
>>> On 14/10/2022 03:57, Longfang Liu wrote:
>>>> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
>>>> +{
>>>> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
>>>> + struct device *dev = &vf_pdev->dev;
>>>> + int ret;
>>>> +
>>>> + if (!atomic_read(&hisi_acc_root_ref))
>>>> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc", NULL);
>>>> + atomic_inc(&hisi_acc_root_ref);
>>>> +
>>>
>>> This looks totally racy, such that I wonder why even bother using an atomic for hisi_acc_root_ref.
>>
>>
>> When enabling VF, it is possible for multiple VMs to enable VF at the same time. The atomic variable
>> is used to ensure that only one "hisi_vfio_acc" is created. When other VFs are enabled,
>> it will not be created again, but will be used directly.
>
> It is still completely racy. Use a lock
>
Do you have any suggested solutions?
> Jason
> .
>
Thanks,
Longfang
On 2022/10/18 14:55, liulongfang Wrote:
> On 2022/10/17 21:57, Shameerali Kolothum Thodi Wrote:
>>
>>
>>> -----Original Message-----
>>> From: liulongfang
>>> Sent: 17 October 2022 10:21
>>> To: John Garry <[email protected]>; [email protected];
>>> [email protected]; Shameerali Kolothum Thodi
>>> <[email protected]>
>>> Cc: [email protected]; [email protected];
>>> [email protected]
>>> Subject: Re: [Linuxarm] [PATCH 1/2] hisi_acc_vfio_pci: Add debugfs to
>>> migration driver
>>>
>>> On 2022/10/14 17:20, John Garry wrote:
>>>> On 14/10/2022 03:57, Longfang Liu wrote:
>>>>> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device
>>> *hisi_acc_vdev)
>>>>> +{
>>>>> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
>>>>> + struct device *dev = &vf_pdev->dev;
>>>>> + int ret;
>>>>> +
>>>>> + if (!atomic_read(&hisi_acc_root_ref))
>>>>> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc",
>>> NULL);
>>>>> + atomic_inc(&hisi_acc_root_ref);
>>>>> +
>>>>
>>>> This looks totally racy, such that I wonder why even bother using an atomic
>>> for hisi_acc_root_ref.
>>>
>>>
>>> When enabling VF, it is possible for multiple VMs to enable VF at the same
>>> time. The atomic variable
>>> is used to ensure that only one "hisi_vfio_acc" is created. When other VFs
>>> are enabled,
>>> it will not be created again, but will be used directly.
>>>
>>> Indeed, why is hisi_acc_debugfs_root not created in the driver module init?
>>>>
>>> Because the normal function of VF is to perform encryption and decryption
>>> services, the live migration
>>> function is an auxiliary function, which no need to be used in scenarios
>>> where only encryption and
>>> decryption services are performed.
>>>
>>> During module init, it can register ops(hisi_acc_vfio_pci_ops) that only
>>> perform encryption and
>>> decryption services, and can also register with live migration function
>>> ops(hisi_acc_vfio_pci_migrn_ops),
>>> and this debugfs only needs to register it when the the ops is
>>> hisi_acc_vfio_pci_migrn_ops.
>>
>> Isn't the ops registration happens at probe()?
Both ops are registered in probe(), but if you move hisi_acc_debugfs_root() to probe(),
this debugfs will be created regardless of whether you need to perform live migration.
In any case, I think you can move
>> the hisi_acc_debugfs_root creation to module_init() as suggested above
>> and avoid the race.
>>
>
> mig_root_ref uses atomic to prevent race, and another function is for reference counting.
> Even if this hisi_acc_debugfs_root() is moved to module_init(), this mig_root_ref is still
> needed, because the driver needs to ensure that the debugfs file is created when the first
> VF is enabled, destroy it when the last VF have unloaded.
>
>
>> Thanks,
>> Shameer
>>
> Thanks,
> Longfang.
>
Thanks,
Longfang.
On Tue, Oct 18, 2022 at 03:06:43PM +0800, liulongfang wrote:
> On 2022/10/17 20:11, Jason Gunthorpe Wrote:
> > On Mon, Oct 17, 2022 at 05:20:34PM +0800, liulongfang wrote:
> >> On 2022/10/14 17:20, John Garry wrote:
> >>> On 14/10/2022 03:57, Longfang Liu wrote:
> >>>> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> >>>> +{
> >>>> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
> >>>> + struct device *dev = &vf_pdev->dev;
> >>>> + int ret;
> >>>> +
> >>>> + if (!atomic_read(&hisi_acc_root_ref))
> >>>> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc", NULL);
> >>>> + atomic_inc(&hisi_acc_root_ref);
> >>>> +
> >>>
> >>> This looks totally racy, such that I wonder why even bother using an atomic for hisi_acc_root_ref.
> >>
> >>
> >> When enabling VF, it is possible for multiple VMs to enable VF at the same time. The atomic variable
> >> is used to ensure that only one "hisi_vfio_acc" is created. When other VFs are enabled,
> >> it will not be created again, but will be used directly.
> >
> > It is still completely racy. Use a lock
> >
>
> Do you have any suggested solutions?
If you want to keep it like this, use a lock around the creation.
Jason
On 2022/10/18 20:01, Jason Gunthorpe Wrote:
> On Tue, Oct 18, 2022 at 03:06:43PM +0800, liulongfang wrote:
>> On 2022/10/17 20:11, Jason Gunthorpe Wrote:
>>> On Mon, Oct 17, 2022 at 05:20:34PM +0800, liulongfang wrote:
>>>> On 2022/10/14 17:20, John Garry wrote:
>>>>> On 14/10/2022 03:57, Longfang Liu wrote:
>>>>>> +static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
>>>>>> +{
>>>>>> + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev;
>>>>>> + struct device *dev = &vf_pdev->dev;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (!atomic_read(&hisi_acc_root_ref))
>>>>>> + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc", NULL);
>>>>>> + atomic_inc(&hisi_acc_root_ref);
>>>>>> +
>>>>>
>>>>> This looks totally racy, such that I wonder why even bother using an atomic for hisi_acc_root_ref.
>>>>
>>>>
>>>> When enabling VF, it is possible for multiple VMs to enable VF at the same time. The atomic variable
>>>> is used to ensure that only one "hisi_vfio_acc" is created. When other VFs are enabled,
>>>> it will not be created again, but will be used directly.
>>>
>>> It is still completely racy. Use a lock
>>>
Um! I understand your previous solution. Since we did not put module_init() in the driver,
to avoid competition, we first need to modify the module_init() interface of the driver,
then put debugfs_create_dir() into module_init(), and finally put debugfs_create_file()
into probe().
I will implement this way in the next version.
>>
>> Do you have any suggested solutions?
>
> If you want to keep it like this, use a lock around the creation.
>
> Jason
> .
>
Thanks,
Longfang