Received-SPF: pass (google.com: domain of linux-crypto+bounces-2470-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sun, 3 Mar 2024 12:05:11 +0100
From: Lukas Wunner <lukas@wunner.de>
To: Stefan Berger <stefanb@linux.ibm.com>
Cc: keyrings@vger.kernel.org, linux-crypto@vger.kernel.org,
	herbert@gondor.apana.org.au, davem@davemloft.net,
	linux-kernel@vger.kernel.org, saulo.alessandre@tse.jus.br
Subject: Re: [PATCH v4 04/12] crypto: ecc - Implement vli_mmod_fast_521 for
 NIST p521
Message-ID: <20240303110511.GA394@wunner.de>
References: <20240301022007.344948-1-stefanb@linux.ibm.com>
 <20240301022007.344948-5-stefanb@linux.ibm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240301022007.344948-5-stefanb@linux.ibm.com>
User-Agent: Mutt/1.10.1 (2018-07-13)

On Thu, Feb 29, 2024 at 09:19:59PM -0500, Stefan Berger wrote:
> +static void vli_mmod_fast_521(u64 *result, const u64 *product,
> +				const u64 *curve_prime, u64 *tmp)
> +{
> +	const unsigned int ndigits = 9;
> +	size_t i;
> +
> +	for (i = 0; i < ndigits; i++)
> +		tmp[i] = product[i];
> +	tmp[8] &= 0x1ff;

Hm, the other vli_mmod_fast_*() functions manually unroll those loops.
Wondering if that would make sense here as well?  It's also possible
to tell gcc to unroll a loop with a per-function...

    __attribute__((optimize("unroll-loops")))

...but I'm not sure about clang portability.


> @@ -941,6 +966,12 @@ static bool vli_mmod_fast(u64 *result, u64 *product,
> +	case 9:
> +		if (!strcmp(curve->name, "nist_521")) {
> +			vli_mmod_fast_521(result, product, curve_prime, tmp);
> +			break;
> +		}
> +		fallthrough;

If you reorder patch 4 and 5, you could check for curve->nbits == 521 here,
which might be cheaper than the string comparison.


> -#define ECC_MAX_DIGITS              (512 / 64) /* due to ecrdsa */
> +#define ECC_MAX_DIGITS              (576 / 64) /* due to NIST P521 */

Maybe DIV_ROUND_UP(521, 64) for clarity?

Thanks,

Lukas