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

[Alistair Francis]

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.

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.
>
> 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.

I guess with some good documentation this wouldn't be too confusing though.

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
> >>
> >>
>