From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753378AbcBVE4A (ORCPT ); Sun, 21 Feb 2016 23:56:00 -0500 Received: from mail-bn1on0083.outbound.protection.outlook.com ([157.56.110.83]:34030 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753284AbcBVEz6 (ORCPT ); Sun, 21 Feb 2016 23:55:58 -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 V4 2/6] perf/amd/iommu: Modify functions to query max banks and counters To: Borislav Petkov References: <1455182127-17551-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1455182127-17551-3-git-send-email-Suravee.Suthikulpanit@amd.com> <20160218111116.GE3694@pd.tnic> CC: , , , , , , From: Suravee Suthikulpanit Message-ID: <56CA94C2.9090501@amd.com> Date: Mon, 22 Feb 2016 11:55:30 +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: <20160218111116.GE3694@pd.tnic> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [124.121.8.20] X-ClientProxiedBy: SG2PR03CA0039.apcprd03.prod.outlook.com (25.160.233.49) To CY1PR12MB0442.namprd12.prod.outlook.com (25.163.91.20) X-MS-Office365-Filtering-Correlation-Id: d49f4dde-680c-4910-8c4c-08d33b447352 X-Microsoft-Exchange-Diagnostics: 1;CY1PR12MB0442;2:7haSFB4G6epJfS2fB4PUJN7ijrtclNeZlQ8cWaL2Fv9nage9drj/5rBEBnAxyIAIgDYiNPVywuy4KWpE/xrCbTAy+IGZCiGOT5ASEi/Gp0A+4TRuhnxjSpYNDf3fK/Q1cSJfNNNk3cTS4mzfID2A6HkzM6QgTaTDuTze/Hrq54bnDbsOIB/vRZSOn0aU5BSS;3:s88lac7gm4Fv0odWgvsB62nIJQUqgGvgDTX0IkGYqQTIitoJA6Yrvsl1QAopT1ObUcHsQCn3uPdJJ0ZsBKqQS9kC4/yl4uVLZCES/0QIIrNT4yD4ktC97E7RTkZjU8ZP;25:x9wk+uh/ZdJt9hxu+2nmKTPlgaS5dBK+md9LhlizBiRri8FQyce3w1pVJ85ew3DNOBs9qVOR0wzgq5f2YrIQ2zz0OUKjq5hXKBc7q1AkhqvxYDd38vrqlB71lxakt1TajFbd4PoYdpRo2GeldSWjuwyPYCNw0zuSnwSpOwvTkS6OTgtR7szOWiF5yjdMc+/j7LfoTR2J21BNAHF7+hvVXFnd4uW1ouGJZAzSGgyqZiXD/1J6wE9swG7n9uefmSyhAdSgMidhZBAbRtQGKOqn3lUTwPk8iJdJ+ux+R6U97NUF83vi+dmd1Py2GATps+qlSSjsOS5nLWtg7DfYe7ektWJwp1fZRy5RPFaYtFpHoiU= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:CY1PR12MB0442; X-Microsoft-Exchange-Diagnostics: 1;CY1PR12MB0442;20:fLwZQEP6BTuQdojQ+XOXgDkSQYLjTsw3sSAQPGnkGeM3715Fe2qnkvV5nfQFVn9LT3vCDwTjNFl0nIjOQEBmSfATdciNww0Z/M/m/uddpAXZvMBqRofPSS1/LL9V+fszwpPL/ndD4lajFqMdhCGcpKPg9ZB82Y6m6TqhXOz3EQ1B3UdRaSsT0sjlqWrpsAYZ/7fm4Yelxf5GNeYa/Xo4BLewvXIlywDRqw2399hPtEtssoVaqqTFGaczJ6Hhl0c6BbTnNqwr3+wfKeZfccNCDCBWHW8xXa520YiCadWkjeuxfXvySVnnA+GT+KlDtIeRFCOlrmXil3EQd8IaTyXntEC8V2FX7BQoONBJWuccQMLywQ+yElsTfzUcA8Ci3Yhj5Omg4DEkIH2As8Yf7+FBaDTw1UPOFNh6lJBbSU7P9sR6QCYyh/9LAs9io+n2bEHRlcgAzXjVAfv/HH0yDFv9d2F+337aaYVUY4eGVWPmW03P+Szf20O6uwyA6GQYLDXr;4:Q9WcprTJ2SqVTSX+sCVFM/1L9EafknTpc2J8m8MEq5V8lhUMj3yYMDAr+tqzIq+rR1lN0dVr5iBMehwNHELFkEQxUyvNB1A0u+5vPqCXUMGWdAeOPThINEzDWuSB6aC6HbjCTrbCpwYNNKep6YeQtS363WCIhjVTamjWcYr4ZuuUVQIvwSGD4xpTvAAx5IhnIpvZI1DcAjgT5FnxC2zD5RB/GnhvbJRy842/NhSGXWQYyD00MW3pWYUo5M9vY/9TATYEF5wMZz1Da+A9iMJa2T0J+mQR2J7l5+qiKTjQSTLccTb0Ygz1nlxq9uJ5/cDJkY/G2+ZtI9AM1vhBS2h788cjFNufROkbDT+0YYCYRtk7MHL7FJvRbUmyng3gQil+HhzfljxlprN3MZy9jlu44w== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(767451399110); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046);SRVR:CY1PR12MB0442;BCL:0;PCL:0;RULEID:;SRVR:CY1PR12MB0442; X-Forefront-PRVS: 0860FE717F X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6049001)(6009001)(377454003)(479174004)(164054003)(24454002)(33656002)(4001350100001)(23676002)(110136002)(76176999)(5001960100002)(54356999)(87266999)(83506001)(50986999)(2950100001)(77096005)(86362001)(36756003)(15650500001)(50466002)(117156001)(87976001)(5004730100002)(2906002)(4326007)(92566002)(5008740100001)(230700001)(586003)(6116002)(3846002)(1096002)(42186005)(65956001)(66066001)(47776003)(40100003)(189998001)(19580405001)(80316001)(122386002)(19580395003);DIR:OUT;SFP:1101;SCL:1;SRVR:CY1PR12MB0442;H:[192.168.0.19];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtDWTFQUjEyTUIwNDQyOzIzOmE1QTdRa21jMHFpbm0zRmRoZ0dBTU0rV3pE?= =?utf-8?B?MTB3NXhWZU1hQ2l1R2o4em1JVVJhOHVHN3BuNEpSTU9CRmVCMmNCV2dMS1NX?= =?utf-8?B?UnphL2JtNDNGT05uN2gwZEtuZEo0eEhlUmFQSlFkQTlBek5HWmxwUWtYL2Rs?= =?utf-8?B?TGUyTm00Sm1NUFltUUVxSGJvNEJuL1gyc3dkUDhGZExVZmZqUjRrbk9TQmlh?= =?utf-8?B?bjBBaGR1MFdzUkc3Q0RlYmV6b0dzZnNFZ3FZdWVHV2hOZy9RR3lLT3ZnNHAx?= =?utf-8?B?RGF5R3JDTFJCb3luSlpxSUxEa2hKYUxkS0llS0pIdXJEQnZUQ25GaUVjYzVT?= =?utf-8?B?WnJKK0lDTDFXRDNVVXZraDVHajNXb0FTZFpSMW1US2huLy9ZMSsyanVRVUNJ?= =?utf-8?B?eXNCOVhHUWZUMXMzeDNubHJtaER1VGxIdlBzTFJyNmdQMlplMUhHVE10Mm9G?= =?utf-8?B?Rkx0ODJrU0MzNUpwK0ZPZjF2b2xuK1BJeG9kV2Z4dDZIREljUHNPaFphamRH?= =?utf-8?B?QWk0bkw0aHRXT0NGcHZaS3hSSG9zUXZKQ1NNZTk4Y3BCaUlZZW1PWDFOUUE2?= =?utf-8?B?b2xUUUlPWXZXUEJ0czNwWmJWcUt4WCtxbXBxQmUyMFd2Y3Nuc0JtQlZqREtS?= =?utf-8?B?Qm5yYllKSXN2eE01ZXJpRjNWUURYYm9laE5YSXhhbEduelVxY0Y4R3NqV2hQ?= =?utf-8?B?YWtWT3ZBTEt5QklBN1dUZ3J0b2JuYXRwaENyTE9rMnZYMWRyZTJaVVNReFp0?= =?utf-8?B?SEExdnBaMFNxZU1QNFdmZ3pLWnFuT1J0d0Ruc291RGtpTVpQZzYweWV3dXhT?= =?utf-8?B?SmlSc2R4SFZRbm04RldmQzdHbEJzOGc5bDVwZDU5eW43TVlIejltWTNWSzR1?= =?utf-8?B?dFNwcWVRS3h1M0hpK1JYdExWeGVYQ1Z0c1czZW1qWVh2eVdCR0FUaHhXdE5q?= =?utf-8?B?OEFNU3hHZVBNTlp5Q1Z5R0Y2NC9ZL0dkU2pXeGZNdUVXUHFVanpaeDdKR2xD?= =?utf-8?B?OHdoTjc5ZG4ydjR3QlZCMDNjblNXZ1kxWVVzYTNTYzJpSWhkUWQxQjZPazVU?= =?utf-8?B?NncwaDJFdjZndUorSHYxdHJHbjgzV0tjblVYT2p1VVVEekNZa2tyenFKZnVB?= =?utf-8?B?aFlPZFhDME1kUzJLODg0aG5ZTHRMYm5XUXhSOWpZalBBQWJ6SWFKbUgxd2po?= =?utf-8?B?SFB2RGNjSDNYRVpuMlUvenZaNnI4aUJzZW9qOVRnaXp2QXRzYUVmNXlrL3g5?= =?utf-8?B?d2RaeTBIUXVLZVpKWVB6U1JHVkxEM0pNQXRSS3BLWDdzWDM3bVhNMnd6Q0dK?= =?utf-8?B?RFZCMDdsSmVack03TjRwMCswTU5rV2xHVVZYMTd1RldXZjdTUmVMMTBzK1Ur?= =?utf-8?B?VWIrRU5YNGRqVGRKZEN6NWI0NVVkYmVQbzBkdThwcUFOT2c1bkxiTzRKc1BO?= =?utf-8?B?aE1BbTRJOXVFYjA2bC9RK2Z6TVVFZ1pQbGplbHFOMUYvQ0hYcUVST2tDL2Vy?= =?utf-8?B?c01GVUVVU2sxK3BVT0xCWG01YVlhRVZrVW13bFI0eWFJR3VreUZLTmoyVTZP?= =?utf-8?Q?vyGhyoybXlfr88SXtzoMtk0YlkpZyp7DxLQiCm2ogeYc=3D?= X-Microsoft-Exchange-Diagnostics: 1;CY1PR12MB0442;5:q94F4aMyna1Mn9lkuB2OgYpjAk+f14zyZwztwEhgXRZdj0PxrAqlgnN8Utm7ej9IHivAKytY8Yy0FIOV9yRJxO7hFCHxIsHw6pjD09PKtvEkLis4mC/rxhAM+U5xMs+EWSUgsVLs5zN0FxjHk4S6Gg==;24:adRfWI67Wc9X3L03vacEMYD3U1+2dg3ibD64BX7ZYsOaJJY2fSaPtYXGq6SL5rXUKaj0yLxYRSB5r63N/ZSo6aXRJEqNnoEyGQ+pcd2slaE=;20:pHu7HiMz91ByoJZWt90xDQTJHtBUtRvO9Pr4DOSRIFRLKr0pOwt6q2fJ8DfV9ZHbvTzkKYrCfNQbAUGDeH0tK9WyIv+Z/1xvcJd6HI8fYMOWWuuBpuycLE3WF31m4S8Vz2AQHBDLkTPlHedWpNg+OXNeEl3iXYBBjrX7ZD4qZG8grMzcRRRWl70mTnxvO1bNhnYIVHspszKhheIAe4pOMCtknWCtKsY7V0YiiZPBZqzXZ+yqKIFnsEFrKY6Xn3Lz SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2016 04:55:54.4069 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR12MB0442 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 02/18/2016 06:11 PM, Borislav Petkov wrote: > On Thu, Feb 11, 2016 at 04:15:23PM +0700, Suravee Suthikulpanit wrote: >> Currently, amd_iommu_pc_get_max_[banks|counters]() require devid, >> which should not be the case. > > Why? > > Commit message could use an explanation. > >> Also, these don't properly support >> multi-IOMMU system. >> >> Current and future AMD systems with IOMMU that support perf counter > > "with an IOMMU that supports performance counters" > >> would likely contain homogeneous IOMMUs where multiple IOMMUs are > > What are homogeneous IOMMUs? I intended to mean the same IOMMU version/capability for all IOMMUs in the system. > >> availalbe. So, this patch modifies these function to iterate all IOMMU > > Please integrate a spellchecker in your patch creation workflow: > > s/availalbe/available/ > Thanks. I have now rephrased and spell check the new commit message for the V5. >> >> Reviewed-by: Joerg Roedel >> Signed-off-by: Suravee Suthikulpanit >> --- >> arch/x86/events/amd/iommu.c | 17 +++++++---------- >> arch/x86/include/asm/perf/amd/iommu.h | 7 ++----- >> drivers/iommu/amd_iommu_init.c | 20 ++++++++++++-------- >> 3 files changed, 21 insertions(+), 23 deletions(-) >> >> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c >> index 2f96072..debf22d 100644 >> --- a/arch/x86/events/amd/iommu.c >> +++ b/arch/x86/events/amd/iommu.c >> @@ -232,14 +232,6 @@ static int perf_iommu_event_init(struct perf_event *event) >> return -EINVAL; >> } >> >> - /* integrate with iommu base devid (0000), assume one iommu */ >> - perf_iommu->max_banks = >> - amd_iommu_pc_get_max_banks(IOMMU_BASE_DEVID); >> - perf_iommu->max_counters = >> - amd_iommu_pc_get_max_counters(IOMMU_BASE_DEVID); >> - if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0)) >> - return -EINVAL; >> - >> /* update the hw_perf_event struct with the iommu config data */ >> hwc->config = config; >> hwc->extra_reg.config = config1; >> @@ -450,6 +442,11 @@ static __init int _init_perf_amd_iommu( > > Btw, that _init_perf_amd_iommu is unnecessarily split from > amd_iommu_pc_init(). You should merge those two. But that's another > patch. In that same cleanup patch you could do > > s/perf_iommu/pi/g > > or so because that perf_iommu local var is unnecesarily long and impairs > readability. > Sure, I'll clean up both of these. >> if (_init_events_attrs(perf_iommu) != 0) >> pr_err("perf: amd_iommu: Only support raw events.\n"); >> >> + perf_iommu->max_banks = amd_iommu_pc_get_max_banks(); >> + perf_iommu->max_counters = amd_iommu_pc_get_max_counters(); >> + if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0)) > > Simplify: > > if (!perf_iommu->max_banks || > !perf_iommu->max_counters) Ok [....] >> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c >> index d30f4b2..a62b5ce 100644 >> --- a/drivers/iommu/amd_iommu_init.c >> +++ b/drivers/iommu/amd_iommu_init.c >> @@ -2251,15 +2251,17 @@ EXPORT_SYMBOL(amd_iommu_v2_supported); >> * >> ****************************************************************************/ >> >> -u8 amd_iommu_pc_get_max_banks(u16 devid) >> +u8 amd_iommu_pc_get_max_banks(void) >> { >> struct amd_iommu *iommu; >> u8 ret = 0; >> >> - /* locate the iommu governing the devid */ >> - iommu = amd_iommu_rlookup_table[devid]; >> - if (iommu) >> + for_each_iommu(iommu) { >> + if (!iommu->max_banks || >> + (ret && (iommu->max_banks != ret))) > > What is that supposed to do? > > Check that the max_banks of a previous IOMMU are == to the max_banks of > the current IOMMU? > > Why? Could definitely use a comment. Current AMD IOMMU perf implementation assumes that all IOMMUs must have the same value of max_counters. Therefore, this logic iterates through all IOMMUs to check this. I'll add the comment here. Thanks, Suravee