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 <th...@redhat.com> > Cc: David Hildenbrand <da...@redhat.com> > Cc: Richard Henderson <richard.hender...@linaro.org> > Cc: Cornelia Huck <coh...@redhat.com> > Cc: Harald Freudenberger <fre...@linux.ibm.com> > Cc: Holger Dengler <deng...@linux.ibm.com> > Signed-off-by: Jason A. Donenfeld <ja...@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