Re: [PATCH v1 1/1] s390/arch_random: Buffer true random data

[Holger Dengler <dengler@linux.ibm.com>]

Hi Jason,

On 05/07/2022 18:35, Jason A. Donenfeld wrote:
> Hi Holger,
> 
> On Tue, Jul 05, 2022 at 06:27:59PM +0200, Holger Dengler wrote:
>> I saw a few calls in interrupt context during my tracing, but I didn't
>> look to see which ones they were. Let me figure that out in the next
>> few days and provide more information on that.
> 
> One thing to keep in mind is that it's used at boot time, when
> technically IRQs are turned off, so it appears like interrupt context
> depending on which way you squint. But boot time obviously isn't a
> problem. So be sure that's not the usage you're seeing.

Ok, let me check this. I will also think about the tree-wide cleanup you mentioned in an earlier mail. It looks, that s390 could fill the block.rdseed with a single call.

>> For the moment, I would propose to drop the buffering but also return
>> false, if arch_random_get_seed_long() is called in interrupt context.
> 
> As a last ditch, maybe that's best. Maybe... Do you know off hand how
> many cycles each call takes?

I don't know the exact number of cycles, but as I mentioned in the coverletter, the trng instruction is one of the specialties of the s390 platform. It looks like an instruction, but it is some kind of firmware executed (it is called millicode). These kind of long-running instructions are also interruptable and can resume.

A trng call runs for minimal ~20-190us for 32 bytes. 20us on newer machine generations, 190us on older ones. These are not 100% exact measurements, but the dimension should be correct.

> 
>> diff --git a/arch/s390/include/asm/archrandom.h b/arch/s390/include/asm/archrandom.h
>> index 2c6e1c6ecbe7..711357bdc464 100644
>> --- a/arch/s390/include/asm/archrandom.h
>> +++ b/arch/s390/include/asm/archrandom.h
>> @@ -32,7 +32,8 @@ static inline bool __must_check arch_get_random_int(unsigned int *v)
>>
>>  static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
>>  {
>> -       if (static_branch_likely(&s390_arch_random_available)) {
>> +       if (static_branch_likely(&s390_arch_random_available) &&
>> +           !in_interrupt()) {
> 
> in_interrupt() is deprecated. You want in_hardirq() here. You'll also
> want to verify that this doesn't prevent random_init() from working.
> 
> Jason

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com