From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752397AbcBKIfB (ORCPT ); Thu, 11 Feb 2016 03:35:01 -0500 Received: from mail-by2on0055.outbound.protection.outlook.com ([207.46.100.55]:36706 "EHLO na01-by2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751343AbcBKIfA (ORCPT ); Thu, 11 Feb 2016 03:35:00 -0500 Authentication-Results: lists.linux-foundation.org; dkim=none (message not signed) header.d=none;lists.linux-foundation.org; dmarc=none action=none header.from=amd.com; Subject: Re: [PATCH V3 5/5] perf/amd/iommu: Enable support for multiple IOMMUs To: Borislav Petkov References: <1455058435-8716-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1455058435-8716-6-git-send-email-Suravee.Suthikulpanit@amd.com> <20160210171401.GI23914@pd.tnic> CC: , , , , , , From: Suravee Suthikulpanit Message-ID: <56BC479E.2070004@amd.com> Date: Thu, 11 Feb 2016 15:34:38 +0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160210171401.GI23914@pd.tnic> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [124.121.8.20] X-ClientProxiedBy: SIXPR04CA0013.apcprd04.prod.outlook.com (10.141.119.13) To SN1PR12MB0446.namprd12.prod.outlook.com (25.162.105.14) X-MS-Office365-Filtering-Correlation-Id: 84c3354b-1c7e-4c18-4f62-08d332be38a2 X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0446;2:JXnf94P5qa5cQXqH9focYKRtJaymq7tyNKzCD2Xmk1r/TyrOL5Evl7D0fqmWUEEXyGJe+clkXhiH25CYbSevHBq6ySvkXxxlV/3ecgbnLLfsFFKZZutr5FVZ8bH32QxTNYskWyySZbmcCVhCfrpRriC/MyaYVSKJrSiUSE+w9QVtDVoBEJTkuN1034tKe20H;3:6RmCS3x8iq2uYbczQEQNcC4juCucyN4nW5ZHI8uKrJzVvvAVa76JyzDG1aoHIV0RKpSGcnOr0uOe6SdLHOWv+CS5NQ/M2W2PDV3pMdeOEPmbaIQU29CQbNovHaI+YBAb;25:ZunFgg2LAIqtxsicG0fC36w+jIYV2fbDXzhBRT6KC2FGCBqxdypt4cGRHWygnQlyDWJ7UZ5mxnoX0XZ8xa3BYdooB1qeLCxmnpHNlZ9bL2GyrqPi02qw5ZnqygKP7UW6YMQd5XUqlM8ylTk6EpE41KYcuI/D08K81xcjFryY7f2/tlDt2NImUK/uI7KCiLvwXsFOYZhMmkeV0wSJNzz6c1m8ao7RcgeJiEr2iHtiH5O+k2RPnHyqXiP+IktFYuTtT8Juf1k6ZAZkS7jVSidzqro/aS9K3eNp24+8se3fSUvhvVJ4xF4S7slskEyMi1ar X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:SN1PR12MB0446; X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0446;20:ukYJK4NdvabqZaeIXu+DcCQegSuv+g32SaG3oQ9IqltK8b2ANndREGh007vCHwqgWAtxi5ZYWAXoWXrH+mfkVlugFCAYNaw0uFC4SOCMNShzZFOjwQ6mqunP2GSRag0njCidUcs2hxlBCGZU5jaeXdIYelk9Oa+9LwjuHwaN/VjXij4ZHdD3sJkLYOXXDDBlmfZU1auxDDSNnn1+Y9ZYRJOmuUZnbZqo2CoPv9lj5xUghhzxrldcQxdrVt/tESBjRjcL6BVINj4vauCaQ9J2WWcpcM6t5A647jh0In4cTmhOJDI1jPw2YbTm3Sk8bO4yw3mNFsHCh/UavyWMnmgM/fMRTMqcSv+P92ZDjN1rb0JdGFZqNZbNCOq+jw4vVCuUt7RuplYJY8IXXCE6jBsv3IrAqV4gF9gzbh96DHxO7IBBCXZ6U7Sc9+FKLhwbKgB141zWgjGVaytmdHyQCfmRmIbr6RKD/8/CW6LMQTag1blBiizfH1QJJ7Oocx1WctCp;4:UmomkmJLXFGr2yr0tsp+jbXPYaWHanGXBTS+kag5evvwmVwRfMcZEFHhViAhRua+wQKpGlXXVN0IGvE4R+2fafdvK8aRZQ9s8peG3Y7pakuLgas230qGyW5euqf0psiwC82TWJiOM+M5RmVtxHp5fCkcrduCOZdnhYwHjy+k120HTz98tSnUIrQk9+b2E/Df16Bq2L2+3fwz3im8gK6YdOnTcAaXsOVXNIQe6XJmwUYtSgQ70ER4o1EYFdEme+gJs5FhG3w15mv6vcRy2aGNKEmWufycEbrk7l0aOg25QL5vgwk0iQ03ayqI9CsmXhMBaXKaj0PnY/3cnOiIp6ARW0cbQOWg7XEh2qoPBfmOH8Mz2Ncl7gJ0otGosjRBwZ0r X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046);SRVR:SN1PR12MB0446;BCL:0;PCL:0;RULEID:;SRVR:SN1PR12MB0446; X-Forefront-PRVS: 08497C3D99 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6049001)(6009001)(24454002)(43544003)(45984002)(164054003)(377454003)(479174004)(4001350100001)(5004730100002)(47776003)(23676002)(4326007)(92566002)(586003)(2950100001)(2906002)(3846002)(230700001)(50466002)(1096002)(64126003)(40100003)(117156001)(6116002)(77096005)(66066001)(36756003)(33656002)(65816999)(83506001)(76176999)(54356999)(50986999)(80316001)(87266999)(122386002)(5008740100001)(110136002)(5001960100002)(65806001)(87976001)(189998001)(42186005)(86362001)(65956001);DIR:OUT;SFP:1101;SCL:1;SRVR:SN1PR12MB0446;H:[192.168.0.19];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtTTjFQUjEyTUIwNDQ2OzIzOmVaL0tFWGtva2dFbys0UVEzb1pYcVdzRWNm?= =?utf-8?B?Ly8wdlFQaldISFV0TzZiY0J1TlJ4RWV0Q1BwcDJocHR0M1prN2F3MTFwNm5q?= =?utf-8?B?S1dSKzFTdm0zb204RGcrN0Y2MWVIS0JBL082ci9DTmRWcXJaN0wwS2xKVHBs?= =?utf-8?B?cHZpd3ZCQXVtRHhrVDMrbGw1OHNjbmVzVkR6MVZVQmY3ejVnelo5Szc4ZGtL?= =?utf-8?B?VzFkS3BjUHFJcjAzUUpsMXp3V2xMK09tN0J1NlJabkx4cWZLaTI0dmd6YVRD?= =?utf-8?B?YTBrdllpL0pqbGpBVlpQTHhFajcwUnBQZEduV3hpY1Zxc2p2QXBEUjRYaGVI?= =?utf-8?B?WHRZTFEyNGtweDBuY2lsUUNBOG1EQlR6a1Y5dk9aS205akhGVDhxUXYyTHdZ?= =?utf-8?B?T2FsblZRL3Z2THFTK0ZQQlhLaVJYQmlLaWxjR1VteVE2dWRQSlRxU3U3aU02?= =?utf-8?B?bXc1Wkhwaytqcjd2WUZkTTZEcHBYd1pFeEhBQkxOdDhVR0xpZ1l4elZGQ0V5?= =?utf-8?B?SmdXYkFTcXhMQUpvQW5acWk0RlZFbEthV21jcitWbVVkOEx0TFdVYzJQdXJ6?= =?utf-8?B?N2Y5Sk9PaHVEM0FhN3V2aVlneU43NkVoRXRzKzJlWUZGdUJHRW9VYmJWN0FH?= =?utf-8?B?NmQxSXk0MWVEQ3dOZXRwU1k3ZlMvUWVabFlwelJSMDFlSk1jc1VxWkZEOXFU?= =?utf-8?B?dmpIcDNIWktjK0M4bFYrdlFlVFJSczQ3eHR0UWNlZXB2WDgzakZWZm5mQlJY?= =?utf-8?B?ckNtdSt5NnRVMHRHQXhsNTRqV2hsd1kvdFJlOVlHNDdQcFpGU2hjM3k5Q1g1?= =?utf-8?B?cWpmWkFUZklUdWFNd2Vvd0tHYUM3MTJtbUl2VlNSalJxejdTYmtMSWs0TmZh?= =?utf-8?B?OE1wNENQOE8zVDZEMThFM3ZpODJ6MDU5djRsYVl4RFV4b3dzV0FMWXhIV2Ev?= =?utf-8?B?RzZSMjNwaW5iZXpOQm8yU1ZnbG01ZGFwaS92VUsxT2tCdTBVMTV5OGtjL3Fa?= =?utf-8?B?TGJiVnFscUc0c3laQVFHTzBzZ0dkVG1kRWNYS0ppWDliRU1sT0RBeGxJb050?= =?utf-8?B?WmRUU2VueUlSbXB4NjdmaitZL0Foc2RHTVdiamxtdmcvUGMwajVoVFJrMTZs?= =?utf-8?B?MG14ZndDaGhZSUphVGxSc0p4eWVxZjVONGhmUk02SEtPLzVPd2hweUtUdkRj?= =?utf-8?B?UmpLajF0dEVsWlN0cUdvYlJVaW8ySDJteUlpQVpSa01SNGlINGlsaFdndk8w?= =?utf-8?B?ckxCY0NaZDNxNVNoWkxYckF1dVZ4c0trOUg2b1IwR3BDMnM3enlmYThmMUdX?= =?utf-8?B?VFZuZ0p5djhDWmozYUZXS1ZiS3BBZWRrOHNTaGJ3clNLRFpiV1JNUVR6SHJz?= =?utf-8?B?dGsxNXFKTmQwWVJ3UmJwcDhJNjd6dUJldkNMREJHOEFXRjRTV3p3bWJnYm4v?= =?utf-8?B?cGlmbkF3VFIyUExzWWhjdUtoNkhwdWkxUU93aXhFTGtwaXA2MkJ5Sy9wWFdK?= =?utf-8?B?S2I2aXR4ZHpDMWtCNkR2M1QwUGVkLzBjOS8xSmZxYTNHYjIvNzhsVWRFcTln?= =?utf-8?Q?aGfXkCWjrMPy+9Zu1KUUXqHP1Zk8855welrLhTfnsdO0=3D?= X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0446;5:2nMmRCQSFnhR6EQj8POESJL/Qz8NR25P5073fep4JJlJ6X3Xk1NZKoMliicOy29YhQ/hOEsBjgM3Bx54LsMROoFdCJMTU/cMtMgoPwbnAeQK7Rq1VGkWaNG4849W/symGeH/sYKAiwNgA7UyMlWiFw==;24:oDrhFxoIqkWaI7p8RO1D1qb8QTiR9BShreBdjNKb24apmtdA/b7yZqH/Ru4gRBF6kYtRkHMipeyKDczdkUM4SnvFoEVhcEntGcHXbfucH7E=;20:1ayexUDbGoEzoXIHcHfmIMN6rrEiqqiGuht+oANs6HjZV/9SW6pkWKc5lcktv0ahbAFkQYwb87LpMsf+Xf5j8cxBlUP+eSqFrvgW82XR+9WvSCR9RXhmYGEQcE4/yN5YQKqQToBr6rausbAmwyBUtpSnCJYBdny1uKvvUrO5TtHSzMpdbPZ/UsrfsK1KivcUsjPzOB7YZfIjY4fTvaR/SVJQUK7g4HClP6/xLYnVxCB776NDALjiTyKea6H3Yskf SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Feb 2016 08:34:54.1269 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0446 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Boris, Thank you very much for catching several styling issues. I should have taken care of that earlier. I'll clean them up. Please see my other comments below. Thanks, Suravee On 02/11/2016 12:14 AM, Borislav Petkov wrote: > On Tue, Feb 09, 2016 at 04:53:55PM -0600, Suravee Suthikulpanit wrote: >> [...] >> >> - /* IOMMU pc counter register is only 48 bits */ >> - count &= 0xFFFFFFFFFFFFULL; >> + pr_debug("perf: amd_iommu:perf_iommu_read\n"); > > What is that debug statement good for? Can it go? > > From looking at that file, it has a bunch more which are completely > useless and can be deleted. In a separate patch. Sure. I'll add a new patch to remove all these pr_debug. >> [...] >> + if (!perf_iommu_cnts) >> + return -ENOMEM; >> + >> ret = perf_pmu_register(&perf_iommu->pmu, name, -1); >> if (ret) { >> pr_err("perf: amd_iommu: Failed to initialized.\n"); > > You can define pr_fmt to "perf/amd_iommu: " at the beginning of this > file and then you don't need to supply that prefix each time you call > pr_* Good point. I'll take care of this. > And that sentence above it wrong. It should be: > > pr_err("Error initializing AMD IOMMU perf counters.\n"); > > or something like that. > OK. >> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c >> index 531b2e1..7b1b561 100644 >> --- a/drivers/iommu/amd_iommu_init.c >> +++ b/drivers/iommu/amd_iommu_init.c >> @@ -1133,6 +1133,9 @@ static int __init init_iommu_all(struct acpi_table_header *table) >> return 0; >> } >> >> +static int _amd_iommu_pc_get_set_reg_val(struct amd_iommu *iommu, >> + u8 bank, u8 cntr, u8 fxn, >> + u64 *value, bool is_write); > > static int _amd_iommu_this_func_name_is_def_too_long > I'll rename all these functions to make it shorter and less confusing. >> [...] >> + >> +int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value) >> +{ >> + struct amd_iommu *iommu; >> + >> + for_each_iommu(iommu) { >> + int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr, >> + fxn, value, true); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(amd_iommu_pc_set_reg_val); > > Why isn't this EXPORT_SYMBOL_GPL? Actually, I am just trying to match other exported functions in the AMD IOMMU driver. May be Joerg can let us know why these functions are not EXPORT_SYMBOL_GPL to begin with. >> + * which should have been programmed the same way. >> + * and aggregate the counter values. > > That comment is a sentence with a fullstop in the middle. > >> + */ >> + for_each_iommu(iommu) { >> + u64 tmp; >> + >> + if (i >= num) >> + return -EINVAL; >> + >> + ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr, >> + IOMMU_PC_COUNTER_REG, >> + &tmp, false); >> + if (ret) >> + return ret; > > So if one of those intermediary calls of _amd_iommu_pc_get_set_reg_val() > fails, we return here not having read all counters. Unwind and handle > that error properly maybe? > > Same issue above. Good point. So, for writing, I'll make sure to unset the registers/counters when we encounter errors. For reading counters, I think it is acceptable to say that users cannot rely on the return values in the array if the function returns non-zero value (success). Thanks, Suravee