On Wed, Aug 7, 2024 at 2:06 AM Daniel Henrique Barboza <dbarb...@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 <ati...@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 > >> > >> >