Received-SPF: pass (google.com: domain of linux-kernel+bounces-93338-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <265a906f-6179-4ec4-b46d-491930b3d9ac@amd.com>
Date: Wed, 6 Mar 2024 10:47:53 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] KVM: x86/svm/pmu: Set PerfMonV2 global control bits
 correctly
To: Sean Christopherson <seanjc@google.com>
Cc: Like Xu <like.xu.linux@gmail.com>, Dapeng Mi
 <dapeng1.mi@linux.intel.com>, pbonzini@redhat.com, mizhang@google.com,
 jmattson@google.com, ravi.bangoria@amd.com, nikunj.dadhania@amd.com,
 santosh.shukla@amd.com, manali.shukla@amd.com, babu.moger@amd.com,
 kvm list <kvm@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20240301075007.644152-1-sandipan.das@amd.com>
 <06061a28-88c0-404b-98a6-83cc6cc8c796@gmail.com>
 <cc8699be-3aae-42aa-9c70-f8b6a9728ee3@amd.com>
 <f5bbe9ac-ca35-4c3e-8cd7-249839fbb8b8@linux.intel.com>
 <ZeYlEGORqeTPLK2_@google.com>
 <8a846ba5-d346-422e-817b-e00ab9701f19@gmail.com>
 <ZedUwKWW7PNkvUH1@google.com>
Content-Language: en-US
From: Sandipan Das <sandipan.das@amd.com>
In-Reply-To: <ZedUwKWW7PNkvUH1@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?cDVENUg4MVZYT3JRcVRodkNMV3JjZE9FcFM1MFEvS3V4UjkxMVI4OWJqVWx6?=
 =?utf-8?B?UlVkNE5UYjZOWlBiMTJBUGhyODFvdlJzNjV5ZWsvUE5VQTJWZGp4ODhEL0F1?=
 =?utf-8?B?M3pzQU43M2FJcnRkYVQ0WGV5bDQzNVdib1dlUUFIQmJqbzNJOVRpd3BHbTV4?=
 =?utf-8?B?cGNGSytTemgwUFhGWEF5N3Z1TXRZVE02eGxVdWpGMGpmQkIxTm83OCtYYkFj?=
 =?utf-8?B?M2tDWVdNRG8yMXcvVG9XQmpvTmdZN3NBdzlGREtNUFdlS1Y3dzQvZVIrODhE?=
 =?utf-8?B?elpXQ1ZYRXkyaG84OHJlQU41TjdSVlZ4bS9XbFN3eGQrZDQ4R1lzdHFRWStI?=
 =?utf-8?B?K1VXRnM4WjI3UzVPYTU2bWhFbnlJYXl1cStlM1drZWw4K3FMQ0JKa005VWlJ?=
 =?utf-8?B?dUQvaXlRV2VOVzY5eE1wekZjSHRXelIweDZDWmYvV3Nmd2RyL25oQm1sRVUv?=
 =?utf-8?B?elpKVVpOV3kzL1BSbzJ6aXM4WUVwYkdTcWNRWUI4QTk0SExid2NRWG1JMlNM?=
 =?utf-8?B?ZHJuazhrLzRSQVZQNXY5SHEwWlF3cVZJZHFIT0hRaGY5YVV3NlNHc2RBb21K?=
 =?utf-8?B?Y3d4dVNzVUJCT3o4Y3BGVlA3WGRtamw5dVZicCticWM3d1F2TFZJVEk3TmEz?=
 =?utf-8?B?Qm1zQllMeVhPRVVVeDlJM3NDZHlmMlZDYTJoYWZoTTF0bnBsd1dVSjhFeVUy?=
 =?utf-8?B?Z1REMmNjN2RGOFdnSjFnaWR3UmxuZ3hPWDEyRXh4Q1AxYURENVZTaXBEa2Q5?=
 =?utf-8?B?N3dtRnd2L2laa1N2UlRhT3VsUm9QbG5DMTNSTFhtSm9nUzdnRGMxaUhQZHUy?=
 =?utf-8?B?d0VNdE05L20ySUNSUXQ5b05KR1lqaTJCcFJFTWlQa0h3SlBoYy9BVDhWcSsz?=
 =?utf-8?B?SFFka2FpWWlqZ2J4QVFjbmprM0N2NXBIK2N6eXRWcXhlbC9QRHR6RzFuUFFp?=
 =?utf-8?B?dVhnWFNYRG9IVGFPZ3pwbld0bEc5cjRkeElveDZINWJpSUoxOVJLQXhNNDVq?=
 =?utf-8?B?YW9hcXh1ZEk3cXAyMEdxT0RJU1pONzdyTHVyZXQ3REZCRi9Yd01BSm1KVWZl?=
 =?utf-8?B?Q25ndHFEcEdTODdTT1d3d0RzcEtOek1ZWlh5QXREQW5PLy9Lcm9rUGp6OG85?=
 =?utf-8?B?WjhZVzY1ZGpRVStGcmIxKzBzeDNDWkQrcXFWdWFlUXNsclVJaGVLMDNHSXJ6?=
 =?utf-8?B?eDQzdy91OTRTbHZDQU5YOUNNNSt3cXVvRmdCeEZjZXh5MGVaVWJsWjk4bklp?=
 =?utf-8?B?NkVybWdzbCsvcGhIU1FEN0dMbXlDaHJiZVpiZ0tOa0wyNXhNaHJJWUFSd0F6?=
 =?utf-8?B?clIzNWpBZDNBZ0YvQXdpMWdBSFZpdE5hajI5djBlUjF5emV4REttTUV3a012?=
 =?utf-8?B?QkZBUThYUjV1dHpySk5YemNsUFlHRUFWZFZGWWM0emlFWUNzN1EwbnUwejM1?=
 =?utf-8?B?WFN2V2dGNzlSOHB2RVlxenNRZm4zY1M2VWhyaUhGcGc1azI4Y3EzbDhXb1cw?=
 =?utf-8?B?aWhTVU4zSG51dVZCdTBoaWtvbGNPUTZyc21MNnUyZStJUDNpRmU1ZHJVMzVN?=
 =?utf-8?B?c2JKZXRIdEF4ZS9pckVsc2tIU1E2TzEwWmttbjc5ZlhLRDVNVk9CZFA3YUwy?=
 =?utf-8?B?Y29FS2dPM0wxa1hSZkpublJXUTZBejRuUE10aXhkTjdzdmxRVTgzRHFIOElk?=
 =?utf-8?B?YWhjRUtkQ3BEalNqMEEzQTdNTldQUys2cTk2WENxQWwzaWJIN2NWeE93dXBY?=
 =?utf-8?B?V2duZzZKSS83dkliSkFFY0xOdldST28zMURJeTlCbmdJVjFjc20rZmg0UmNi?=
 =?utf-8?B?QTBlZkhQdzlvckE0V1NwaVFmOVNrQytGZFI5eE83d1l0ZzJqbzMvOXQxMlg1?=
 =?utf-8?B?Ung1RzhSZzdiN3haL1AyT05GT3gvUDc4WHVsTzFQV0xhM2tkeEJScjQ1Yk04?=
 =?utf-8?B?M0pZRVdNL2dlR1E1VFFFd1hxK3NwQXk5TFpBT05Cekd2Y1piMjF5NTF5bFR0?=
 =?utf-8?B?alZtMlVnZnJ3dWkxblVINzhPeWVpWkMxK1BRNkRtQ0ZjWVprcHB5WmFLSGdh?=
 =?utf-8?B?R3hHMTdBQUZSSHNjcmtZZ2tDK29uWE1OajBmbSt2cExWTUwzTzk3ejNvTEFZ?=
 =?utf-8?Q?THYWHaHAEw29u9Av9f3Meijtl?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c6dc0dfd-f817-42f4-5d09-08dc3d9cce4f
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB5712.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2024 05:18:06.9481
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KcP6BYUFcEN7iBtNg5L6ZNUlgz+C8Q2a3IqDRyVfg1bgU/HWPuUNzmMo+oe0A2wn7nM4JDmF9vn9U/YJEc1rfA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4062

On 3/5/2024 10:52 PM, Sean Christopherson wrote:
> On Tue, Mar 05, 2024, Like Xu wrote:
>> On 5/3/2024 3:46 am, Sean Christopherson wrote:
>>>>>>> ---
>>>>>>>     arch/x86/kvm/svm/pmu.c | 1 +
>>>>>>>     1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
>>>>>>> index b6a7ad4d6914..14709c564d6a 100644
>>>>>>> --- a/arch/x86/kvm/svm/pmu.c
>>>>>>> +++ b/arch/x86/kvm/svm/pmu.c
>>>>>>> @@ -205,6 +205,7 @@ static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
>>>>>>>         if (pmu->version > 1) {
>>>>>>>             pmu->global_ctrl_mask = ~((1ull << pmu->nr_arch_gp_counters) - 1);
>>>>>>>             pmu->global_status_mask = pmu->global_ctrl_mask;
>>>>>>> +        pmu->global_ctrl = ~pmu->global_ctrl_mask;
>>>>
>>>> It seems to be more easily understand to calculate global_ctrl firstly and
>>>> then derive the globol_ctrl_mask (negative logic).
>>>
>>> Hrm, I'm torn.  On one hand, awful name aside (global_ctrl_mask should really be
>>> something like global_ctrl_rsvd_bits), the computation of the reserved bits should
>>> come from the capabilities of the PMU, not from the RESET value.
>>>
>>> On the other hand, setting _all_ non-reserved bits will likely do the wrong thing
>>> if AMD ever adds bits in PerfCntGlobalCtl that aren't tied to general purpose
>>> counters.  But, that's a future theoretical problem, so I'm inclined to vote for
>>> Sandipan's approach.
>>
>> I suspect that Intel hardware also has this behaviour [*] although guest
>> kernels using Intel pmu version 1 are pretty much non-existent.
>>
>> [*] Table 10-1. IA-32 and Intel® 64 Processor States Following Power-up,
>> Reset, or INIT (Contd.)
> 
> Aha!  Nice.  To save people lookups, the table says:
> 
>   IA32_PERF_GLOBAL_CTRL:  Sets bits n-1:0 and clears the upper bits.
> 
> and 
> 
>   Where "n" is the number of general-purpose counters available in the processor.
> 
> Which means that (a) KVM can handle this in common code and (b) we can dodge the
> whole reserved bits chicken-and-egg problem since global_ctrl *can't* be derived
> from global_ctrl_mask.
> 
> This?  (compile tested only)
> 
> ---
> From: Sean Christopherson <seanjc@google.com>
> Date: Tue, 5 Mar 2024 09:02:26 -0800
> Subject: [PATCH] KVM: x86/pmu: Set enable bits for GP counters in
>  PERF_GLOBAL_CTRL at "RESET"
> 
> Set the enable bits for general purpose counters in IA32_PERF_GLOBAL_CTRL
> when refreshing the PMU to emulate the MSR's architecturally defined
> post-RESET behavior.  Per Intel's SDM:
> 
>   IA32_PERF_GLOBAL_CTRL:  Sets bits n-1:0 and clears the upper bits.
> 
> and
> 
>   Where "n" is the number of general-purpose counters available in the processor.
> 
> This is a long-standing bug that was recently exposed when KVM added
> supported for AMD's PerfMonV2, i.e. when KVM started exposing a vPMU with
> PERF_GLOBAL_CTRL to guest software that only knew how to program v1 PMUs
> (that don't support PERF_GLOBAL_CTRL).  Failure to emulate the post-RESET
> behavior results in such guests unknowingly leaving all general purpose
> counters globally disabled (the entire reason the post-RESET value sets
> the GP counter enable bits is to maintain backwards compatibility).
> 
> The bug has likely gone unnoticed because PERF_GLOBAL_CTRL has been
> supported on Intel CPUs for as long as KVM has existed, i.e. hardly anyone
> is running guest software that isn't aware of PERF_GLOBAL_CTRL on Intel
> PMUs.
> 
> Note, kvm_pmu_refresh() can be invoked multiple times, i.e. it's not a
> "pure" RESET flow.  But it can only be called prior to the first KVM_RUN,
> i.e. the guest will only ever observe the final value.
> 
> Reported-by: Reported-by: Babu Moger <babu.moger@amd.com>
> Cc: Like Xu <like.xu.linux@gmail.com>
> Cc: Mingwei Zhang <mizhang@google.com>
> Cc: Dapeng Mi <dapeng1.mi@linux.intel.com>
> Cc: Sandipan Das <sandipan.das@amd.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/pmu.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 87cc6c8809ad..f61ce26aeb90 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -741,6 +741,8 @@ static void kvm_pmu_reset(struct kvm_vcpu *vcpu)
>   */
>  void kvm_pmu_refresh(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> +
>  	if (KVM_BUG_ON(kvm_vcpu_has_run(vcpu), vcpu->kvm))
>  		return;
>  
> @@ -750,8 +752,18 @@ void kvm_pmu_refresh(struct kvm_vcpu *vcpu)
>  	 */
>  	kvm_pmu_reset(vcpu);
>  
> -	bitmap_zero(vcpu_to_pmu(vcpu)->all_valid_pmc_idx, X86_PMC_IDX_MAX);
> +	bitmap_zero(pmu->all_valid_pmc_idx, X86_PMC_IDX_MAX);
>  	static_call(kvm_x86_pmu_refresh)(vcpu);
> +
> +	/*
> +	 * At RESET, both Intel and AMD CPUs set all enable bits for general
> +	 * purpose counters in IA32_PERF_GLOBAL_CTRL (so that software that
> +	 * was written for v1 PMUs don't unknowingly leave GP counters disabled
> +	 * in the global controls).  Emulate that behavior when refreshing the
> +	 * PMU so that userspace doesn't need to manually set PERF_GLOBAL_CTRL.
> +	 */
> +	if (kvm_pmu_has_perf_global_ctrl(pmu))
> +		pmu->global_ctrl = GENMASK_ULL(pmu->nr_arch_gp_counters - 1, 0);
>  }
>  
>  void kvm_pmu_init(struct kvm_vcpu *vcpu)
> 
> base-commit: 1d7ae977d219e68698fdb9bed1049dc561038aa1

Thanks. This looks good.

Tested-by: Sandipan Das <sandipan.das@amd.com>