Re: [PATCH v14 07/19] target/s390x: Support AES ECB for cpacf km instruction

On 2026-08-17 10:11, Ilya Leoshkevich wrote:
> On 8/6/26 17:12, Harald Freudenberger wrote:
>> Support the subfunctions CPACF_KM_AES_128, CPACF_KM_AES_192
>> and CPACF_KM_AES_256 for the cpacf km instruction.
>> 
>> Tested-by: Holger Dengler <[email protected]>
>> Reviewed-by: Finn Callies <[email protected]>
>> Signed-off-by: Harald Freudenberger <[email protected]>
>> ---
>>   target/s390x/gen-features.c      |   3 +
>>   target/s390x/tcg/cpacf.h         |   6 ++
>>   target/s390x/tcg/cpacf_aes.c     | 107 
>> +++++++++++++++++++++++++++++++
>>   target/s390x/tcg/crypto_helper.c |  24 +++++++
>>   target/s390x/tcg/meson.build     |   1 +
>>   5 files changed, 141 insertions(+)
>>   create mode 100644 target/s390x/tcg/cpacf_aes.c
>> 
> 
> [...]
> 
>> +/*
>> + * read exactly one AES block from guest memory into a local buffer
>> + */
>> +static inline void aes_read_block(CPUS390XState *env, const int 
>> mmu_idx,
>> +                                  const uintptr_t ra, uint64_t 
>> guest_addr,
>> +                                  uint8_t *buf)
>> +{
>> +    read_guest_wrap_u8(env, mmu_idx, ra, guest_addr, buf, 
>> AES_BLOCK_SIZE);
>> +}
>> +
>> +/*
>> + * write exactly one AES block from local buffer to guest memory
>> + */
>> +static void aes_write_block(CPUS390XState *env, const int mmu_idx,
>> +                            const uintptr_t ra, uint64_t guest_addr,
>> +                            uint8_t *buf)
>> +{
>> +    write_guest_wrap_u8(env, mmu_idx, ra, guest_addr, buf, 
>> AES_BLOCK_SIZE);
>> +}
> 
> nit: I would these two functions, I don't think they deserve a wrapper.
> 

I'd like to keep these two wrappers: The code is much more readable (for 
my
feeling) and the compiler hopefully does his job and replace it anyway.

>> +
>> +int cpacf_aes_ecb(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];
>> +    uint64_t len = *src_len_reg, done = 0;
>> +    int i, keysize, addr_reg_size = 64;
>> +    uint8_t key[32];
>> +    AES_KEY exkey;
>> +
>> +    g_assert(type == S390_FEAT_TYPE_KM);
>> +    switch (fc) {
>> +    case CPACF_KM_AES_128:
>> +        keysize = 16;
>> +        break;
>> +    case CPACF_KM_AES_192:
>> +        keysize = 24;
>> +        break;
>> +    case CPACF_KM_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 key from param block */
>> +    read_guest_wrap_u8(env, mmu_idx, ra, param_addr, key, keysize);
>> +
>> +    /* 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, ra, *src_ptr_reg + done, in);
>> +        if (mod) {
>> +            AES_decrypt(in, out, &exkey);
>> +        } else {
>> +            AES_encrypt(in, out, &exkey);
>> +        }
>> +        aes_write_block(env, mmu_idx, ra, *dst_ptr_reg + done, out);
>> +        len -= AES_BLOCK_SIZE;
>> +        done += AES_BLOCK_SIZE;
>> +    }
>> +
>> +    *src_ptr_reg = deposit64(*src_ptr_reg, 0, addr_reg_size,
>> +                             *src_ptr_reg + done);
>> +    *dst_ptr_reg = deposit64(*dst_ptr_reg, 0, addr_reg_size,
>> +                             *dst_ptr_reg + done);
>> +    *src_len_reg -= done;
>> +
>> +    return !len ? 0 : 3;
>> +}
> 
> The issue with writing memory, but not registers, on fault is still
> there, but I think today's behavior is good enough for emulating real
> sane code.
> 

I have opened up bugzilla 
https://bugzilla.linux.ibm.com/show_bug.cgi?id=221910 to track this.
But it will not be part of this patch series.

> 
> With our without the above addressed:
> 
> Reviewed-by: Ilya Leoshkevich <[email protected]>