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

[David Hildenbrand]

On 12.07.22 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>

Please cc maintainers+lists as described MAINTAINERS next time.
Otherwise I won't stumble over that ever unless pinged by other people ;)

> ---
>  target/s390x/gen-features.c      |  2 ++
>  target/s390x/tcg/crypto_helper.c | 23 +++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> 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,
>  };

Please see

commit 4756b106b372e525365c62b41df38052571c0a71
Author: David Hildenbrand <david@redhat.com>
Date:   Thu Apr 28 11:46:57 2022 +0200

    s390x/cpu_models: drop "msa5" from the TCG "max" model
    
    We don't include the "msa5" feature in the "qemu" model because it
    generates a warning. The PoP states:
    
    "The message-security-assist extension 5 requires
    the secure-hash-algorithm (SHA-512) capabilities of
    the message-security-assist extension 2 as a prereq-
    uisite. (March, 2015)"
    
    As SHA-512 won't be supported in the near future, let's just drop the
    feature from the "max" model. This avoids the warning and allows us for
    making the "max" model match the "qemu" model (except for compat
    machines). We don't lose much, as we only implement the function stubs
    for MSA, excluding any real subfunctions.
    

How is that warning avoided now? We have to sort that out first -- either by
removing that dependency (easy) or implementing SHA-512 (hard).

>  
>  /****** END FEATURE DEFS ******/
> 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);
> +        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);


There seems to be something missing regarding exception + register handling.

The doc states:

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.

And:

Regardless of whether the operation ends due to
normal or partial completion, general registers R1
and R1 + 1 are incremented and decremented,
respectively, by the number of bytes stored into the
first operand, and general registers R2 and R2 + 1 are
incremented and decremented, respectively, by the
number of bytes stored into the second operand.



So I suspect we are not updating the registers accordingly,
especially before an exception could strike, or am I missing
something important?


Further, to be 100% correct you might have to wrap the address whenever
you increment it.

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

empty line please.

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

Thanks!

-- 
Thanks,

David / dhildenb