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

[Atish Kumar Patra]

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.

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 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.
That's why I chose to use a standard extension which covers the basic
PMU access directly in S-mode.

The future extensions such as CTR or Sampling events would also
benefit by just adding the Ssccfg in the preferred rule
which in turn will enable other preferred/mandatory extensions.

> Alistair
>
> >
> > With this bundle we can also use implied rule only if an extension really 
> > needs
> > (i.e. it breaks without) a dependency being enabled, instead of overloading 
> > it
> > with extensions that 'would be nice to have together' like it seems to be 
> > the
> > case for the last 3 extensions in that list.
> >
> > I believe users would benefit more from a single flag to enable everything 
> > and
> > be done with it.
> >
> >
> > Thanks,
> >
> > Daniel
> >
> >
> >
> > >
> > > Thanks,
> > > drew
> > >
> > >> That's why, enable all of these if Ssccfg extension is
> > >> enabled from the commandline.
> > >>
> > >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > >> ---
> > >>   target/riscv/cpu.c | 14 +++++++++++++-
> > >>   1 file changed, 13 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > >> index 22ba43c7ff2a..abebfcc46dea 100644
> > >> --- a/target/riscv/cpu.c
> > >> +++ b/target/riscv/cpu.c
> > >> @@ -2665,8 +2665,20 @@ RISCVCPUImpliedExtsRule 
> > >> *riscv_multi_ext_implied_rules[] = {
> > >>       NULL
> > >>   };
> > >>
> > >> +static RISCVCPUPreferredExtsRule SSCCFG_PREFERRED = {
> > >> +    .ext = CPU_CFG_OFFSET(ext_ssccfg),
> > >> +    .preferred_multi_exts = {
> > >> +        CPU_CFG_OFFSET(ext_smcsrind), CPU_CFG_OFFSET(ext_sscsrind),
> > >> +        CPU_CFG_OFFSET(ext_ssaia), CPU_CFG_OFFSET(ext_smaia),
> > >> +        CPU_CFG_OFFSET(ext_smstateen), CPU_CFG_OFFSET(ext_sscofpmf),
> > >> +        CPU_CFG_OFFSET(ext_smcntrpmf), CPU_CFG_OFFSET(ext_smcdeleg),
> > >> +
> > >> +        RISCV_PREFRRED_EXTS_RULE_END
> > >> +    },
> > >> +};
> > >> +
> > >>   RISCVCPUPreferredExtsRule *riscv_multi_ext_preferred_rules[] = {
> > >> -    NULL
> > >> +    &SSCCFG_PREFERRED, NULL
> > >>   };
> > >>
> > >>   static Property riscv_cpu_properties[] = {
> > >>
> > >> --
> > >> 2.34.1
> > >>
> > >>
> >