[PATCH v1 2/2] lib: sbi: Implement Sstc extension
Jessica Clarke
jrtc27 at jrtc27.com
Sat Mar 5 10:43:42 PST 2022
On 5 Mar 2022, at 09:26, Atish Patra <atishp at atishpatra.org> wrote:
>
> On Mon, Feb 28, 2022 at 11:59 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>>
>> On 28 Feb 2022, at 19:51, Atish Kumar Patra <atishp at rivosinc.com> wrote:
>>>
>>> On Mon, Feb 28, 2022 at 11:42 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>>>>
>>>> On 28 Feb 2022, at 18:52, Atish Kumar Patra <atishp at rivosinc.com> wrote:
>>>>>
>>>>> On Mon, Feb 28, 2022 at 9:03 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>>>>>>
>>>>>> On 28 Feb 2022, at 08:26, 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>
>>>>>>
>>>>>> M-mode changing behaviour and clobbering S-mode state seems wrong; how
>>>>>> do you know Sstimecmp isn’t in use too?
>>>>>
>>>>> <Fixed Anup's email address>
>>>>>
>>>>> Because, S-mode will either use the stimecmp or SBI call. It will not
>>>>> use both randomly.
>>>>
>>>> S-mode can do what it likes. You’re changing semantics for an existing
>>>> interface from “just injects a timer interrupt at the requested point”
>>>> to “... and also clobbers S-mode-writable state”.
>>>>
>>>
>>> If S-mode does whatever it likes, it will end up in a suboptimal
>>> performance which nobody wants.
>>> I don't understand why or how S-mode would want to switch between
>>> stimecmp or SBI calls randomly.
>>>
>>> Do you have a use case in mind ?
>>>
>>>>> This is also suggested in the sstc spec[1] as well.
>>>>
>>>> This is non-normative and not up to that spec to decide.
>>>>
>>>
>>> Yes. But if both parties(S-mode & M-mode) have a sane implementation,
>>> it can result in a better performant system.
>>
>> I don’t care about use cases or “sane” implementations, I care about
>> not doing things the spec doesn’t say will happen. M-mode state is
>> invisible to S-mode so does not need to be specified as being changed.
>> S-mode state is completely visible to, and controlled by, S-mode, so
>> changing it without the spec saying so (a spec you’ve frozen and want
>> to ratify) is totally wrong, regardless of whether “sane” software
>> should care. The only way it’s allowed to mess with stimecmp from
>> M-mode is if you don't expose it at all to S-mode, which is clearly a
>> bad idea. So leave it alone, don’t change the semantics of existing
>> frozen interfaces, and avoid this unnecessary complexity in OpenSBI
>> that exists solely to make legacy software go faster.
>>
>
> The Sstc spec also made STIP bit in mip as read only. Here is exact
> snippet from the spec
>
> "If the stimecmp (supervisor-mode timer compare) register is implemented,
> STIP is read-only in mip and reflects the supervisor-level timer
> interrupt signal resulting from
> stimecmp."
>
> Thus, M-mode can't update the STIP bit if sstc is enabled.
This directly contradicts text from ratified privileged specs (quoting
Volume 2, Privileged Spec v. 20211203, the current version linked from
riscv.org):
"If supervisor mode is implemented, bits mip.STIP and mie.STIE are the
interrupt-pending and interrupt-enable bits for supervisor-level timer
interrupts. STIP is writable in mip, and may be written by M-mode
software to deliver timer interrupts to S-mode.”
That makes Sstc in direct violation of the existing privileged spec and
not a compatible extension, being completely backwards-incompatible
with every software stack that exists today. What a mess. No wonder we
get mocked by everyone outside the community if we can’t even get this
kind of thing right.
Jess
>>>>> In systems in which a supervisor execution environment (SEE) provides
>>>>> timer facilities via an SBI
>>>>> function call, this SBI call will continue to support requests to
>>>>> schedule a timer interrupt. The SEE
>>>>> will simply make use of stimecmp, changing its value as appropriate.
>>>>> This ensures compatibility
>>>>> with existing S-mode software that uses this SEE facility, while new
>>>>> S-mode software takes
>>>>> advantage of stimecmp directly.)
>>>>>
>>>>> [1] https://drive.google.com/file/d/1m84Re2yK8m_vbW7TspvevCDR82MOBaSX/view
>>>>>
>>>>>> It’s owned by S-mode, leave it
>>>>>> alone and use M-mode-only methods to implement the firmware calls.
>>>>>>
>>>>>> Jess
>>>>>>
>>>>>>> ---
>>>>>>> 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;
>>>>>>> +
>>>>>>> /* 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);
>>>>>>> + }
>>>>>>> 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
>>>>>>> + * 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
>>
>>
>> --
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
>
>
>
> --
> Regards,
> Atish
More information about the opensbi
mailing list