Re: [PATCH v3] lib: sbi: Enable Ssqosid Ext using mstateen0

["Radim Krčmář" <rkrcmar@ventanamicro.com>]

2025-11-08T19:22:00+08:00, <cp0613@linux.alibaba.com>:
> On 2025-11-07 11:13 AM, Radim Krčmář wrote:
>
>> > diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
>> > @@ -79,6 +79,8 @@ enum sbi_hart_extensions {
>> >   SBI_HART_EXT_SMCTR,
>> >   /** HART has CTR S-mode CSRs */
>> >   SBI_HART_EXT_SSCTR,
>> > + /** Hart has Ssqosid extension */
>> > + SBI_HART_EXT_SSQOSID,
>
>> Don't we also need something like
>
>>   __check_ext_csr(SBI_HART_PRIV_VER_UNKNOWN,
>>                   CSR_SRMCFG, SBI_HART_EXT_SSQOSID);
>
>> to detect the SSQOSID extension?
>
> Hi Radim,
>
> I added this detection mechanism in the first patch, but Anup said, "For newer
> extensions, detecting from ISA strings is sufficient, so there's no need to
> trap-n-detect here." Therefore, it has been removed here.

Ah, that is a great decision.

(I missed that this isn't the first version, please try to maintain a
 changelog when posting subsequent modifications.)

>> > diff --git a/lib/sbi/sbi_domain_context.c b/lib/sbi/sbi_domain_context.c
>> > @@ -143,8 +145,11 @@ static int switch_to_next_domain_context(struct hart_context *ctx,
>> >   ctx->satp = csr_swap(CSR_SATP, dom_ctx->satp);
>> >   if (sbi_hart_priv_version(scratch) >= SBI_HART_PRIV_VER_1_10)
>> >    ctx->scounteren = csr_swap(CSR_SCOUNTEREN, dom_ctx->scounteren);
>> > - if (sbi_hart_priv_version(scratch) >= SBI_HART_PRIV_VER_1_12)
>> > + if (sbi_hart_priv_version(scratch) >= SBI_HART_PRIV_VER_1_12) {
>> >    ctx->senvcfg = csr_swap(CSR_SENVCFG, dom_ctx->senvcfg);
>> > +  if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSQOSID))
>> > +   ctx->srmcfg = csr_swap(CSR_SRMCFG, dom_ctx->srmcfg);
>> > + }
>
>> Why would we want Ssqosid to depend on S >= 1.12?
>
> I'm guessing you read the RISC-V Instruction Set Manual, which describes ssqoid as
> requiring priv version 1.14. However, the highest priv version currently supported
> by OpenSBI is 1.12. Therefore, this implementation is based on the current situation,
> and the previous implementations such as CTR also follow this approach.

Does it?  There is even a non-normative note:

  The Ssqosid extension does not require that S-mode mode be implemented.

I think it's because srmcfg has effect on M-mode, so the extension is
completely independent to allow it in pure M systems.

(We enable srmcfg in mstateen regardless of S version, so there is a
 potential bug where we wouldn't correctly context switch the csr, given
 a system with weird combination of extensions.)

>> > diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>> > @@ -112,6 +112,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
>> > +  else
>> > +   mstateen_val &= ~(SMSTATEEN0_SRMCFG);
>
>> The else branch is pointless, because the bit is known to be 0.
>
>> Thanks.
>
> Here we need to consider the following scenario: even if HART supports the SSQOSID
> extension, but the user doesn't want to use this feature and removes the ssqosid
> field from the ISA string, then this bit needs to be explicitly cleared.

We zero a zero bit.  The code flows like this:

  mstateen_val = 0;
  ...
  mstateen_val &= ~(SMSTATEEN0_SRMCFG);

I would prefer if we didn't add the else branch, but I don't mind it too
much either if that's the opensbi style.
(Optimizing compiler should generate the same code.)

Thanks.

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi