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

On 8/18/26 12:34, Harald Freudenberger wrote:
> On 2026-08-17 17:00, Ilya Leoshkevich wrote:
>> On 8/17/26 16:23, Harald Freudenberger wrote:
>>> 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.
>>
>> No, userspace doesn't support it either.
>> So neither qemu-user nor qemu-system + Linux will need it.
>> Some weird non-Linux setup might, I'm not sure if we want to go out of
>> our way to support them. I think we should either drop it or
>> implement it correctly.
>>
>> [...]
> 
> It it ok then to assume 32 bit mode if not 64 bit? Like this snippet:
> 
> 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)
> {
>      ...
>      int i, keysize, addr_reg_size = 64;
>      ...
> 
>      ...
> 
>      if (!(env->psw.mask & PSW_MASK_64)) {
>          len = (uint32_t)len;
>          addr_reg_size = 32;
>      }
> 
>      ...
> 
> Or check for 32 bit and if that is also not the case abort() or something ?
> 

I would raise a program interrupt, like you already do for, for example,
non-zero block number in one of your patches.