linux-kernel - Re: [PATCH 2/3] crypto: exynos

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <87zi6x9lcx.fsf%l.stelmach@samsung.com>
Date:   Tue, 05 Dec 2017 17:43:10 +0100
From:   Łukasz Stelmach <l.stelmach@...sung.com>
To:     Stephan Mueller <smueller@...onox.de>
Cc:     Krzysztof Kozlowski <krzk@...nel.org>, robh+dt@...nel.org,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Kukjin Kim <kgene@...nel.org>, linux-crypto@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
        m.szyprowski@...sung.com, b.zolnierkie@...sung.com
Subject: Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG

It was <2017-12-05 wto 14:54>, when Stephan Mueller wrote:
> Am Dienstag, 5. Dezember 2017, 13:35:57 CET schrieb Łukasz Stelmach:
>
> Hi Łukasz,
>
>> Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
>> to retrieve generated numbers from the registers of PRNG.
>> 
>> Remove unnecessary invocation of cpu_relax().
>> 
>> Signed-off-by: Łukasz Stelmach <l.stelmach@...sung.com>
>> ---
>>  drivers/crypto/exynos-rng.c | 36 +++++-------------------------------
>>  1 file changed, 5 insertions(+), 31 deletions(-)
>> 
>> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
>> index 894ef93ef5ec..002e9d2a83cc 100644
>> --- a/drivers/crypto/exynos-rng.c
>> +++ b/drivers/crypto/exynos-rng.c

[...]

>> @@ -171,6 +143,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev
>> *rng, {
>>  	int retry = EXYNOS_RNG_WAIT_RETRIES;
>> 
>> +	*read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
>> +
>>  	if (rng->type == EXYNOS_PRNG_TYPE4) {
>>  		exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
>>  				  EXYNOS_RNG_CONTROL);
>> @@ -180,8 +154,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev
>> *rng, }
>> 
>>  	while (!(exynos_rng_readl(rng,
>> -			EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && --retry)
>> -		cpu_relax();
>> +			EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) &&
>> +	       --retry);
SM>
SM> Is this related to the patch?

KK> It looks like unrelated change so split it into separate commit with
KK> explanation why you are changing the common busy-loop pattern.
KK> exynos_rng_readl() uses relaxed versions of readl() so I would expect
KK> here cpu_relax().

Yes. As far as I can tell this gives the major part of the performance
improvement brought by this patch.

The busy loop is not very busy. Every time I checked the loop (w/o
cpu_relax()) was executed twice (retry was 98) and the operation was
reliable. I don't see why do we need a memory barrier here. On the other
hand, I am not sure the whole exynos_rng_get_random() shouldn't be ran
under a mutex or a spinlock (I don't see anything like this in the upper
layers of the crypto framework).

The *_relaxed() I/O operations do not enforce memory 

Thank you for asking the questions. I will put the above explanations in
the commit message.

>> 
>>  	if (!retry)
>>  		return -ETIMEDOUT;
>> @@ -189,7 +163,7 @@ static int exynos_rng_get_random(struct exynos_rng_dev
>> *rng, /* Clear status bit */
>>  	exynos_rng_writel(rng, EXYNOS_RNG_STATUS_RNG_DONE,
>>  			  EXYNOS_RNG_STATUS);
>> -	*read = exynos_rng_copy_random(rng, dst, dlen);
>> +	memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
>> 
>>  	return 0;
>>  }

Kind regards,
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Download attachment "signature.asc" of type "application/pgp-signature" (473 bytes)