Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 11 Apr 2023 14:21:48 +0100
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     Liang Kan <kan.liang@linux.intel.com>, <linux-cxl@vger.kernel.org>,
        <peterz@infradead.org>, <mingo@redhat.com>, <acme@kernel.org>,
        <mark.rutland@arm.com>, <will@kernel.org>, <linuxarm@huawei.com>,
        <linux-perf-users@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Dave Jiang <dave.jiang@intel.com>
Subject: Re: [PATCH v4 0/5] CXL 3.0 Performance Monitoring Unit support
Message-ID: <20230411142148.00003f1e@Huawei.com>
In-Reply-To: <642b9fb7d6986_29cc2945e@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230330164556.31533-1-Jonathan.Cameron@huawei.com>
        <642b9fb7d6986_29cc2945e@dwillia2-xfh.jf.intel.com.notmuch>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 3 Apr 2023 20:55:35 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > Changes since v3: Kan Liang gave feedback on v2 incorporated here.
> > - All updates in patch 4, see details there.
> > 
> > Updated cover letter.
> > 
> > The CXL rev 3.0 specification introduces a CXL Performance Monitoring
> > Unit definition. CXL components may have any number of these blocks. The
> > definition is highly flexible, but that does bring complexity in the
> > driver.
> > 
> > Notes.
> > 
> > 1) The QEMU model against which this was developed needs tidying up.
> >    Latest tree at https://gitlab.com/jic23/qemu branch cxl-2023-02-28
> >    It's backed up behind other series that I plan to upstream first.
> > 2) There are quite a lot of corner cases that will need working through
> >    with variants of the model, or I'll have to design a pathological
> >    set of CPMUs to hit all the corner cases in one go. So whilst I believe
> >    the driver should be fine for what it supports we may find issues
> >    as those corners of what is allowed are explored.
> > 3) I'm not sure it makes sense to hang this of the cxl/pci driver but
> >    couldn't really figure out where else in the current structure we could
> >    make it fit cleanly.
> > 4) Driver location. In past perf maintainers have requested perf drivers
> >    for PCI etc be under drivers/perf. That would require moving some
> >    CXL headers to be more generally visible, but is certainly possible
> >    if there is agreement between CXL and perf maintainers on the correct
> >    location.  
> 
> I am ok the bulk of the logic living under drivers/perf/
> 
> Are drivers/perf/ folks ok with a:
> 
> CFLAGS_cxl.o := -I$(srctree)/drivers/cxl/
> 
> ...or similar in their Makefile, because I don't think this by itself is
> a reason to push CXL headers to include/.

For now I've gone with ../cxl/ which doesn't look too bad.
If cflags stuff is preferred can change that.


> 
> I say this without having looked at the code yet and whether this driver
> will need new exports from the cxl/core etc.
> 
> > 5) Patch 1 led to a discussion of how to handle the self describing
> >    and extensible nature of the CPMU counters.  That is likely to involve
> >    a description in the "caps" sysfs directory and perf tool code that is
> >    aware of the different event combinations that make sense and can
> >    establish which sets are available on a given device.
> >    That is likely to take a while to converge on, so as the driver is useful
> >    in the current state, I'm looking to upstream this first and deal with
> >    the more complex handling later.  
> 
> What's "this" in this last sentence, a canned set of base counters?

All the counters defined in the CXL spec itself without any summing of
counters (so you can't do all 'reads' for example without 'guessing' the
magic numbers).

> 
> > 6) There is a lot of other functionality that can be added in future
> >    include allowing for simpler hardware implementations that may not
> >    support the minimum level of features currently required by the driver
> >    (freeze, overflow interrupts etc).  
> 
> Curious if the the minimal set determined by the specification, like the
> minimal features a CXL Memory Expander device must implement, or by what
> you decided was fit to be emulated in QEMU?

There is no specification requirement to implement any particular subset
of counters or features for those counters.  I think that's considered out
of scope for the CXL spec itself.  Of course particularly customers or
industry bodies may define a minimal set of requirements. I don't think anyone
has done so yet.

Current set of counters is all the ones in the specification itself at their
finest granularity (summed cases for the configurable counters 'work' but
aren't advertised - working out sensible choices is a job for perftool +
an enhanced driver that advertises what combinations it can sum).

Counter features / types include the most generic, fully featured and flexible
versions.  In many cases it's harder to support the less complex hardware
because a configuration dance can be needed.
I'd imagine the remaining 'likely' hardware support will be added over the
next kernel cycle or two.

Jonathan

> 
> > 
> > CXL rev 3.0 specification available from https://www.computeexpresslink.org
> > 
> > 
> > Jonathan Cameron (5):
> >   cxl: Add function to count regblocks of a given type
> >   perf: Allow a PMU to have a parent
> >   cxl/pci: Find and register CXL PMU devices
> >   cxl: CXL Performance Monitoring Unit driver
> >   docs: perf: Minimal introduction the the CXL PMU device and driver
> > 
> >  Documentation/admin-guide/perf/cxl.rst   |  65 ++
> >  Documentation/admin-guide/perf/index.rst |   1 +
> >  drivers/cxl/Kconfig                      |  13 +
> >  drivers/cxl/Makefile                     |   1 +
> >  drivers/cxl/core/Makefile                |   1 +
> >  drivers/cxl/core/core.h                  |   1 +
> >  drivers/cxl/core/cpmu.c                  |  72 ++
> >  drivers/cxl/core/pci.c                   |   2 +-
> >  drivers/cxl/core/port.c                  |   4 +-
> >  drivers/cxl/core/regs.c                  |  50 +-
> >  drivers/cxl/cpmu.c                       | 940 +++++++++++++++++++++++
> >  drivers/cxl/cpmu.h                       |  56 ++
> >  drivers/cxl/cxl.h                        |  17 +-
> >  drivers/cxl/cxlpci.h                     |   1 +
> >  drivers/cxl/pci.c                        |  27 +-
> >  include/linux/perf_event.h               |   1 +
> >  kernel/events/core.c                     |   1 +
> >  17 files changed, 1245 insertions(+), 8 deletions(-)
> >  create mode 100644 Documentation/admin-guide/perf/cxl.rst
> >  create mode 100644 drivers/cxl/core/cpmu.c
> >  create mode 100644 drivers/cxl/cpmu.c
> >  create mode 100644 drivers/cxl/cpmu.h
> > 
> > -- 
> > 2.37.2
> >