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

On 9/28/2023 9:58 AM, Baolu Lu wrote:
> On 9/27/23 11:15 PM, Jingqi Liu wrote:
......
>> +
>> +/* Create a debugfs directory for each device. */
>> +void intel_iommu_debugfs_create_dev(struct device *dev)
>> +{
>> +    struct dentry *dev_dir;
>> +
>> +    dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug);
>> +    if (!dev_dir) {
>> +        dev_dir = debugfs_create_dir(dev_name(dev), intel_iommu_debug);
>> +        if (IS_ERR(dev_dir))
>> +            pr_info("%s: Failed to create debugfs directory.\n",
>> +                dev_name(dev));
>> +    } else
>> +        dput(dev_dir);
>> +}
>
> Above could simply be like this:
>
> void intel_iommu_debugfs_create_dev(struct device *dev)
> {
>     struct device_domain_info *info = dev_iommu_priv_get(dev);
>
>     info->debugfs_entry = debugfs_create_dir(dev_name(dev),
>             intel_iommu_debug);
> }
>
> Isn't it?
Thanks. Good point.
If add an "info->debugfs_dentry" to the device "info" to save the dentry
of device
debugfs directory, there's no need to lookup the dentry by debugfs_lookup().
Just simply get it from the device "info".

>
>> +
>> +void intel_iommu_debugfs_remove_dev(struct device *dev)
>> +{
>> +    struct dentry *dev_dir, *sub_dir, *dentry;
>> +    struct list_head *plist;
>> +
>> +    dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug);
>> +    if (!dev_dir)
>> +        return;
>> +
>> +    list_for_each(plist, &(dev_dir->d_subdirs)) {
>> +        sub_dir = list_entry(plist, struct dentry, d_child);
>> +        if(sub_dir) {
>> +            dentry = debugfs_lookup("domain_translation_struct",
>> +                        sub_dir);
>> +            if (!dentry)
>> +                continue;
>> +
>> +            if (dentry->d_inode->i_private)
>> +                kfree(dentry->d_inode->i_private);
>> +
>> +            dput(dentry);
>> +        }
>> +    }
>> +
>> +    debugfs_remove_recursive(dev_dir);
>> +    dput(dev_dir);
>> +}
>
> And this could simply be like this:
>
> void intel_iommu_debugfs_remove_dev(struct device *dev)
> {
>     struct device_domain_info *info = dev_iommu_priv_get(dev);
>
>     debugfs_remove(info->debugfs_entry);
> }
>
Yes.
Just get the debugfs dentry of device simply for removing.
This helper should be called before the "info" is freed
in intel_iommu_release_device(). Like this:

+      intel_iommu_debugfs_remove_dev(dev);
        kfree(info);

>> +
>> +/*
>> + * 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 iommu_domain *domain,
>> +                      struct device *dev, u32 pasid)
>> +{
>> +    struct dentry *dev_dir, *pasid_dir;
>> +    struct show_domain_info *sinfo;
>> +    char dir_name[10];
>> +
>> +    /*
>> +     * 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);
>> +    if (!dev_dir)
>> +        return;
>> +
>> +    sprintf(dir_name, "%x", pasid);
>> +    pasid_dir = debugfs_create_dir(dir_name, dev_dir);
>> +    if (IS_ERR(pasid_dir))
>> +        goto dput_out;
>> +
>> +    sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
>> +    if (!sinfo)
>> +        goto dput_out;
>> +
>> +    sinfo->dev = dev;
>> +    sinfo->pasid = pasid;
>> +    debugfs_create_file("domain_translation_struct", 0444,
>> +                pasid_dir, sinfo,
>> +                &domain_translation_struct_fops);
>> +dput_out:
>> +    dput(dev_dir);
>> +}
>
> And here,
>
> void intel_iommu_debugfs_create_dev_pasid(struct iommu_domain *domain,
>                       struct device *dev, u32 pasid)
> {
>     sprintf(dir_name, "%x", pasid);
>     dev_pasid->debugfs_entry = debugfs_create_dir(dir_name,
>             info->debugfs_entry);
>
>     debugfs_create_file("domain_translation_struct", 0444,
>                 dev_pasid->debugfs_entry, dev_pasid,
>                 &domain_translation_struct_fops);
> }
>
Thanks.
You mean to add 'debugfs_entry' in below structure.
    struct dev_pasid_info *dev_pasid;
This structure is also allocated per pair of {dev, pasid}.
The debugfs dentry of  {dev, pasid} can be simply obtained from
'dev_pasid_info'.

So the 'dev_pasid_info' can be passed as a parameter of this helper, right ?
Like this:
void intel_iommu_debugfs_create_dev_pasid(struct iommu_domain *domain,
                      struct dev_pasid_info *dev_pasid) ;
>> +
>> +/*
>> + * Remove the debugfs directory and file corresponding to each pair of
>> + * {device, pasid}.
>> + */
>> +void intel_iommu_debugfs_remove_dev_pasid(struct device *dev, u32
>> pasid)
>> +{
>> +    struct dentry *dev_dir, *pasid_dir, *dentry;
>> +    char dir_name[10];
>> +
>> +    dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug);
>> +    if (!dev_dir)
>> +        return;
>> +
>> +    sprintf(dir_name, "%x", pasid);
>> +    pasid_dir = debugfs_lookup(dir_name, dev_dir);
>> +    if (!pasid_dir)
>> +        goto dput_dev;
>> +
>> +    dentry = debugfs_lookup("domain_translation_struct", pasid_dir);
>> +    if (!dentry)
>> +        goto dput_pasid;
>> +
>> +    if (dentry->d_inode->i_private)
>> +        kfree(dentry->d_inode->i_private);
>> +
>> +    debugfs_remove_recursive(pasid_dir);
>> +
>> +    dput(dentry);
>> +dput_pasid:
>> +    dput(pasid_dir);
>> +dput_dev:
>> +    dput(dev_dir);
>> +}
>
> The same thing here:
>
>     debugfs_remove(dev_pasid->debugfs_entry);
>
Yes.
Just get the debugfs dentry from "dev_pasid" instead of 'debugfs_lookup()'.
And this helper should be called before the "struct dev_pasid_info" is freed
in intel_iommu_remove_dev_pasid().
Like this:

+        intel_iommu_debugfs_remove_dev_pasid(dev, pasid);
          kfree(dev_pasid);

>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index dd8ff358867d..af9c989035a2 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -2488,6 +2488,13 @@ static int dmar_domain_attach_device(struct
>> dmar_domain *domain,
>>         iommu_enable_pci_caps(info);
>>   +    /*
>> +     * Create a debugfs directory specified by RID_PASID
>> +     * in the debugfs device directory.
>> +     */
>> + intel_iommu_debugfs_create_dev_pasid(&info->domain->domain,
>> +                                 dev, IOMMU_NO_PASID);
>
> The function name is self-explained. So no need to add comments. Ditto
> to all other places.
>
Indeed.
I'll delete all related comments.

Thanks,
Jingqi


\
 
 \ /
  Last update: 2023-09-28 11:03    [W:0.048 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site