Re: [PATCH v3 2/6] iommu/vt-d: Add Intel IOMMU debugfs to show context internals

From: Lu Baolu
Date: Wed Dec 06 2017 - 03:16:37 EST

Hi,

On 12/06/2017 11:43 AM, Sohil Mehta wrote:
> From: Gayatri Kammela <gayatri.kammela@xxxxxxxxx>
>
> IOMMU internals states such as root and context can be exported to the
> userspace.
>
> Example of such dump in Kabylake:
>
> root@OTC-KBLH-01:~# cat
> /sys/kernel/debug/intel_iommu/dmar_translation_struct
>
> IOMMU dmar2: Root Table Addr:4558a3000
> Root tbl entries:
> Bus 0 L: 4558aa001 H: 0
> Context table entries for Bus: 0
> [entry] DID :B :D .F Low High
> [160] 0000:00:14.00 4558a9001 102
> [184] 0000:00:17.00 400eac001 302
> [248] 0000:00:1f.00 4558af001 202
> [251] 0000:00:1f.03 40070e001 502
> [254] 0000:00:1f.06 4467c9001 602
> Root tbl entries:
> Bus 1 L: 3fc8c2001 H: 0
> Context table entries for Bus: 1
> [entry] DID :B :D .F Low High
> [0] 0000:01:00.00 3fc8c3001 402
>
> Cc: Sohil Mehta <sohil.mehta@xxxxxxxxx>
> Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Cc: Ashok Raj <ashok.raj@xxxxxxxxx>
> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> Signed-off-by: Gayatri Kammela <gayatri.kammela@xxxxxxxxx>
> ---
>
> v3: Add a macro for seq file operations
> Change the intel_iommu_ctx file name to dmar_translation_struct
>
> v2: No change
>
> drivers/iommu/Makefile | 1 +
> drivers/iommu/intel-iommu-debug.c | 152 ++++++++++++++++++++++++++++++++++++++
> drivers/iommu/intel-iommu.c | 4 +
> 3 files changed, 157 insertions(+)
> create mode 100644 drivers/iommu/intel-iommu-debug.c
>
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 1fb6958..fdbaf46 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
> obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
> obj-$(CONFIG_DMAR_TABLE) += dmar.o
> obj-$(CONFIG_INTEL_IOMMU) += intel-iommu.o
> +obj-$(CONFIG_INTEL_IOMMU_DEBUG) += intel-iommu-debug.o
> obj-$(CONFIG_INTEL_IOMMU_SVM) += intel-svm.o
> obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
> obj-$(CONFIG_IRQ_REMAP) += intel_irq_remapping.o irq_remapping.o
> diff --git a/drivers/iommu/intel-iommu-debug.c b/drivers/iommu/intel-iommu-debug.c
> new file mode 100644
> index 0000000..8ae0c4d
> --- /dev/null
> +++ b/drivers/iommu/intel-iommu-debug.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright  2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * Authors: Gayatri Kammela <gayatri.kammela@xxxxxxxxx>
> + * Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> + *
> + */
> +
> +#define pr_fmt(fmt) "INTEL_IOMMU: " fmt
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/debugfs.h>
> +#include <linux/pci.h>
> +#include <linux/iommu.h>
> +#include <linux/intel-iommu.h>
> +#include <linux/intel-svm.h>
> +#include <linux/dmar.h>
> +#include <linux/spinlock.h>
> +
> +#include "irq_remapping.h"
> +
> +#define TOTAL_BUS_NR (256) /* full bus range 256 */
> +#define DEFINE_SHOW_ATTRIBUTE(__name) \
> +static int __name ## _open(struct inode *inode, struct file *file) \
> +{ \
> + return single_open(file, __name ## _show, inode->i_private); \
> +} \
> +static const struct file_operations __name ## _fops = \
> +{ \
> + .open = __name ## _open, \
> + .read = seq_read, \
> + .llseek = seq_lseek, \
> + .release = single_release, \
> + .owner = THIS_MODULE, \
> +}
> +
> +static void ctx_tbl_entry_show(struct seq_file *m, void *unused,
> + struct intel_iommu *iommu, int bus, bool ext,
> + bool new_ext)
> +{
> + struct context_entry *context;
> + int ctx;
> + unsigned long flags;
> +
> + seq_printf(m, "%s Context table entries for Bus: %d\n",
> + ext ? "Lower" : "", bus);
> + seq_printf(m, "[entry]\tDID :B :D .F\tLow\t\tHigh\n");

WARNING: Prefer seq_puts to seq_printf
#119: FILE: drivers/iommu/intel-iommu-debug.c:59:
+ seq_printf(m, "[entry]\tDID :B :D .F\tLow\t\tHigh\n");

(caught by checkpatch.pl)

> +
> + spin_lock_irqsave(&iommu->lock, flags);
> +
> + /* Publish either context entries or extended contenxt entries */
> + for (ctx = 0; ctx < (ext ? 128 : 256); ctx++) {
> + context = iommu_context_addr(iommu, bus, ctx, 0);
> + if (!context)
> + goto out;
> + if (context_present(context)) {
> + seq_printf(m, "[%d]\t%04x:%02x:%02x.%02x\t%llx\t%llx\n",
> + ctx, iommu->segment, bus, PCI_SLOT(ctx),
> + PCI_FUNC(ctx), context[0].lo, context[0].hi);
> + }
> + }
> +out:
> + spin_unlock_irqrestore(&iommu->lock, flags);
> +}
> +
> +static void root_tbl_entry_show(struct seq_file *m, void *unused,

Why do you define the "unused" parameter which will never been used?
The same questions to other show functions.

> + struct intel_iommu *iommu, u64 rtaddr_reg,
> + bool ext, bool new_ext)
> +{
> + int bus;
> +
> + seq_printf(m, "\nIOMMU %s: %2s Root Table Addr:%llx\n", iommu->name,
> + ext ? "Extended" : "", rtaddr_reg);
> + /* Publish extended root table entries or root table entries here */
> + for (bus = 0; bus < TOTAL_BUS_NR; bus++) {
> + if (!iommu->root_entry[bus].lo)
> + continue;
> +
> + seq_printf(m, "%s Root tbl entries:\n", ext ? "Extended" : "");
> + seq_printf(m, "Bus %d L: %llx H: %llx\n", bus,
> + iommu->root_entry[bus].lo, iommu->root_entry[bus].hi
> + );
> +
> + ctx_tbl_entry_show(m, unused, iommu, bus, ext, new_ext);
> + }
> +}
> +
> +static int dmar_translation_struct_show(struct seq_file *m, void *unused)
> +{
> + struct dmar_drhd_unit *drhd;
> + struct intel_iommu *iommu;
> + u64 rtaddr_reg;
> + bool new_ext, ext;
> +
> + rcu_read_lock();
> + for_each_active_iommu(iommu, drhd) {
> + if (iommu) {
> + /* Check if root table type is set */
> + rtaddr_reg = dmar_readq(iommu->reg + DMAR_RTADDR_REG);
> + ext = !!(rtaddr_reg & DMA_RTADDR_RTT);
> + new_ext = !!ecap_ecs(iommu->ecap);
> + if (new_ext != ext) {
> + seq_printf(m, "IOMMU %s: invalid ecs\n",
> + iommu->name);
> + rcu_read_unlock();
> + return -EINVAL;
> + }
> + root_tbl_entry_show(m, unused, iommu, rtaddr_reg, ext,
> + new_ext);
> + }
> + }
> + rcu_read_unlock();
> +
> + return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(dmar_translation_struct);
> +
> +void __init intel_iommu_debugfs_init(void)
> +{
> + struct dentry *iommu_debug_root;
> +
> + iommu_debug_root = debugfs_create_dir("intel_iommu", NULL);
> +
> + if (!iommu_debug_root) {
> + pr_err("can't create debugfs dir\n");

I don't think we need a pr_err() here. System works well even
debugfs_create_dir() returns NULL.

This is same to all pr_err() in this file.

> + goto err;
> + }
> +
> + if (!debugfs_create_file("dmar_translation_struct", S_IRUGO,
> + iommu_debug_root, NULL,
> + &dmar_translation_struct_fops)) {

WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
#202: FILE: drivers/iommu/intel-iommu-debug.c:142:
+ if (!debugfs_create_file("dmar_translation_struct", S_IRUGO,

(caught by checkpatch.pl)

> + pr_err("Can't create dmar_translation_struct file\n");
> + goto err;
> + }
> +
> +err:
> + debugfs_remove_recursive(iommu_debug_root);
> +

Blank line isn't necessary here.

> +}
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 419a559..7794e0c 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4797,6 +4797,10 @@ int __init intel_iommu_init(void)
> intel_iommu_cpu_dead);
> intel_iommu_enabled = 1;
>
> +#ifdef CONFIG_INTEL_IOMMU_DEBUG
> + intel_iommu_debugfs_init();
> +#endif
> +
> return 0;
>
> out_free_reserved_range: