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

On 2026-08-17 11:17, 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
> 
> [...]
> 
>> +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;
>> +    }
> 
> Just noticed something, here and in all other patches. POp says:
> 
> 
> In the 24-bit addressing mode, the contents of bit
> positions 40-63 of general registers R1 and R2 consti-
> tute the addresses of the first and second operands,
> respectively, and the contents of bit positions 0-39
> are ignored; bits 40-63 of the updated addresses
> replace the corresponding bits in general registers R1
> and R2, carries out of bit position 40 of the updated
> address are ignored, and the contents of bit positions
> 32-39 of general registers R1 and R 2 are set to zeros.
> 
> 
> Seems like we are not zeroing out bits 32-39?
> 

Do we support 24 bit mode? Isn't that ditched some years ago?
At least we definitely do not support s390 kernel in 24 bit mode.
Maybe userspace is valid.

>> +
>> +    /* 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);
> 
> Another weird nit, also here and everywhere else. POp says:
> 
> 
> When the second-operand length is initially zero, the
> following occurs. The parameter block, first, and sec-
> ond operands are not accessed, and general regis-
> ters R1, R2, and R2 + 1 are not changed.
> 
> 
> I guess we need to bail early if length is 0.

I'll fix this.

> 
> [...]