Re: [PATCH] perf: RISC-V: fix IRQ detection on T-Head C908

From: Atish Patra
Date: Tue Mar 19 2024 - 16:11:41 EST

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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-riscv