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

[David Hildenbrand]

On 19.07.22 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,
>  };


Again, what about the warning? We don't want to report warnings in the
QEMU default.

>  
>  /****** 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);

So, whenever we would get an exception, we would not update the
registers. This implies that we'd always start anew on an exception,
even though we already modified page content.

What the real HW does is constantly update the registers before the
exception is delivered, such that you can simply pick up work again when
re-entering the instruction after the exception was handled by the guest.

I assume we could do the same: updating the registers whenever a store
succeeded. Doing that after each and every byte is a bit inefficient,
but not sure if we care.

But as we're only storing random data, maybe we don't really care for
now and can simply always start anew. (the guest can observe this wrong
handling, though)

At a minimum we might want to add

/*
 * TODO: we should update the registers constantly, such that we reflect
 * which memory was already processed/modified if we get an exception
 * in-between.
 */

> +                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;

You can drop the "break;", we'll jump right out of that function and
never return -- the function is flagged as G_NORETURN.

> +        }
> +
> +        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;

We have to be careful in 24-bit an 31-bit address mode, we may only
update selected parts of the registers.

For example, note that instead of env->regs[r1 + 1], you'd actually have
to use the reduced length you currently compute in fill_buf_random.

We have to make sure the following holds:


"For the PRNO-TRNG function, the first-operand
address, first-operand length, second-operand
address, and second-operand length in general reg-
isters R1, R1 + 1, R2, and R2 + 1, respectively, are
updated at the completion of the instruction. In the
24-bit addressing mode, bits 40-63 of the even-num-
bered register are incremented by the number of
bytes processed for the respective operand, bits 0-31
of the register remain unchanged, and regardless of
the operand’s length, bits 32-39 of the register may
be set to zero or may remain unchanged. In the 31-
bit addressing mode, bits 33-63 of the even-num-
bered register are incremented by the number of
bytes processed for the respective operand, bits 0-31
of the register remain unchanged, and regardless of
the operand’s length, bit 32 of the register may be set
to zero or may remain unchanged. In the 64-bit
addressing mode, bits 0-63 of the even-numbered
register are incremented by the number of bytes pro-
cessed for the respective operand. In either the 24-
or 31-bit addressing mode, bits 32-63 of the odd-
numbered register are decremented by the number
of bytes processed for the respective operand, and
bits 0-31 of the register remain unchanged. In the 64-
bit addressing mode, bits 0-63 of the odd-numbered
register are decremented by the number of bytes pro-
cessed for the respective operand."

See target/s390x/tcg/mem_helper.c:set_address() as an example on how to
modify parts of registers using deposit64().


Thanks!

-- 
Thanks,

David / dhildenb