Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v10 3/9] crypto: Add math to support fast NIST P384
To:     Vitaly Chikunov <vt@altlinux.org>
Cc:     Stefan Berger <stefanb@linux.vnet.ibm.com>,
        keyrings@vger.kernel.org, linux-crypto@vger.kernel.org,
        davem@davemloft.net, herbert@gondor.apana.org.au,
        dhowells@redhat.com, zohar@linux.ibm.com,
        linux-kernel@vger.kernel.org, patrick@puiterwijk.org,
        linux-integrity@vger.kernel.org,
        Saulo Alessandre <saulo.alessandre@tse.jus.br>
References: <20210305005203.3547587-1-stefanb@linux.vnet.ibm.com>
 <20210305005203.3547587-4-stefanb@linux.vnet.ibm.com>
 <20210306192553.lxy5w262g2vs2hvv@altlinux.org>
 <9ac456d7-a5e1-9e42-5d34-7dc1c70082e0@linux.ibm.com>
 <20210307000310.ytes6la73x5bkori@altlinux.org>
From:   Stefan Berger <stefanb@linux.ibm.com>
Message-ID: <340a3176-05a2-6a4a-7c19-6c2aa2667083@linux.ibm.com>
Date:   Sat, 6 Mar 2021 20:21:00 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <20210307000310.ytes6la73x5bkori@altlinux.org>
Content-Type: text/plain; charset=koi8-r; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 3/6/21 7:03 PM, Vitaly Chikunov wrote:
> Stefan,
>
> On Sat, Mar 06, 2021 at 06:29:18PM -0500, Stefan Berger wrote:
>> On 3/6/21 2:25 PM, Vitaly Chikunov wrote:
>>> On Thu, Mar 04, 2021 at 07:51:57PM -0500, Stefan Berger wrote:
>>>> From: Saulo Alessandre <saulo.alessandre@tse.jus.br>
>>>>
>>>> * crypto/ecc.c
>>>>     - add vli_mmod_fast_384
>>>>     - change some routines to pass ecc_curve forward until vli_mmod_fast
>>>>
>>>> * crypto/ecc.h
>>>>     - add ECC_CURVE_NIST_P384_DIGITS
>>>>     - change ECC_MAX_DIGITS to P384 size
>>>>
>>>> Signed-off-by: Saulo Alessandre <saulo.alessandre@tse.jus.br>
>>>> Tested-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> ---
>>>>    crypto/ecc.c | 266 +++++++++++++++++++++++++++++++++++++--------------
>>>>    crypto/ecc.h |   3 +-
>>>>    2 files changed, 194 insertions(+), 75 deletions(-)
>>>>
>>>> diff --git a/crypto/ecc.c b/crypto/ecc.c
>>>> index f6cef5a7942d..c125576cda6b 100644
>>>> --- a/crypto/ecc.c
>>>> +++ b/crypto/ecc.c
>>>> @@ -778,18 +778,133 @@ static void vli_mmod_fast_256(u64 *result, const u64 *product,
>>>>    ...
>>>>    /* Computes result = product % curve_prime for different curve_primes.
>>>>     *
>>>>     * Note that curve_primes are distinguished just by heuristic check and
>>>>     * not by complete conformance check.
>>>>     */
>>>>    static bool vli_mmod_fast(u64 *result, u64 *product,
>>>> -			  const u64 *curve_prime, unsigned int ndigits)
>>>> +			  const struct ecc_curve *curve)
>>>>    {
>>>>    	u64 tmp[2 * ECC_MAX_DIGITS];
>>>> +	const u64 *curve_prime = curve->p;
>>>> +	const unsigned int ndigits = curve->g.ndigits;
>>>> -	/* Currently, both NIST primes have -1 in lowest qword. */
>>>> -	if (curve_prime[0] != -1ull) {
>>>> +	/* Currently, all NIST have name nist_.* */
>>>> +	if (strncmp(curve->name, "nist_", 5) != 0) {
>>> I am not sure, but maybe this strncmp should not be optimized somehow,
>>> since vli_mmod_fast could be called quite frequently. Perhaps by integer
>>> algo id or even callback?
>> Should be optimized or should not be? You seem to say both.
> Excuse me for the typo. I meant "should be optimized". I think, maybe
> it's time to add algo selector id (for the case statement, for example
> instead of `switch (ndigits)') or just callback for a low level mmod
> function.
>
> If you think this would not impact performance then nevermind.

I think it would only be a few cycles. Of course we could introduce a 
flag to indicate nist functions (rather than using strncmp on the name) 
or work with the callbacks (only for the nist functions?) as you 
mentioned, but maybe that's something we could do after? Either way we 
would have to pass the ecc_curve pointer all the way into vli_mmod_fast. 
So this change here is preparing for this as well.

 ? Stefan