lkml.org 
[lkml]   [2023]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 2/3] iommu/vt-d: debugfs: Create/remove debugfs file per {device, pasid}
From

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

\
 
 \ /
  Last update: 2023-09-25 14:04    [W:0.087 / U:2.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site