| 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: <20130528110752.GB2575@8bytes.org>
Date: Tue, 28 May 2013 13:07:52 +0200
From: Joerg Roedel <joro@...tes.org>
To: suravee.suthikulpanit@....com
Cc: linux-kernel@...r.kernel.org, a.p.zijlstra@...llo.nl,
mingo@...hat.com, iommu@...ts.linux-foundation.org,
Steven L Kinney <Steven.Kinney@....com>
Subject: Re: [PATCH 1/2 V3] perf/x86/amd: Adding IOMMU PC resource management
On Fri, May 17, 2013 at 02:43:31PM -0500, Suthikulpanit, Suravee wrote:
> From: Steven L Kinney <Steven.Kinney@....com>
>
> Add functionality to check the availability of the AMD IOMMU Performance
> Counters and export this functionality to other core drivers, such as in this
> case, a perf AMD IOMMU PMU. This feature is not bound to any specific AMD
> family/model other than the presence of the IOMMU with PC enabled.
>
> The AMD IOMMU PC support static counting only at this time.
>
> Signed-off-by: Steven Kinney <steven.kinney@....com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> ---
> V2 Changes:
> - Modify the amd_iommu_pc_get_set_reg_val function
> to support 64 bit values.
> - Fix logic to properly clear amd_iommu_pc_present when
> initialization failed.
>
> drivers/iommu/amd_iommu_init.c | 121 ++++++++++++++++++++++++++++++++++++++-
> drivers/iommu/amd_iommu_proto.h | 7 +++
> drivers/iommu/amd_iommu_types.h | 12 +++-
> 3 files changed, 134 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index bf51abb..395fa29 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -154,6 +154,7 @@ bool amd_iommu_iotlb_sup __read_mostly = true;
> u32 amd_iommu_max_pasids __read_mostly = ~0;
>
> bool amd_iommu_v2_present __read_mostly;
> +bool amd_iommu_pc_present __read_mostly;
>
> bool amd_iommu_force_isolation __read_mostly;
>
> @@ -371,21 +372,21 @@ static void iommu_disable(struct amd_iommu *iommu)
> */
> static u8 __iomem * __init iommu_map_mmio_space(u64 address)
> {
> - if (!request_mem_region(address, MMIO_REGION_LENGTH, "amd_iommu")) {
> + if (!request_mem_region(address, MMIO_REG_END_OFFSET, "amd_iommu")) {
This length needs to be different on IOMMUv1 systems where the MMI
region is only 16kb large. Only use MMIO_REG_END_OFFSET on IOMMUv2
systems.
> +static void init_iommu_perf_ctr(struct amd_iommu *iommu)
> +{
> + u64 val = 0xabcd, val2 = 0;
> +
> + if (!iommu_feature(iommu, FEATURE_PC))
> + return;
> +
> + amd_iommu_pc_present = true;
> +
> + /* Check if the performance counters can be written to */
> + if ((0 != amd_iommu_pc_get_set_reg_val(0, 0, 0, 0, &val, true)) ||
> + (0 != amd_iommu_pc_get_set_reg_val(0, 0, 0, 0, &val2, false)) ||
> + (val != val2)) {
> + pr_err("AMD-Vi: Unable to write to IOMMU perf counter.\n");
> + amd_iommu_pc_present = false;
> + return;
> + }
> +
> + pr_info("AMD-Vi: IOMMU performance counters " "supported\n");
Why are this two strings? This makes it hard to grep for the message.
> +u8 amd_iommu_pc_get_max_banks(u16 devid)
> +{
> + struct amd_iommu *iommu;
> +
> + /* locate the iommu governing the devid */
> + iommu = amd_iommu_rlookup_table[devid];
> +
> + if (iommu)
> + return iommu->max_banks;
> +
> + return -ENODEV;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_get_max_banks);
You can't return -ENODEV in a function returning u8. Return int instead.
> +
> +bool amd_iommu_pc_supported(void)
> +{
> + return amd_iommu_pc_present;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_supported);
> +
> +u8 amd_iommu_pc_get_max_counters(u16 devid)
> +{
> + struct amd_iommu *iommu;
> +
> + /* locate the iommu governing the devid */
> + iommu = amd_iommu_rlookup_table[devid];
> +
> + if (iommu)
> + return iommu->max_counters;
> +
> + return -ENODEV;
> +}
> +EXPORT_SYMBOL(amd_iommu_pc_get_max_counters);
Same here. Please don't return negative values as u8.
> +
> +int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn,
> + u64 *value, bool is_write)
> +{
> + struct amd_iommu *iommu;
> + u32 offset;
> + u32 max_offset_lim;
> +
> + /* Make sure the IOMMU PC resource is available */
> + if (!amd_iommu_pc_present) {
> + pr_info("AMD IOMMU - PC Not supported.\n");
This message is redundant. You already print about about performance
counter availability in the init code. Please remove it.
> + return -ENODEV;
> + }
> +
> + /* locate the iommu associated with the device ID */
> + iommu = amd_iommu_rlookup_table[devid];
> + if (iommu == NULL)
> + return -ENODEV;
> +
> + /* check for valid iommu pc register indexing */
> + if (fxn < 0 || fxn > 0x28 || (fxn & 7))
> + return -ENODEV;
Does this check for calling errors? If so, you might consider turning
this into a WARN_ON.
> +
> + offset = (u32)(((0x40|bank) << 12) | (cntr << 8) | fxn);
> +
> + /* limit the offset to the hw defined mmio region aperture */
> + max_offset_lim = (u32)(((0x40|iommu->max_banks) << 12) |
> + (iommu->max_counters << 8) | 0x28);
> + if ((offset < MMIO_CNTR_REG_OFFSET) ||
> + (offset > max_offset_lim))
> + return -EINVAL;
> +
> + if (is_write) {
> + writel((u32)*value, iommu->mmio_base + offset);
> + writel((*value >> 32), iommu->mmio_base + offset + 4);
> + } else {
> + *value = readl(iommu->mmio_base + offset + 4);
> + *value <<= 32;
> + *value = readl(iommu->mmio_base + offset);
> + }
> +
> + return 0;
> +}
Joerg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists