Re: [PATCH v2] x86/intel: ensure Global Performance Counter Control is setup correctly

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jan 12, 2024 at 08:42:27AM +0100, Jan Beulich wrote:
> On 11.01.2024 17:53, Roger Pau Monné wrote:
> > On Thu, Jan 11, 2024 at 04:52:04PM +0100, Jan Beulich wrote:
> >> On 11.01.2024 15:15, Roger Pau Monné wrote:
> >>> On Thu, Jan 11, 2024 at 03:01:01PM +0100, Jan Beulich wrote:
> >>>> On 11.01.2024 13:22, Roger Pau Monné wrote:
> >>>>> Oh, indeed, can adjust on this same patch if that's OK (seeing as the
> >>>>> issue was already there previous to my change).
> >>>>
> >>>> Well, I'm getting the impression that it was deliberate there, i.e. set
> >>>> setting of the feature flag may want to remain thus constrained.
> >>>
> >>> Hm, I find it weird, but the original commit message doesn't help at
> >>> all.  Xen itself only uses PMC0, and I don't find any other
> >>> justification in the current code to require at least 2 counters in
> >>> order to expose arch performance monitoring to be present.
> >>>
> >>> Looking at the SDM vol3, the figures there only contain PMC0 and PMC1,
> >>> so someone only reading that manual might assume there must always be
> >>> 2 global PMCs?
> >>
> >> That may have been the impression at the time. It may have been wrong
> >> already back then, or ...
> >>
> >>> (vol4 clarifies the that the number of global PMCs is variable).
> >>
> >> ... it may have been clarified in the SDM later on. My vague guess is
> >> that the > 1 check was to skip what may have been "obviously buggy"
> >> back at the time.
> > 
> > Let me know if you are OK with the adjustment in v3, or whether you
> > would rather leave the > 1 check as-is (or maybe adjust in a different
> > patch).
> 
> Well, I haven't been able to make up my mind as to whether the original
> check was wrong. Without clear indication, I think we should retain the
> original behavior by having the __set_bit() gated by an additional if().
> Then, since the line needs touching anyway, a further question would be
> whether to properly switch to setup_force_cpu_cap() at the same time.

Having looked at Linux, it has exactly the same check for > 1, which I
guess is to be expected since the code in Xen is quite likely adapted
from the code in Linux.

Overall, it might be best to leave the check as > 1.  It's possible (as
I think you also mention in a previous email) that there's simply no
hardware with 1 counter.  This might no longer be true when
virtualized, but given the current checks in both Xen and Linux any
virtualization environment that attempts to expose arch perf support
would need to expose at least 2 PMCs.

My suggestion is to leave the cnt > 1 check as it is in v2.

I can send a v4 with that check fixed if there's nothing else in v3
that needs fixing.

IMO doing the adjustment to PERF_GLOBAL_CTRL without setting
ARCH_PERFMON would be contradictory.  Either we set ARCH_PERFMON
support and consequently adjust PERF_GLOBAL_CTRL, or we don't.

Thanks, Roger.