Re: [PATCH v12 4/4] Documentation: add debugfs description for vfio

[Alex Williamson]

On Mon, 7 Aug 2023 09:33:07 +0800
liulongfang <liulongfang@xxxxxxxxxx> wrote:

> On 2023/8/4 22:58, Jason Gunthorpe wrote:
> > On Fri, Jul 28, 2023 at 03:21:04PM +0800, liulongfang wrote:  
> >> From: Longfang Liu <liulongfang@xxxxxxxxxx>
> >>
> >> 1.Add two debugfs document description file to help users understand
> >> how to use the accelerator live migration driver's debugfs.
> >> 2.Update the file paths that need to be maintained in MAINTAINERS
> >>
> >> Signed-off-by: Longfang Liu <liulongfang@xxxxxxxxxx>
> >> ---
> >>  .../ABI/testing/debugfs-hisi-migration        | 36 +++++++++++++++++++
> >>  Documentation/ABI/testing/debugfs-vfio        | 25 +++++++++++++
> >>  MAINTAINERS                                   |  2 ++
> >>  3 files changed, 63 insertions(+)
> >>  create mode 100644 Documentation/ABI/testing/debugfs-hisi-migration
> >>  create mode 100644 Documentation/ABI/testing/debugfs-vfio
> >>
> >> diff --git a/Documentation/ABI/testing/debugfs-hisi-migration b/Documentation/ABI/testing/debugfs-hisi-migration
> >> new file mode 100644
> >> index 000000000000..791dd8a09575
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/debugfs-hisi-migration
> >> @@ -0,0 +1,36 @@
> >> +What:		/sys/kernel/debug/vfio/<device>/migration/hisi_acc/data
> >> +Date:		Aug 2023
> >> +KernelVersion:  6.6
> >> +Contact:	Longfang Liu <liulongfang@xxxxxxxxxx>
> >> +Description:	Read the live migration data of the vfio device.
> >> +		These data include device status data, queue configuration
> >> +		data and some task configuration data.
> >> +		The output format of the data is defined by the live
> >> +		migration driver.
> >> +
> >> +What:		/sys/kernel/debug/vfio/<device>/migration/hisi_acc/attr
> >> +Date:		Aug 2023
> >> +KernelVersion:  6.6
> >> +Contact:	Longfang Liu <liulongfang@xxxxxxxxxx>
> >> +Description:	Read the live migration attributes of the vfio device.
> >> +		it include device status attributes and data length attributes
> >> +		The output format of the attributes is defined by the live
> >> +		migration driver.
> >> +
> >> +What:		/sys/kernel/debug/vfio/<device>/migration/hisi_acc/io_test
> >> +Date:		Aug 2023
> >> +KernelVersion:  6.6
> >> +Contact:	Longfang Liu <liulongfang@xxxxxxxxxx>
> >> +Description:	Trigger the HiSilicon accelerator device to perform
> >> +		the io test through the read operation. If successful,
> >> +		it returns the execution result of mailbox. If fails,
> >> +		it returns error log result.
> >> +
> >> +What:		/sys/kernel/debug/vfio/<device>/migration/hisi_acc/save
> >> +Date:		Aug 2023
> >> +KernelVersion:  6.6
> >> +Contact:	Longfang Liu <liulongfang@xxxxxxxxxx>
> >> +Description:	Trigger the Hisilicon accelerator device to perform
> >> +		the state saving operation of live migration through the read
> >> +		operation, and output the operation log results.  
> > 
> > I still very much do not like this use of debugfs.
> > 
> > If you want to test migration then make a test program and use the
> > normal api
> >  
> These debugfs are just to get internal state data.
> The test function is no longer executed.
> The store file with test function has been deleted.

The vfio/<device>/migration/state file can provide useful monitoring of
the device progress during a migration, but I think the point Jason is
trying to make is that these hisi_acc seqfiles aren't really doing
anything that couldn't be done by a simple userspace test driver.

Based on my review of the previous patch, we're playing pretty loose
with concurrency and data buffers.  Access to the migration data of
the device outside of the process that owns the device is also a
concern.

The value-add here needs to be that there's something useful about the
kernel being able to dump this data rather than either a simple
userspace program or instrumenting a userspace driver like QEMU, where
we can avoid the complexity that's going to be required to resolve the
issues in the previous patch and ensure that sensitive data from the
device isn't available through debugfs.  Thanks,

Alex