crypto: doc - clarify hash callbacks state machine
diff mbox

Message ID 20180305103945.3517-1-horia.geanta@nxp.com
State Changes Requested
Delegated to: Herbert Xu
Headers show

Commit Message

Horia Geanta March 5, 2018, 10:39 a.m. UTC
Even though it doesn't make too much sense, it is perfectly legal to:
- call .init() and then (as many times) .update()
- subseqently _not_ call any of .final(), .finup() or .export()

Update documentation since this is an important issue to consider
from resource management perspective.

Link: https://lkml.kernel.org/r/20180222114741.GA27631@gondor.apana.org.au
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
 Documentation/crypto/devel-algos.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Herbert Xu March 16, 2018, 3:16 p.m. UTC | #1
On Mon, Mar 05, 2018 at 12:39:45PM +0200, Horia Geantă wrote:
> Even though it doesn't make too much sense, it is perfectly legal to:
> - call .init() and then (as many times) .update()
> - subseqently _not_ call any of .final(), .finup() or .export()

Actually it makes perfect sense, because there can be an arbitrary
number of requests for a given tfm.  There is no requirement that
you must finalise the first request before submitting new ones.

IOW there can be an arbitrary number of outstanding requests even
without the user intentionally abandoning any hash request.

So please modify your commit description.

Thanks,
Horia Geanta March 19, 2018, 6:39 a.m. UTC | #2
On 3/16/2018 5:16 PM, Herbert Xu wrote:
> On Mon, Mar 05, 2018 at 12:39:45PM +0200, Horia Geantă wrote:
>> Even though it doesn't make too much sense, it is perfectly legal to:
>> - call .init() and then (as many times) .update()
>> - subseqently _not_ call any of .final(), .finup() or .export()
> 
> Actually it makes perfect sense, because there can be an arbitrary
> number of requests for a given tfm.  There is no requirement that
> you must finalise the first request before submitting new ones.
> 
> IOW there can be an arbitrary number of outstanding requests even
> without the user intentionally abandoning any hash request.
> 
The fact that there can be multiple requests in parallel (for a given tfm) is a
different topic.
Each request object has its state in its own state machine, independent from the
other request objects.
I assume this is clear enough.

Why I wanted to underline is that "abandoning" a hash request is allowed (even
though doing this is at least questionable), thus implementations must take
special care not to leak resources in this case.

If you think the commit message should be updated, then probably so should the
documentation update.

Thanks,
Horia
Herbert Xu March 19, 2018, 9:24 a.m. UTC | #3
On Mon, Mar 19, 2018 at 06:39:50AM +0000, Horia Geantă wrote:
>
> The fact that there can be multiple requests in parallel (for a given tfm) is a
> different topic.
> Each request object has its state in its own state machine, independent from the
> other request objects.
> I assume this is clear enough.

My point is that all of the state associated with a request needs
to be stored in the request object.  If you're start storing things
in the driver/hardware, then things will get ugly one way or another.

Cheers,
Horia Geanta March 19, 2018, 11:04 a.m. UTC | #4
On 3/19/2018 11:25 AM, Herbert Xu wrote:
> On Mon, Mar 19, 2018 at 06:39:50AM +0000, Horia Geantă wrote:
>>
>> The fact that there can be multiple requests in parallel (for a given tfm) is a
>> different topic.
>> Each request object has its state in its own state machine, independent from the
>> other request objects.
>> I assume this is clear enough.
> 
> My point is that all of the state associated with a request needs
> to be stored in the request object.  If you're start storing things
> in the driver/hardware, then things will get ugly one way or another.
> 
Agree, the request state should be stored in the request object; I am not
debating that.

Still there are limitations even when keeping state in the request object.
For e.g. an implementation cannot DMA map a buffer for the entire lifetime of a
request object, because this lifetime is unknown - user can "abandon" the object
after a few .update() calls, or even after .init(). By "abandon" I mean not call
_ever_ any of .final(), .finup() or .export() on the object.

The only solution to avoid leaks in this case is to repeatedly DMA map & unmap
the buffer.
IOW, if one wants to load/save HW state in a buffer after an .update() and to
instruct the crypto engine to do this operation, the following steps are involved:
-gpp: DMA map the buffer, get its IOVA
-gpp: program the crypto engine with IOVA, wait for crypto engine's signal
-crypto engine: load HW state from buffer, perform the partial hash, save HW
state in buffer, signal gpp
-gpp: DMA unmap the buffer

I'd say this is pretty inefficient, yet I don't see an alternative.

Good or bad, the documentation should reflect this limitation - hence this patch.

Thanks,
Horia
Herbert Xu March 19, 2018, 11:33 p.m. UTC | #5
On Mon, Mar 19, 2018 at 11:04:24AM +0000, Horia Geantă wrote:
>
> The only solution to avoid leaks in this case is to repeatedly DMA map & unmap
> the buffer.
> IOW, if one wants to load/save HW state in a buffer after an .update() and to
> instruct the crypto engine to do this operation, the following steps are involved:
> -gpp: DMA map the buffer, get its IOVA
> -gpp: program the crypto engine with IOVA, wait for crypto engine's signal
> -crypto engine: load HW state from buffer, perform the partial hash, save HW
> state in buffer, signal gpp
> -gpp: DMA unmap the buffer

What buffer are you talking about here? Is it the hash state?

If it's the hash state and assuming DMA mapping was slow enough
on your platform, you could solve it by maintaining a fixed set
of hash states that are rotated through the actual hash requests.

Let's say you allocate n such hash states which are always mapped,
then if there are less than n hash requests outstanding, you could
directly use the mapped hash states in the requests.

If there are more than n, then you simply copy in/copy out for each
hash operation.

The number n limits how many operations can be pending to be
processed by the hardware.

Cheers,

Patch
diff mbox

diff --git a/Documentation/crypto/devel-algos.rst b/Documentation/crypto/devel-algos.rst
index 66f50d32dcec..0f4617019227 100644
--- a/Documentation/crypto/devel-algos.rst
+++ b/Documentation/crypto/devel-algos.rst
@@ -236,6 +236,14 @@  when used from another part of the kernel.
                                |
                                '---------------> HASH2
 
+Note that it is perfectly legal to:
+- call .init() and then (as many times) .update()
+- subseqently _not_ call any of .final(), .finup() or .export()
+
+In other words mind the resource allocation and clean-up,
+since this basically means no resources can remain allocated
+after a call to .init() or .update().
+
 
 Specifics Of Asynchronous HASH Transformation
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~