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

[Jim Mattson]

On Tue, Apr 14, 2026 at 3:49 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Tue, Apr 14, 2026, Jim Mattson wrote:
> > On Tue, Apr 14, 2026 at 12:14 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>