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

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

On 2022-07-19 13:43, 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: David Hildenbrand <david@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Cornelia Huck <cohuck@redhat.com>
> 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/cpu_models.c        |  2 --
>  target/s390x/gen-features.c      |  2 ++
>  target/s390x/tcg/crypto_helper.c | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 1a562d2801..90aac3d795 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -421,8 +421,6 @@ static void check_consistency(const S390CPUModel 
> *model)
>          { S390_FEAT_DFP_FAST, S390_FEAT_DFP },
>          { S390_FEAT_TRANSACTIONAL_EXE, S390_FEAT_STFLE_49 },
>          { S390_FEAT_EDAT_2, S390_FEAT_EDAT},
> -        { S390_FEAT_MSA_EXT_5, S390_FEAT_KIMD_SHA_512 },
> -        { S390_FEAT_MSA_EXT_5, S390_FEAT_KLMD_SHA_512 },
>          { S390_FEAT_MSA_EXT_4, S390_FEAT_MSA_EXT_3 },
>          { S390_FEAT_SIE_CMMA, S390_FEAT_CMM },
>          { S390_FEAT_SIE_CMMA, S390_FEAT_SIE_GSLS },
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index ad140184b9..3d333e2789 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -749,6 +749,8 @@ static uint16_t qemu_V7_0[] = {
>   */
>  static uint16_t qemu_MAX[] = {
>      S390_FEAT_VECTOR_ENH2,
> +    S390_FEAT_MSA_EXT_5,
> +    S390_FEAT_PRNO_TRNG,
>  };
> 
>  /****** END FEATURE DEFS ******/
> diff --git a/target/s390x/tcg/crypto_helper.c 
> b/target/s390x/tcg/crypto_helper.c
> index 138d9e7ad9..dccce7f707 100644
> --- a/target/s390x/tcg/crypto_helper.c
> +++ b/target/s390x/tcg/crypto_helper.c
> @@ -12,12 +12,30 @@
> 
>  #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)
> +{
> +        uint8_t tmp[256];
> +
> +        if (!(env->psw.mask & PSW_MASK_64))
> +                len = (uint32_t)len;
> +
> +        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, wrap_address(env,
> buf++), 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 +70,20 @@ 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:
> +        if (r1 & 1 || !r1 || r2 & 1 || !r2) {
> +                tcg_s390_program_interrupt(env, PGM_SPECIFICATION, 
> ra);
> +                break;
> +        }
> +
> +        fill_buf_random(env, ra, env->regs[r1], env->regs[r1 + 1]);
> +        fill_buf_random(env, ra, env->regs[r2], env->regs[r2 + 1]);
> +
> +        env->regs[r1] += env->regs[r1 + 1];
> +        env->regs[r1 + 1] = 0;
> +        env->regs[r2] += env->regs[r2 + 1];
> +        env->regs[r2 + 1] = 0;
> +        break;
>      default:
>          /* we don't implement any other subfunction yet */
>          g_assert_not_reached();

I am absolutely no expert in this qemu code - in fact seen in just now 
but
my feeling is that there should be some more than just handling then 114 
subfunction.
For example something like this:

diff --git a/target/s390x/tcg/crypto_helper.c 
b/target/s390x/tcg/crypto_helper.c
index 138d9e7ad9..a0163675b2 100644
--- a/target/s390x/tcg/crypto_helper.c
+++ b/target/s390x/tcg/crypto_helper.c
@@ -12,12 +12,30 @@

  #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)
+{
+        uint8_t tmp[256];
+
+        if (!(env->psw.mask & PSW_MASK_64))
+                len = (uint32_t)len;
+
+        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, wrap_address(env, buf++), 
tmp[i], ra);
+                len -= block;
+        }
+}
+
  uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, uint32_t r2, 
uint32_t r3,
                       uint32_t type)
  {
@@ -45,16 +63,39 @@ uint32_t HELPER(msa)(CPUS390XState *env, uint32_t 
r1, uint32_t r2, uint32_t r3,
          tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra);
      }

-    switch (fc) {
-    case 0: /* query subfunction */
+    /* handle query subfunction */
+    if (fc == 0) {
          for (i = 0; i < 16; i++) {
              param_addr = wrap_address(env, env->regs[1] + i);
              cpu_stb_data_ra(env, param_addr, subfunc[i], ra);
          }
+        return 0;
+    }
+
+    switch (type) {
+    case S390_FEAT_PRNO_TRNG:
+        switch (fc) {
+        case 114: /* CPACF_PRNO_TRNG */
+            if (r1 & 1 || !r1 || r2 & 1 || !r2) {
+                tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+                break;
+            }
+            fill_buf_random(env, ra, env->regs[r1], env->regs[r1 + 1]);
+            fill_buf_random(env, ra, env->regs[r2], env->regs[r2 + 1]);
+            env->regs[r1] += env->regs[r1 + 1];
+            env->regs[r1 + 1] = 0;
+            env->regs[r2] += env->regs[r2 + 1];
+            env->regs[r2 + 1] = 0;
+            break;
+        default:
+            /* we don't implement any other subfunctions yet */
+            g_assert_not_reached();
+        }
          break;
      default:
-        /* we don't implement any other subfunction yet */
-        g_assert_not_reached();
+            /* we don't implement any other functions yet */
+            g_assert_not_reached();
+        }
      }

      return 0;

And ... it is valid to have each of the two buffer pointers NULL. Not 
sure
how this env->regs[x] thing here handles this.