[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Horia Geantă]

On 6/17/2020 11:48 PM, Michael Walle wrote:
> Am 2020-06-17 21:15, schrieb Horia Geantă:
>> On 6/4/2020 6:48 PM, Michael Walle wrote:
>>> +
>>> +	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?
> 
Yes, HW will use only [desc, desc + desc_size].

I think this is needed to avoid cacheline sharing issues
on non-coherent platforms: CPU needs to make sure a larger area
is written back to memory and corresponding cache lines are invalidated.

Looking at flush_dcache_range() implementation, it does its own rounding,
based on CTR_EL0[DminLine] - "smallest data cache line size".
I guess this value might be smaller than ARCH_DMA_MINALIGN,
hence the explicit rounding to ARCH_DMA_MINALIGN is needed.

>>> @@ -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?
> 
You are correct, let's keep this as is.

Thanks,
Horia