RE: [PATCH v6 4/5] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Friday, May 3, 2024 6:21 PM
> To: liulongfang <liulongfang@huawei.com>
> Cc: jgg@nvidia.com; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; linuxarm@openeuler.org
> 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@huawei.com> 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@huawei.com>
> > ---
> >  .../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