| Message ID | 20240723-counter_delegation-v2-13-c4170a5348ca@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add RISC-V Counter delegation ISA extension support | expand |
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. 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 > >
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. 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. 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 >> >>
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 > >> > >> >
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 > > >> > > >> > >
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. I don't love either though, isn't this what profiles is supposed to fix! > > > 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. Alistair > 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
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 ? 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. > 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. 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) > Alistair > > > 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
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
On Thu, Aug 8, 2024 at 1:24 AM Alistair Francis <alistair23@gmail.com> wrote: > > 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. > Yes. That would be great. I will drop the Preferred rule and use the implied rule for the extensions mandated by the ISA for now in v3. > > 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
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[] = {
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. 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(-)