| Message ID | 20220719114307.102643-1-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] target/s390x: support PRNO_TRNG instruction | expand |
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!
Hi David, Thanks for the feedback. On Wed, Jul 20, 2022 at 01:43:48PM +0200, David Hildenbrand wrote: > > --- 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. The change to cpu_models.c above gets rid of the warning. > > + 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. Oh. The thing I had in mind was the r1&1 exception, not realizing that of course cpu_stb_data_ra() can also generate an exception. I'll update those registers incrementally. > 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. > */ I think I can do it incrementally pretty easy, actually. Let's see how it looks in v+1 first before I give up and add the TODO. > > + 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. Thanks, will do. > > > + } > > + > > + 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. > > See target/s390x/tcg/mem_helper.c:set_address() as an example on how to > modify parts of registers using deposit64(). That's not pretty, but I think I see how to do it. New revision incoming. Thanks for the review! Jason
>> Again, what about the warning? We don't want to report warnings in the >> QEMU default. > > The change to cpu_models.c above gets rid of the warning. Ah, stupid me. I missed that hunk somehow completely. [...] >> We have to be careful in 24-bit an 31-bit address mode, we may only >> update selected parts of the registers. >> >> See target/s390x/tcg/mem_helper.c:set_address() as an example on how to >> modify parts of registers using deposit64(). > > That's not pretty, but I think I see how to do it. > > New revision incoming. Thanks for the review! Thanks Jason!
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.
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();
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(-)