> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> Sent: Tuesday, July 30, 2019 3:43 PM
> To: Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx>
> Cc: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>; Pascal van Leeuwen
> <pascalvanl@xxxxxxxxx>; linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx
> Subject: Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
>
> Hi Pascal,
>
> On Tue, Jul 30, 2019 at 10:20:43AM +0000, Pascal Van Leeuwen wrote:
> > > On Fri, Jul 26, 2019 at 02:43:24PM +0200, Pascal van Leeuwen wrote:
> >
> > > Is there a reason to have this one linked to Marvell? Aren't there other
> > > EIP197 (or EIP97) engines not on Marvell SoCs? (I'm pretty sure I know
> > > at least one).
> > >
> > Yes, there is a very good reason. These flags control features that are
> > very specific to those three Marvell socs and have nothing to do with
> > 'generic' EIP97IES's, EIP197B's or EIP197D's (these are just high-level
> > marketing/sales denominators and do not cover all the intricate config
> > details of the actual delivery that the driver needs to know about, so
> > this naive approach would not be maintainable scaling to other devices)
> > Therefore, I wanted to make that abundantly clear, hence the prefix.
> > (Renaming them to the specific Marvell SoC names was another option,
> > if only I knew which engine ended up in which SoC ...)
> >
> > While there are many SoC's out there with EIP97 and EIP197 engines,
> > the driver in its current form will NOT work (properly) on them, for
> > that there is still much work to be done beyond the patches I already
> > submitted. I already have the implementation for that (you tested that
> > already!), but chopping it into bits and submitting it all will take
> > a lot more time. But you have to understand that, without that, it's
> > totally useless to either me or Verimatrix.
> >
> > TL;DR: These flags are really just for controlling anything specific
> > to those particular Marvell instances and nothing else.
>
> I had this driver running on another non-Marvell SoC with very minor
> modifications, so there's then at least one hardware which is similar
> enough. In this case I don't see why this should be named "Marvell".
>
Just because it more or less appeared to run, does NOT mean that it
actually works properly, reliably and efficiently across the board.
Trust me, this is my design and I know it inside out and there is NO
engine out there equal to any Marvell engine.
> What are the features specific to those Marvell SoC that won't be used
> in other integrations? I'm pretty sure there are common features between
> all those EIP97 engines on different SoCs.
>
- specific algorithms supported
- number of pipes present
- number of rings present
- number of ring interrupt controllers present
- full featured (TRC) versus simplified record cache (STRC)
- for TRC: size of cache admin & data RAMs, affects configuration
- size of various internal buffers which affects configuration
- width of the databus, which affects descriptor size
- workarounds needed for specific design bugs and/or limitations
- newer features unsupported by now old MRVL engines
That's just the low-hanging fruit. We've *never* delivered the same
engine configuration to different customers. It's always customized.
> > > > - switch (priv->version) {
> > > > - case EIP197B:
> > > > - dir = "eip197b";
> > > > - break;
> > > > - case EIP197D:
> > > > - dir = "eip197d";
> > > > - break;
> > > > - default:
> > > > + if (priv->version == EIP97IES_MRVL)
> > > > /* No firmware is required */
> > > > return 0;
> > > > - }
> > > > + else if (priv->version == EIP197D_MRVL)
> > > > + dir = "eip197d";
> > > > + else
> > > > + /* Default to minimum EIP197 config */
> > > > + dir = "eip197b";
> > >
> > > You're moving the default choice from "no firmware" to being a specific
> > > one.
> > >
> > The EIP97 being the exception as the only firmware-less engine.
> > This makes EIP197_DEVBRD fall through to EIP197B firmware until
> > my patches supporting other EIP197 configs eventually get merged,
> > after which this part will change anyway.
>
> We don't know when/in what shape those patches will be merged, so in
> the meantime please make the "no firmware" the default choice.
>
"No firmware" is not possible with an EIP197. Trying to use it without
loading firmware will cause it to hang, which I don't believe is what
you would want. So the alternative would be to return an error, which
is fine by me, so then I'll change it into that as default.
> > > > - /* Fallback to the old firmware location for the
> > > > + /*
> > > > + * Fallback to the old firmware location for the
> > >
> > > This is actually the expected comment style in net/ and crypto/. (There
> > > are other examples).
> > >
> > Not according to the Linux coding style (which only makes an exception
> > for /net) and not in most other crypto code I've seen. So maybe both
> > styles are allowed(?) and they are certainly both used, but this style
> > seems to be prevalent ...
>
> I agree having non-written rules is not good (I don't make them), but
> not everything is described in the documentation or in the coding style.
> I don't really care about the comment style when adding new ones, but
> those are valid (& recommended) in crypto/ and it just make the patch
> bigger.
>
Ah, ok, your point being not to touch an already existing comment.
I missed that tiny detail ... I guess that's sort of automatic out of
my desire for consistency. I'll try to suppress those urges :-)
> > > > /* For EIP197 set maximum number of TX commands to 2^5 = 32 */
> > > > - if (priv->version == EIP197B || priv->version == EIP197D)
> > > > + if (priv->version != EIP97IES_MRVL)
> > >
> > > I would really prefer having explicit checks here. More engines will be
> > > supported in the future and doing will help. (There are others).
> > >
> > Same situation as with the crypto mode: I know for a fact the EIP97
> > is the *only* configuration that *doesn't* need this code. So why
> > would I have a long list of configurations there (that will keep
> > growing indefinitely) that *do* need that code? That will for sure
> > not improve maintainability ...
>
> OK, I won't debate this for hours. At least add a comment, for when
> *others* will add support for new hardware (because that really is the
> point, *others* might update and modify the driver).
>
Sure, I can add some comments to clarify these.
> > > > @@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct
> safexcel_crypto_priv
> > > *priv)
> > > > for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
> > > > safexcel_algs[i]->priv = priv;
> > > >
> > > > - if (!(safexcel_algs[i]->engines & priv->version))
> > > > - continue;
> > >
> > > You should remove the 'engines' flag in a separate patch. I'm really not
> > > sure about this. I don't think all the IS EIP engines support the same
> > > sets of algorithms?
> > >
> > All algorithms provided at this moment are available from all engines
> > currently supported. So the whole mechanism, so far, is redundant.
> >
> > This will change as I add support for generic (non-Marvell) engines,
> > but the approach taken here is not scalable or maintainable. So I will
> > end up doing it differently, eventually. I don't see the point in
> > maintaining dead/unused/redundant code I'm about to replace anyway.
>
> But it's not done yet and we might discuss how you'll handle this. You
> can't know for sure you'll end up with a different approach.
>
We can discuss all we want, but the old approach for sure won't work
so there's no point in keeping those (effectively redundant up until
now, so why did they even exist?) 'engines' flags.
> At least remove this in a separate patch.
>
Ok, I can do that
> > > > + if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {
> > >
> > > You have extra parenthesis here.
> > >
> > Our internal coding style (as well as my personal preference)
> > actually mandates to put parenthesis around everything so expect
> > that to happen a lot going forward as I've been doing it like that
> > for nearly 30 years now.
> >
> > Does the Linux coding style actually *prohibit* the use of these
> > "extra" parenthesis? I don't recall reading that anywhere ...
>
> I don't know if this is a written rule (as many others), but you'll find
> plenty of examples of reviews asking not to have extra parenthesis.
>
I can remove them, I was just wondering if there was actually any
rationale for not wanting to have them (considering that that at least
seems far more error prone). I have this strange desire to want to
know WHY I have to do something before I do it.
Is it just to show off your intimate knowledge of C operator precedence
and associativity ;-) (which, incidentally, I'm not too familiar with)
> > > > + if (priv->version == EIP197_DEVBRD) {
> > >
> > > It seems to me this is mixing an engine version information and a board
> > > were the engine is integrated. Are there differences in the engine
> > > itself, or only in the way it's wired?
> > >
> > Actually, no. The way I see it, priv->version does not convey any engine
> > information, just integration context (i.e. a specific Marvell SoC or, in
> > this case, our FPGA dev board), see also my explanation at the beginning.
>
> So that's really the point here :) This variable was introduced to
> convey the engine information, not the way it is integrated. There are
> EIP97 engines not on Marvell SoC which will just work out of the box
>
No, they will not. Certainly not if the driver becomes more capable.
You are making (dangerous) assumptions you don't have the knowledge
to make. With all due respect, not your fault, you just can't know
as this is not public information.
> (or with let's say a one liner adding support for using a clock)
>
No, that's naive thought. Marvell configs, for example, have a pretty
extensive algorithm loadout and relatively large record cache RAMs
as well as internal buffer RAMs. That will cause problems with most
other engines being a subset of that (which may be reliability
problems you won't immediately notice(!) ...).
> And the version could be in both cases something like 'EIP97'.
>
It needs to be something unique. And that will very quickly become
overwhelming as you need to maintain lists of which unique identifier
has which specific features somewhere. Or have huge case and/or
if-else if statements all over the place.
While all required parameters needed to configure the engine and
the driver can be directly probed from said hardware (including the
difference between an EIP97 and an EIP197, the number of pipes,
rings, everything, all you need is a base address), so why bother?
The only place where this 'version' can still be useful is to
distinguish the *platform*, which may need some specific initialization
(e.g. platform clocks, power domains, interrupt controllers and such).
> > Conveying engine information through a simple set of flags or some
> > integer or whatever is just not going to fly. No two of our engines
> > are ever the same, so that would quickly blow up in your face.
>
> Well, you have more info about this than I do, I can only trust you on
> this (it's just weird based on the experience I described just before,
> it seems to me the differences are not that big, but again, you know the
> h/w better).
>
Knowing the hardware better is actually a huge understatement, being the
responsible architect and lead designer :-) I specified and implemented
most of the stuff you are talking from the driver with my own two hands.
> I just don't want to end up with:
>
> if (version == EIP97_MRVL || version == EIP97_XXX || ...)
>
That's an interesting remark as that is exactly what I am trying to
*avoid* through the way I'm using the version field and, for me,
contradicts some comments you had earlier?
Oh well, I'll take this statement as confirmation we're more or less
on the same page here.
> > > We had this discussion on the v1. Your point was that you wanted this
> > > information to be in .data. One solution I proposed then was to use a
> > > struct (with both a 'version' and a 'flag' variable inside) instead of
> > > a single 'version' variable, so that we still can make checks on the
> > > version itself and not on something too specific.
> > >
> > As a result of that discussion, I kept your original version field
> > as intact as I could, knowing what I know and where I want to go.
> >
> > But to truly support generic engines, we really need all the stuff
> > that I added. Because it simply has that many parameters that are
> > different for each individual instance. But at the same time these
> > parameters can all be probed from the hardware directly, so
> > maintaining huge if statements all over the place decoding some
> > artificial version field is not the way to go (not maintainable).
> > Just probe all the parameters from the hardware and use them
> > directly where needed ... which my future patch set will do.
>
> OK, I do think this would be a good solution :)
>
I hope so ... as you're not exactly easy to convince and I can't
afford to keep spending this much effort on this for much longer
without getting in trouble with my dayjob.
> Thanks!
> Antoine
>
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Thanks,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
