Re: [Qemu-devel] [PATCH v4 3/4] target/arm: implement SM3 instructions

[Ard Biesheuvel]

On 22 January 2018 at 16:52, Ard Biesheuvel <address@hidden> wrote:
> On 22 January 2018 at 16:39, Peter Maydell <address@hidden> wrote:
>> On 19 January 2018 at 18:22, Ard Biesheuvel <address@hidden> wrote:
>>> This implements emulation of the new SM3 instructions that have
>>> been added as an optional extension to the ARMv8 Crypto Extensions
>>> in ARM v8.2.
>>>
>>> Signed-off-by: Ard Biesheuvel <address@hidden>
>>
>>
>>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>>> index 787b94047286..1e3ff9a6152f 100644
>>> --- a/target/arm/translate-a64.c
>>> +++ b/target/arm/translate-a64.c
>>> @@ -11148,28 +11148,39 @@ static void 
>>> disas_crypto_three_reg_sha512(DisasContext *s, uint32_t insn)
>>>      CryptoThreeOpFn *genfn;
>>>      int feature;
>>>
>>> -    if (o != 0) {
>>> -        unallocated_encoding(s);
>>> -        return;
>>> -    }
>>
>> If you wrote this code in the first patch in the
>>      if (o == 0) {
>>          stuff;
>>      } else {
>>          unallocated_encoding(s);
>>          return;
>>      }
>>
>> form, it would make this patch easier to read as it wouldn't
>> need to change the lines for the SHA512 cases.
>>
>
> ok
>
>>> -
>>> -    switch (opcode) {
>>> -    case 0: /* SHA512H */
>>> -        feature = ARM_FEATURE_V8_SHA512;
>>> -        genfn = gen_helper_crypto_sha512h;
>>> -        break;
>>> -    case 1: /* SHA512H2 */
>>> -        feature = ARM_FEATURE_V8_SHA512;
>>> -        genfn = gen_helper_crypto_sha512h2;
>>> -        break;
>>> -    case 2: /* SHA512SU1 */
>>> -        feature = ARM_FEATURE_V8_SHA512;
>>> -        genfn = gen_helper_crypto_sha512su1;
>>> -        break;
>>> -    case 3: /* RAX1 */
>>> -        feature = ARM_FEATURE_V8_SHA3;
>>> -        genfn = NULL;
>>> -        break;
>>> +    if (o == 0) {
>>> +        switch (opcode) {
>>> +        case 0: /* SHA512H */
>>> +            feature = ARM_FEATURE_V8_SHA512;
>>> +            genfn = gen_helper_crypto_sha512h;
>>> +            break;
>>> +        case 1: /* SHA512H2 */
>>> +            feature = ARM_FEATURE_V8_SHA512;
>>> +            genfn = gen_helper_crypto_sha512h2;
>>> +            break;
>>> +        case 2: /* SHA512SU1 */
>>> +            feature = ARM_FEATURE_V8_SHA512;
>>> +            genfn = gen_helper_crypto_sha512su1;
>>> +            break;
>>> +        case 3: /* RAX1 */
>>> +            feature = ARM_FEATURE_V8_SHA3;
>>> +            genfn = NULL;
>>> +            break;
>>> +        }
>>> +    } else {
>>> +        switch (opcode) {
>>> +        case 0: /* SM3PARTW1 */
>>> +            feature = ARM_FEATURE_V8_SM3;
>>> +            genfn = gen_helper_crypto_sm3partw1;
>>> +            break;
>>> +        case 1: /* SM3PARTW2 */
>>> +            feature = ARM_FEATURE_V8_SM3;
>>> +            genfn = gen_helper_crypto_sm3partw2;
>>> +            break;
>>> +        default:
>>> +            unallocated_encoding(s);
>>> +            return;
>>> +        }
>>
>> This seems to be missing support for SM4EKEY (which is O==1
>> opcode == 0b10 and also part of the v8.2 SM feature) ?
>>
>
> It is part of the v8.2 SM extension, which consists of SM3 secure hash
> and SM4 encryption, which are two different things (and AA64ISAR0 has
> separate feature bits for each). The ARM ARM does stipulate that both
> should be set if either one is set, but still provides two separate
> bits, and so one can be enabled without the other.
>

Same for ELF_HWCAPs btw