Re: [PATCH v13 12/18] target/s390x: Support protected key AES ECB for cpacf km instruction

On 8/5/26 10:28, Harald Freudenberger wrote:
> On 2026-08-05 00:48, Ilya Leoshkevich wrote:
>> On 8/3/26 18:12, Harald Freudenberger wrote:
>>> Support the subfunctions CPACF_KM_PAES_128, CPACF_KM_PAES_192
>>> and CPACF_KM_PAES_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         |  4 ++
>>>   target/s390x/tcg/cpacf_aes.c     | 91 ++++++++++++++++++++++++++++++++
>>>   target/s390x/tcg/crypto_helper.c |  7 +++
>>>   4 files changed, 105 insertions(+)
>>
>> [...]
>>
>>> +
>>> +    /* 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;
>>
>> Should we update registers after each iteration?
>> Otherwise there may be interesting effects due to swapped out pages when
>> running in system emulation.
> 
> I don't get this. Swapping and interruption of this code should not affect
> the encrypted/decrypted result in memory and also not the register content.
> But I assume that a CPACF instruction itself is some atomic operation. So
> there needs to be a consistent state before and after the instruction. But
> "while" the instruction is executed does not need to be consistent all the
> time. Otherwise for example here the memory write and the update of the
> registers should be atomic.

I agree that the "while" state is in general not important, except for
the special case when we get a memory-related exception in the middle of
processing. Then we end up committing the "while" state and it can be
observed by at least the exception handler. In this specific case I
guess the whole instruction will be restarted and the application code
will not have a chance to observe the inconsistency (memory updated, but
registers are not), but it a) doesn't look very clean and b) has
quadratic complexity: if 10 pages are swapped out, we will perform
1+2+...+10 decryptions.

DFLTCC, which is similar, handles it like this: it processes as many
pages as it can, and if the next page is not accessible, it returns CC3,
updating memory and registers accordingly. Only if the very first page
is not accessible it raises an exception. This makes sure we don't end
up with a quadratic number of bytes decompressed.
>> Blocks crossing the page boundary is a similar issue, not sure if it's
>> that easy to solve.
> 
> Well yes. This is a clear issue hanging around in all the memory read/write
> crypto code here. I have no idea on how this could be solved. However, 
> sounds
> like there will participate a new guy in the Qemu cpacf area soon. So maybe
> he has some ideas to work this out.

Looking at tcg/mem_helper.c, they use a static access_prepare()
instruction to pre-check the address ranges; this function takes page
crossings into account.

Perhaps it can be made non-static and reused here as is?

>> [...]