Re: [PATCH] x86/perf: Default freeze_on_smi on for Comet Lake and later.

[Liang, Kan]

On 1/26/2022 9:04 AM, Peter Zijlstra wrote:
> On Tue, Jan 25, 2022 at 08:57:09AM -0500, Liang, Kan wrote:
> > I see. I was thought the unprivileged user can observe the SMM code on the
> > previous platforms. The CML+ change only makes part of the SMM code CPL0.
> > Seems I'm wrong. The change looks like changing the previous CPL0 code to
> > CPL3 code. If so, yes, I think we should prevent the information leaks for
> > the unprivileged user.
>
> Right.
>
> > Changing it to all platforms seems a too big hammer. I agree we should limit
> > it to the impacted platforms.
> >
> > I've contacted the author of the white paper. I was told that the change is
> > for the client vPro platforms. They are not sure whether it impacts Server
> > platform or Atom platforms. I'm still working on it. I will let you and
> > Peter know once I get more information.
>
> For now I've updated the patch as per the below. I'm tempted to simply
> apply it as is and let it be.
>
> Having different defaults for vPro vs !vPro chips seems more confusing
> than not.

But I think it should make sense to have different defaults for client 
vs server, or big core vs atom.

I'm still working on it and trying to figure out the affected 
generations. (Hope I can find the right contacts in the next few days.)

> We should also very much get this change reverted for future chips.

Yes. I will discuss it with the contacts once I get the name.

Thanks,
Kan

> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -6094,6 +6094,16 @@ __init int intel_pmu_init(void)
>   			x86_pmu.commit_scheduling = intel_tfa_commit_scheduling;
>   		}
>   
> +		if (boot_cpu_data.x86_model == INTEL_FAM6_COMETLAKE_L ||
> +		    boot_cpu_data.x86_model == INTEL_FAM6_COMETLAKE) {
> +			/*
> +			 * For some idiotic reason SMM is visible to USR
> +			 * counters. Since this is a privilege issue, default
> +			 * disable counters in SMM for these chips.
> +			 */
> +			x86_pmu.attr_freeze_on_smi = 1;
> +		}
> +
>   		pr_cont("Skylake events, ");
>   		name = "skylake";
>   		break;
> @@ -6135,6 +6145,8 @@ __init int intel_pmu_init(void)
>   		x86_pmu.num_topdown_events = 4;
>   		x86_pmu.update_topdown_event = icl_update_topdown_event;
>   		x86_pmu.set_topdown_event_period = icl_set_topdown_event_period;
> +		/* SMM visible in USR, see above */
> +		x86_pmu.attr_freeze_on_smi = 1;
>   		pr_cont("Icelake events, ");
>   		name = "icelake";
>   		break;
> @@ -6172,6 +6184,8 @@ __init int intel_pmu_init(void)
>   		x86_pmu.num_topdown_events = 8;
>   		x86_pmu.update_topdown_event = icl_update_topdown_event;
>   		x86_pmu.set_topdown_event_period = icl_set_topdown_event_period;
> +		/* SMM visible in USR, see above */
> +		x86_pmu.attr_freeze_on_smi = 1;
>   		pr_cont("Sapphire Rapids events, ");
>   		name = "sapphire_rapids";
>   		break;
> @@ -6217,6 +6231,8 @@ __init int intel_pmu_init(void)
>   		 * x86_pmu.rtm_abort_event.
>   		 */
>   		x86_pmu.rtm_abort_event = X86_CONFIG(.event=0xc9, .umask=0x04);
> +		/* SMM visible in USR, see above */
> +		x86_pmu.attr_freeze_on_smi = 1;
>   
>   		td_attr = adl_hybrid_events_attrs;
>   		mem_attr = adl_hybrid_mem_attrs;