Re: [PATCH v2 02/19] riscv: add ISA extension parsing for scalar crypto

["Clément Léger" <cleger@rivosinc.com>]

On 24/10/2023 18:37, Evan Green wrote:
> On Tue, Oct 24, 2023 at 2:30 AM Clément Léger <cleger@rivosinc.com> wrote:
>>
>>
>>
>> On 24/10/2023 09:18, Clément Léger wrote:
>>>
>>>
>>> On 23/10/2023 18:21, Evan Green wrote:
>>>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote:
>>>>>
>>>>> From: Evan Green <evan@rivosinc.com>
>>>>>
>>>>> The Scalar Crypto specification defines Zk as a shorthand for the
>>>>> Zkn, Zkr and Zkt extensions. The same follows for both Zkn, Zks and Zbk,
>>>>> which are all shorthands for various other extensions. The detailed
>>>>> breakdown can be found in their dt-binding entries.
>>>>>
>>>>> Since Zkn also implies the Zbkb, Zbkc and Zbkx extensions, simply passing
>>>>> "zk" through a DT should enable all of Zbkb, Zbkc, Zbkx, Zkn, Zkr and Zkt.
>>>>> For example, setting the "riscv,isa" DT property to "rv64imafdc_zk"
>>>>> should generate the following cpuinfo output:
>>>>> "rv64imafdc_zicntr_zicsr_zifencei_zihpm_zbkb_zbkc_zbkx_zknd_zkne_zknh_zkr_zkt"
>>>>>
>>>>> riscv_isa_ext_data grows a pair of new members, to permit setting the
>>>>> relevant bits for "bundled" extensions, both while parsing the ISA string
>>>>> and the new dedicated extension properties
>>>>>
>>>>> Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
>>>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>>>> Signed-off-by: Evan Green <evan@rivosinc.com>
>>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>>
>>>> My tree might be out of sync, but in my search for riscv_isa_ext, I
>>>> also found a use in print_isa() (cpu.c) where we're reaching into
>>>> riscv_isa_ext[].id and assuming it's always valid. If that's still in
>>>> there we'll want to fix up that spot too, since now with bundles .id
>>>> may or may not be valid.
>>>
>>> Oh indeed, the array is visible outside of this compilation unit :/.
>>> I'll check that before sending V3.
>>
>> After looking a bit more at that, it actually seems that id is used in
>> cpuinfo to determine which extensions are present which means you are
>> right, bundle_size needs to be accounted for.
>>
>> Looking at it also raises the question (again) of exposing the "bundles"
>> extensions themselves or not in cpuinfo output. With the current setup,
>> the bundles extensions won't be visible in cpuinfo output. For instance
>> if Zk was in the isa string, then it will not be visible in the cpuinfo
>> output, only the child extensions. One solution would be to always have
>> a valid id for each extension. So we would have one for Zk for instance.
>>
>> We would then have a similar setup for all "bundles" or "subset"
>> extensions, they would have a id for all of them. For instance, Zk would
>> become:
>>
>> __RISCV_ISA_EXT_DATA_BUNDLE(zk, RISCV_ISA_EXT_ZK, riscv_zk_bundled_exts)
>>
>> Same would go for zvbb (riscv_zvbb_subset_exts would only contain Zvkb):
>>
>> __RISCV_ISA_EXT_DATA_BUNDLE(zk, RISCV_ISA_EXT_ZVBB, riscv_zvbb_subset_exts)
>>
>> For the sake of completeness, I feel like it would be good to have all
>> the extensions (bundles or not) visible in the riscv_isa_ext.
>>
>> Any objection ?
> 
> I could be persuaded that it's a good idea, but there are arguments to
> be made for not defining them as separate bits:
> 
> 1. Having two (sets of) bits that mean the same thing means usermode
> now has to decide which one to query, or query both. Multiple values
> that mean the same thing is always something that makes me nervous.

That is indeed an acceptable cons.

> 2. To avoid these two sets having different answers, we'd have to
> solve the reverse problem too: If all of the bundled extensions that
> make up Zk are on, we'd need to detect that and turn Zk on as well.

Oh yes, sorry, already forgot that point :/ Well, I guess things sorted
out by themselves. Let's do what you proposed:

- Pure lasso extensions (Zk) will simply result in all the sub
extensions being enable, there won't be any .id specified for these
ones, simply a bundle of extensions.

- "Superset" extensions (Zvbb for instance) will have their own .id as
well as a bundle made of subsets extensions.

Clément

> That code would also need to know the difference between a pure lasso
> like Zk, where you should flip it on if its components are on, and the
> loose change variant we were discussing on the other thread (Zvkb?),
> where you cannot.
> 
> Pretending pure lasso extensions didn't exist on their own was a way
> to sidestep the problem.
> -Evan