Re: [PATCH 1/2] perf: riscv: preliminary RISC-V support

[Palmer Dabbelt]

On Mon, 09 Apr 2018 00:07:11 PDT (-0700), alankao@xxxxxxxxxxxxx wrote:
> On Thu, Apr 05, 2018 at 09:47:50AM -0700, Palmer Dabbelt wrote:
> > On Mon, 26 Mar 2018 00:57:54 PDT (-0700), alankao@xxxxxxxxxxxxx wrote:
> > >This patch provide a basic PMU, riscv_base_pmu, which supports two
> > >general hardware event, instructions and cycles.  Furthermore, this
> > >PMU serves as a reference implementation to ease the portings in
> > >the future.
> > >
> > >riscv_base_pmu should be able to run on any RISC-V machine that
> > >conforms to the Priv-Spec.  Note that the latest qemu model hasn't
> > >fully support a proper behavior of Priv-Spec 1.10 yet, but work
> > >around should be easy with very small fixes.  Please check
> > >https://github.com/riscv/riscv-qemu/pull/115 for future updates.
> > >
> > >Cc: Nick Hu <nickhu@xxxxxxxxxxxxx>
> > >Cc: Greentime Hu <greentime@xxxxxxxxxxxxx>
> > >Signed-off-by: Alan Kao <alankao@xxxxxxxxxxxxx>
> >
> > We should really be able to detect PMU types at runtime (via a device tree
> > entry) rather than requiring that a single PMU is built in to the kernel.
> > This will require a handful of modifications to how this patch works, which
> > I'll try to list below.
>
> > >+menu "PMU type"
> > >+	depends on PERF_EVENTS
> > >+
> > >+config RISCV_BASE_PMU
> > >+	bool "Base Performance Monitoring Unit"
> > >+	def_bool y
> > >+	help
> > >+	  A base PMU that serves as a reference implementation and has limited
> > >+	  feature of perf.
> > >+
> > >+endmenu
> > >+
> >
> > Rather than a menu where a single PMU can be selected, there should be
> > options to enable or disable support for each PMU type -- this is just like
> > how all our other drivers work.
>
> I see.  Sure.  The descriptions and implementation will be refined in v3.
>
> > >+struct pmu * __weak __init riscv_init_platform_pmu(void)
> > >+{
> > >+	riscv_pmu = &riscv_base_pmu;
> > >+	return riscv_pmu->pmu;
> > >+}
> >
> > Rather than relying on a weak symbol that gets overridden by other PMU
> > types, this should look through the device tree for a compatible PMU (in the
> > case of just the base PMU it could be any RISC-V hart) and install a PMU
> > handler for it.  There'd probably be some sort of priority scheme here, like
> > there are for other driver subsystems, where we'd pick the best PMU driver
> > that's compatible with the PMUs on every hart.
> >
> > >+
> > >+int __init init_hw_perf_events(void)
> > >+{
> > >+	struct pmu *pmu = riscv_init_platform_pmu();
> > >+
> > >+	perf_irq = NULL;
> > >+	perf_pmu_register(pmu, "cpu", PERF_TYPE_RAW);
> > >+	return 0;
> > >+}
> > >+arch_initcall(init_hw_perf_events);
> >
> > Since we only have a single PMU type right now this isn't critical to handle
> > right away, but we will have to refactor this before adding another PMU.
>
> I see.  My rough plan is to do the device tree parsing here, and if no specific
> PMU string is found then just register the base PMU proposed in this patch.
> How about this idea?

Sounds good.  We know the generic PMU will work on all RISC-V harts, so there's 
no need to add an explicit device tree entry for it.  Then we can figure out 
how to add device tree entries for custom performance monitors later :)