Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3300 bytes --]

On Sun, Feb 18, 2024 at 09:00:14AM -0600, Samuel Holland wrote:
> >>>> Or maybe we can still with the properties you have, but instead of
> >>>> treating them like any other extension, handle these separately,
> >>>> focusing on the numbering, so that only having the exact version
> >>>> supported by a cpu is possible.
> >>>
> >>> Maybe I'm misunderstanding what you're saying here, but it is already the case
> >>> that the DT for a CPU would only contain the exact version of the privileged ISA
> >>> supported by that CPU.
> >>
> >> If privileged spec versions are boolean extensions, then you would say "ss1p11",
> >> "ss1p12", "ss1p13" as separate/simultaneous extensions.
> > 
> >> This is needed in order
> >> to allow simple support checks as described in the riscv,isa-extensions cover
> >> letter.
> > 
> > Yes, it is explicitly a goal of riscv,isa-extensions that you can look
> > for a specific extension in the list if that is all you care about. If
> > you go and drop ss1p11 because you support ss1p12 it defeats that.
> 
> Okay, that makes sense, but that is not how the parsing code works right now,
> which is probably what led me down the wrong path. :)
> 
> To have the intended semantics, we cannot parse _anything_ in
> riscv,isa-extensions as a "bundled" or "superset" extension.

That's not true I don't think. You can parse as a "bundle" but...

> Because to
> accomplish your goal, each extension in the bundle must be listed explicitly, in
> case the consumer only cares about that one extension.

...as you note here, the extensions also have to be listed explicitly so
that they can be detected in isolation if that is all that a consumer
does.

> So it sounds like
> riscv_fill_hwcap_from_ext_list() needs to ignore subset_ext_size/subset_ext_ids.

Do you mean this:
if (ext->subset_ext_size) {
	for (int j = 0; j < ext->subset_ext_size; j++) {
		if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
			set_bit(ext->subset_ext_ids[j], isainfo->isa);
	}
}

I think that is fine? If you find the "superset" you can enable the
individual elements. The problem would just be if someone put only the
superset in a DT (or ACPI tables) and the software looked for the
individual element only, but IIRC the kernel currently checks both the
superset and individual elements.
It would be possible to check a bundle and then skip looking for the
individual elements if the bundle was already found if the parsing is
wont to be sped up.

I think all we need to do is enforce that all individual elements are
present on a schema validation level (I have no clue what we can do
for ACPI) and no change is required in the kernel.

Am I misunderstanding what you think is the problem here?

> > I don't know off the top of my head how to enforce ss1p12 requiring ss1p11
> > in json schema, but I do have an idea of where to start...
> 
> Yeah, this is different from normal "dependencies:" because it is a string list.

I think it is actually doable, just will look sorta clunky. I meant to
go and do it this weekend, but I've been rather sick unfortunately.
Something similar is definitely doable for compatibles, so either it'll
"just work" or I may have to extend the validation tooling.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]