Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 28 May 2019 09:55:48 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Nikolay Borisov <nborisov@suse.com>
Cc:     herbert@gondor.apana.org.au, davem@davemloft.net,
        linux-crypto@vger.kernel.org, terrelln@fb.com, jthumshirn@suse.de
Subject: Re: [PATCH v2] crypto: xxhash - Implement xxhash support
Message-ID: <20190528165548.GC739@sol.localdomain>
References: <20190528121451.5954-1-nborisov@suse.com>
 <20190528152248.GB739@sol.localdomain>
 <e7f5ac70-ff22-4471-2682-52c8cd246bf0@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e7f5ac70-ff22-4471-2682-52c8cd246bf0@suse.com>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Tue, May 28, 2019 at 07:49:58PM +0300, Nikolay Borisov wrote:
> 
> 
> On 28.05.19 г. 18:22 ч., Eric Biggers wrote:
> > On Tue, May 28, 2019 at 03:14:51PM +0300, Nikolay Borisov wrote:
> >> xxhash is currently implemented as a self-contained module in /lib.
> >> This patch enables that module to be used as part of the generic kernel
> >> crypto framework. It adds a simple wrapper to the 64bit version.
> >>
> > 
> > Thanks, this looks a lot better.  A couple minor comments below.
> > 
> >> +static int xxhash64_init(struct shash_desc *desc)
> >> +{
> >> +	struct xxhash64_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
> >> +	struct xxhash64_desc_ctx *dctx = shash_desc_ctx(desc);
> >> +
> >> +	xxh64_reset(&dctx->xxhstate, tctx->seed);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int xxhash64_setkey(struct crypto_shash *tfm, const u8 *key,
> >> +			 unsigned int keylen)
> >> +{
> >> +	struct xxhash64_tfm_ctx *tctx = crypto_shash_ctx(tfm);
> >> +
> >> +	if (keylen != sizeof(tctx->seed)) {
> >> +		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
> >> +		return -EINVAL;
> >> +	}
> >> +	tctx->seed = get_unaligned_le64(key);
> >> +	return 0;
> >> +}
> > 
> > Can you please move xxhash64_setkey() to before xxhash64_init() to match the
> > order in which the functions get called?
> > 
> > Sometimes people get confused and think that crypto_shash_init() comes before
> > crypto_shash_setkey(), so it's helpful to keep definitions in order.
> > 
> >> +module_init(xxhash_mod_init);
> > 
> > Can you change this to subsys_initcall?  We're using subsys_initcall for the
> > generic implementations of crypto algorithms now, so that when other
> > implementations (e.g. assembly language implementations) are added, the crypto
> > self-tests can compare them to the generic implementations.
> 
> Will fix those but wanted to ask you whether it's really necessary to
> use get_unaligned in setkey given generic code guarantees the buffer is
> going to be aligned. E.g. wouldn't cpu_to_le64(*(u64 *)key)) be
> "cheaper"? In any case this is a minor point but just want to be sure I
> have correctly understood the generic code?

The generic code only aligns the buffer to the .cra_alignmask set in the
shash_alg, which in this case is 0, so the buffer is not aligned.

I don't recommend setting an alignmask, since it's more efficient to use the
unaligned access helpers.  The code in lib/xxhash.c already uses them.

- Eric