- Linuxarm - mailweb.openeuler.org

Re: [PATCH v12 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver
by Shameerali Kolothum Thodi 05 Nov '24

05 Nov '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, November 5, 2024 3:53 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)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; > +} > +

1 0

Re: [PATCH v11 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver
by Shameerali Kolothum Thodi 04 Nov '24

04 Nov '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Monday, November 4, 2024 8:31 AM > To: Alex Williamson <alex.williamson(a)redhat.com> > Cc: jgg(a)nvidia.com; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com>; kvm(a)vger.kernel.org; linux- > kernel(a)vger.kernel.org; linuxarm(a)openeuler.org > Subject: Re: [PATCH v11 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon > migration driver [...] > >>> + > >>> + seq_printf(seq, > >>> + "acc device:\n" > >>> + "guest driver load: %u\n" > >>> + "device opened: %d\n" > >>> + "migrate data length: %lu\n", > >>> + hisi_acc_vdev->vf_qm_state, > >>> + hisi_acc_vdev->dev_opened, > >>> + debug_migf->total_length); > >> > >> This debugfs entry is described as returning the data from the last > >> migration, but vf_qm_state and dev_opened are relative to the current > >> device/guest driver state. Both seem to have no relevance to the data > >> in debug_migf. > >> > > > > The benefit of dev_opened retention is that user can obtain the device > status > > during the cat migf_data operation. > > > > I will remove dev_opened in the next version. > And hisi_acc_vdev->vf_qm_state is changed to debug_migf- > >vf_data.vf_qm_state > Keep information about whether the device driver in the Guest OS is loaded > when live migration occurs. I think you already get that when you dump debug_migf->vf_data. So not required. Thanks, Shameer

1 0

Re: [PATCH v10 0/4] debugfs to hisilicon migration driver
by Shameerali Kolothum Thodi 25 Oct '24

25 Oct '24

> -----Original Message----- > From: Jason Gunthorpe <jgg(a)nvidia.com> > Sent: Thursday, October 24, 2024 4:28 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi(a)huawei.com> > Cc: liulongfang <liulongfang(a)huawei.com>; alex.williamson(a)redhat.com; > Jonathan Cameron <jonathan.cameron(a)huawei.com>; > kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org > Subject: Re: [PATCH v10 0/4] debugfs to hisilicon migration driver > > On Thu, Oct 24, 2024 at 01:18:55PM +0000, Shameerali Kolothum Thodi > wrote: > > > Add a debugfs function to the hisilicon migration driver in VFIO to > > > provide intermediate state values and data during device migration. > > > > > > When the execution of live migration fails, the user can view the > > > status and data during the migration process separately from the > > > source and the destination, which is convenient for users to analyze > > > and locate problems. > > > > Could you please take another look at this series as it looks like almost > there. > > Why are we so keen to do this? Nobody else needed a complex debugfs for > their live migration? I don't think it is that complex debugfs. Longfang has found this very helpful in testing and debug with hardware. And hopefully this can be expanded in future with different hardware revisions. Thanks, Shameer

1 0

Re: [PATCH v10 0/4] debugfs to hisilicon migration driver
by Shameerali Kolothum Thodi 24 Oct '24

24 Oct '24

Hi Alex, > -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Wednesday, October 16, 2024 2:23 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v10 0/4] debugfs to hisilicon migration driver > > Add a debugfs function to the hisilicon migration driver in VFIO to > provide intermediate state values and data during device migration. > > When the execution of live migration fails, the user can view the > status and data during the migration process separately from the > source and the destination, which is convenient for users to analyze > and locate problems. Could you please take another look at this series as it looks like almost there. Thanks, Shameer

1 0

Re: [PATCH v10 4/4] Documentation: add debugfs description for hisi migration
by Shameerali Kolothum Thodi 24 Oct '24

24 Oct '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Wednesday, October 16, 2024 2:23 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v10 4/4] Documentation: add debugfs description for hisi > migration > > Add a debugfs document description file to help users understand > how to use the hisilicon accelerator live migration driver's > debugfs. > > Update the file paths that need to be maintained in MAINTAINERS > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> LGTM: Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer > --- > .../ABI/testing/debugfs-hisi-migration | 25 +++++++++++++++++++ > 1 file changed, 25 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..89e4fde5ec6a > --- /dev/null > +++ b/Documentation/ABI/testing/debugfs-hisi-migration > @@ -0,0 +1,25 @@ > +What: > /sys/kernel/debug/vfio/<device>/migration/hisi_acc/dev_data > +Date: Oct 2024 > +KernelVersion: 6.12 > +Contact: Longfang Liu <liulongfang(a)huawei.com> > +Description: Read the configuration data and some status data > + required for device live migration. These data include device > + status data, queue configuration data, some task > configuration > + data and device attribute data. The output format of the > data > + is defined by the live migration driver. > + > +What: > /sys/kernel/debug/vfio/<device>/migration/hisi_acc/migf_data > +Date: Oct 2024 > +KernelVersion: 6.12 > +Contact: Longfang Liu <liulongfang(a)huawei.com> > +Description: Read the data from the last completed live migration. > + This data includes the same device status data as in > "dev_data". > + The migf_data is the dev_data that is migrated. > + > +What: > /sys/kernel/debug/vfio/<device>/migration/hisi_acc/cmd_state > +Date: Oct 2024 > +KernelVersion: 6.12 > +Contact: Longfang Liu <liulongfang(a)huawei.com> > +Description: Used to obtain the device command sending and receiving > + channel status. Returns failure or success logs based on the > + results. > -- > 2.24.0

1 0

Re: [PATCH v10 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver
by Shameerali Kolothum Thodi 24 Oct '24

24 Oct '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Wednesday, October 16, 2024 2:23 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v10 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon > migration driver [..] > @@ -1342,6 +1538,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; > @@ -1413,6 +1610,9 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev > *pdev, const struct pci_device > ret = vfio_pci_core_register_device(&hisi_acc_vdev->core_device); > if (ret) > goto out_put_vdev; > + > + if (ops == &hisi_acc_vfio_pci_migrn_ops) > + hisi_acc_vfio_debug_init(hisi_acc_vdev); As commented earlier, the ops check can be moved to the debug_init() function and you can remove ops check for the debug_exit() below. You may have to rearrange the functions to avoid the compiler error you mentioned in previous version to do so. > return 0; > > out_put_vdev: > @@ -1423,8 +1623,11 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev > *pdev, const struct pci_device > static void hisi_acc_vfio_pci_remove(struct pci_dev *pdev) > { > struct hisi_acc_vf_core_device *hisi_acc_vdev = > hisi_acc_drvdata(pdev); > + struct vfio_device *vdev = &hisi_acc_vdev->core_device.vdev; > > vfio_pci_core_unregister_device(&hisi_acc_vdev->core_device); > + if (vdev->ops == &hisi_acc_vfio_pci_migrn_ops) > + hisi_acc_vf_debugfs_exit(hisi_acc_vdev); As mentioned above remove the ops check here. With the above ones checked and fixed, Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com>

1 0

Re: [PATCH v9 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver
by Shameerali Kolothum Thodi 11 Oct '24

11 Oct '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Friday, September 13, 2024 10:55 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v9 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon > migration driver > > On the debugfs framework of VFIO, if the CONFIG_VFIO_DEBUGFS macro is > enabled, the debug function is registered for the live migration driver > of the HiSilicon accelerator device. > > After registering the HiSilicon accelerator device on the debugfs > framework of live migration of vfio, a directory file "hisi_acc" > of debugfs is created, and then three debug function files are > created in this directory: > > vfio > | > +---<dev_name1> > | +---migration > | +--state > | +--hisi_acc > | +--dev_data > | +--migf_data > | +--cmd_state > | > +---<dev_name2> > +---migration > +--state > +--hisi_acc > +--dev_data > +--migf_data > +--cmd_state > > dev_data file: read device data that needs to be migrated from the > current device in real time > migf_data file: read the migration data of the last live migration > from the current driver. > cmd_state: used to get the cmd channel state for the device. > > +----------------+ +--------------+ +---------------+ > | migration dev | | src dev | | dst dev | > +-------+--------+ +------+-------+ +-------+-------+ > | | | > | +------v-------+ +-------v-------+ > | | saving_migf | | resuming_migf | > read | | file | | file | > | +------+-------+ +-------+-------+ > | | copy | > | +------------+----------+ > | | > +-------v--------+ +-------v--------+ > | data buffer | | debug_migf | > +-------+--------+ +-------+--------+ > | | > cat | cat | > +-------v--------+ +-------v--------+ > | dev_data | | migf_data | > +----------------+ +----------------+ > > When accessing debugfs, user can obtain the most recent status data > of the device through the "dev_data" file. It can read recent > complete status data of the device. If the current device is being > migrated, it will wait for it to complete. > The data for the last completed migration function will be stored > in debug_migf. Users can read it via "migf_data". > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 204 ++++++++++++++++++ > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 7 + > 2 files changed, 211 insertions(+) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index a8c53952d82e..da9f5b9e6c5b 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) > +{ > + 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)); > +} > + > 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,181 @@ 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; > + } Still not very sure this vf_qm ready() check actually helps or not? What guarantee is there that the qm will stay Ready after this call? Any read/write afterwards will eventually fail if it is not ready later for some reason, right? Perhaps helps in early detection and bails out. > + 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 = NULL; > + int ret; > + > + vf_data = kzalloc(sizeof(struct acc_vf_data), GFP_KERNEL); > + if (!vf_data) > + return -ENOMEM;\ You could move the allocation after the below checks and to just before vf_qm_read_data(). > + > + mutex_lock(&hisi_acc_vdev->open_mutex); > + ret = hisi_acc_vf_debug_check(seq, vdev); > + if (ret) { > + mutex_unlock(&hisi_acc_vdev->open_mutex); > + goto migf_err; > + } > + > + mutex_lock(&hisi_acc_vdev->state_mutex); > + 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) { > + mutex_unlock(&hisi_acc_vdev->open_mutex); > + mutex_unlock(&hisi_acc_vdev->state_mutex); I think it is better to unlock in the reverse order. Also probably you can move the unlocks to a goto area. > + goto migf_err; > + } > + > + mutex_unlock(&hisi_acc_vdev->open_mutex); > + mutex_unlock(&hisi_acc_vdev->state_mutex); Same as above. > + seq_hex_dump(seq, "Dev Data:", DUMP_PREFIX_OFFSET, 16, 1, > + (unsigned char *)vf_data, > + vf_data_sz, false); > + > + seq_printf(seq, > + "acc device:\n" > + "guest driver load: %u\n" > + "data size: %lu\n", > + hisi_acc_vdev->vf_qm_state, > + sizeof(struct acc_vf_data)); > + > +migf_err: > + kfree(vf_data); > + > + 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, > + (unsigned char *)&debug_migf->vf_data, > + vf_data_sz, false); > + > + seq_printf(seq, > + "acc device:\n" > + "guest driver load: %u\n" > + "device opened: %d\n" > + "migrate data length: %lu\n", > + hisi_acc_vdev->vf_qm_state, > + hisi_acc_vdev->dev_opened, > + debug_migf->total_length); > + > + return 0; > +} > + > +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; > + > + 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 (hisi_acc_vdev->debug_migf) { > + kfree(hisi_acc_vdev->debug_migf); > + hisi_acc_vdev->debug_migf = NULL; > + } > +} > + > 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 +1495,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 +1516,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 +1539,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; > @@ -1413,6 +1611,9 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev > *pdev, const struct pci_device > ret = vfio_pci_core_register_device(&hisi_acc_vdev->core_device); > if (ret) > goto out_put_vdev; > + > + if (ops == &hisi_acc_vfio_pci_migrn_ops) > + hisi_acc_vfio_debug_init(hisi_acc_vdev); I think there was a comment earlier on this. Still not sure why it is not possible to move ops == &hisi_acc_vfio_pci_migrn_ops check inside hisi_acc_vfio_debug_init(). > return 0; > > out_put_vdev: > @@ -1423,8 +1624,11 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev > *pdev, const struct pci_device > static void hisi_acc_vfio_pci_remove(struct pci_dev *pdev) > { > struct hisi_acc_vf_core_device *hisi_acc_vdev = > hisi_acc_drvdata(pdev); > + struct vfio_device *vdev = &hisi_acc_vdev->core_device.vdev; > > vfio_pci_core_unregister_device(&hisi_acc_vdev->core_device); > + if (vdev->ops == &hisi_acc_vfio_pci_migrn_ops) > + hisi_acc_vf_debugfs_exit(hisi_acc_vdev); Do we need to do vdev->ops == &hisi_acc_vfio_pci_migrn_ops check here? Since we are checking hisi_acc_vdev->debug_migf inside the exit function, which I think is only set when the ops == migrn_ops. Right? Thanks, Shameer

1 0

Re: [PATCH v8 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver
by Shameerali Kolothum Thodi 14 Aug '24

14 Aug '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, August 6, 2024 1:29 PM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v8 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon > migration driver ... > +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 = NULL; > + int ret; > + > + vf_data = kzalloc(sizeof(struct acc_vf_data), GFP_KERNEL); > + if (!vf_data) > + return -ENOMEM; > + > + mutex_lock(&hisi_acc_vdev->state_mutex); > + ret = hisi_acc_vf_debug_check(seq, vdev); > + if (ret) { > + mutex_unlock(&hisi_acc_vdev->state_mutex); > + goto migf_err; > + } > + > + 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) { > + mutex_unlock(&hisi_acc_vdev->state_mutex); > + goto migf_err; > + } > + > + mutex_unlock(&hisi_acc_vdev->state_mutex); > + > + seq_hex_dump(seq, "Dev Data:", DUMP_PREFIX_OFFSET, 16, 1, > + (unsigned char *)vf_data, > + vf_data_sz, false); > + > + seq_printf(seq, > + "acc device:\n" > + "device ready: %u\n" > + "device opened: %d\n" > + "data size: %lu\n", > + hisi_acc_vdev->vf_qm_state, > + hisi_acc_vdev->dev_opened, I think the dev_opened will be always true if it reaches here and can be removed from here and from hisi_acc_vf_migf_read() as well. Please don't respin just for this. Let us wait for others to review this. Thanks, Shameer

1 0

Re: [PATCH v8 4/4] Documentation: add debugfs description for hisi migration
by Shameerali Kolothum Thodi 14 Aug '24

14 Aug '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, August 6, 2024 1:29 PM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v8 4/4] Documentation: add debugfs description for hisi > migration > > Add a debugfs document description file to help users understand > how to use the hisilicon accelerator live migration driver's > debugfs. > > Update the file paths that need to be maintained in MAINTAINERS > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> LGTM, Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer

1 0

Re: [PATCH v7 2/4] hisi_acc_vfio_pci: create subfunction for data reading
by Shameerali Kolothum Thodi 05 Aug '24

05 Aug '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, July 30, 2024 1:15 PM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v7 2/4] hisi_acc_vfio_pci: create subfunction for data > reading > > This patch generates the code for the operation of reading data from > the device into a sub-function. > Then, it can be called during the device status data saving phase of > the live migration process and the device status data reading function > in debugfs. > Thereby reducing the redundant code of the driver. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- LGTM, Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 54 +++++++++++-------- > 1 file changed, 33 insertions(+), 21 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 45351be8e270..a8c53952d82e 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -486,31 +486,11 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return 0; > } > > -static int vf_qm_state_save(struct hisi_acc_vf_core_device *hisi_acc_vdev, > - struct hisi_acc_vf_migration_file *migf) > +static int vf_qm_read_data(struct hisi_qm *vf_qm, struct acc_vf_data > *vf_data) > { > - struct acc_vf_data *vf_data = &migf->vf_data; > - struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > struct device *dev = &vf_qm->pdev->dev; > int ret; > > - if (unlikely(qm_wait_dev_not_ready(vf_qm))) { > - /* Update state and return with match data */ > - vf_data->vf_qm_state = QM_NOT_READY; > - hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > - migf->total_length = QM_MATCH_SIZE; > - return 0; > - } > - > - vf_data->vf_qm_state = QM_READY; > - hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > - > - ret = vf_qm_cache_wb(vf_qm); > - if (ret) { > - dev_err(dev, "failed to writeback QM Cache!\n"); > - return ret; > - } > - > ret = qm_get_regs(vf_qm, vf_data); > if (ret) > return -EINVAL; > @@ -536,6 +516,38 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return -EINVAL; > } > > + return 0; > +} > + > +static int vf_qm_state_save(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct hisi_acc_vf_migration_file *migf) > +{ > + struct acc_vf_data *vf_data = &migf->vf_data; > + struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &vf_qm->pdev->dev; > + int ret; > + > + if (unlikely(qm_wait_dev_not_ready(vf_qm))) { > + /* Update state and return with match data */ > + vf_data->vf_qm_state = QM_NOT_READY; > + hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > + migf->total_length = QM_MATCH_SIZE; > + return 0; > + } > + > + vf_data->vf_qm_state = QM_READY; > + hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > + > + ret = vf_qm_cache_wb(vf_qm); > + if (ret) { > + dev_err(dev, "failed to writeback QM Cache!\n"); > + return ret; > + } > + > + ret = vf_qm_read_data(vf_qm, vf_data); > + if (ret) > + return -EINVAL; > + > migf->total_length = sizeof(struct acc_vf_data); > return 0; > } > -- > 2.24.0

1 0

2024

2023

2022

2021

2020

Linuxarm