linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Atish Patra <atishp@rivosinc.com>
To: Conor Dooley <conor@kernel.org>, Andrew Jones <ajones@ventanamicro.com>
Cc: Inochi Amaoto <inochiama@outlook.com>,
	Qingfang Deng <dqfext@gmail.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Atish Patra <atishp@atishpatra.org>,
	Anup Patel <anup@brainfault.org>, Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Conor Dooley <conor.dooley@microchip.com>,
	Heiko Stuebner <heiko@sntech.de>, Guo Ren <guoren@kernel.org>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] perf: RISC-V: fix IRQ detection on T-Head C908
Date: Tue, 19 Mar 2024 13:11:30 -0700	[thread overview]
Message-ID: <2b002585-eec9-40d3-95bc-e010b7def7a9@rivosinc.com> (raw)
In-Reply-To: <20240319-underfoot-dispense-dc30ea860e7c@spud>

On 3/19/24 08:36, Conor Dooley wrote:
> On Tue, Mar 19, 2024 at 02:39:21PM +0100, Andrew Jones wrote:
>> On Tue, Mar 19, 2024 at 09:06:34AM +0000, 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.
>>
>> The vendor should name these extensions themselves.
>>
>>>>
>>>> 2. Erratas: An genuine bug/design issue in the expected behavior from a
>>>> standard RVI extension (including violating standard encoding space)
>>
>> More on this below, but I think vendors should name these too.
> 
> Yah, both of these the vendor /should/ name it themselves but I don't
> want some set in stone /must/ that locks someone who is not the vendor
> from upstreaming.
> 
>>>> 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).
>>
>> The line between #1 and #2 is blurry because the only difference is the
>> original intentions. The end result is that a vendor has implemented
>> something that resembles a standard extension, but is not the same as the
>> standard extension.
> 
> Aye, a large part of this is definitely based on intent. Or maybe
> marketing rather than intent, but the two aren't all that different
> as the public may not be privy to which it actually is.
> 
> I think you're missing a factor though - when the difference is
> discovered. Equating #1 and #2 is fine when that difference is known
> when the platform is originally supported, but if the divergence between
> what's implemented and the spec is only discovered down the line...
> 
>>> 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.
>>
>> Case-by-case is reasonable, since it's probably too strict to always
>> require new names. We can consider each proposed workaround as they
>> come, but it's a slippery slope once workarounds are accepted.
> 

May be we treat #1 and #3 are same. In this case, we just call it 
XTheadSscofpmf or something similar. The patches would look similar to 
what was proposed in Andes PMU v7 series in that case.

> ... I think that means that having some workarounds are inescapable
> really. Some sort of workaround could then be only way fix the problem
> without a firmware update. That workaround might be triggered by the
> m*id CSRs or it could be based on the firmware's SBI implemenation
> or version IDs.
> When it's something that never worked at all or was discovered early,
> then ye equate the two.
> 
>>>> 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?
>>
>> Other than the concern of the ballooning bitmap, I'd prefer this approach.
>> If a vendor has implemented some extension which happens to be "almost
>> Sscofpmf", then whether it was implemented before the Sscofpmf spec
>> existed, or after, isn't really important. What's important is that it's
>> only "almost Sscofpmf" and not _exactly_ Sscofpmf, which means it should
>> not use the Sscofpmf extension name.
> 
> One of the reasons I keep bringing up when things were created prior to
> the creation of Sscofpmf (and I guess the fact that the vendor never
> claims to support Sscofpmf) is to highlight that we are not looking at
> at one of these edge cases where we're only discovering that there's an
> implementation issue on these CPUs that we need to work around silently.
> 
>> Since vendors are allowed to create
>> their own XVendor names, then that shouldn't be a problem. Indeed, the
>> abuse concern seems to be in the opposite direction, that vendors will
>> try to pass off almost-standard extensions as standard extensions by
>> trying to get workarounds into Linux. Maybe Linux policy should be to
>> simply reject workarounds to extensions, requiring vendors to create new
>> names instead.
> 
> I'd be inclined to agree (although there's gonna be some exceptions as I
> mention above) - but it's easy for me to say that given I want people to
> use riscv,isa-extensions on the DT side which cites specific versions of
> extensions that a device is compliant with.
> If people have issues with riscv,isa in DT, I'm can take a bit of a "oh
> it doesn't work with riscv,isa? Tough shit, that's why we made the new
> properties." approach and push people into new names.
> 
> With ACPI, I have absolutely no idea how you police any of that. The way
> the code works rn is that both DT properties and ACPI share the exact
> same extension "names". Obviously it doesn't need to be that way, but it
> is handy. I'm kinda derailing here, but how would we map names to exact
> features in ACPI land? When I last read the ACPI stuff it was very
> non-specific and the kernel documentation
> (Documentation/riscv/acpi.rst) cites a specific version of the ISA
> manual that provides no information (and I guess never will?) about
> vendor extensions.
> 
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2024-03-19 20:11 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-11  6:30 [PATCH] perf: RISC-V: fix IRQ detection on T-Head C908 Qingfang Deng
2024-03-11  7:13 ` Inochi Amaoto
2024-03-11  7:56   ` Qingfang Deng
2024-03-12 14:07     ` Conor Dooley
2024-03-13  1:31       ` Inochi Amaoto
2024-03-14 20:41         ` Conor Dooley
2024-03-15  5:23           ` Inochi Amaoto
2024-04-12  6:27           ` Yangyu Chen
2024-04-12  7:40             ` Conor Dooley
2024-03-15  8:11         ` Andrew Jones
2024-03-15 12:22           ` Inochi Amaoto
2024-03-18 22:46           ` Atish Patra
2024-03-18 23:48             ` Conor Dooley
2024-03-19  0:48               ` Atish Patra
2024-03-19  9:06                 ` Conor Dooley
2024-03-19 13:39                   ` Andrew Jones
2024-03-19 15:36                     ` Conor Dooley
2024-03-19 20:11                       ` Atish Patra [this message]
2024-03-19 20:08                   ` Atish Patra
2024-04-12  6:09 ` Yangyu Chen
2024-04-17  6:29   ` Guo Ren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2b002585-eec9-40d3-95bc-e010b7def7a9@rivosinc.com \
    --to=atishp@rivosinc.com \
    --cc=ajones@ventanamicro.com \
    --cc=anup@brainfault.org \
    --cc=aou@eecs.berkeley.edu \
    --cc=atishp@atishpatra.org \
    --cc=conor.dooley@microchip.com \
    --cc=conor@kernel.org \
    --cc=dqfext@gmail.com \
    --cc=guoren@kernel.org \
    --cc=heiko@sntech.de \
    --cc=inochiama@outlook.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).