On Thu, Aug 8, 2024 at 6:12 PM Atish Kumar Patra <ati...@rivosinc.com> wrote: > > On Wed, Aug 7, 2024 at 5:27 PM Alistair Francis <alistai...@gmail.com> wrote: > > > > On Wed, Aug 7, 2024 at 5:44 PM Atish Kumar Patra <ati...@rivosinc.com> > > wrote: > > > > > > On Tue, Aug 6, 2024 at 7:01 PM Alistair Francis <alistai...@gmail.com> > > > wrote: > > > > > > > > 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. > > > > > > > > > > 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
