Re: [PATCH] perf/amd/ibs: Clear stale IBS_{FETCH|OP}_CTL in perf_ibs_start() with CTL2[Dis]

[Ravi Bangoria <ravi.bangoria@amd.com>]

>> +	if (config & perf_ibs->valid_mask) {
>> +		ctl = config;
>> +		while (delay-- && ctl & perf_ibs->valid_mask) {
>> +			udelay(1);
>> +			rdmsrq(hwc->config_base, ctl);
>> +		}
>> +	}
> 
> This loop will time out when running in NMI context:
> 
>     perf_ibs_handle_irq() ->
>       perf_event_overflow() ->
>         __perf_event_overflow() ->
>           __perf_event_account_interrupt() ->
>             perf_event_throttle_group() ->
>               perf_event_throttle() ->
>                 perf_ibs_stop() ->
>                   perf_ibs_stop_dis() 
> 
> Should the loop just be skipped when in_nmi()?

Right. However, I'm planning to move the polling to ->start() and
also fix ->stop() with:

---
@@ -980,6 +980,14 @@ static void perf_ibs_stop_dis(struct perf_event *event, int flags)
        rdmsrq(hwc->config_base, config);

        config &= ~perf_ibs->valid_mask;
+
+       /*
+        * Clear the Val bit otherwise ->start() will unnecessarily
+        * wait for 50us.
+        */
+       if (in_nmi())
+               wrmsrq(hwc->config_base, config);
+
        perf_ibs_event_update(perf_ibs, event, &config);
        hwc->state |= PERF_HES_UPTODATE;

---

>> +	/* Catch delayed NMIs which arrives after disabling IBS PMUs. */
>> +	if (ibs_caps & IBS_CAPS_DIS) {
>> +		rdmsrq(perf_ibs->msr2, ctl2);
>> +		if (ctl2 & perf_ibs->disable_mask) {
>> +			rdmsrq(perf_ibs->msr, ctl);
>> +			if (ctl & perf_ibs->valid_mask) {
>> +				ctl &= ~(perf_ibs->enable_mask &
>> +					 perf_ibs->valid_mask);
> 
> enable_mask and valid_mask are disjoint. Should '&' be '|'?

Yes, will fix this.

>> +				wrmsrq(perf_ibs->msr, ctl);
>> +				return 1;
>> +			}
>> +			return 0;
>> +		}
>> +
> 
> [...]
> 
>>       if (WARN_ON_ONCE(!event))
> 
> With IBS_CAPS_DIS, we are now treating CTL2[Dis] as authoritative on the
> question of "is IBS idle." However, on the BSP after S3 resume

Yes, the syscore->resume() path needs to be fixed as well.

> or on any
> LPU after an AE #VMEXIT from an SEV-ES/SNP guest with IBS virtualization
> enabled, CTL2[Dis] will be reset to 0 and may no longer be authoritative.

KVM should save/restore host IBS MSRs when vIBS is enabled.

>> I think the intention is, host IBS should be disabled (could be from
>> CTL2[Dis]) before running VMRUN. But I'll confirm with the HW team.

Hardware checks the effective IBS op and fetch enables in VMRUN to a
SEV-SE/SEV-SNP guest. Effective enable being CTL[En] && !CTL2[Dis].
(This will be documented in the upcoming APM)

> Another question for the HW team: can the hardware set CTL[Val], or change
> CTL in any other way, after CTL2[Dis] has been set? The answer may
> obviate the need for the 50us poll.

Once IBS is disabled via CTL2, the hardware will not capture any new
samples, so the CTL register should remain unchanged. However, the
50us polling window is still necessary to account for potential NMI
latency.

Thanks,
Ravi