Re: [PATCH v2 2/3] iommu/vt-d: debugfs: Create/remove debugfs file per {device, pasid}
From: Liu, Jingqi
Date: Mon Sep 25 2023 - 08:04:11 EST
On 9/25/2023 9:47 AM, Baolu Lu wrote:
On 9/22/23 11:16 PM, Jingqi Liu wrote:
Add a debugfs directory per pair of {device, pasid} if the mappings of
its page table are created and destroyed by the iommu_map/unmap()
interfaces. i.e. /sys/kernel/debug/iommu/intel/<device source
id>/<pasid>.
Create a debugfs file in the directory for users to dump the page
table corresponding to {device, pasid}. e.g.
/sys/kernel/debug/iommu/intel/0000:00:02.0/0/domain_translation_struct.
When attaching device without pasid, create a debugfs file with
PASID#0, i.e. RID_PASID. When attaching a domain to a pasid of device,
create a debugfs file with the specified pasid.
When detaching without pasid, remove the directory and file for
PASID#0. When detaching with pasid, remove the debugfs directory and
file of the specified pasid. Remove the entire debugfs directory of
the specified device for releasing device.
e.g. /sys/kernel/debug/iommu/intel/0000:00:01.0
Signed-off-by: Jingqi Liu <Jingqi.liu@xxxxxxxxx>
---
drivers/iommu/intel/debugfs.c | 117 ++++++++++++++++++++++++++++++++--
drivers/iommu/intel/iommu.c | 16 +++++
drivers/iommu/intel/iommu.h | 4 ++
3 files changed, 132 insertions(+), 5 deletions(-)
......
+
+/*
+ * Create a debugfs directory per pair of {device, pasid},
+ * then create the corresponding debugfs file in this directory
+ * for user to dump its page table. e.g.
+ *
/sys/kernel/debug/iommu/intel/0000:00:01.0/0/domain_translation_struct
+ */
+void intel_iommu_debugfs_create_dev_pasid(struct device *dev, u32
pasid)
+{
+ struct dentry *pasid_dir = NULL, *dev_dir = NULL;
+ struct iommu_domain *domain = NULL;
+ struct show_domain_info *sinfo;
+ char pname[10];
+
+ if (pasid == IOMMU_NO_PASID) {
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ if (!info)
+ return;
+ domain = &info->domain->domain;
+ } else
+ domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
Can you just add domain as a parameter to this helper? This helper is
probably called in the process of attaching. so the domain pointers may
not be initialized yet.
Thanks.
Yes. This helper may be called before domain is initialized.
I'll add 'domain' as a parameter, and remove the check code above.
Check if the 'domain' is valid as the following code.
+
+ /*
+ * The debugfs only dumps the page tables whose mappings are
created
+ * and destroyed by the iommu_map/unmap() interfaces. Check the
+ * mapping type of the domain before creating debugfs directory.
+ */
+ if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
+ return;
+
+ dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug);
The comments of debugfs_lookup says:
/**
* debugfs_lookup() - look up an existing debugfs file
* @name: a pointer to a string containing the name of the file to
look up.
* @parent: a pointer to the parent dentry of the file.
*
* This function will return a pointer to a dentry if it succeeds. If
the file
* doesn't exist or an error occurs, %NULL will be returned. The
returned
* dentry must be passed to dput() when it is no longer needed.
*
* If debugfs is not enabled in the kernel, the value -%ENODEV will be
* returned.
*/
where is the paired dput()?
Ditto to all debugfs_lookup() calls in this file.
Good catch.
Need to 'dput()' the entry before returning from the helper.
I'll revise it.
......
+
+/*
+ * Remove the debugfs directory and file corresponding to each pair of
+ * {device, pasid}. There're two scenarios that will call this helper:
+ * 1) Detach the specified device with pasid.
+ * 2) IOMMU release a device.
+ */
+void intel_iommu_debugfs_remove_dev_pasid(struct device *dev, u32
pasid)
+{
+ struct dentry *pasid_dir = NULL, *dev_dir = NULL;
+ struct dentry *dentry = NULL;
+ char pname[10];
+
+ dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug);
+ if (!dev_dir)
+ return;
+
+ /* Check if the entire debugfs directory needs to be removed. */
+ if (pasid == IOMMU_PASID_INVALID) {
+ struct list_head *plist;
+ struct dentry *sub_dentry;
+
+ list_for_each(plist, &(dev_dir->d_subdirs)) {
+ sub_dentry = list_entry(plist, struct dentry, d_child);
+ if(sub_dentry) {
+ dentry = debugfs_lookup("domain_translation_struct",
sub_dentry);
+ if (!dentry)
+ continue;
+
+ if (dentry->d_inode->i_private)
+ kfree(dentry->d_inode->i_private);
+ }
+ }
+
+ debugfs_remove_recursive(dev_dir);
+ } else { /* Remove the directory with specified pasid. */
+ sprintf(pname, "%x", pasid);
+ pasid_dir = debugfs_lookup(pname, dev_dir);
+ if (!pasid_dir)
+ return;
+
+ dentry = debugfs_lookup("domain_translation_struct",
pasid_dir);
+ if (!dentry)
+ return;
+
+ if (dentry->d_inode->i_private)
+ kfree(dentry->d_inode->i_private);
+
+ debugfs_remove_recursive(pasid_dir);
+ }
+}
The above is too complex to review and maintain.
If I were to make the change, I would move the device directory
management to the IOMMU probe/release devices path, and keep the PASID
directory management in the domain attaching/detaching paths.
Or I overlooked anything?
Good point.
In this way, the debugfs device directory and pasid directory can be
managed separately.
intel_iommu_probe_device()/intel_iommu_release_device()
is responsible for adding/removing the device directory.
In the domain attaching/detaching paths,
add/remove the PASID directory and the corresponding debugfs file.
I'll implement like this.
Thanks,
Jingqi