Messages in this thread | | | Date | Mon, 25 Sep 2023 20:03:13 +0800 | Subject | Re: [PATCH v2 2/3] iommu/vt-d: debugfs: Create/remove debugfs file per {device, pasid} | From | "Liu, Jingqi" <> |
| |
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@intel.com> >> --- >> 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
| |