From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932509AbdKOTEp (ORCPT ); Wed, 15 Nov 2017 14:04:45 -0500 Received: from mail-bl2nam02on0059.outbound.protection.outlook.com ([104.47.38.59]:41024 "EHLO NAM02-BL2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753579AbdKOTEb (ORCPT ); Wed, 15 Nov 2017 14:04:31 -0500 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Janakarajan.Natarajan@amd.com; Subject: Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest To: Borislav Petkov Cc: kvm@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , Paolo Bonzini , Radim Krcmar , Len Brown , Kyle Huey , Kan Liang , Grzegorz Andrejczuk , Tom Lendacky , Tony Luck References: <5113a9d6e76d2c6050c1fba4007068340321521c.1509985085.git.Janakarajan.Natarajan@amd.com> <20171109183425.rhqc5wrltznt5tcf@pd.tnic> From: "Natarajan, Janakarajan" Message-ID: <07f23e5e-2747-b0bc-1b93-f83f3982649a@amd.com> Date: Wed, 15 Nov 2017 13:04:03 -0600 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171109183425.rhqc5wrltznt5tcf@pd.tnic> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [165.204.78.1] X-ClientProxiedBy: MWHPR2201CA0087.namprd22.prod.outlook.com (10.174.103.40) To MWHPR12MB1678.namprd12.prod.outlook.com (10.172.56.142) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: dd30368d-a791-4fd3-21ba-08d52c5bb1cd X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(48565401081)(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(2017052603199);SRVR:MWHPR12MB1678; X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1678;3:C1Dpp0H7C+oHFbsUcJvjAUTPdBBQChQnMWGO+Rg3hsgvBK4LEqFDuux8PHwzF7ZVbok1q/fxWN4tIBQFTu76clSX/Ir9w68OSE0K2sKzyMLCGWSEGsPAtRfMITmkj/JDud0RCqzduuU+mvkbyLcZjX3W1m6cgU1IcZM6dvP6/3PuGypDI0HCjiGi8EifqD3NmFegLp4g5rqjYXDZXD/Pm8EQZbsRC4Sud+qV06t0Icf4Fqk3kHREx1mn9Cf8zM6h;25:6UcXM1q1q4S9jLoBJABlBHk8XXYXqGuplmu82UzNeFhKAwsnHE/ShjAQk1Dahku5wVEeWyY5oKHlVXgEgxQEjTEKY3WCTJKh/X60Ubnaki4uhedJKxcBmaXADqRt4d3bw5A8j/6urqWCMGY7Wi0GTJgBkM3yGdSGkTmocsQEgOo37npsV8RHi7vstAcu4nqd2KcDznbBivnex0Od2wPekR2stk6hBRpjzfBM3C7zALnosl8DSJ4m3ivRCQ+gEsYW4lyZyvEC2pXxcQuT23mJTWCqqtVXj+k80X6e4Y/OQmYWvBfhptl42FESB5Ejph2uNprece1NFunqUVTQX8TTdg==;31:1/pVidB3urP2iTtnZzmz3LiIM73OWcCSgVTkfAqMFiK1N3yn3LWlfmT6S3vZP/KBfps6l55XzjPXFQwMW6Q/gd/FCRBs/r6VfJ4+uAEOI9rnIeAKDucD2pGw+rVV9JA1n+Rg+H/hFEnW/i+R+EPWvhpP5D/fpiYeT0Bdmpmy9c6qDRr5GLV3mGN89pSBGh82y3Wyi/rlvbdbIBa+jdtKLnD/165GZZm535kkXZlvUFY= X-MS-TrafficTypeDiagnostic: MWHPR12MB1678: X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1678;20:FHlhpxXd6+RwowiOK4Ql+OyB2Ax/PDCPj6ULrQyfsPpvn+Xm/I/ou/+POGa/itJegPVkNESW3NGzyz5K8M6Fbb4zs2UEubawtv5sWHD96lGXo/zr11HOpOU7Y75ZQuGQ1Ml9c4EoL6uajULXVa8KS19349W5tu9ggo8HmiG7Ljftcf4reRGo7siTiH1XLO7M2woGAt3+mI8FQHfEN353VVZ6fH8kx7Qce9jsYnCF7i7kYMelssNOzjPgYIqBn2jJim/eLSuiKNUDXtHc3Bgp71L/cNnxVkGw1UZXkkE1Qgc6TmkmxsJZc/6reQd4YxO4xd1oIuwt3lNGOc6n0rY0M/h35FV5CfxUSvXzgjqNM1OeWz/xSjl1MIEMOtvpvYhBrSBSclnbwPwb37+x1jwk416dzjiwFLk5Oii+9sd6HPJtqFJSeqgRWI9wJW5fmUsu1opSeH5S3lv2/KjcwrR7iTAhCLqJqmzvOa/aJqv9fp6Ijf+t8CSSbjyDwFxldWwY;4:yV346lW830U/WZt0SCqBj5Y1fK/6nA2KU5wc7nodhh9PAm4oOZVRCDXrmpCKwGCsL0ZdEiGTzo2iPr9HMtEwEyEGQ33Iwh/DgmksxlGNDDcZZak9tu6cqNo4qrrcqCO7gqbVumcyBety8H+iU7L8OOriz2tQZjAuhad0iWo0B8LRC6Wg3qTT4mnMlbnMx3RR1l/ChZx9yuLoOr0UG357ZEqdrofYDAdIqGEReMXpfWCKPSOMmXnuE9xYY6PfrLJoPDVUlDlPnyJE7Ca4F69n9W2ObMkB+uuFlV5SeP7r45GBCPts0qylMeFTulu0F7Fh X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(767451399110); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(5005006)(8121501046)(100000703101)(100105400095)(3002001)(93006095)(93001095)(3231022)(10201501046)(6055026)(6041248)(20161123560025)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123564025)(20161123555025)(20161123558100)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:MWHPR12MB1678;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:MWHPR12MB1678; X-Forefront-PRVS: 0492FD61DD X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6009001)(6049001)(346002)(376002)(39860400002)(199003)(189002)(24454002)(65806001)(65826007)(23676003)(3846002)(229853002)(65956001)(189998001)(478600001)(77096006)(316002)(53936002)(72206003)(4326008)(105586002)(6916009)(66066001)(16576012)(33646002)(6116002)(86362001)(67846002)(47776003)(2950100002)(101416001)(25786009)(6486002)(230700001)(31696002)(106356001)(6246003)(54356999)(305945005)(31686004)(97736004)(50466002)(7736002)(50986999)(90366009)(16526018)(36756003)(8676002)(76176999)(2906002)(58126008)(5660300001)(6666003)(7416002)(8936002)(68736007)(83506002)(54906003)(81166006)(81156014)(64126003)(53546010);DIR:OUT;SFP:1101;SCL:1;SRVR:MWHPR12MB1678;H:[10.236.68.193];FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtNV0hQUjEyTUIxNjc4OzIzOnJseGVPL3AyRy93Q0owb2xhSkVNbUU0RnBv?= =?utf-8?B?RTE1MGNUcVRKZWJzVjE5N09tZkdkenpvU2hSNzc5QU95aXlyT1d3VVhzZnI1?= =?utf-8?B?MXB1bHM2U2FpSmJhWjdwUFZRWHI3TmVUOCt3SC9wUjNtQjZ2bXdjb0tZclN1?= =?utf-8?B?VFpEc3NxbUNZY3JTMWZyNndWNjJMOGVtL1lwd1hZU1pRTisvZnZybGQrM1RT?= =?utf-8?B?NWE4dEtzMkV3QmluZlVhQkFhMEVjSkNsTWRHOVhPTmw2dUFkcnRqRVkrOWRa?= =?utf-8?B?RExhL2Uweml1RzczNU5tMHhkQk5iMXJ2aW5mTkVzSFZwSHZCMDhOQTZNL2wv?= =?utf-8?B?Wm5WQjdEYnJ1Wjh3UzQ4TG9aZUUyL1pZQ2hQd0xMZ2l1TGlEanoxSnFNV1Zz?= =?utf-8?B?eE1sbzFlN1ZlZjlzcmRyMU16cmFOWW1TVmM1dkc1aXFRWHZxQm9YK1dSMXNU?= =?utf-8?B?SXlhMlFWQVpCUFpJeTZMOUNMWFg0Sy9FODRFemVmaW9SYzJSYWtFbnVXS2o2?= =?utf-8?B?YVNXWU1CQ1ppUUpzQWM1SklLUUsxNlI0Z2IzbjJTY1VEVFJEeTVOSmxhS3hC?= =?utf-8?B?dDk4VzgyTXVTa2IxSGV4Ui9UdkpEOFF0bFE1K1Rhano4UWFHVlpUaGNhVHFr?= =?utf-8?B?OVFFVTRHeVF2amRsUW9yaVNPaHg5ZGdSMVo3a2ZLRC9BenNSVGdvK01NSXR6?= =?utf-8?B?MmxMNEh4Nm9mYzBiamVZK2NCQVBYUmJGOVdzSEppODJqZlN2bnA5eTZzMmE0?= =?utf-8?B?enZaT25jUkRhQlFVSy9JTEg2bkh6emdjUEJ6SmlxY3VZRWRVWW55RVFONjNG?= =?utf-8?B?SjNRV3dLR1lTTVZ2MDBzRjlkM01mMnlnRlhxQ1NDMnVmUEU5U2NvTlNPUk5O?= =?utf-8?B?YzlsZWRjT1pSdDFzMWVhM0FqemswTWVNK3hLc2JMUFJ4NEpIRjE4eVlleU5R?= =?utf-8?B?bUNCR0RxalJMdWVISWJKNWU3ZWtkTWNudDZPZzV6TU1xMVhLOE1rdjJPVnVW?= =?utf-8?B?QmVTRGVsS01jNHlMcEV1SnpuZHBDb0FseWVlTm5OSk4yTWw2b2NQYXVtSGd3?= =?utf-8?B?cWRid3lQRkgyb0J2SHFtdHUwM2g2NkNpWXRkSnBTK1p4c2RBam12Q2kwa3hr?= =?utf-8?B?M0Z6N0FML1ZnbUI4c1VzaFRYTFc2aXJyQjNpd3UvZmc0UWpJTkVLNDZybmdh?= =?utf-8?B?QmVoaG5uRm44QjRQbDVlRFlEdGVpRWtwZzl5NFlmZFZhS0h4N1pGSmQ3NWdJ?= =?utf-8?B?Uk5WUE0wSDJ1TFlQVnBpM1RBaHgwcmhqQmNhL09JUFV3Z1hsdmxmVUt4ZkFt?= =?utf-8?B?dTIvSTNMejJtblNrOTc5ajFtbUNhRHpxa295VE9ibk55L05zNjVSdXFheDdK?= =?utf-8?B?d1N1TVN4NFBNcmN5dkRIZWhiWUZEenB4SkI1Y3BFNSswaHU3cEE0QXV0c25y?= =?utf-8?B?eUVXYStETTRER21LK0x2T1JwN0JTY1h1c0pDNDZvdGQ5WkVZWXo3YzhnNDV0?= =?utf-8?B?TTJ4TUp2WmxwSm1uVUlmc1ZPamFiSlhYend4VW9FRHJzMWFiNWxvWGdTWXpZ?= =?utf-8?B?ZGVaVWhPcnFvUW5VWkFPd3FEcWtKMFo1a2NWWlA1T0Z0eDF6ZVN3Wk0rbnBm?= =?utf-8?B?bFF2RlA5V2RmNUo2ZEl6My9vQmMrVUZjMmNLNVVjM3lRb2toWE1WU1hTaDIw?= =?utf-8?B?UitwTTkzcitqVmVKTjdRT1lDTHdoUENFZ0ZXNzVQbE5VeXRFYit6ZmtjYnRh?= =?utf-8?B?MFJQMVMxNnBtK1puYnlpWWliS1FwTDFzTUxzQXYxRjdzR3BXYzNXcUROVUJq?= =?utf-8?B?R3NPWEtiTXlvN0VSZk5BTGtEamhvc1hZOGFIVFRhWnVwU3VnRWVYV2swN0w3?= =?utf-8?B?bEFyT2lodkRJNk5kOXl0dFZCUW9Wd2N4d01FVFNVZzRrS3NrN1A2RmVESHJU?= =?utf-8?Q?FmeVmYA0tnt0PD+d/JQjnPfdG6s2fQ=3D?= X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1678;6:EJx41SlIIDNolSYWYnMgFQ7xqplhX5hAJfnDcN+ICggGIDePR74i23BwQNYOZvLQwB8r1e7FIrVdoU8RLaom6zhGSXoK9ZbM4Qrc4GqU/oDG6vLSJsN49p2Bik76hjPvL5OP5xsaEnTfqXa9oTXl07lwUzuxlhWdIulTZz9CEIu4PoFZP6Vuinrm7/Z2BRJlARrIdN2k553XH5lTTz8vtuOFUdMsBDz/8HKw/B+As+p47+QNdGfh2A4YVEeSjtMPu1lWIWd8YaayAurwDVt9l+sjabnpECaf/cgyQjbRZsj0UN82qrsXiJVdGbRMpD+ko4u2cGpitLhZ+BV5LgnswDWi0GXFbPvYRvQ1FcEbvLA=;5:dEIkM5cK5IO36TQewn5RFfe4+np1f3eSlRiYRVowo4Nra5Etoa06bBswOtbF299exJ8xnTOoTkOBnHSOf46Stj2jBUYSIvm8t33X63NKVlO74DX3xGMMgG0LncWgt68JHu7CqNewhzR6wGyI/CkM1osv/rzGOlNORf53wlsQj8Q=;24:g/EGfePTVWqG+Bs+kqjtosZA7uIz2FGdARf7fmebIx/GKtED2XF8F5M0OdKgZJjIXpU80Zt6rCr4U3mg4hrYsknjZJFc6IpdLn+CJFcPYTs=;7:ewtRjRZgd1TnlyzwpR0dyAYd+N4OMFfFGMvlBOAh9cJwNsOaycJT4XJRIAD01te+cxtEfnvZNydBpFYP1CBliDg3vT2jCte9tkw4PmpIQyweGsnggPJBqu0k8fVfo/APi0E6M86U5szCH6lNSCk7nWj1FUdLuVUvhZH3Vwna80lyHZFKYJu1Np5CQStd6UCI9e69+7MZUtujcgKCv44xEsvlMDRZjprARMTt/l3nIrz438JqNSIK0WP0G5BuXBwb SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1678;20:edHnVTt4i7v739GaHIUebGdvKlAjpROtfweDIkgqV4/h32PdPhXMd+lnN2sbC1yEYQ76f1BZSF0vVElZ8XrVjx5ClYIUqHr2drTERtmHwJlnVQweW/7O/sQyw/Qn4bYvaUIfl71OsadZMc5lRJCvxja5oN14phgtB3XLwZZCMTvTajaGtnKE0M6L3NuPE4pg3InJetD6I7QAJFCfv/sGo4DNdgV0A/yZoTViXJy9pbumzxent0Spd6aqxHeAk3MF X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Nov 2017 19:04:26.0074 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: dd30368d-a791-4fd3-21ba-08d52c5bb1cd X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1678 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/9/2017 12:34 PM, Borislav Petkov wrote: >> Subject: Re: [PATCH v2 3/4] Add support for AMD Core Perf Extension in guest > Btw, your subjects need a prefix: > > "x86/kvm: Add guest support for the AMD core performance counters" > > for example. Okay. > On Mon, Nov 06, 2017 at 11:44:25AM -0600, Janakarajan Natarajan wrote: >> This patch adds support for AMD Core Performance counters in the guest. > Never say "This patch" in the commit message of a patch. It is > tautologically useless. Okay. >> The base event select and counter MSRs are changed. In addition, with >> the core extension, there are 2 extra counters available for performance >> measurements for a total of 6. >> >> With the new MSRs, the logic to map them to the gp_counters[] is changed. >> New functions are introduced to get the right base MSRs and to check the >> validity of the get/set MSRs. >> >> If the guest has vcpus of either family 16h or a generation < 15h, it > You're only talking about families here, the "generation" thing is > confusing. I'll change that. >> falls back to using K7 MSRs and the number of counters the guest can >> access is set to 4. >> >> Signed-off-by: Janakarajan Natarajan >> --- >> arch/x86/kvm/pmu_amd.c | 133 +++++++++++++++++++++++++++++++++++++++++++------ >> arch/x86/kvm/x86.c | 1 + >> 2 files changed, 120 insertions(+), 14 deletions(-) > ... > >> +static inline struct kvm_pmc *get_gp_pmc_amd(struct kvm_pmu *pmu, u32 msr, >> + enum pmu_type type) >> +{ >> + unsigned int base = get_msr_base(pmu, type); >> + >> + if (base == MSR_F15H_PERF_CTL) { >> + switch (msr) { >> + case MSR_F15H_PERF_CTL0: >> + case MSR_F15H_PERF_CTL1: >> + case MSR_F15H_PERF_CTL2: >> + case MSR_F15H_PERF_CTL3: >> + case MSR_F15H_PERF_CTL4: >> + case MSR_F15H_PERF_CTL5: >> + /* >> + * AMD Perf Extension MSRs are not continuous. >> + * >> + * E.g. MSR_F15H_PERF_CTR0 -> 0xc0010201 >> + * MSR_F15H_PERF_CTR1 -> 0xc0010203 >> + * >> + * These are mapped to work with gp_counters[]. >> + * The index into the array is calculated by >> + * dividing the difference between the requested >> + * msr and the msr base by 2. >> + * >> + * E.g. MSR_F15H_PERF_CTR1 uses >> + * ->gp_counters[(0xc0010203-0xc0010201)/2] >> + * ->gp_counters[1] >> + */ >> + return &pmu->gp_counters[(msr - base) >> 1]; > Ok, it took me a bit of staring to understand what you're doing here. > And frankly, this scheme is silly and fragile. You're relying on the > fact that you can do math with the MSR numbers to get you the GP counter > number. The moment that changes in future families, you are going to > have to devise a new scheme for the new family. > > And instead of doing that, you're much better off producing a simple > MSR -> counter mapping for each family which is a simple switch-case. > No need to do get_msr_base() and whatnot - you simply feed in the MSR > number and the function spits out a gp_counters index. You only need to > check the family of the vcpu. > > Then lookers will be able to understand the code at a quick glance too. > > ... I'll put out a v3 with those changes. >> @@ -153,8 +251,15 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >> static void amd_pmu_refresh(struct kvm_vcpu *vcpu) >> { >> struct kvm_pmu *pmu = vcpu_to_pmu(vcpu); >> + int family, nr_counters; >> + >> + family = guest_cpuid_family(vcpu); >> + if (family == 0x15 || family == 0x17) >> + nr_counters = AMD64_NUM_COUNTERS_CORE; >> + else >> + nr_counters = AMD64_NUM_COUNTERS; >> >> - pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS; >> + pmu->nr_arch_gp_counters = nr_counters; >> pmu->counter_bitmask[KVM_PMC_GP] = ((u64)1 << 48) - 1; >> pmu->reserved_bits = 0xffffffff00200000ull; >> /* not applicable to AMD; but clean them to prevent any fall out */ >> @@ -169,7 +274,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu) >> struct kvm_pmu *pmu = vcpu_to_pmu(vcpu); >> int i; >> >> - for (i = 0; i < AMD64_NUM_COUNTERS ; i++) { >> + for (i = 0; i < AMD64_NUM_COUNTERS_CORE ; i++) { > This all works because INTEL_PMC_MAX_GENERIC is bigger than the AMD num > counters but you need to check all that. > > Also, the finding out of the nr_counters you do in amd_pmu_refresh() > should happen here, in the init function so that you have > pmu->nr_arch_gp_counters properly set and then when you iterate over > counters in the remaining functions, you do: > > for (i = 0; i < pmu->nr_arch_gp_counters ; i++) { > > instead of using those defines which are not always correct, depending > on the family. So, when the amd_pmu_init is called, a query to guest_cpuid_family() gives a value of -1. If this is the case where qemu sets the family details later on with a kvm ioctl, it would make sense to just initialize the maximum number of gp_counters that will be used and set the nr_arch_gp_counters based on the family during the amd_pmu_refresh(). Thanks, Janakarajan >