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

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

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 ?

Clément

> 
> Clément
> 
>>
>> -Evan