RE: [PATCH v6 4/5] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver
From: Shameerali Kolothum Thodi
Date: Tue May 07 2024 - 04:07:02 EST
> -----Original Message-----
> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Friday, May 3, 2024 6:21 PM
> To: liulongfang <liulongfang@xxxxxxxxxx>
> Cc: jgg@xxxxxxxxxx; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@xxxxxxxxxx>; Jonathan Cameron
> <jonathan.cameron@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linuxarm@xxxxxxxxxxxxx
> Subject: Re: [PATCH v6 4/5] hisi_acc_vfio_pci: register debugfs for hisilicon
> migration driver
>
> On Thu, 25 Apr 2024 21:23:21 +0800
> Longfang Liu <liulongfang@xxxxxxxxxx> wrote:
>
> > 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_mif | | 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 real-time status data
> > of the device through the "dev_data" file. It will directly read
> > the device status data and will not affect the live migration
> > function. Its data is stored in the allocated memory buffer,
> > and the memory is released after data returning to user mode.
> >
> > To obtain the data of the last complete migration, user need to
> > obtain it through the "migf_data" file. Since the live migration
> > data only exists during the migration process, it is destroyed
> > after the migration is completed.
> > In order to save this data, a debug_migf file is created in the
> > driver. At the end of the live migration process, copy the data
> > to debug_migf.
> >
> > Signed-off-by: Longfang Liu <liulongfang@xxxxxxxxxx>
> > ---
> > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 225 ++++++++++++++++++
> > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 7 +
> > 2 files changed, 232 insertions(+)
> >
> > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > index bf358ba94b5d..656b3d975940 100644
> > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > @@ -627,15 +627,33 @@ 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;
> > +
> > + mutex_lock(&hisi_acc_vdev->enable_mutex);
> > + dst_migf->disabled = src_migf->disabled;
> > + dst_migf->total_length = src_migf->total_length;
> > + memcpy(&dst_migf->vf_data, &src_migf->vf_data,
> > + sizeof(struct acc_vf_data));
> > + mutex_unlock(&hisi_acc_vdev->enable_mutex);
> > +}
> > +
> > 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;
> > @@ -1144,6 +1162,7 @@ static int hisi_acc_vf_qm_init(struct
> hisi_acc_vf_core_device *hisi_acc_vdev)
> > if (!vf_qm->io_base)
> > return -EIO;
> >
> > + mutex_init(&hisi_acc_vdev->enable_mutex);
> > vf_qm->fun_type = QM_HW_VF;
> > vf_qm->pdev = vf_dev;
> > mutex_init(&vf_qm->mailbox_lock);
> > @@ -1294,6 +1313,203 @@ 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;
> > + struct device *dev = vdev->dev;
> > + int ret;
> > +
> > + if (!vdev->mig_ops) {
> > + dev_err(dev, "device does not support live migration!\n");
>
> Sorry, every error path should not spam dmesg with dev_err(). I'm
> going to wait until your co-maintainer approves this before looking at
> any further iterations of this series. Thanks,
Sure. I will sync up with Longfang and also make sure we address all the existing
comments on this before posting the next revision.
Thanks,
Shameer