| Message ID | 20220712164627.581570-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [qemu] target/s390x: support PRNO_TRNG instruction | expand |
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!
Hi Jason, thanks for you patch! Additionally to the things that David already mentioned, please have also a look at my comments below... On 12/07/2022 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> > --- > target/s390x/gen-features.c | 2 ++ > target/s390x/tcg/crypto_helper.c | 23 +++++++++++++++++++++++ > 2 files changed, 25 insertions(+) ...> 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); I think you have to limit "len" to the lower 32-bit if not running in 64-bit mode. > + 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); > + 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]; According to the Principles of Operation: "A specification exception is recognized and no other action is taken if any of the following conditions exist: ... The R1 or R2 fields designate an odd-numbered register or general register 0. This exception is recognized regardless of the function code." It would be good to have that check here, too. > + 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(); Thomas
Hi David, Thanks for your feedback. I'll CC you on v+1. Note that I don't know very much about s390x, so I may require some slight hand holding, but let's see how far I can get... On Tue, Jul 19, 2022 at 11:54:04AM +0200, David Hildenbrand wrote: > How is that warning avoided now? We have to sort that out first -- either by > removing that dependency (easy) or implementing SHA-512 (hard). Ahhh... Well, I can do either one I guess. Implementing SHA512 isn't really that hard. I can cook up a short enough software implementation of that which we could plonk into crypto_helper.c fairly minimally. Of course, then those instructions would have to be wired up. So maybe I'll try removing the dependency for v2 instead, and we'll see what you and Thomas think of that. > > +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. Thanks, missed this. I'll do that. > Further, to be 100% correct you might have to wrap the address whenever > you increment it. Ack. Jason
Hi Thomas, On Tue, Jul 19, 2022 at 12:00 PM Thomas Huth <thuth@redhat.com> wrote: > > +{ > > + uint64_t addr = wrap_address(env, buf); > > I think you have to limit "len" to the lower 32-bit if not running in 64-bit > mode. Will do. > According to the Principles of Operation: > > "A specification exception is recognized and no other > action is taken if any of the following conditions exist: > ... > The R1 or R2 fields designate an odd-numbered > register or general register 0. This exception is > recognized regardless of the function code." > > It would be good to have that check here, too. Ack. v2 incoming. Regards, Jason
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..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); + 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]; + 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();
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> --- target/s390x/gen-features.c | 2 ++ target/s390x/tcg/crypto_helper.c | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+)