Re: [PATCH 1/3] RISC-V: add Bitmanip/Scalar Crypto parsing from DT

[Conor Dooley <conor@kernel.org>]

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

On Wed, Jun 28, 2023 at 06:24:40PM +0100, Conor Dooley wrote:
> On Wed, Jun 28, 2023 at 10:18:34AM -0700, Evan Green wrote:
> > On Wed, Jun 28, 2023 at 4:10 AM Conor Dooley <conor.dooley@microchip.com> wrote:
> > >
> > > On Wed, Jun 28, 2023 at 12:01:11PM +0200, Samuel Ortiz wrote:
> > > > On Tue, Jun 27, 2023 at 07:48:15PM +0100, Conor Dooley wrote:
> > > > > On Tue, Jun 27, 2023 at 11:14:30AM -0700, Evan Green wrote:
> > > > > > On Tue, Jun 27, 2023 at 7:38 AM Samuel Ortiz <sameo@rivosinc.com> wrote:
> > >
> > > > > > It would be nice to consolidate the ones together that search for a
> > > > > > single string and set multiple bits, though I don't have any super
> > > > > > elegant ideas for how off the top of my head.
> > > > >
> > > > > I've got a refactor of this code in progress, dropping all of these
> > > > > copy-paste in place of a loop. It certainly looks more elegant than
> > > > > this, but it will fall over a bit for these "one string matches many
> > > > > extensions" cases. See here:
> > > > > https://patchwork.kernel.org/project/linux-riscv/patch/20230626-thieving-jockstrap-d35d20b535c5@wendy/
> > > > > My immediate thought is to add another element to riscv_isa_ext_data,
> > > > > that contains "parent" extensions to check for. Should be fairly doable,
> > > > > I'll whip something up on top of that...
> > > >
> > > > Nice, and thanks for the review.
> > >
> > > > Should I wait for your refactor to be merged before pushing this one?
> > >
> > > I don't know. I think that you should continue on with your series here,
> > > and whichever goes in second gets rebased on top of the other.
> > > I don't think it makes material difference to review of this patchset as
> > > to whether you rebase on top of what I'm working on, so I wouldn't
> > > bother until it gets merged.
> > >
> > > Rather hacky, had less time than expected this morning:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-extensions-strings-supersets
> > > Clearly there's issues with looping to RISCV_ISA_MAX_SUPERSETS & I just
> > > repurposed Zicsr for the sake of testing something in the time I had.
> > >
> > > Evan, at a high level, does that look more elegant to you, or have I made
> > > things worse?
> > >
> > 
> > I see what you're going for at least. It's unfortunate that when
> > someone bumps up RISCV_ISA_MAX_SUPERSETS it squares the whole array.
> > Another way to go might be to define the elements in a separate array,
> > like:
> > 
> > unsigned int riscv_zks_exts[] = {
> >        RISCV_ISA_EXT_ZBKB,
> >        RISCV_ISA_EXT_ZBKC,
> >        ....
> > };
> > 
> > then the macro entry looks like:
> > 
> > SET_ISA_EXT_MAP_MULTI("zks", riscv_zks_exts),
> > 
> > where the SET_ISA_EXT_MAP_MULTI() could use ARRAY_SIZE() to stash both
> > the pointer to the array and the number of elements.
> 
> Yup, I like the sound of that. I like the variadic stuff as it'd not
> require defining a bunch of sub-arrays of supersets. I guess if it grows
> too badly, we can just dump it off into another file or w/e.

Also, I realised the other day that I had a bug in my series - I was using
"name" to read the property, not "property", which is what required the
extra "supersets" property.
The simplest thing to do actually seems to be to expand the "property"
member to an array of strings named "properties", rather than
introducing a "supersets" or similar.

Perhaps I am forgetting a good reason for why I had it split, but I'll
give it a whirl and see what I think...

Cheers,
Conor.


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