Re: [PATCH 2/4] perf/x86/intel: Don't context switch DS_AREA (and PEBS config) if PEBS is unused

Jim Mattson
2026-04-15T13:00:15Z

On Tue, Apr 14, 2026 at 3:49 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Apr 14, 2026, Jim Mattson wrote:
> > On Tue, Apr 14, 2026 at 12:14 PM Sean Christopherson <seanjc@google.com> wrote:
> > > @@ -5065,6 +5056,28 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
> > > if (pebs_mask & ~cpuc->intel_ctrl_guest_mask)
> > > guest_pebs_mask = 0;
> > >
> > > + /*
> > > + * Context switch DS_AREA and PEBS_DATA_CFG if and only if PEBS will be
> > > + * active in the guest; if no records will be generated while the guest
> > > + * is running, then running with host values is safe (see above).
> > > + */
> > > + if (!guest_pebs_mask)
> > > + return arr;
> >
> > I think this has an unintended side effect. If DS_AREA and
> > PEBS_DATA_CFG were previously listed (because pebs_guest_mask was
> > previously non-zero), KVM will leave the stale entries in the MSR-load
> > lists.
> >
> > KVM only clears MSR-load list entries for enumerated MSRs with
> > matching guest and host values.
>
> Argh, right. This as a delta change I guess? I'd love to overhaul the interface,
> but reworking KVM to play nice with core_guest_get_msrs() would be much more
> difficult.
>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 20a153aa33cb..3244d5589981 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -5059,22 +5059,20 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
> /*
> * Context switch DS_AREA and PEBS_DATA_CFG if and only if PEBS will be
> * active in the guest; if no records will be generated while the guest
> - * is running, then running with host values is safe (see above).
> + * is running, then simply keep the host values resident in hardware.
> */
> - if (!guest_pebs_mask)
> - return arr;
> -
> arr[(*nr)++] = (struct perf_guest_switch_msr){
> .msr = MSR_IA32_DS_AREA,
> .host = (unsigned long)cpuc->ds,
> - .guest = kvm_pmu->ds_area,
> + .guest = guest_pebs_mask ? kvm_pmu->ds_area : (unsigned long)cpuc->ds,
> };
>
> if (x86_pmu.intel_cap.pebs_baseline) {
> arr[(*nr)++] = (struct perf_guest_switch_msr){
> .msr = MSR_PEBS_DATA_CFG,
> .host = cpuc->active_pebs_data_cfg,
> - .guest = kvm_pmu->pebs_data_cfg,
> + .guest = guest_pebs_mask ? kvm_pmu->data_cfg :
> + cpuc->active_pebs_data_cfg,
> };
> }
>
That looks reasonable, if a bit obtuse.

Reviewed-by: Jim Mattson <jmattson@google.com>