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

[cp0613@linux.alibaba.com]

On 2025-11-10 08:32 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.)

Thanks for the reminder, I will keep a changelog in the next patch.

> 
> >> > 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.)
> 

I have a different opinion on this point. This extension is basically unrelated
to M-mode, instead, it runs in S-mode. It may be necessary to consider the background
of ssqosid's introduction. This is to ensure that certain critical business processes
can obtain sufficient hardware resources to run and thus guarantee service quality.

Similar technologies include Intel-RDT and ARM MPAM. On the Linux side, Drew has already
submitted relevant patches [1]. It can be seen that in the future, it will inevitably be
used in conjunction with Linux resctrl, so there is no need to consider the execution
situation in M-mode, it is only necessary to enable the relevant permissions.

> >> > 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.

Yes, I'm just maintaining consistency with existing coding styles.

Thanks,
Pei

[1] https://lore.kernel.org/lkml/20251007-ssqosid-v4-0-e8b57e59d812@kernel.org/

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