Re: [PATCH 06/12] RISC-V: crypto: add accelerated AES-CBC/CTR/ECB/XTS implementations

[Andy Chiu]

Hi Eric,

On Thu, Nov 9, 2023 at 3:16 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> On Tue, Nov 07, 2023 at 04:53:13PM +0800, Jerry Shih wrote:
> > On Nov 2, 2023, at 13:16, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> > > On Thu, Oct 26, 2023 at 02:36:38AM +0800, Jerry Shih wrote:
> > >> +static int ecb_encrypt(struct skcipher_request *req)
> > >> +{
> > >> +  struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> > >> +  const struct riscv64_aes_ctx *ctx = crypto_skcipher_ctx(tfm);
> > >> +  struct skcipher_walk walk;
> > >> +  unsigned int nbytes;
> > >> +  int err;
> > >> +
> > >> +  /* If we have error here, the `nbytes` will be zero. */
> > >> +  err = skcipher_walk_virt(&walk, req, false);
> > >> +  while ((nbytes = walk.nbytes)) {
> > >> +          kernel_vector_begin();
> > >> +          rv64i_zvkned_ecb_encrypt(walk.src.virt.addr, walk.dst.virt.addr,
> > >> +                                   nbytes & AES_BLOCK_VALID_SIZE_MASK,
> > >> +                                   &ctx->key);
> > >> +          kernel_vector_end();
> > >> +          err = skcipher_walk_done(
> > >> +                  &walk, nbytes & AES_BLOCK_REMAINING_SIZE_MASK);
> > >> +  }
> > >> +
> > >> +  return err;
> > >> +}
> > >
> > > There's no fallback for !crypto_simd_usable() here.  I really like it this way.
> > > However, for it to work (for skciphers and aeads), RISC-V needs to allow the
> > > vector registers to be used in softirq context.  Is that already the case?
> >
> > The kernel-mode-vector could be enabled in softirq, but we don't have nesting
> > vector contexts. Will we have the case that kernel needs to jump to softirq for
> > encryptions during the regular crypto function? If yes, we need to have fallbacks
> > for all algorithms.
>
> Are you asking what happens if a softirq is taken while the CPU is between
> kernel_vector_begin() and kernel_vector_end()?  I think that needs to be
> prevented by making kernel_vector_begin() and kernel_vector_end() disable and
> re-enable softirqs, like what kernel_neon_begin() and kernel_neon_end() do on
> arm64.  Refer to commit 13150149aa6ded which implemented that behavior on arm64.

Yes, if making Vector available to softirq context is a must, then it
is reasonable to call local_bh_disable() in kernel_vector_begin().
However, softirq would not be the only user for Vector and disabling
it may cause extra latencies. Meanwhile, simply disabling bh in
kernel_vector_begin() will conflict with the patch[1] that takes an
approach to run Preemptible Vector. Though it is not clear yet on
whether we should run Vector without turning off preemption, I have
tested running preemptible Vector and observed some latency
improvements without sacrificing throughput. We will have a discussion
on LPC2023[2] and it'd be great if you could join or continue to
discuss it here.

Approaches can be done such as nesting, if running Vector in softirq
is required. Since it requires extra save/restore on nesting, I think
we should run some tests to get more performance (latency/throughput)
figure let the result decide the final direction. For example, we
could run Vector in either nesting with preempt-V and  non-nesting
without preempt-V and compare the following performance catachristics:
 - System-wide latency impact
 - Latency and throughput of softirq-Vector itself

>
> - Eric

 - [1] https://lore.kernel.org/all/20231019154552.23351-6-andy.chiu@xxxxxxxxxx/
 - [2] https://lpc.events/event/17/contributions/1474/

Regard,
Andy