Re: [PATCH qemu] target/s390x: support PRNO_TRNG instruction

[Thomas Huth]

 Hi Jason,

thanks for you patch! Additionally to the things that David already 
mentioned, please have also a look at my comments below...

On 12/07/2022 18.46, Jason A. Donenfeld wrote:
> In order for hosts running inside of TCG to initialize the kernel's
> random number generator, we should support the PRNO_TRNG instruction,
> backed in the usual way with the qemu_guest_getrandom helper. This is
> confirmed working on Linux 5.19-rc6.
>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Harald Freudenberger <freude@linux.ibm.com>
> Cc: Holger Dengler <dengler@linux.ibm.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>   target/s390x/gen-features.c      |  2 ++
>   target/s390x/tcg/crypto_helper.c | 23 +++++++++++++++++++++++
>   2 files changed, 25 insertions(+)
...> diff --git a/target/s390x/tcg/crypto_helper.c 
b/target/s390x/tcg/crypto_helper.c
> index 138d9e7ad9..cefdfd114e 100644
> --- a/target/s390x/tcg/crypto_helper.c
> +++ b/target/s390x/tcg/crypto_helper.c
> @@ -12,12 +12,28 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu/main-loop.h"
> +#include "qemu/guest-random.h"
>   #include "s390x-internal.h"
>   #include "tcg_s390x.h"
>   #include "exec/helper-proto.h"
>   #include "exec/exec-all.h"
>   #include "exec/cpu_ldst.h"
>   
> +static void fill_buf_random(CPUS390XState *env, uintptr_t ra,
> +                            uint64_t buf, uint64_t len)
> +{
> +        uint64_t addr = wrap_address(env, buf);

I think you have to limit "len" to the lower 32-bit if not running in 64-bit 
mode.

> +        uint8_t tmp[256];
> +
> +        while (len) {
> +                size_t block = MIN(len, sizeof(tmp));
> +                qemu_guest_getrandom_nofail(tmp, block);
> +                for (size_t i = 0; i < block; ++i)
> +                        cpu_stb_data_ra(env, addr++, tmp[i], ra);
> +                len -= block;
> +        }
> +}
> +
>   uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t 
> r3,
>                        uint32_t type)
>   {
> @@ -52,6 +68,13 @@ uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, 
> uint32_t r2, uint32_t r3,
>               cpu_stb_data_ra(env, param_addr, subfunc[i], ra);
>           }
>           break;
> +    case 114: {
> +        const uint64_t ucbuf = env->regs[r1], ucbuf_len = env->regs[r1 + 1];
> +        const uint64_t cbuf = env->regs[r2], cbuf_len = env->regs[r2 + 1];

According to the Principles of Operation:

"A specification exception is recognized and no other
action is taken if any of the following conditions exist:
...
The R1 or R2 fields designate an odd-numbered
register or general register 0. This exception is
recognized regardless of the function code."

It would be good to have that check here, too.

> +        fill_buf_random(env, ra, ucbuf, ucbuf_len);
> +        fill_buf_random(env, ra, cbuf, cbuf_len);
> +        break;
> +    }
>       default:
>           /* we don't implement any other subfunction yet */
>           g_assert_not_reached();

 Thomas