Re: [PATCH v3] target/s390x: support PRNO_TRNG instruction

[David Hildenbrand]

On 02.08.22 17:15, Christian Borntraeger wrote:
> 
> 
> Am 02.08.22 um 16:53 schrieb David Hildenbrand:
>> On 02.08.22 16:01, Christian Borntraeger wrote:
>>>
>>>
>>> Am 02.08.22 um 15:54 schrieb David Hildenbrand:
>>>> On 02.08.22 15:26, Christian Borntraeger wrote:
>>>>>
>>>>>
>>>>> Am 20.07.22 um 14:08 schrieb Jason A. Donenfeld:
>>>>>> In order for hosts running inside of TCG to initialize the kernel's
>>>>>> random number generator, we should support the PRNO_TRNG instruction,
>>>>>> backed in the usual way with the qemu_guest_getrandom helper. This is
>>>>>> confirmed working on Linux 5.19-rc6.
>>>>>>
>>>>>> Cc: Thomas Huth <thuth@redhat.com>
>>>>>> Cc: David Hildenbrand <david@redhat.com>
>>>>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>>>>> Cc: Cornelia Huck <cohuck@redhat.com>
>>>>>> Cc: Harald Freudenberger <freude@linux.ibm.com>
>>>>>> Cc: Holger Dengler <dengler@linux.ibm.com>
>>>>>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>>>>> [...]
>>>>>> +    case 114:
>>>>>> +        if (r1 & 1 || !r1 || r2 & 1 || !r2)
>>>>>> +                tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>>>>> +        fill_buf_random(env, ra, &env->regs[r1], &env->regs[r1 + 1]);
>>>>>> +        fill_buf_random(env, ra, &env->regs[r2], &env->regs[r2 + 1]);
>>>>>> +        break;
>>>>>
>>>>> I think I agree with Harald that some aspects are missing.
>>>>> Linux does not seem to check, but we should also modify the query 
>>>>> function to
>>>>> indicate the availability of 114.
>>>>>
>>>>> As the msa helper deals with many instructions
>>>>> ...
>>>>> target/s390x/tcg/insn-data.def:    D(0xb91e, KMAC,    RRE,   MSA,  0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KMAC)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb928, PCKMO,   RRE,   MSA3, 0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_PCKMO)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb92a, KMF,     RRE,   MSA4, 0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KMF)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb92b, KMO,     RRE,   MSA4, 0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KMO)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb92c, PCC,     RRE,   MSA4, 0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_PCC)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb92d, KMCTR,   RRF_b, MSA4, 0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KMCTR)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb92e, KM,      RRE,   MSA,  0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KM)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb92f, KMC,     RRE,   MSA,  0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KMC)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb929, KMA,     RRF_b, MSA8, 0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KMA)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb93c, PPNO,    RRE,   MSA5, 0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_PPNO)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb93e, KIMD,    RRE,   MSA,  0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KIMD)
>>>>> target/s390x/tcg/insn-data.def:    D(0xb93f, KLMD,    RRE,   MSA,  0, 0, 
>>>>> 0, 0, msa, 0, S390_FEAT_TYPE_KLMD)
>>>>> ...
>>>>> and in theory other instructions might also have 114 we should at least 
>>>>> check that this is ppno/prno.
>>>>> Or we split out a prno helper from the msa helper.
>>>>>
>>>>
>>>> Doesn't
>>>>
>>>> s390_get_feat_block(type, subfunc);
>>>> if (!test_be_bit(fc, subfunc)) {
>>>>    tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>>> }
>>>>
>>>> check that? As long as we don't implement 114 for any other instruction.
>>>> that should properly fence off the other instructions.
>>>
>>> Right that would help. We should still take care of the query function.
>>>
>> s390_get_feat_block() should already take care of that as well, no?
> 
> Ah right, yes it fills subfunc. So yes, that should do the trick. Sorry for 
> the noise.
> 

I had to look at that 2 times as well ...

-- 
Thanks,

David / dhildenb