Re: [PATCH v7 10/17] target/s390x: Support protected key AES ECB for cpacf km instruction

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

On 2026-06-18 07:57, Finn Callies wrote:
> On 17.06.26 11:48, Harald Freudenberger wrote:
>> Support the subfunctions CPACF_KM_PAES_128, CPACF_KM_PAES_192
>> and CPACF_KM_PAES_256 for the cpacf km instruction.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> Tested-by: Holger Dengler <dengler@linux.ibm.com>
> 
> With the comments at least considered:
> 
> 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     | 87 
>> ++++++++++++++++++++++++++++++++
>>   target/s390x/tcg/crypto_helper.c |  7 +++
>>   4 files changed, 101 insertions(+)
>> 
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index d3e69aaca6..71e0e41d6e 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -924,6 +924,9 @@ static uint16_t qemu_MAX[] = {
>>       S390_FEAT_KM_AES_128,
>>       S390_FEAT_KM_AES_192,
>>       S390_FEAT_KM_AES_256,
>> +    S390_FEAT_KM_EAES_128,
>> +    S390_FEAT_KM_EAES_192,
>> +    S390_FEAT_KM_EAES_256,
> 
> I would strongly prefer PAES instead of EAES. I know the CPACF
> function is called KM...ENCRYPTED...AES... but since we use protected
> or PAES in any other context and never Encrypted AES I would highly
> suggest PAES here as well. Or as an alternative expand the E to
> S390_FEAT_KM_ENCRYPTED_AES_128 to match the CPACF naming.
> 

Yes, I am with you. But this is not part of this patch series.
These defines have been introduced into qemu long before this patch.
Maybe someone could rework them but not here and now with this patch
series.

>>       S390_FEAT_KM_XTS_AES_128,
>>       S390_FEAT_KM_XTS_AES_256,
>>       S390_FEAT_KMC_AES_128,
> 
> [ snip ]
> 
>> diff --git a/target/s390x/tcg/cpacf_aes.c 
>> b/target/s390x/tcg/cpacf_aes.c
>> index 5a0a3473d5..bcfcf3b660 100644
>> --- a/target/s390x/tcg/cpacf_aes.c
>> +++ b/target/s390x/tcg/cpacf_aes.c
>> @@ -533,3 +533,90 @@ int cpacf_aes_pckmo(CPUS390XState *env, const int 
>> mmu_idx, uintptr_t ra,
>>         return 0;
>>   }
>> +
>> +int cpacf_paes_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)
>> +{
> 
> [ snip ]
> 
>> +
>> +    /* fetch protected key from param block */
>> +    for (i = 0; i < keysize; i++) {
>> +        addr = wrap_address(env, param_addr + i);
>> +        key[i] = cpu_ldb_mmu(env, addr, oi, ra);
>> +    }
>> +    /* 'decrypt' the protected key */
>> +    for (i = 0; i < keysize; i++) {
>> +        key[i] ^= protkey_xor_pattern[i];
>> +    }
> 
> Think about outsourcing this into a function as it would improve
> readability and reduce code dublication in future commits.
> 

Yes, let's do this - see v8 of the patch series.

>> +
>> +    /* expand key */
>> +    if (mod) {
>> +        AES_set_decrypt_key(key, keysize * 8, &exkey);
>> +    } else {
>> +        AES_set_encrypt_key(key, keysize * 8, &exkey);
>> +    }
> 
> [ snip ]