[PATCH v1 2/2] lib: sbi: Implement Sstc extension
Anup Patel
apatel at ventanamicro.com
Fri Apr 22 22:05:55 PDT 2022
On Sat, Apr 23, 2022 at 1:55 AM Atish Patra <atishp at atishpatra.org> wrote:
>
> On Wed, Apr 13, 2022 at 10:21 PM Anup Patel <anup at brainfault.org> wrote:
> >
> > On Mon, Feb 28, 2022 at 1:56 PM Atish Patra <atishp at rivosinc.com> wrote:
> > >
> > > Recently, Sstc extension was ratified. It defines stimecmp which allows
> > > the supervisor mode to directly update the timecmp value without the
> > > need of the SBI call. The hardware also can inject the S-mode timer
> > > interrupt direclty to the supervisor without going through the M-mode.
> > > To maintain backward compatibility with the older software, SBI call
> > > now uses stimecmp directly if the hardware supports.
> > >
> > > Implement the Sstc extension.
> > >
> > > Signed-off-by: Atish Patra <atishp at rivosinc.com>
> >
> > Please rebase this patch on latest OpenSBI. Also, see a few comments below.
> >
> > > ---
> > > include/sbi/riscv_encoding.h | 4 ++++
> > > include/sbi/sbi_hart.h | 4 +++-
> > > lib/sbi/sbi_hart.c | 28 ++++++++++++++++++++++++++++
> > > lib/sbi/sbi_timer.c | 25 ++++++++++++++++++++++---
> > > 4 files changed, 57 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> > > index c0020f43f022..cce66ddc8b12 100644
> > > --- a/include/sbi/riscv_encoding.h
> > > +++ b/include/sbi/riscv_encoding.h
> > > @@ -312,6 +312,10 @@
> > > #define CSR_STVAL 0x143
> > > #define CSR_SIP 0x144
> > >
> > > +/* Sstc extension */
> > > +#define CSR_STIMECMP 0x14D
> > > +#define CSR_STIMECMPH 0x15D
> > > +
> > > /* Supervisor Protection and Translation */
> > > #define CSR_SATP 0x180
> > >
> > > diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
> > > index 9e5a2b416017..93be7eddd6f1 100644
> > > --- a/include/sbi/sbi_hart.h
> > > +++ b/include/sbi/sbi_hart.h
> > > @@ -28,9 +28,11 @@ enum sbi_hart_features {
> > > SBI_HART_HAS_AIA = (1 << 5),
> > > /** HART has MENVCFG CSR */
> > > SBI_HART_HAS_MENVCFG = (1 << 6),
> > > + /** HART has SSTC extension implemented in hardware */
> > > + SBI_HART_HAS_SSTC = (1 << 7),
> > >
> > > /** Last index of Hart features*/
> > > - SBI_HART_HAS_LAST_FEATURE = SBI_HART_HAS_MENVCFG,
> > > + SBI_HART_HAS_LAST_FEATURE = SBI_HART_HAS_SSTC,
> > > };
> > >
> > > struct sbi_scratch;
> > > diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
> > > index e08d9d32266a..8c9cad219c71 100644
> > > --- a/lib/sbi/sbi_hart.c
> > > +++ b/lib/sbi/sbi_hart.c
> > > @@ -43,6 +43,7 @@ static void mstatus_init(struct sbi_scratch *scratch)
> > > int cidx;
> > > unsigned int num_mhpm = sbi_hart_mhpm_count(scratch);
> > > uint64_t mhpmevent_init_val = 0;
> > > + unsigned long menvcfg_val = 0;
> > >
> > > /* Enable FPU */
> > > if (misa_extension('D') || misa_extension('F'))
> > > @@ -54,6 +55,23 @@ static void mstatus_init(struct sbi_scratch *scratch)
> > >
> > > csr_write(CSR_MSTATUS, mstatus_val);
> > >
> > > + /*
> > > + * The spec doesn't explicitly describe the reset value of menvcfg.
> > > + * Enable access to stimecmp if sstc extension is present in the
> > > + * hardware.
> > > + */
> > > + if (sbi_hart_has_feature(scratch, SBI_HART_HAS_SSTC)) {
> > > +#if __riscv_xlen == 32
> > > + menvcfg_val = csr_read(CSR_MENVCFGH);
> > > + menvcfg_val |= MENVCFGH_STCE;
> > > + csr_write(CSR_MENVCFGH, menvcfg_val);
> > > +#else
> > > + menvcfg_val = csr_read(CSR_MENVCFG);
> > > + menvcfg_val |= MENVCFG_STCE;
> > > + csr_write(CSR_MENVCFG, menvcfg_val);
> > > +#endif
> > > + }
> > > +
> > > /* Disable user mode usage of all perf counters except default ones (CY, TM, IR) */
> > > if (misa_extension('S') &&
> > > sbi_hart_has_feature(scratch, SBI_HART_HAS_SCOUNTEREN))
> > > @@ -303,6 +321,8 @@ static inline char *sbi_hart_feature_id2string(unsigned long feature)
> > > break;
> > > case SBI_HART_HAS_AIA:
> > > fstr = "aia";
> > > + case SBI_HART_HAS_SSTC:
> > > + fstr = "sstc";
> > > break;
> > > default:
> > > break;
> > > @@ -533,6 +553,14 @@ __mhpm_skip:
> > > if (!trap.cause)
> > > hfeatures->features |= SBI_HART_HAS_MENVCFG;
> > >
> > > + /**
> > > + * Detect if hart supports stimecmp CSR(Sstc extension) and menvcfg is
> > > + * implemented.
> > > + */
> > > + csr_read_allowed(CSR_STIMECMP, (unsigned long)&trap);
> > > + if (!trap.cause && sbi_hart_has_feature(scratch, SBI_HART_HAS_MENVCFG))
> > > + hfeatures->features |= SBI_HART_HAS_SSTC;
> > > +
> >
> > You need to set menvcfg.STCE before detecting stimecmp CSR and restore
> > menvcfg after detecting stimecmp CSR.
> >
>
> That won't be required. The STCE bit in menvcfg controls access of
> stimecmp CSR from priv modes other than M.
> Here is the relevant snippet from the spec.
>
> Bit 63 of menvcfg (or bit 31 of menvcfgh) - named STCE (STimecmp
> Enable) - enables stimecmp
> for S-mode when set to one, and the same bit of henvcfg enables
> vstimecmp for VS-mode.
> When STCE in menvcfg is zero, an attempt to access stimecmp or
> vstimecmp in a mode other
> than M-mode raises an illegal instruction exception,
Thanks for the clarification, please rebase and send this patch again.
Regards,
Anup
>
>
> >
> > > /* Detect if hart has AIA local interrupt CSRs */
> > > csr_read_allowed(CSR_MTOPI, (unsigned long)&trap);
> > > if (trap.cause)
> > > diff --git a/lib/sbi/sbi_timer.c b/lib/sbi/sbi_timer.c
> > > index acdba922b1e9..c89c7ca51100 100644
> > > --- a/lib/sbi/sbi_timer.c
> > > +++ b/lib/sbi/sbi_timer.c
> > > @@ -124,16 +124,35 @@ void sbi_timer_set_delta_upper(ulong delta_upper)
> > > void sbi_timer_event_start(u64 next_event)
> > > {
> > > sbi_pmu_ctr_incr_fw(SBI_PMU_FW_SET_TIMER);
> > > - if (timer_dev && timer_dev->timer_event_start)
> > > +
> > > + /**
> > > + * Update the stimecmp directly if available. This allows
> > > + * the older software to leverage sstc extension on newer hardware.
> > > + */
> > > + if (sbi_hart_has_feature(sbi_scratch_thishart_ptr(), SBI_HART_HAS_SSTC)) {
> > > +#if __riscv_xlen == 32
> > > + csr_write(CSR_STIMECMP, next_event & 0xFFFFFFFF);
> > > + csr_write(CSR_STIMECMPH, next_event >> 32);
> > > +#else
> > > + csr_write(CSR_STIMECMP, next_event);
> > > +#endif
> > > + } else if (timer_dev && timer_dev->timer_event_start) {
> > > timer_dev->timer_event_start(next_event);
> > > - csr_clear(CSR_MIP, MIP_STIP);
> > > + csr_clear(CSR_MIP, MIP_STIP);
> > > + }
> >
> > At this point in time, I think it is best to directly set stimecmp CSR
> > upon SBI call because mip.STIP is read-only when Sstc extension
> > is implemented.
> >
> > In future, we can think of an alternative approach to avoid touching
> > stimecmp CSR from SBI call.
> >
> > > csr_set(CSR_MIE, MIP_MTIP);
> > > }
> > >
> > > void sbi_timer_process(void)
> > > {
> > > csr_clear(CSR_MIE, MIP_MTIP);
> > > - csr_set(CSR_MIP, MIP_STIP);
> > > + /*
> > > + * If sstc extension is available, supervisor can receive the timer
> > > + * direclty without M-mode come in between. This function should
> >
> > s/direclty/directly/
> >
> > > + * only invoked if M-mode programs the timer for its own purpose.
> > > + */
> > > + if (!sbi_hart_has_feature(sbi_scratch_thishart_ptr(), SBI_HART_HAS_SSTC))
> > > + csr_set(CSR_MIP, MIP_STIP);
> > > }
> > >
> > > const struct sbi_timer_device *sbi_timer_get_device(void)
> > > --
> > > 2.30.2
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
> >
> > Regards,
> > Anup
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
>
>
>
> --
> Regards,
> Atish
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
More information about the opensbi
mailing list