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

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

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