| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 29 Mar 2018 23:16:35 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Gary R Hook <gary.hook@....com>, iommu@...ts.linux-foundation.org
Cc: joro@...tes.org, jacob.jun.pan@...ux.intel.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iommu/amd: Add basic debugfs infrastructure for AMD
IOMMU
On 3/29/2018 5:54 PM, Gary R Hook wrote:
> Implement a skeleton framework for debugfs support in the
> AMD IOMMU.
>
>
> Signed-off-by: Gary R Hook <gary.hook@....com>
> ---
> drivers/iommu/Kconfig | 6 ++---
> drivers/iommu/Makefile | 2 +-
> drivers/iommu/amd_iommu_debugfs.c | 47 +++++++++++++++++++++++++++++++++++++
> drivers/iommu/amd_iommu_init.c | 9 ++++---
> drivers/iommu/amd_iommu_proto.h | 6 +++++
> drivers/iommu/amd_iommu_types.h | 3 ++
> include/linux/iommu.h | 8 +++---
> 7 files changed, 69 insertions(+), 12 deletions(-)
> create mode 100644 drivers/iommu/amd_iommu_debugfs.c
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index d40248446214..8d50151d5bf4 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -65,9 +65,9 @@ config IOMMU_DEBUG
> depends on DEBUG_FS
> default n
> help
> - Base enablement for access to any IOMMU device. Allows individual
> - drivers to populate debugfs for access to IOMMU registers and
> - data structures.
> + Enable exposure of IOMMU device internals. Allow devices to
> + populate debugfs for access to IOMMU registers and data
> + structures.
This help text shouldn't change just because a driver is making use of
the interface that was created in the previous patch.
>
> config IOMMU_IOVA
> tristate
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 5e5c3339681d..0ca250f626d9 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -11,7 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o
> obj-$(CONFIG_OF_IOMMU) += of_iommu.o
> obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
> obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
> -obj-$(CONFIG_AMD_IOMMU_DEBUG) += amd_iommu_debugfs.o
> +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o
> obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
> obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
> obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
> diff --git a/drivers/iommu/amd_iommu_debugfs.c b/drivers/iommu/amd_iommu_debugfs.c
> new file mode 100644
> index 000000000000..3547ad3339c1
> --- /dev/null
> +++ b/drivers/iommu/amd_iommu_debugfs.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD IOMMU driver
> + *
> + * Copyright (C) 2018 Advanced Micro Devices, Inc.
> + *
> + * Author: Gary R Hook <gary.hook@....com>
> + */
> +
> +#ifdef CONFIG_IOMMU_DEBUG
Since the module won't be built unless this is defined, you don't
need this.
> +
> +#include <linux/debugfs.h>
> +#include <linux/iommu.h>
> +#include <linux/pci.h>
> +#include "amd_iommu_proto.h"
> +#include "amd_iommu_types.h"
> +
> +static struct dentry *iommu_debugfs_dir;
> +static DEFINE_RWLOCK(iommu_debugfs_lock);
Use amd_iommu_...
Also, didn't you run into an issue with the CCP and debugfs creation
during probe using a RWLOCK? Should probably make this a mutex.
> +
> +#define MAX_NAME_LEN 20
> +
> +void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> +{
> + char name[MAX_NAME_LEN + 1];
> + struct dentry *d_top;
> + unsigned long flags;
> +
> + if (!debugfs_initialized())
> + return;
> +
> + write_lock_irqsave(&iommu_debugfs_lock, flags);
> + if (!iommu_debugfs_dir && (d_top = iommu_debugfs_setup()))
> + iommu_debugfs_dir = debugfs_create_dir("amd", d_top);
> + write_unlock_irqrestore(&iommu_debugfs_lock, flags);
> + if (iommu_debugfs_dir) {
> + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
> + iommu->debugfs_instance = debugfs_create_dir(name,
> + iommu_debugfs_dir);
> + if (!iommu->debugfs_instance)
> + debugfs_remove_recursive(iommu_debugfs_dir);
So this might cause an error. You remove it, but don't set the variable
to NULL, so the next IOMMU that is probed could try to use it. But why
are you deleting it anyway?
> + }
> +
> + return;
> +}
> +
> +#endif
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 99d48c42a12f..43856c7f4ea1 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -89,6 +89,7 @@
> #define ACPI_DEVFLAG_ATSDIS 0x10000000
>
> #define LOOP_TIMEOUT 100000
> +
Spurious newline.
> /*
> * ACPI table definitions
> *
> @@ -2720,6 +2721,7 @@ int __init amd_iommu_enable_faulting(void)
> */
> static int __init amd_iommu_init(void)
> {
> + struct amd_iommu *iommu;
> int ret;
>
> ret = iommu_go_to_state(IOMMU_INITIALIZED);
> @@ -2729,14 +2731,15 @@ static int __init amd_iommu_init(void)
> disable_iommus();
> free_iommu_resources();
> } else {
> - struct amd_iommu *iommu;
> -
> uninit_device_table_dma();
> for_each_iommu(iommu)
> iommu_flush_all_caches(iommu);
> }
> }
>
> + for_each_iommu(iommu)
> + amd_iommu_debugfs_setup(iommu);
> +
> return ret;
> }
>
> @@ -2783,8 +2786,6 @@ int __init amd_iommu_detect(void)
> iommu_detected = 1;
> x86_init.iommu.iommu_init = amd_iommu_init;
>
> -dump_stack();
> -
> return 1;
> }
>
> diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
> index 640c286a0ab9..e19cebc5c740 100644
> --- a/drivers/iommu/amd_iommu_proto.h
> +++ b/drivers/iommu/amd_iommu_proto.h
> @@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void);
> extern void amd_iommu_init_notifier(void);
> extern int amd_iommu_init_api(void);
>
> +#ifdef CONFIG_IOMMU_DEBUG
Use space instead of tab.
> +extern void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
> +#else
> +static inline void amd_iommu_debugfs_setup(struct amd_iommu *iommu) {}
> +#endif
> +
> /* Needed for interrupt remapping */
> extern int amd_iommu_prepare(void);
> extern int amd_iommu_enable(void);
> diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
> index f6b24c7d8b70..6dca9fe38518 100644
> --- a/drivers/iommu/amd_iommu_types.h
> +++ b/drivers/iommu/amd_iommu_types.h
> @@ -591,6 +591,9 @@ struct amd_iommu {
>
> u32 flags;
> volatile u64 __aligned(8) cmd_sem;
> +
> + /* DebugFS Info */
> + struct dentry *debugfs_instance;
> };
>
> static inline struct amd_iommu *dev_to_amd_iommu(struct device *dev)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 98527f9b473b..dbfff811aa25 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -412,6 +412,10 @@ void iommu_fwspec_free(struct device *dev);
> int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
>
> +#ifdef CONFIG_IOMMU_DEBUG
> +extern struct dentry *iommu_debugfs_setup(void);
> +#endif
> +
Any reason why this moved? If it was not in the right place in the first
patch, that's where it should be fixed.
Thanks,
Tom
> #else /* CONFIG_IOMMU_API */
>
> struct iommu_ops {};
> @@ -696,10 +700,6 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
> return NULL;
> }
>
> -#ifdef CONFIG_IOMMU_DEBUG
> -extern struct dentry *iommu_debugfs_setup(void);
> -#endif
> -
> #endif /* CONFIG_IOMMU_API */
>
> #endif /* __LINUX_IOMMU_H */
>
Powered by blists - more mailing lists