From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (146.0.238.70:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 07 Feb 2019 14:38:03 -0000 Received: from mga01.intel.com ([192.55.52.88]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1grkoT-0003Ma-VO for speck@linutronix.de; Thu, 07 Feb 2019 15:38:02 +0100 Date: Thu, 7 Feb 2019 06:37:59 -0800 From: Andi Kleen Subject: [MODERATED] Re: [PATCH v2 0/8] PERFv2 Message-ID: <20190207143759.GP31598@tassilo.jf.intel.com> References: <20190207132709.GA32477@hirez.programming.kicks-ass.net> MIME-Version: 1.0 In-Reply-To: <20190207132709.GA32477@hirez.programming.kicks-ass.net> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: > A few questions: > > - How will this 'feature' interact with MSR_CORE_PERF_GLOBAL_CTRL ? It's completely independent of GLOBAL_CTRL on the software level. Internally it uses it, but it shouldn't be software visible. > - Is having GLOBAL_CTRL.3 0 and not using RTM instructions enough > guarantee to avoid PMC3 from being clobbered? With no use of RTM clobbering should be fairly unlikely, but in theory it could happen if RETPOLINE is not used (due to an indirect branch mistakenenly executing something that looks like XBEGIN). If we assume that RETPOLINE is used I believe it shouldn't happen in the kernel at least. Even without RETPOLINE such a case should be fairly unlikely, but cannot be 100% ruled out. > Esp. that latter question, because both your patches and the below seems > to rely on that; and if the answer is yet, the below can be further > simplified. I don't see how I rely on that? In fact I avoided some simple optimizations to not rely on it, but only ever use counter 3 after the MSR write. > > So what (aside from being _completely_ untested and not doing virt crap) > is wrong with the below? It sure as heck is simpler. > +static struct event_constraint * > +skl_get_event_constraints(struct cpu_hw_events *cpuc, int idx, > + struct perf_event *event) > +{ > + struct event_constraint *c; > + > + c = hsw_get_event_constraints(cpuc, idx, event); > + > + if (!force_rtm_abort) { > + /* > + * Without TFA we must not use PMC3. > + */ > + c->idxmsk64 &= ~(1ULL << 3); Doesn't that overwrite the global shared constraints constraints? Would need to make them non const at least, it will likely fault. Okay I see you rely on it being guarded by the global, but for virtualization support will need the copy. Would need a copy at least. Other than that there doesn't seem to be anything obviously wrong. I will do some testing/checking and readd the opt-in/opt-out interfaces. -Andi