Re: [Qemu-devel] [PATCH RESEND v2 11/17] target-ppc: implement darn instruction

[Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>]

David Gibson <david@gibson.dropbear.id.au> writes:

> [ Unknown signature status ]
> On Mon, Sep 12, 2016 at 12:11:40PM +0530, Nikunj A Dadhania wrote:
>> From: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>> 
>> darn: Deliver A Random Number
>> 
>> Currently return invalid random number for all the case. This needs
>> proper algorithm to provide cryptographically suitable random data.
>> Reading from /dev/random can block and that is not an expected behaviour
>> while the cpu instruction is getting executed. Moreover, /dev/random
>> would only work for linux-user
>> 
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  target-ppc/helper.h     |  2 ++
>>  target-ppc/int_helper.c | 16 ++++++++++++++++
>>  target-ppc/translate.c  | 18 ++++++++++++++++++
>>  3 files changed, 36 insertions(+)
>> 
>> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
>> index e75d070..966f2ce 100644
>> --- a/target-ppc/helper.h
>> +++ b/target-ppc/helper.h
>> @@ -50,6 +50,8 @@ DEF_HELPER_FLAGS_1(cnttzd, TCG_CALL_NO_RWG_SE, tl, tl)
>>  DEF_HELPER_FLAGS_1(popcntd, TCG_CALL_NO_RWG_SE, tl, tl)
>>  DEF_HELPER_FLAGS_2(bpermd, TCG_CALL_NO_RWG_SE, i64, i64, i64)
>>  DEF_HELPER_3(srad, tl, env, tl, tl)
>> +DEF_HELPER_0(darn32, tl)
>> +DEF_HELPER_0(darn64, tl)
>>  #endif
>>  
>>  DEF_HELPER_FLAGS_1(cntlsw32, TCG_CALL_NO_RWG_SE, i32, i32)
>> diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
>> index 291fba0..51a9ac5 100644
>> --- a/target-ppc/int_helper.c
>> +++ b/target-ppc/int_helper.c
>> @@ -182,6 +182,22 @@ target_ulong helper_cnttzd(target_ulong t)
>>  {
>>      return ctz64(t);
>>  }
>> +
>> +/* Return invalid random number.
>> + *
>> + * FIXME: Add rng backend or other mechanism to get cryptographically suitable
>> + * random number
>> + */
>> +target_ulong helper_darn32(void)
>> +{
>> +    return -1;
>> +}
>> +
>> +target_ulong helper_darn64(void)
>> +{
>> +    return -1;
>> +}
>> +
>
> TBH, I think you're going to want a single helper for both 32-bit and
> 64-bit cases.

Initially, we used to split 32/64-bit in helper, Richard suggested to
make that decision in translate.c

>
>>  #endif
>>  
>>  #if defined(TARGET_PPC64)
>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> index 133c531..e9dad3f 100644
>> --- a/target-ppc/translate.c
>> +++ b/target-ppc/translate.c
>> @@ -528,6 +528,8 @@ EXTRACT_HELPER(FPW, 16, 1);
>>  
>>  /* addpcis */
>>  EXTRACT_HELPER_DXFORM(DX, 10, 6, 6, 5, 16, 1, 1, 0, 0)
>> +/* darn */
>> +EXTRACT_HELPER(L, 16, 2);
>>  
>>  /***                            Jump target decoding                       ***/
>>  /* Immediate address */
>> @@ -1895,6 +1897,21 @@ static void gen_cnttzd(DisasContext *ctx)
>>          gen_set_Rc0(ctx, cpu_gpr[rA(ctx->opcode)]);
>>      }
>>  }
>> +
>> +/* darn */
>> +static void gen_darn(DisasContext *ctx)
>> +{
>> +    int l = L(ctx->opcode);
>> +
>> +    if (l == 0) {
>> +        gen_helper_darn32(cpu_gpr[rD(ctx->opcode)]);
>> +    } else if (l <= 2) {
>> +        /* Return 64-bit random for both CRN and RRN */
>> +        gen_helper_darn64(cpu_gpr[rD(ctx->opcode)]);
>
> So it might be simpler to just leave out the helper stubs for now, and
> always return the invalid value from the generated code.

The idea was to retain all the translation work that we did as stubs.
And fill it whenever we have suitable implementation. Wouldn't hurt to
leave it like this. I am fine both ways.

Regards,
Nikunj