| 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
| ||
|
Message-ID: <3eb931c8-fd4f-f08e-3449-00eb02aff52b@amd.com>
Date: Fri, 30 Mar 2018 14:08:23 -0500
From: Gary R Hook <gary.hook@....com>
To: Tom Lendacky <thomas.lendacky@....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 03/29/2018 11:16 PM, Tom Lendacky wrote:
> 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.
Roger. It should be changed in the base patch only.
>
>>
>> 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.
For some reason my build is trying to compile this file, even though I
have the debug option disabled. Gotta track this down.
>
>> +
>> +#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.
Yes, and had to make a change. I will do so here.
>
>> +
>> +#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?
Right you are. My thought was to remove everything driver-specific in
the event of a failure. Do we not like that?
>
>> + }
>> +
>> + 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.
D'oh!
>
>> /*
>> * 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.
Yep, fixed it.
>
>> +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.
It was in the wrong place, but you are correct. I'll properly move it in
the other patch. Thanks.
>
> 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