Re: [PATCH v10 08/21] target/s390x: Minimal AES XTS support for cpacf pcc instruction

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

On 2026-07-07 09:21, Holger Dengler wrote:
> On 7/6/26 11:43, Harald Freudenberger wrote:
>> Support CPACF pcc subfunctions PCC-Compute-XTS-Parameter-AES-128
>> and PCC-Compute-XTS-Parameter-AES-128 but only for the special
> 
> typo(?): --> PCC-Compute-XTS-Parameter-AES-256
> 
>> case block sequential number is 0. However, this covers the s390
>> AES XTS implementation in the Linux kernel and Libica and thus
>> also Opencryptoki clear key via Libica.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> Tested-by: Holger Dengler <dengler@linux.ibm.com>
> 
> Ordering? (T-b first, then S-o-b). See my comment below.
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
> 

Yea ... ordering

>> ---
>>   target/s390x/gen-features.c      |  2 +
>>   target/s390x/tcg/cpacf.h         |  2 +
>>   target/s390x/tcg/cpacf_aes.c     | 63 
>> ++++++++++++++++++++++++++++++++
>>   target/s390x/tcg/crypto_helper.c | 20 ++++++++++
>>   4 files changed, 87 insertions(+)
>> 
> [...]
>> diff --git a/target/s390x/tcg/cpacf_aes.c 
>> b/target/s390x/tcg/cpacf_aes.c
>> index 3d6aa19df2..f41b7dc541 100644
>> --- a/target/s390x/tcg/cpacf_aes.c
>> +++ b/target/s390x/tcg/cpacf_aes.c
>> @@ -290,3 +290,66 @@ int cpacf_aes_ctr(CPUS390XState *env, const int 
>> mmu_idx, uintptr_t ra,
>>         return !len ? 0 : 3;
>>   }
>> +
>> +int cpacf_aes_pcc(CPUS390XState *env, const int mmu_idx, uintptr_t 
>> ra,
>> +                  uint64_t param_addr, uint8_t fc)
>> +{
>> +    uint8_t key[32], tweak[AES_BLOCK_SIZE], buf[AES_BLOCK_SIZE];
>> +    const MemOpIdx oi = make_memop_idx(MO_8, mmu_idx);
>> +    int keysize, i;
>> +    uint64_t addr;
>> +    AES_KEY exkey;
>> +
>> +    switch (fc) {
>> +    case CPACF_PCC_XTS_AES_128:
>> +        keysize = 16;
>> +        break;
>> +    case CPACF_PCC_XTS_AES_256:
>> +        keysize = 32;
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    /* fetch block sequence nr from param block into buf */
>> +    for (i = 0; i < AES_BLOCK_SIZE; i++) {
>> +        addr = wrap_address(env, param_addr + keysize + 
>> AES_BLOCK_SIZE + i);
>> +        buf[i] = cpu_ldb_mmu(env, addr, oi, ra);
>> +    }
>> +
>> +    /* is the block sequence nr 0 ? */
>> +    for (i = 0; i < AES_BLOCK_SIZE && !buf[i]; i++) {
>> +            ;
>> +    }
>> +    if (i < AES_BLOCK_SIZE) {
>> +        /* no, sorry handling of non zero block sequence is not 
>> implemented */
>> +        tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> +        return 1;
>> +    }
> 
> Why not
> 
> for (i = 0; i < AES_BLOCK_SIZE && !buf[i]; i++);
> if (i < AES_BLOCK_SIZE) {
> ...
> 
> Or would that be against the coding guidelines?
> 

Yes. Qemu requires to always use {}.
But I could do some (inline) function is_mem_zero(mem, len)
instead. Maybe in v11 then.

> I personally would prefer the following, because it is more obvious.
> But your solution also works, so feel free to change it or leave it as
> is.
> 
> for (i = 0; i < AES_BLOCK_SIZE; i++) {
>     if (buf[i]) {
>         /*
>          * no, sorry handling of non zero block sequence is not
>          * implemented
>          */
>         tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>         return 1;
>     }
> }
> 
> IMO, it is easier to read.
> 
> [...]