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

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

[cp0613]

From: Chen Pei <cp0613@linux.alibaba.com>

The QoS Identifiers extension (Ssqosid) introduces the srmcfg register,
which configures a hart with two identifiers: a Resource Control ID
(RCID) and a Monitoring Counter ID (MCID). These identifiers accompany
each request issued by the hart to shared resource controllers.

If extension Smstateen is implemented together with Ssqosid, then
Ssqosid also requires the SRMCFG bit in mstateen0 to be implemented. If
mstateen0.SRMCFG is 0, attempts to access srmcfg in privilege modes less
privileged than M-mode raise an illegal-instruction exception. If
mstateen0.SRMCFG is 1 or if extension Smstateen is not implemented,
attempts to access srmcfg when V=1 raise a virtual-instruction exception.

This extension can be found in the RISC-V Instruction Set Manual:
https://github.com/riscv/riscv-isa-manual

Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
---
 include/sbi/riscv_encoding.h | 5 +++++
 include/sbi/sbi_hart.h       | 2 ++
 lib/sbi/sbi_domain_context.c | 7 ++++++-
 lib/sbi/sbi_hart.c           | 6 ++++++
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
index 61e4b635..b738e209 100644
--- a/include/sbi/riscv_encoding.h
+++ b/include/sbi/riscv_encoding.h
@@ -378,6 +378,9 @@
 #define CSR_SSTATEEN2			0x10E
 #define CSR_SSTATEEN3			0x10F
 
+/* Supervisor Resource Management Configuration CSRs */
+#define CSR_SRMCFG			0x181
+
 /* Machine-Level Control transfer records CSRs */
 #define CSR_MCTRCTL                     0x34e
 
@@ -849,6 +852,8 @@
 #define SMSTATEEN0_FCSR			(_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
 #define SMSTATEEN0_CTR_SHIFT		54
 #define SMSTATEEN0_CTR			(_ULL(1) << SMSTATEEN0_CTR_SHIFT)
+#define SMSTATEEN0_SRMCFG_SHIFT		55
+#define SMSTATEEN0_SRMCFG		(_ULL(1) << SMSTATEEN0_SRMCFG_SHIFT)
 #define SMSTATEEN0_CONTEXT_SHIFT	57
 #define SMSTATEEN0_CONTEXT		(_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
 #define SMSTATEEN0_IMSIC_SHIFT		58
diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
index e66dd52f..c51b0689 100644
--- 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,
 	/** HART has Ssstateen extension **/
 	SBI_HART_EXT_SSSTATEEN,
 	/** Hart has Xsfcflushdlone extension */
diff --git a/lib/sbi/sbi_domain_context.c b/lib/sbi/sbi_domain_context.c
index 74ad25e8..a5b7c530 100644
--- a/lib/sbi/sbi_domain_context.c
+++ b/lib/sbi/sbi_domain_context.c
@@ -45,6 +45,8 @@ struct hart_context {
 	unsigned long scounteren;
 	/** Supervisor environment configuration register */
 	unsigned long senvcfg;
+	/** Supervisor resource management configuration register */
+	unsigned long srmcfg;
 
 	/** Reference to the owning domain */
 	struct sbi_domain *dom;
@@ -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);
+	}
 
 	/* Save current trap state and restore target domain's trap state */
 	trap_ctx = sbi_trap_get_context(scratch);
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index a91703b4..549906f6 100644
--- 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_CTR;
 
+		if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSQOSID))
+			mstateen_val |= (SMSTATEEN0_SRMCFG);
+		else
+			mstateen_val &= ~(SMSTATEEN0_SRMCFG);
+
 		csr_write64(CSR_MSTATEEN0, mstateen_val);
 		csr_write64(CSR_MSTATEEN1, SMSTATEEN_STATEN);
 		csr_write64(CSR_MSTATEEN2, SMSTATEEN_STATEN);
@@ -685,6 +690,7 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
 	__SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
 	__SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
 	__SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
+	__SBI_HART_EXT_DATA(ssqosid, SBI_HART_EXT_SSQOSID),
 	__SBI_HART_EXT_DATA(ssstateen, SBI_HART_EXT_SSSTATEEN),
 	__SBI_HART_EXT_DATA(xsfcflushdlone, SBI_HART_EXT_XSIFIVE_CFLUSH_D_L1),
 	__SBI_HART_EXT_DATA(xsfcease, SBI_HART_EXT_XSIFIVE_CEASE),
-- 
2.50.1


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

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

[Radim Krčmář]

2025-11-07T16:32:00+08:00, <cp0613@linux.alibaba.com>:
> From: Chen Pei <cp0613@linux.alibaba.com>
>
> The QoS Identifiers extension (Ssqosid) introduces the srmcfg register,
> which configures a hart with two identifiers: a Resource Control ID
> (RCID) and a Monitoring Counter ID (MCID). These identifiers accompany
> each request issued by the hart to shared resource controllers.
>
> If extension Smstateen is implemented together with Ssqosid, then
> Ssqosid also requires the SRMCFG bit in mstateen0 to be implemented. If
> mstateen0.SRMCFG is 0, attempts to access srmcfg in privilege modes less
> privileged than M-mode raise an illegal-instruction exception. If
> mstateen0.SRMCFG is 1 or if extension Smstateen is not implemented,
> attempts to access srmcfg when V=1 raise a virtual-instruction exception.
>
> This extension can be found in the RISC-V Instruction Set Manual:
> https://github.com/riscv/riscv-isa-manual
>
> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
> ---
> 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?

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

> 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_CTR;
>  
> +		if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSQOSID))
> +			mstateen_val |= (SMSTATEEN0_SRMCFG);

(Style nit: please remove parentheses arounds SMSTATEEN0_SRMCFG.)

> +		else
> +			mstateen_val &= ~(SMSTATEEN0_SRMCFG);

The else branch is pointless, because the bit is known to be 0.

Thanks.

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

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

[cp0613]

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.

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

> > 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_CTR;
> >  
> > +  if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSQOSID))
> > +   mstateen_val |= (SMSTATEEN0_SRMCFG);

> (Style nit: please remove parentheses arounds SMSTATEEN0_SRMCFG.)

Thank you for pointing this out.

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

Thanks,
Pei

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

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

[Radim Krčmář]

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

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

[cp0613]

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

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

[Radim Krčmář]

2025-11-10T20:31:15+08:00, <cp0613@linux.alibaba.com>:
> On 2025-11-10 08:32 AM, Radim Krčmář wrote:
>> (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.

(Appreciated.  I would have searched mailbox before posting if I noticed
 the v3 in the header, but having it summarized is even better.)

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

We might have taked past each other here.

I'm arguing that RISC-V allows a hart with ISA S1p11_Ssqosid to exist,
because Ssqosid doesn't depend on S, so opensbi should context switch
srmcfg on such potential (albeit unlikely) machine.

Otherwise domains could observe srmcfg set by other domains.

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

Yeah, I'm planning to look at the patches as well, because Ssqosid's
design has a lot of potential for side-channels.

>                                         so there is no need to consider the execution
> situation in M-mode, it is only necessary to enable the relevant permissions.

The execution of M-mode is affected by srmcfg, so we have to be
extremely careful about isolation in opensbi too, especially when
juggling multiple domains.

(I think we might be fine now, but having opensbi switch to a srmcfg
 that isn't controlled by S-mode will result in a lot less headaches...)

Thanks.

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

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

[cp0613]

On 2025-11-10 02:41 PM, Radim Krčmář wrote:

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

> We might have taked past each other here.
> 
> I'm arguing that RISC-V allows a hart with ISA S1p11_Ssqosid to exist,
> because Ssqosid doesn't depend on S, so opensbi should context switch
> srmcfg on such potential (albeit unlikely) machine.
> 
> Otherwise domains could observe srmcfg set by other domains.
> 
> > 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,
> 
> Yeah, I'm planning to look at the patches as well, because Ssqosid's
> design has a lot of potential for side-channels.
> 
> >                                         so there is no need to consider the execution
> > situation in M-mode, it is only necessary to enable the relevant permissions.
> 
> The execution of M-mode is affected by srmcfg, so we have to be
> extremely careful about isolation in opensbi too, especially when
> juggling multiple domains.
> 
> (I think we might be fine now, but having opensbi switch to a srmcfg
>  that isn't controlled by S-mode will result in a lot less headaches...)
> 
> Thanks.

Indeed, strictly speaking, the execution of M-mode is also affected by srmcfg.
When dealing with multiple domains, attention needs to be paid to isolation and
context switching issues. I understand your point of view.

However, I wonder if it could be done in a later stage of the work, because there
is currently a draft specification smmtt[1] that is also related to this. This
extension involves multiple Supervisor Domains, and at this time, it is necessary
to implement context switching of srmcfg between different domains.

Thanks,
Pei

[1] https://github.com/riscv/riscv-smmtt

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

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

[Radim Krčmář]

2025-11-11T17:49:36+08:00, <cp0613@linux.alibaba.com>:
> On 2025-11-10 02:41 PM, Radim Krčmář wrote:
>> >                                         so there is no need to consider the execution
>> > situation in M-mode, it is only necessary to enable the relevant permissions.
>> 
>> The execution of M-mode is affected by srmcfg, so we have to be
>> extremely careful about isolation in opensbi too, especially when
>> juggling multiple domains.
>> 
>> (I think we might be fine now, but having opensbi switch to a srmcfg
>>  that isn't controlled by S-mode will result in a lot less headaches...)
>> 
>> Thanks.
>
> Indeed, strictly speaking, the execution of M-mode is also affected by srmcfg.
> When dealing with multiple domains, attention needs to be paid to isolation and
> context switching issues. I understand your point of view.
>
> However, I wonder if it could be done in a later stage of the work, because there
> is currently a draft specification smmtt[1] that is also related to this. This
> extension involves multiple Supervisor Domains, and at this time, it is necessary
> to implement context switching of srmcfg between different domains.

I agree that we can return to that issue later, as it's not clearly
buggy for now.  For this patch, I would just like to fix the condition.

Thanks.

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