Re: [PATCH v2 13/13] target/riscv: Enable PMU related extensions to preferred rule

[Alistair Francis]

On Thu, Aug 8, 2024 at 6:12 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Wed, Aug 7, 2024 at 5:27 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Aug 7, 2024 at 5:44 PM Atish Kumar Patra <atishp@rivosinc.com> 
> > wrote:
> > >
> > > On Tue, Aug 6, 2024 at 7:01 PM Alistair Francis <alistair23@gmail.com> 
> > > wrote:
> > > >
> > > > On Wed, Aug 7, 2024 at 2:06 AM Daniel Henrique Barboza
> > > > <dbarboza@ventanamicro.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 8/6/24 5:46 AM, Andrew Jones wrote:
> > > > > > On Tue, Jul 23, 2024 at 04:30:10PM GMT, Atish Patra wrote:
> > > > > >> Counter delegation/configuration extension requires the following
> > > > > >> extensions to be enabled.
> > > > > >>
> > > > > >> 1. Smcdeleg - To enable counter delegation from M to S
> > > > > >> 2. S[m|s]csrind - To enable indirect access CSRs
> > > > > >> 3. Smstateen - Indirect CSR extensions depend on it.
> > > > > >> 4. Sscofpmf - To enable counter overflow feature
> > > > > >> 5. S[m|s]aia - To enable counter overflow feature in virtualization
> > > > > >> 6. Smcntrpmf - To enable privilege mode filtering for cycle/instret
> > > > > >>
> > > > > >> While first 3 are mandatory to enable the counter delegation,
> > > > > >> next 3 set of extension are preferred to enable all the PMU related
> > > > > >> features.
> > > > > >
> > > > > > Just my 2 cents, but I think for the first three we can apply the 
> > > > > > concept
> > > > > > of extension bundles, which we need for other extensions as well. 
> > > > > > In those
> > > > > > cases we just auto enable all the dependencies. For the three 
> > > > > > preferred
> > > > > > extensions I think we can just leave them off for 'base', but we 
> > > > > > should
> > > > > > enable them by default for 'max' along with Ssccfg.
> > > >
> > >
> > > Max cpu will have everything enabled by default. The problem with max
> > > cpu is that you
> > > may not want to run all the available ISA extensions while testing perf.
> > >
> > > > Agreed
> > > >
> > > > >
> > > > > I like this idea. I would throw in all these 6 extensions in a 
> > > > > 'pmu_advanced_ops'
> > > > > (or any other better fitting name for the bundle) flag and then 
> > > > > 'pmu_advanced_ops=true'
> > > > > would enable all of those. 'pmu_advanced_ops=true,smcntrpmf=false' 
> > > > > enables all but
> > > > > 'smcntrpmf' and so on.
> > > > >
> > >
> > > I thought distinguishing preferred vs implied would be useful because
> > > it would allow the user
> > > to clearly understand which is mandated by ISA vs which would be good to 
> > > have.
> >
> > It's not really clear though what extensions are good to have. Other
> > people might think differently about the extensions. It also then
> > means we end up with complex combinations of extensions to manage.
> >
> > >
> > > The good to have extensions can be disabled similar to above but not
> > > the mandatory ones.
> > >
> > > > > As long as we document what the flag is enabling I don't see any 
> > > > > problems with it.
> > > > > This is how profiles are implemented after all.
> > > >
> > > > I only worry that we end up with a huge collection of flags that users
> > > > need to decipher.
> > > >
> > >
> > > My initial idea was a separate flag as well. But I was not sure if
> > > that was good for the
> > > above reason. This additional custom pmu related option would be lost
> > > in that huge collection.
> >
> > I do feel a separate flag is better than trying to guess what extra
> > extensions the user wants enabled.
> >
>
> Sure. A separate pmu flag that enables all available pmu related
> extensions - Correct ?

That seems like the best option. Although just using the max CPU is
even better :)

> Do you prefer to have those enabled via a separate preferred rule or
> just reuse the implied
> rule ? I can drop the preferred rule patches for the later case.

As this is now a custom flag a separate rule is probably the way to
go. Something similar to the existing `RISCVCPUImpliedExtsRule` is
probably the way to go. Keep an implied rule for what is required by
the spec, but maybe a "helper" rule for the special flag?

>
>
> > I don't love either though, isn't this what profiles is supposed to fix!
> >
>
> Yeah. But given the optionality in profiles, I am sure if it will fix
> the ever growing
> extension dependency graph problem ;)
>
> > >
> > > > I guess with some good documentation this wouldn't be too confusing 
> > > > though.
> > > >
> > >
> > > Sure. It won't be confusing but most users may not even know about it
> > > without digging.
> >
> > At that point they can use the max CPU or just manually enable the
> > extensions though.
> >
>
> If everybody thinks max CPU is going to be used more frequently in the
> future, I am okay with
> that as well. Implied rule will only specify mandatory extensions
> defined by ISA.
>
> It's up to the user to figure out the extensions names and enable them
> individually if max CPU
> is not used.

A user can just specify max. It's just as much work as specifying this new flag.

Is the issue just the defaults? We can think about max CPU being the default.

> FYI: There are at least 6 more PMU related extensions that this series
> did not specify.
> ~4 are being discussed in the RVI TG(precise event sampling, events)
> and 2 are frozen (Smctr/Ssctr)

Urgh!

Alistair