| Message ID | 20220613003838.13028-1-samuel@sholland.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | lib: sbi: Fix version dependency for Sscofpmf | expand |
On Mon, Jun 13, 2022 at 6:08 AM Samuel Holland <samuel@sholland.org> wrote: > > mcounteren was added in the privileged spec v1.10 and mcountinhibit was > added in v1.11. Sscofpmf does not depend on anything in v1.12, so the > minimum privileged spec version should have been v1.11. > > Fixes: d4b563c881d6 ("lib: sbi: Remove MCOUNTEREN and SCOUNTEREN hart features") > Signed-off-by: Samuel Holland <samuel@sholland.org> I think there is some confusion here. The mcounteren and mcountinhibit are not part of the Sscofpmf extension and OpenSBI PMU implementation does support platforms without Sscofpmf. Regards, Anup > --- > > lib/sbi/sbi_hart.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c > index de86fee..5bc6d52 100644 > --- a/lib/sbi/sbi_hart.c > +++ b/lib/sbi/sbi_hart.c > @@ -645,7 +645,7 @@ __mhpm_skip: > hfeatures->priv_version = SBI_HART_PRIV_VER_1_12; > > /* Counter overflow/filtering is not useful without mcounter/inhibit */ > - if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) { > + if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) { > /* Detect if hart supports sscofpmf */ > csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap); > if (!trap.cause) > -- > 2.35.1 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
Hi Anup, On 6/16/22 11:12 PM, Anup Patel wrote: > On Mon, Jun 13, 2022 at 6:08 AM Samuel Holland <samuel@sholland.org> wrote: >> >> mcounteren was added in the privileged spec v1.10 and mcountinhibit was >> added in v1.11. Sscofpmf does not depend on anything in v1.12, so the >> minimum privileged spec version should have been v1.11. >> >> Fixes: d4b563c881d6 ("lib: sbi: Remove MCOUNTEREN and SCOUNTEREN hart features") >> Signed-off-by: Samuel Holland <samuel@sholland.org> > > I think there is some confusion here. > > The mcounteren and mcountinhibit are not part of the Sscofpmf extension and > OpenSBI PMU implementation does support platforms without Sscofpmf. I am aware of that. I am referencing the comment on the previous line, and the code that was changed by the referenced commit. The code, as originally written, attempted to optimize out the Sscofpmf check based on the presence of those two CSRs. When the hart features were refactored, the CSR feature was replaced by the wrong version, i.e. the behavior was accidentally changed by the refactoring. This patch only attempts to restore the behavior from before the refactoring. If that behavior was wrong to start with, that is a different matter. Regards, Samuel > Regards, > Anup > >> --- >> >> lib/sbi/sbi_hart.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c >> index de86fee..5bc6d52 100644 >> --- a/lib/sbi/sbi_hart.c >> +++ b/lib/sbi/sbi_hart.c >> @@ -645,7 +645,7 @@ __mhpm_skip: >> hfeatures->priv_version = SBI_HART_PRIV_VER_1_12; >> >> /* Counter overflow/filtering is not useful without mcounter/inhibit */ >> - if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) { >> + if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) { >> /* Detect if hart supports sscofpmf */ >> csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap); >> if (!trap.cause) >> -- >> 2.35.1 >> >> >> -- >> opensbi mailing list >> opensbi@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/opensbi
On Fri, Jun 17, 2022 at 9:58 AM Samuel Holland <samuel@sholland.org> wrote: > > Hi Anup, > > On 6/16/22 11:12 PM, Anup Patel wrote: > > On Mon, Jun 13, 2022 at 6:08 AM Samuel Holland <samuel@sholland.org> wrote: > >> > >> mcounteren was added in the privileged spec v1.10 and mcountinhibit was > >> added in v1.11. Sscofpmf does not depend on anything in v1.12, so the > >> minimum privileged spec version should have been v1.11. > >> > >> Fixes: d4b563c881d6 ("lib: sbi: Remove MCOUNTEREN and SCOUNTEREN hart features") > >> Signed-off-by: Samuel Holland <samuel@sholland.org> > > > > I think there is some confusion here. > > > > The mcounteren and mcountinhibit are not part of the Sscofpmf extension and > > OpenSBI PMU implementation does support platforms without Sscofpmf. > > I am aware of that. I am referencing the comment on the previous line, and the > code that was changed by the referenced commit. The code, as originally written, > attempted to optimize out the Sscofpmf check based on the presence of those two > CSRs. > > When the hart features were refactored, the CSR feature was replaced by the > wrong version, i.e. the behavior was accidentally changed by the refactoring. > > This patch only attempts to restore the behavior from before the refactoring. If > that behavior was wrong to start with, that is a different matter. I agree there is nothing in Sscofpmf which prevents a Priv v1.11 HART from implementing it so this now becomes a strange choice on the OpenSBI side. IMO, the previous behaviour of detecting and using Sscofpmf irrespective of the Priv spec version was wrong because Sscofpmf is defined assuming Priv v1.12 spec. In fact, that's why Sscofpmf defines bits for filtering events in VS/VU modes. Is there a case where we would need Sscofpmf for a Priv v1.11 HART ? Regards, Anup > > Regards, > > Anup > > > >> --- > >> > >> lib/sbi/sbi_hart.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c > >> index de86fee..5bc6d52 100644 > >> --- a/lib/sbi/sbi_hart.c > >> +++ b/lib/sbi/sbi_hart.c > >> @@ -645,7 +645,7 @@ __mhpm_skip: > >> hfeatures->priv_version = SBI_HART_PRIV_VER_1_12; > >> > >> /* Counter overflow/filtering is not useful without mcounter/inhibit */ > >> - if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) { > >> + if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) { > >> /* Detect if hart supports sscofpmf */ > >> csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap); > >> if (!trap.cause) > >> -- > >> 2.35.1 > >> > >> > >> -- > >> opensbi mailing list > >> opensbi@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/opensbi >
On 6/16/22 11:56 PM, Anup Patel wrote: > On Fri, Jun 17, 2022 at 9:58 AM Samuel Holland <samuel@sholland.org> wrote: >> >> Hi Anup, >> >> On 6/16/22 11:12 PM, Anup Patel wrote: >>> On Mon, Jun 13, 2022 at 6:08 AM Samuel Holland <samuel@sholland.org> wrote: >>>> >>>> mcounteren was added in the privileged spec v1.10 and mcountinhibit was >>>> added in v1.11. Sscofpmf does not depend on anything in v1.12, so the >>>> minimum privileged spec version should have been v1.11. >>>> >>>> Fixes: d4b563c881d6 ("lib: sbi: Remove MCOUNTEREN and SCOUNTEREN hart features") >>>> Signed-off-by: Samuel Holland <samuel@sholland.org> >>> >>> I think there is some confusion here. >>> >>> The mcounteren and mcountinhibit are not part of the Sscofpmf extension and >>> OpenSBI PMU implementation does support platforms without Sscofpmf. >> >> I am aware of that. I am referencing the comment on the previous line, and the >> code that was changed by the referenced commit. The code, as originally written, >> attempted to optimize out the Sscofpmf check based on the presence of those two >> CSRs. >> >> When the hart features were refactored, the CSR feature was replaced by the >> wrong version, i.e. the behavior was accidentally changed by the refactoring. >> >> This patch only attempts to restore the behavior from before the refactoring. If >> that behavior was wrong to start with, that is a different matter. > > I agree there is nothing in Sscofpmf which prevents a Priv v1.11 HART from > implementing it so this now becomes a strange choice on the OpenSBI side. > > IMO, the previous behaviour of detecting and using Sscofpmf irrespective > of the Priv spec version was wrong because Sscofpmf is defined assuming > Priv v1.12 spec. In fact, that's why Sscofpmf defines bits for filtering events > in VS/VU modes. > > Is there a case where we would need Sscofpmf for a Priv v1.11 HART ? I was attempting to add C906 PMU support as an errata on top of the standard extension, which is when I noticed this check. (C906 supports v1.11). However, I realized C906's custom overflow handling is different enough from Sscofpmf that I should implement it separately. So I do not know if a v1.11+Sscofpmf case does or should exist. Maybe the check is correct now, in which case the comment is a bit misleading. Regards, Samuel >>>> --- >>>> >>>> lib/sbi/sbi_hart.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c >>>> index de86fee..5bc6d52 100644 >>>> --- a/lib/sbi/sbi_hart.c >>>> +++ b/lib/sbi/sbi_hart.c >>>> @@ -645,7 +645,7 @@ __mhpm_skip: >>>> hfeatures->priv_version = SBI_HART_PRIV_VER_1_12; >>>> >>>> /* Counter overflow/filtering is not useful without mcounter/inhibit */ >>>> - if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) { >>>> + if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) { >>>> /* Detect if hart supports sscofpmf */ >>>> csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap); >>>> if (!trap.cause) >>>> -- >>>> 2.35.1 >>>> >>>> >>>> -- >>>> opensbi mailing list >>>> opensbi@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/opensbi >>
On Fri, Jun 17, 2022 at 10:49 AM Samuel Holland <samuel@sholland.org> wrote: > > On 6/16/22 11:56 PM, Anup Patel wrote: > > On Fri, Jun 17, 2022 at 9:58 AM Samuel Holland <samuel@sholland.org> wrote: > >> > >> Hi Anup, > >> > >> On 6/16/22 11:12 PM, Anup Patel wrote: > >>> On Mon, Jun 13, 2022 at 6:08 AM Samuel Holland <samuel@sholland.org> wrote: > >>>> > >>>> mcounteren was added in the privileged spec v1.10 and mcountinhibit was > >>>> added in v1.11. Sscofpmf does not depend on anything in v1.12, so the > >>>> minimum privileged spec version should have been v1.11. > >>>> > >>>> Fixes: d4b563c881d6 ("lib: sbi: Remove MCOUNTEREN and SCOUNTEREN hart features") > >>>> Signed-off-by: Samuel Holland <samuel@sholland.org> > >>> > >>> I think there is some confusion here. > >>> > >>> The mcounteren and mcountinhibit are not part of the Sscofpmf extension and > >>> OpenSBI PMU implementation does support platforms without Sscofpmf. > >> > >> I am aware of that. I am referencing the comment on the previous line, and the > >> code that was changed by the referenced commit. The code, as originally written, > >> attempted to optimize out the Sscofpmf check based on the presence of those two > >> CSRs. > >> > >> When the hart features were refactored, the CSR feature was replaced by the > >> wrong version, i.e. the behavior was accidentally changed by the refactoring. > >> > >> This patch only attempts to restore the behavior from before the refactoring. If > >> that behavior was wrong to start with, that is a different matter. > > > > I agree there is nothing in Sscofpmf which prevents a Priv v1.11 HART from > > implementing it so this now becomes a strange choice on the OpenSBI side. > > > > IMO, the previous behaviour of detecting and using Sscofpmf irrespective > > of the Priv spec version was wrong because Sscofpmf is defined assuming > > Priv v1.12 spec. In fact, that's why Sscofpmf defines bits for filtering events > > in VS/VU modes. > > > > Is there a case where we would need Sscofpmf for a Priv v1.11 HART ? > > I was attempting to add C906 PMU support as an errata on top of the standard > extension, which is when I noticed this check. (C906 supports v1.11). However, I > realized C906's custom overflow handling is different enough from Sscofpmf that > I should implement it separately. > > So I do not know if a v1.11+Sscofpmf case does or should exist. Maybe the check > is correct now, in which case the comment is a bit misleading. Instead of setting SSCOFPMF extension flag for C906, I suggest we extend sbi_pmu.c using "struct sbi_pmu_overflow_ops" abstraction. The sbi_pmu.c will have two implementations of "struct sbi_pmu_overflow_ops": 1) "dummy" implementation which will be used when there is no HW counter overflow support 2) "sscofpmf" implementation which will be used when HW support Sscofpmf extension. The platform override for Allwinner D1 platform can provide its own implementation of "struct sbi_pmu_overflow_ops". Regards, Anup > > Regards, > Samuel > > >>>> --- > >>>> > >>>> lib/sbi/sbi_hart.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c > >>>> index de86fee..5bc6d52 100644 > >>>> --- a/lib/sbi/sbi_hart.c > >>>> +++ b/lib/sbi/sbi_hart.c > >>>> @@ -645,7 +645,7 @@ __mhpm_skip: > >>>> hfeatures->priv_version = SBI_HART_PRIV_VER_1_12; > >>>> > >>>> /* Counter overflow/filtering is not useful without mcounter/inhibit */ > >>>> - if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) { > >>>> + if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) { > >>>> /* Detect if hart supports sscofpmf */ > >>>> csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap); > >>>> if (!trap.cause) > >>>> -- > >>>> 2.35.1 > >>>> > >>>> > >>>> -- > >>>> opensbi mailing list > >>>> opensbi@lists.infradead.org > >>>> http://lists.infradead.org/mailman/listinfo/opensbi > >> >
On Thu, Jun 16, 2022 at 11:44 PM Anup Patel <anup@brainfault.org> wrote: > > On Fri, Jun 17, 2022 at 10:49 AM Samuel Holland <samuel@sholland.org> wrote: > > > > On 6/16/22 11:56 PM, Anup Patel wrote: > > > On Fri, Jun 17, 2022 at 9:58 AM Samuel Holland <samuel@sholland.org> wrote: > > >> > > >> Hi Anup, > > >> > > >> On 6/16/22 11:12 PM, Anup Patel wrote: > > >>> On Mon, Jun 13, 2022 at 6:08 AM Samuel Holland <samuel@sholland.org> wrote: > > >>>> > > >>>> mcounteren was added in the privileged spec v1.10 and mcountinhibit was > > >>>> added in v1.11. Sscofpmf does not depend on anything in v1.12, so the > > >>>> minimum privileged spec version should have been v1.11. > > >>>> > > >>>> Fixes: d4b563c881d6 ("lib: sbi: Remove MCOUNTEREN and SCOUNTEREN hart features") > > >>>> Signed-off-by: Samuel Holland <samuel@sholland.org> > > >>> > > >>> I think there is some confusion here. > > >>> > > >>> The mcounteren and mcountinhibit are not part of the Sscofpmf extension and > > >>> OpenSBI PMU implementation does support platforms without Sscofpmf. > > >> > > >> I am aware of that. I am referencing the comment on the previous line, and the > > >> code that was changed by the referenced commit. The code, as originally written, > > >> attempted to optimize out the Sscofpmf check based on the presence of those two > > >> CSRs. > > >> > > >> When the hart features were refactored, the CSR feature was replaced by the > > >> wrong version, i.e. the behavior was accidentally changed by the refactoring. > > >> > > >> This patch only attempts to restore the behavior from before the refactoring. If > > >> that behavior was wrong to start with, that is a different matter. > > > > > > I agree there is nothing in Sscofpmf which prevents a Priv v1.11 HART from > > > implementing it so this now becomes a strange choice on the OpenSBI side. > > > > > > IMO, the previous behaviour of detecting and using Sscofpmf irrespective > > > of the Priv spec version was wrong because Sscofpmf is defined assuming > > > Priv v1.12 spec. In fact, that's why Sscofpmf defines bits for filtering events > > > in VS/VU modes. > > > > > > Is there a case where we would need Sscofpmf for a Priv v1.11 HART ? > > > > I was attempting to add C906 PMU support as an errata on top of the standard > > extension, which is when I noticed this check. (C906 supports v1.11). However, I > > realized C906's custom overflow handling is different enough from Sscofpmf that > > I should implement it separately. > > Yes. It is completely different from Sscofpmf with many more CSRs. > > So I do not know if a v1.11+Sscofpmf case does or should exist. Maybe the check > > is correct now, in which case the comment is a bit misleading. > > Instead of setting SSCOFPMF extension flag for C906, I suggest we extend > sbi_pmu.c using "struct sbi_pmu_overflow_ops" abstraction. The sbi_pmu.c > will have two implementations of "struct sbi_pmu_overflow_ops": 1) "dummy" > implementation which will be used when there is no HW counter overflow > support 2) "sscofpmf" implementation which will be used when HW support > Sscofpmf extension. > > The platform override for Allwinner D1 platform can provide its own > implementation of "struct sbi_pmu_overflow_ops". > IIRC, c906 PMU uses a different interrupt (not local) ID for overflow interrupt. Even though we make it configurable in OpenSBI with "struct sbi_pmu_overflow_ops" upstream drive won't support it in the current version. > Regards, > Anup > > > > > Regards, > > Samuel > > > > >>>> --- > > >>>> > > >>>> lib/sbi/sbi_hart.c | 2 +- > > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c > > >>>> index de86fee..5bc6d52 100644 > > >>>> --- a/lib/sbi/sbi_hart.c > > >>>> +++ b/lib/sbi/sbi_hart.c > > >>>> @@ -645,7 +645,7 @@ __mhpm_skip: > > >>>> hfeatures->priv_version = SBI_HART_PRIV_VER_1_12; > > >>>> > > >>>> /* Counter overflow/filtering is not useful without mcounter/inhibit */ > > >>>> - if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) { > > >>>> + if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) { > > >>>> /* Detect if hart supports sscofpmf */ > > >>>> csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap); > > >>>> if (!trap.cause) > > >>>> -- > > >>>> 2.35.1 > > >>>> > > >>>> > > >>>> -- > > >>>> opensbi mailing list > > >>>> opensbi@lists.infradead.org > > >>>> http://lists.infradead.org/mailman/listinfo/opensbi > > >> > > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c index de86fee..5bc6d52 100644 --- a/lib/sbi/sbi_hart.c +++ b/lib/sbi/sbi_hart.c @@ -645,7 +645,7 @@ __mhpm_skip: hfeatures->priv_version = SBI_HART_PRIV_VER_1_12; /* Counter overflow/filtering is not useful without mcounter/inhibit */ - if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) { + if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) { /* Detect if hart supports sscofpmf */ csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap); if (!trap.cause)
mcounteren was added in the privileged spec v1.10 and mcountinhibit was added in v1.11. Sscofpmf does not depend on anything in v1.12, so the minimum privileged spec version should have been v1.11. Fixes: d4b563c881d6 ("lib: sbi: Remove MCOUNTEREN and SCOUNTEREN hart features") Signed-off-by: Samuel Holland <samuel@sholland.org> --- lib/sbi/sbi_hart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)