Message-ID: <f7a44b1a-a53e-4a43-aa94-d6fddbed4b81@rivosinc.com>
Date: Tue, 19 Mar 2024 13:08:07 -0700
From: Atish Patra <atishp@...osinc.com>
To: Conor Dooley <conor@...nel.org>, Guo Ren <guoren@...nel.org>
Cc: Andrew Jones <ajones@...tanamicro.com>,
 Inochi Amaoto <inochiama@...look.com>, Qingfang Deng <dqfext@...il.com>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Atish Patra <atishp@...shpatra.org>, Anup Patel <anup@...infault.org>,
 Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Conor Dooley <conor.dooley@...rochip.com>, Heiko Stuebner <heiko@...ech.de>,
 Guo Ren <guoren@...nel.org>, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] perf: RISC-V: fix IRQ detection on T-Head C908

On 3/19/24 02:06, Conor Dooley wrote:
> On Mon, Mar 18, 2024 at 05:48:13PM -0700, Atish Patra wrote:
>> On 3/18/24 16:48, Conor Dooley wrote:
>>> On Mon, Mar 18, 2024 at 03:46:54PM -0700, Atish Patra wrote:
> 
>>>> For 2.b, either we can start defining pseudo extensions or adding
>>>> vendor/arch/impid checks.
>>>>
>>>> @Conor: You seems to prefer the earlier approach instead of adding the
>>>> checks. Care to elaborate why do you think that's a better method compared
>>>> to a simple check ?
>>>
>>> Because I don't think that describing these as "errata" in the first
>>> place is even accurate. This is not a case of a vendor claiming they
>>> have Sscofpmf support but the implementation is flawed. As far as I
>>> understand, this is a vendor creating a useful feature prior to the
>>> creation of a standard extension.
>>> A bit of a test for this could be "If the standard extension never
>>> existed, would this be considered a new feature or an implementation
>>> issue". I think this is pretty clearly in the former camp.
>>>
>>
>> So we have 3 cases.
>>
>> 1. Pseudo extension: An vendor extension designed and/or implemented before
>> the standard RVI extension was ratified but do not violate any standard
>> encoding space.
>>
>> 2. Erratas: An genuine bug/design issue in the expected behavior from a
>> standard RVI extension (including violating standard encoding space)
>>
>> 3. Vendor extension: A new or a variant of standard RVI extension which is
>> different enough from standard extension.
>>
>> IMO, the line between #2 and #1 may get blurry as we going forward because
>> of the sheer number of small extensions RVI is comping up with (which is a
>> problem as well).
> 
> Aye, I think some of that is verging on ridiculous.
> 
>> Just to clarify: I am not too worried about this particular case as we know
>> that T-head's implementation predates the Sscofpmf extension.
>> But once we define a standard mechanism for this kind of situation, vendor
>> may start to abuse it.
> 
> How do you envisage it being abused by a vendor?
> Pre-dating the standard extension does make this one fairly clear-cut,
> but are you worried about people coming along and claiming to implement
> XConorSscofpmf instead of Sscofpmf rather than suffer the "shame" of a
> broken implementation?

Yes or just use this excuse continue their old implementation in a newer 
chip which was designed after the standard extension is ratified.


> All this stuff is going to be pretty case-by-case (to begin with at
> least) so I'm not too worried about that sort of abuse.
> 
>>> I do not think we should be using m*id detection implementations of a
>>> feature prior to creation of a standard extension for the same purpose.
>>> To me the main difference between a case like this and VentanaCondOps/Zicond
>>> is that we are the ones calling this an extension (hence my use of pseudo)
>>> and not the vendor of the IP. If T-Head were to publish a document tomorrow
>>> on the T-Head github repo for official vendor extensions, that difference
>>> would not even exist any longer.
>>>
>>
>> Exactly! If vendor publishes these as an extension or an errata, that's a
>> binding agreement to call it in a specific way.
> 
> I don't agree that we are bound to call it the way that the vendor does.
> We should just review these sorts of things on a case-by-case basis,
> committing to doing what the vendor says is abusable.
> 
>>> I also do not believe that it is a "simple" check. The number of
>>> implementations that could end up using this PMU could just balloon
>>> if T-Head has no intention of switching to Sscofpmf. If they don't
>>> balloon in this case, there's nothing stopping them ballooning in a
>>
>> Ideally, they shouldn't as it a simple case of CSR number & IRQ number.
>> If they care to implement AIA, then they must change it to standard sscofpmf
>> as the current IRQ violates the AIA spec. But who knows if they care to
>> implement AIA or not.
> 
> What kinda "worried" me here is that the c908 implements /both/ Zicbom
> and the T-Head CMO instructions and /both/ Svpbmt and their original
> misuse of the reserved bits but they do not support Sscofpmf. Maybe it
> just was not feasible to migrate entirely (but they did for vector) or
> to support both interrupt numbers and to alias the CSR, but it seemed
> like the opportunity to standardise a bunch of other stuff was taken,
> but this particular extension was not. That's why I was worried that
> we'd see some ballooning in these specific checks.
> 


This is the exact abuse I was talking about. A vendor can keep churning 
new chips with incompatible extensions because Linux just allowed them 
get away with it by calling it a Vendor Extension.

+Guo: In case he has any insight behind this decision from Thead.

>>> similar case in the future. We should let the platform firmware tell
>>> explicitly, be that via DT or ACPI, what features are supported rather
>>> than try to reverse engineer it ourselves via m*id.
>>>
>> Fair enough.
>>
>>
>>> That leads into another general issue I have with using m*id detection,
>>> which I think I have mentioned several times on the list - it prevents the
>>> platform (hypervisor, emulator or firmware) from disabling that feature.
>>>
>>
>> If that is the only concern, platform can just disable the actual
>> extension(i.e. sscofpmf in this case) to disable that feature for that
>> particular vendor.
> 
> Right. Maybe I wasn't clear that this is a problem with using m*id for
> /detection/ of extensions and not with using m*id to work around
> implementation issues with the extension. In the latter case, you're
> applying a fixup only when the actual extension is communicated to be
> present, which leaves that control in the hands of the platform.
> 

Hmm. I guess there is no way predict how vendors will include standard 
extensions. Let's give them a benefit of doubt hoping they realize 
abusing the system won't help anybody. They will try to adopt the 
standard extensions if it is possible and creates erratas for genuine 
problems.

>>> If I had a time machine back to when the T-Head perf or cmo stuff was
>>> submitted, I was try to avoid any of it being merged with the m*id
>>> detection method.
>>>
>>>> I agree that don't have the crystal ball and may be proven wrong in the
>>>> future (I will be definitely happy about that!). But given the diversity of
>>>> RISC-V ecosystem, I feel that may be our sad reality.
>>>
>>> I don't understand what this comment is referring to, it lacks context
>>> as to what the sad reality actually is.
>>>
>>> I hope that all made sense and explained why I am against this method
>>> for detecting what I believe to be features rather than errata,
>>> Conor.
>>>
>>
>> Yes.Thanks again for the clarification. Again, I am not opposed to the idea.
>> I just wanted to understand if this is the best option we have right now.
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists