From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Walle <michael@walle.cc>
Date: Wed, 17 Jun 2020 22:48:26 +0200
Subject: [PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton
 resistance
In-Reply-To: <631cfda0-c6b5-1337-087f-34db457006da@nxp.com>
References: <20200604154610.19810-1-michael@walle.cc>
 <20200604154610.19810-6-michael@walle.cc>
 <631cfda0-c6b5-1337-087f-34db457006da@nxp.com>
Message-ID: <c7ade87e5bd7c7092bf741e942581ea4@walle.cc>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Am 2020-06-17 21:15, schrieb Horia Geant?:
> On 6/4/2020 6:48 PM, Michael Walle wrote:
>> +static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)
>> +{
>> +	u32 *desc;
>> +	int sh_idx, ret = 0;
>> +	int desc_size = sizeof(u32) * 3;
> As you mentioned, descriptor size should be sizeof(u32) * 2.
> 
>> +
>> +	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
>> +	if (!desc) {
>> +		debug("cannot allocate RNG init descriptor memory\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>> +		/*
>> +		 * If the corresponding bit is set, then it means the state
>> +		 * handle was initialized by us, and thus it needs to be
>> +		 * deinitialized as well
>> +		 */
>> +
>> +		if (state_handle_mask & RDSTA_IF(sh_idx)) {
>> +			/*
>> +			 * Create the descriptor for deinstantating this state
>> +			 * handle.
>> +			 */
>> +			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
>> +			flush_dcache_range((unsigned long)desc,
>> +					   (unsigned long)desc + desc_size);
> Shouldn't this be roundup(desc_size, ARCH_DMA_MINALIGN) instead of 
> desc_size?

I've seen the same idioms sometimes, but it wasn't clear to me why that 
would
be needed; the hardware only uses the desc_size, right?

>> @@ -466,9 +511,18 @@ static int instantiate_rng(u8 sec_idx, int 
>> gen_sk)
>>  		 * If the corresponding bit is set, this state handle
>>  		 * was initialized by somebody else, so it's left alone.
>>  		 */
>> -		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
>> -		if (rdsta_val & (1 << sh_idx))
>> -			continue;
>> +		rdsta_val = sec_in32(&rng->rdsta);
>> +		if (rdsta_val & (RDSTA_IF(sh_idx) | RDSTA_PR(sh_idx))) {
> Adding RDSTA_PR(sh_idx) to the mask is not needed,
> PR bit is meaningless without IF bit set.

Ok.

> 
>> +			if (rdsta_val & RDSTA_PR(sh_idx))
>> +				continue;
> Could combine the condition here with the outer if condition:
> 		if (rdsta_val & RDSTA_IF(sh_idx) && !(rdsta_val & RDSTA_PR(sh_idx))) 
> {

If we keep that it is consistent with the linux driver. So I'd vote to
keep it. Also the continue statement would be missing and thus the
rng would be instantiated again. Or am I missing something?

-michael