Re: [PATCH] target/s390x/tcg: fix register update on guest memory fault in cpacf_aes_ecb

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

On 2026-10-08 17:44, Andrew Holden wrote:
> The KM instruction loop in cpacf_aes_ecb() updated the source pointer,
> destination pointer, and length registers only after the entire loop
> completed, using a 'done' accumulator. If a guest memory access fault
> fires mid-loop, siglongjmp unwinds the C stack before the register
> update is reached. The registers then reflect block 0 rather than the
> block being processed, causing the instruction to restart from the
> beginning on every fault. This results in quadratic reprocessing and
> violates the PoP requirement that registers reflect partial completion
> per block processed.
> 
> Fix this by updating the registers inside the loop body after each
> successfully processed block, and removing the 'done' accumulator.
> 
> Signed-off-by: Andrew Holden <andy0x48@linux.ibm.com>
> ---
>  target/s390x/tcg/cpacf_aes.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/target/s390x/tcg/cpacf_aes.c 
> b/target/s390x/tcg/cpacf_aes.c
> index 92884c4120..41ebc57087 100644
> --- a/target/s390x/tcg/cpacf_aes.c
> +++ b/target/s390x/tcg/cpacf_aes.c
> @@ -62,7 +62,7 @@ int cpacf_aes_ecb(CPUS390XState *env, const int
> mmu_idx, uintptr_t ra,
>  {
>      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;
> +    uint64_t len = *src_len_reg;
>      int i, keysize, addr_reg_size;
>      uint8_t key[32];
>      AES_KEY exkey;

Try to hold this bunch of variable declarations as a
"reverse Christmas tree". I am not sure if this is a strong
coding guideline for qemu but there are a lot of open source
projects insisting on this.

> @@ -116,23 +116,21 @@ int cpacf_aes_ecb(CPUS390XState *env, const int
> mmu_idx, uintptr_t ra,
> 
>      /* 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);
> +        aes_read_block(env, mmu_idx, ra, *src_ptr_reg, 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);
> +        aes_write_block(env, mmu_idx, ra, *dst_ptr_reg, out);

A comment here like
   /* update registers at end of each loop */
or so would be nice.

> +        *src_ptr_reg = deposit64(*src_ptr_reg, 0, addr_reg_size,
> +                                 *src_ptr_reg + AES_BLOCK_SIZE);
> +        *dst_ptr_reg = deposit64(*dst_ptr_reg, 0, addr_reg_size,
> +                                 *dst_ptr_reg + AES_BLOCK_SIZE);
> +        *src_len_reg -= AES_BLOCK_SIZE;
>          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;
> -
>      return !len ? 0 : 3;
>  }

Nice starting point.
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>