On 12.01.2024 11:08, Roger Pau Monné wrote: > 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.
Probably fair enough. Jan
