lkml.org 
[lkml]   [2013]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2 V3] perf/x86/amd: Adding IOMMU PC resource management
Joerg,

Thanks for the review. I'll post new patch set to address your comment
shortly.

Suravee

On 5/28/2013 6:07 AM, Joerg Roedel wrote:
> On Fri, May 17, 2013 at 02:43:31PM -0500, Suthikulpanit, Suravee wrote:
>> From: Steven L Kinney <Steven.Kinney@amd.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@amd.com>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.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
>
>
>




\
 
 \ /
  Last update: 2013-05-28 19:01    [W:0.111 / U:1.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site