Re: [PATCH v8 06/18] target/s390x: Support AES CBC for cpacf kmc instruction

[Harald Freudenberger <freude@linux.ibm.com>]

On 2026-06-24 19:22, Ilya Leoshkevich wrote:
> On 6/24/26 10:10, Harald Freudenberger wrote:
>> Support the subfunctions CPACF_KMC_AES_128, CPACF_KMC_AES_192
>> and CPACF_KMC_AES_256 for the cpacf kmc instruction.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> Tested-by: Holger Dengler <dengler@linux.ibm.com>
>> Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
>> ---
>>   target/s390x/gen-features.c      |   3 +
>>   target/s390x/tcg/cpacf.h         |   4 ++
>>   target/s390x/tcg/cpacf_aes.c     | 102 
>> +++++++++++++++++++++++++++++++
>>   target/s390x/tcg/crypto_helper.c |  24 ++++++++
>>   4 files changed, 133 insertions(+)
> 
> [...]
> 
>> diff --git a/target/s390x/tcg/cpacf_aes.c 
>> b/target/s390x/tcg/cpacf_aes.c
>> index ba836f1473..6412cc187d 100644
>> --- a/target/s390x/tcg/cpacf_aes.c
>> +++ b/target/s390x/tcg/cpacf_aes.c
>> @@ -111,3 +111,105 @@ int cpacf_aes_ecb(CPUS390XState *env, const int 
>> mmu_idx, uintptr_t ra,
>>         return !len ? 0 : 3;
>>   }
>> +
>> +static void aes_xor(const uint8_t *src1, const uint8_t *src2, uint8_t 
>> *dst)
>> +{
>> +    for (int i = 0; i < AES_BLOCK_SIZE; i++) {
>> +        dst[i] = src1[i] ^ src2[i];
>> +    }
>> +}
>> +
>> +int cpacf_aes_cbc(CPUS390XState *env, const int mmu_idx, uintptr_t 
>> ra,
>> +                  uint64_t param_addr, uint64_t *dst_ptr_reg,
>> +                  uint64_t *src_ptr_reg, uint64_t *src_len_reg,
>> +                  uint32_t type, uint8_t fc, uint8_t mod)
>> +{
>> +    enum { MAX_BLOCKS_PER_RUN = 8192 / AES_BLOCK_SIZE };
>> +    uint8_t in[AES_BLOCK_SIZE], out[AES_BLOCK_SIZE], 
>> buf[AES_BLOCK_SIZE];
>> +    const MemOpIdx oi = make_memop_idx(MO_8, mmu_idx);
>> +    uint64_t addr, len = *src_len_reg, done = 0;
>> +    int i, keysize, addr_reg_size = 64;
>> +    uint8_t key[32], iv[AES_BLOCK_SIZE];
>> +    AES_KEY exkey;
>> +
>> +    g_assert(type == S390_FEAT_TYPE_KMC);
>> +
>> +    switch (fc) {
>> +    case 0x12: /* CPACF_KMC_AES_128 */
>> +        keysize = 16;
>> +        break;
>> +    case 0x13: /* CPACF_KMC_AES_192 */
>> +        keysize = 24;
>> +        break;
>> +    case 0x14: /* CPACF_KMC_AES_256 */
>> +        keysize = 32;
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    if (!(env->psw.mask & PSW_MASK_64)) {
>> +        len = (uint32_t)len;
>> +        addr_reg_size = (env->psw.mask & PSW_MASK_32) ? 32 : 24;
>> +    }
>> +
>> +    /* length has to be properly aligned. */
>> +    if (!QEMU_IS_ALIGNED(len, AES_BLOCK_SIZE)) {
>> +        tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +    }
>> +
>> +    /* fetch iv from param block */
>> +    for (i = 0; i < AES_BLOCK_SIZE; i++) {
>> +        addr = wrap_address(env, param_addr + i);
>> +        iv[i] = cpu_ldb_mmu(env, addr, oi, ra);
>> +    }
>> +
>> +    /* fetch key from param block */
>> +    for (i = 0; i < keysize; i++) {
>> +        addr = wrap_address(env, param_addr + AES_BLOCK_SIZE + i);
>> +        key[i] = cpu_ldb_mmu(env, addr, oi, ra);
>> +    }
> 
> Another couple of candidates for copy_from_guest_wrap().
> 

Yes, makes sense to have some (inline) function. I think I will add
something to cpacf.h.

>> +
>> +    /* expand key */
>> +    if (mod) {
>> +        AES_set_decrypt_key(key, keysize * 8, &exkey);
>> +    } else {
>> +        AES_set_encrypt_key(key, keysize * 8, &exkey);
>> +    }
>> +
>> +    /* process up to MAX_BLOCKS_PER_RUN aes blocks */
>> +    for (i = 0; i < MAX_BLOCKS_PER_RUN && len >= AES_BLOCK_SIZE; i++) 
>> {
>> +        aes_read_block(env, mmu_idx, *src_ptr_reg + done, in, ra);
>> +        if (mod) {
>> +            /* decrypt in => buf */
>> +            AES_decrypt(in, buf, &exkey);
>> +            /* buf xor iv => out */
>> +            aes_xor(buf, iv, out);
>> +            /* prep iv for next round */
>> +            memcpy(iv, in, AES_BLOCK_SIZE);
>> +        } else {
>> +            /* in xor iv => buf */
>> +            aes_xor(in, iv, buf);
>> +            /* encrypt buf => out */
>> +            AES_encrypt(buf, out, &exkey);
>> +            /* prep iv for next round */
>> +            memcpy(iv, out, AES_BLOCK_SIZE);
>> +        }
>> +        aes_write_block(env, mmu_idx, *dst_ptr_reg + done, out, ra);
>> +        len -= AES_BLOCK_SIZE, done += AES_BLOCK_SIZE;
> 
> I think
> 
>     len -= AES_BLOCK_SIZE;
>     done += AES_BLOCK_SIZE;
> 
> would be clearer.
> 
> 
> Other than that:
> 
> Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
> 
> [...]