| Message ID | 20220720120859.339788-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] target/s390x: support PRNO_TRNG instruction | expand |
On 20.07.22 14:08, 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..afd29f9cf0 100644 > --- a/target/s390x/tcg/crypto_helper.c > +++ b/target/s390x/tcg/crypto_helper.c > @@ -12,12 +12,38 @@ > > #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_reg, uint64_t *len_reg) > +{ > + uint8_t tmp[256]; > + uint64_t len = *len_reg; > + int reg_len = 64; > + > + if (!(env->psw.mask & PSW_MASK_64)) { > + len = (uint32_t)len; > + reg_len = (env->psw.mask & PSW_MASK_32) ? 32 : 24; > + } > + > + 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_reg), tmp[i], ra); > + *buf_reg = deposit64(*buf_reg, 0, reg_len, *buf_reg + 1); > + --*len_reg; > + } > + len -= block; > + } Yeah, that's better, although kind-off hard to read. We could process one guest page at a time, similar to how we handle target/s390x/tcg/mem_helper.c:access_memset and friends nowadays. But I won't force you to do that ;) This here is good enough for now, with room for improvement regarding efficiency. I did not review the doc in detail once again, maybe I get to that later this week.
Hi David, On Wed, Jul 20, 2022 at 08:41:48PM +0200, David Hildenbrand wrote: > > + 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_reg), tmp[i], ra); > > + *buf_reg = deposit64(*buf_reg, 0, reg_len, *buf_reg + 1); > > + --*len_reg; > > + } > > + len -= block; > > + } > > Yeah, that's better, although kind-off hard to read. > > We could process one guest page at a time, similar to how we handle > target/s390x/tcg/mem_helper.c:access_memset and friends nowadays. > > But I won't force you to do that ;) > > This here is good enough for now, with room for improvement regarding > efficiency. > > I did not review the doc in detail once again, maybe I get to that later > this week. Alright, so we'll leave it be for now then and stick with this v3. The do_access_memset trick is clever, but sheesh, seems a bit overkill for here. On the real hardware, this instruction takes ~190us for every 32 byte chunk, so there's basically no way that we can possibly be worse than that. :) Jason
Hey David, On Wed, Jul 20, 2022 at 08:41:48PM +0200, David Hildenbrand wrote: > I did not review the doc in detail once again, maybe I get to that later > this week. Did you ever get around to merging this patch? Is it in some tree somewhere? Jason
On 27/07/2022 03.35, Jason A. Donenfeld wrote: > Hey David, > > On Wed, Jul 20, 2022 at 08:41:48PM +0200, David Hildenbrand wrote: >> I did not review the doc in detail once again, maybe I get to that later >> this week. > > Did you ever get around to merging this patch? Is it in some tree > somewhere? QEMU is in the freeze phase now, so new feature won't be merged before the next release, see: https://wiki.qemu.org/Planning/7.1 Maybe we could use the time to implement the missing SHA512 stuff to avoid having an inconsistency between the Principles of Operation and the emulated machine in QEMU? Thomas
Hey Thomas, On Wed, Jul 27, 2022 at 08:32:22AM +0200, Thomas Huth wrote: > On 27/07/2022 03.35, Jason A. Donenfeld wrote: > > Hey David, > > > > On Wed, Jul 20, 2022 at 08:41:48PM +0200, David Hildenbrand wrote: > >> I did not review the doc in detail once again, maybe I get to that later > >> this week. > > > > Did you ever get around to merging this patch? Is it in some tree > > somewhere? > > QEMU is in the freeze phase now, so new feature won't be merged before the > next release, see: Yea, I understand, that's fine. > Maybe we could use the time to implement the missing SHA512 stuff to avoid > having an inconsistency between the Principles of Operation and the emulated > machine in QEMU? Ooooooooooooooofffff. You're not /wrong/ of course. This actually makes a lot of sense. But I was hoping to somehow skip out on this part, because I don't know much about s390 and wiring up the handlers seems finicky. But I can learn! Actually, though, any interest in working together on this? I can work on the crypto-side of things, fashioning a minimal sha512 implementation that's small enough it can fit in crypto_helper.c with support for the incremental block state stuff s390 needs, and then you can work on wiring in all the instructions and telling me what semantics you need from the crypto. Interested? (Offer of working together goes out to David too of course.) If so, maybe poke me on IRC? I'm zx2c4 on the various networks. Jason
Am 20.07.22 um 14:08 schrieb Jason A. Donenfeld: > 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> [...] > + case 114: > + if (r1 & 1 || !r1 || r2 & 1 || !r2) > + tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); > + fill_buf_random(env, ra, &env->regs[r1], &env->regs[r1 + 1]); > + fill_buf_random(env, ra, &env->regs[r2], &env->regs[r2 + 1]); > + break; I think I agree with Harald that some aspects are missing. Linux does not seem to check, but we should also modify the query function to indicate the availability of 114. As the msa helper deals with many instructions ... target/s390x/tcg/insn-data.def: D(0xb91e, KMAC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMAC) target/s390x/tcg/insn-data.def: D(0xb928, PCKMO, RRE, MSA3, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCKMO) target/s390x/tcg/insn-data.def: D(0xb92a, KMF, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMF) target/s390x/tcg/insn-data.def: D(0xb92b, KMO, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMO) target/s390x/tcg/insn-data.def: D(0xb92c, PCC, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCC) target/s390x/tcg/insn-data.def: D(0xb92d, KMCTR, RRF_b, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMCTR) target/s390x/tcg/insn-data.def: D(0xb92e, KM, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KM) target/s390x/tcg/insn-data.def: D(0xb92f, KMC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMC) target/s390x/tcg/insn-data.def: D(0xb929, KMA, RRF_b, MSA8, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMA) target/s390x/tcg/insn-data.def: D(0xb93c, PPNO, RRE, MSA5, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PPNO) target/s390x/tcg/insn-data.def: D(0xb93e, KIMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KIMD) target/s390x/tcg/insn-data.def: D(0xb93f, KLMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KLMD) ... and in theory other instructions might also have 114 we should at least check that this is ppno/prno. Or we split out a prno helper from the msa helper.
On 02.08.22 15:26, Christian Borntraeger wrote: > > > Am 20.07.22 um 14:08 schrieb Jason A. Donenfeld: >> 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> > [...] >> + case 114: >> + if (r1 & 1 || !r1 || r2 & 1 || !r2) >> + tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); >> + fill_buf_random(env, ra, &env->regs[r1], &env->regs[r1 + 1]); >> + fill_buf_random(env, ra, &env->regs[r2], &env->regs[r2 + 1]); >> + break; > > I think I agree with Harald that some aspects are missing. > Linux does not seem to check, but we should also modify the query function to > indicate the availability of 114. > > As the msa helper deals with many instructions > ... > target/s390x/tcg/insn-data.def: D(0xb91e, KMAC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMAC) > target/s390x/tcg/insn-data.def: D(0xb928, PCKMO, RRE, MSA3, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCKMO) > target/s390x/tcg/insn-data.def: D(0xb92a, KMF, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMF) > target/s390x/tcg/insn-data.def: D(0xb92b, KMO, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMO) > target/s390x/tcg/insn-data.def: D(0xb92c, PCC, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCC) > target/s390x/tcg/insn-data.def: D(0xb92d, KMCTR, RRF_b, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMCTR) > target/s390x/tcg/insn-data.def: D(0xb92e, KM, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KM) > target/s390x/tcg/insn-data.def: D(0xb92f, KMC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMC) > target/s390x/tcg/insn-data.def: D(0xb929, KMA, RRF_b, MSA8, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMA) > target/s390x/tcg/insn-data.def: D(0xb93c, PPNO, RRE, MSA5, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PPNO) > target/s390x/tcg/insn-data.def: D(0xb93e, KIMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KIMD) > target/s390x/tcg/insn-data.def: D(0xb93f, KLMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KLMD) > ... > and in theory other instructions might also have 114 we should at least check that this is ppno/prno. > Or we split out a prno helper from the msa helper. > Doesn't s390_get_feat_block(type, subfunc); if (!test_be_bit(fc, subfunc)) { tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); } check that? As long as we don't implement 114 for any other instruction. that should properly fence off the other instructions.
Am 02.08.22 um 15:54 schrieb David Hildenbrand: > On 02.08.22 15:26, Christian Borntraeger wrote: >> >> >> Am 20.07.22 um 14:08 schrieb Jason A. Donenfeld: >>> 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> >> [...] >>> + case 114: >>> + if (r1 & 1 || !r1 || r2 & 1 || !r2) >>> + tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); >>> + fill_buf_random(env, ra, &env->regs[r1], &env->regs[r1 + 1]); >>> + fill_buf_random(env, ra, &env->regs[r2], &env->regs[r2 + 1]); >>> + break; >> >> I think I agree with Harald that some aspects are missing. >> Linux does not seem to check, but we should also modify the query function to >> indicate the availability of 114. >> >> As the msa helper deals with many instructions >> ... >> target/s390x/tcg/insn-data.def: D(0xb91e, KMAC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMAC) >> target/s390x/tcg/insn-data.def: D(0xb928, PCKMO, RRE, MSA3, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCKMO) >> target/s390x/tcg/insn-data.def: D(0xb92a, KMF, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMF) >> target/s390x/tcg/insn-data.def: D(0xb92b, KMO, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMO) >> target/s390x/tcg/insn-data.def: D(0xb92c, PCC, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCC) >> target/s390x/tcg/insn-data.def: D(0xb92d, KMCTR, RRF_b, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMCTR) >> target/s390x/tcg/insn-data.def: D(0xb92e, KM, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KM) >> target/s390x/tcg/insn-data.def: D(0xb92f, KMC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMC) >> target/s390x/tcg/insn-data.def: D(0xb929, KMA, RRF_b, MSA8, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMA) >> target/s390x/tcg/insn-data.def: D(0xb93c, PPNO, RRE, MSA5, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PPNO) >> target/s390x/tcg/insn-data.def: D(0xb93e, KIMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KIMD) >> target/s390x/tcg/insn-data.def: D(0xb93f, KLMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KLMD) >> ... >> and in theory other instructions might also have 114 we should at least check that this is ppno/prno. >> Or we split out a prno helper from the msa helper. >> > > Doesn't > > s390_get_feat_block(type, subfunc); > if (!test_be_bit(fc, subfunc)) { > tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); > } > > check that? As long as we don't implement 114 for any other instruction. > that should properly fence off the other instructions. Right that would help. We should still take care of the query function.
On 02.08.22 16:01, Christian Borntraeger wrote: > > > Am 02.08.22 um 15:54 schrieb David Hildenbrand: >> On 02.08.22 15:26, Christian Borntraeger wrote: >>> >>> >>> Am 20.07.22 um 14:08 schrieb Jason A. Donenfeld: >>>> 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> >>> [...] >>>> + case 114: >>>> + if (r1 & 1 || !r1 || r2 & 1 || !r2) >>>> + tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); >>>> + fill_buf_random(env, ra, &env->regs[r1], &env->regs[r1 + 1]); >>>> + fill_buf_random(env, ra, &env->regs[r2], &env->regs[r2 + 1]); >>>> + break; >>> >>> I think I agree with Harald that some aspects are missing. >>> Linux does not seem to check, but we should also modify the query function to >>> indicate the availability of 114. >>> >>> As the msa helper deals with many instructions >>> ... >>> target/s390x/tcg/insn-data.def: D(0xb91e, KMAC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMAC) >>> target/s390x/tcg/insn-data.def: D(0xb928, PCKMO, RRE, MSA3, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCKMO) >>> target/s390x/tcg/insn-data.def: D(0xb92a, KMF, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMF) >>> target/s390x/tcg/insn-data.def: D(0xb92b, KMO, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMO) >>> target/s390x/tcg/insn-data.def: D(0xb92c, PCC, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCC) >>> target/s390x/tcg/insn-data.def: D(0xb92d, KMCTR, RRF_b, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMCTR) >>> target/s390x/tcg/insn-data.def: D(0xb92e, KM, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KM) >>> target/s390x/tcg/insn-data.def: D(0xb92f, KMC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMC) >>> target/s390x/tcg/insn-data.def: D(0xb929, KMA, RRF_b, MSA8, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMA) >>> target/s390x/tcg/insn-data.def: D(0xb93c, PPNO, RRE, MSA5, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PPNO) >>> target/s390x/tcg/insn-data.def: D(0xb93e, KIMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KIMD) >>> target/s390x/tcg/insn-data.def: D(0xb93f, KLMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KLMD) >>> ... >>> and in theory other instructions might also have 114 we should at least check that this is ppno/prno. >>> Or we split out a prno helper from the msa helper. >>> >> >> Doesn't >> >> s390_get_feat_block(type, subfunc); >> if (!test_be_bit(fc, subfunc)) { >> tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); >> } >> >> check that? As long as we don't implement 114 for any other instruction. >> that should properly fence off the other instructions. > > Right that would help. We should still take care of the query function. > s390_get_feat_block() should already take care of that as well, no?
Am 02.08.22 um 16:53 schrieb David Hildenbrand: > On 02.08.22 16:01, Christian Borntraeger wrote: >> >> >> Am 02.08.22 um 15:54 schrieb David Hildenbrand: >>> On 02.08.22 15:26, Christian Borntraeger wrote: >>>> >>>> >>>> Am 20.07.22 um 14:08 schrieb Jason A. Donenfeld: >>>>> 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> >>>> [...] >>>>> + case 114: >>>>> + if (r1 & 1 || !r1 || r2 & 1 || !r2) >>>>> + tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); >>>>> + fill_buf_random(env, ra, &env->regs[r1], &env->regs[r1 + 1]); >>>>> + fill_buf_random(env, ra, &env->regs[r2], &env->regs[r2 + 1]); >>>>> + break; >>>> >>>> I think I agree with Harald that some aspects are missing. >>>> Linux does not seem to check, but we should also modify the query function to >>>> indicate the availability of 114. >>>> >>>> As the msa helper deals with many instructions >>>> ... >>>> target/s390x/tcg/insn-data.def: D(0xb91e, KMAC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMAC) >>>> target/s390x/tcg/insn-data.def: D(0xb928, PCKMO, RRE, MSA3, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCKMO) >>>> target/s390x/tcg/insn-data.def: D(0xb92a, KMF, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMF) >>>> target/s390x/tcg/insn-data.def: D(0xb92b, KMO, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMO) >>>> target/s390x/tcg/insn-data.def: D(0xb92c, PCC, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCC) >>>> target/s390x/tcg/insn-data.def: D(0xb92d, KMCTR, RRF_b, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMCTR) >>>> target/s390x/tcg/insn-data.def: D(0xb92e, KM, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KM) >>>> target/s390x/tcg/insn-data.def: D(0xb92f, KMC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMC) >>>> target/s390x/tcg/insn-data.def: D(0xb929, KMA, RRF_b, MSA8, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMA) >>>> target/s390x/tcg/insn-data.def: D(0xb93c, PPNO, RRE, MSA5, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PPNO) >>>> target/s390x/tcg/insn-data.def: D(0xb93e, KIMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KIMD) >>>> target/s390x/tcg/insn-data.def: D(0xb93f, KLMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KLMD) >>>> ... >>>> and in theory other instructions might also have 114 we should at least check that this is ppno/prno. >>>> Or we split out a prno helper from the msa helper. >>>> >>> >>> Doesn't >>> >>> s390_get_feat_block(type, subfunc); >>> if (!test_be_bit(fc, subfunc)) { >>> tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); >>> } >>> >>> check that? As long as we don't implement 114 for any other instruction. >>> that should properly fence off the other instructions. >> >> Right that would help. We should still take care of the query function. >> > s390_get_feat_block() should already take care of that as well, no? Ah right, yes it fills subfunc. So yes, that should do the trick. Sorry for the noise.
On 02.08.22 17:15, Christian Borntraeger wrote: > > > Am 02.08.22 um 16:53 schrieb David Hildenbrand: >> On 02.08.22 16:01, Christian Borntraeger wrote: >>> >>> >>> Am 02.08.22 um 15:54 schrieb David Hildenbrand: >>>> On 02.08.22 15:26, Christian Borntraeger wrote: >>>>> >>>>> >>>>> Am 20.07.22 um 14:08 schrieb Jason A. Donenfeld: >>>>>> 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> >>>>> [...] >>>>>> + case 114: >>>>>> + if (r1 & 1 || !r1 || r2 & 1 || !r2) >>>>>> + tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); >>>>>> + fill_buf_random(env, ra, &env->regs[r1], &env->regs[r1 + 1]); >>>>>> + fill_buf_random(env, ra, &env->regs[r2], &env->regs[r2 + 1]); >>>>>> + break; >>>>> >>>>> I think I agree with Harald that some aspects are missing. >>>>> Linux does not seem to check, but we should also modify the query function to >>>>> indicate the availability of 114. >>>>> >>>>> As the msa helper deals with many instructions >>>>> ... >>>>> target/s390x/tcg/insn-data.def: D(0xb91e, KMAC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMAC) >>>>> target/s390x/tcg/insn-data.def: D(0xb928, PCKMO, RRE, MSA3, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCKMO) >>>>> target/s390x/tcg/insn-data.def: D(0xb92a, KMF, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMF) >>>>> target/s390x/tcg/insn-data.def: D(0xb92b, KMO, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMO) >>>>> target/s390x/tcg/insn-data.def: D(0xb92c, PCC, RRE, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCC) >>>>> target/s390x/tcg/insn-data.def: D(0xb92d, KMCTR, RRF_b, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMCTR) >>>>> target/s390x/tcg/insn-data.def: D(0xb92e, KM, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KM) >>>>> target/s390x/tcg/insn-data.def: D(0xb92f, KMC, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMC) >>>>> target/s390x/tcg/insn-data.def: D(0xb929, KMA, RRF_b, MSA8, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMA) >>>>> target/s390x/tcg/insn-data.def: D(0xb93c, PPNO, RRE, MSA5, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PPNO) >>>>> target/s390x/tcg/insn-data.def: D(0xb93e, KIMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KIMD) >>>>> target/s390x/tcg/insn-data.def: D(0xb93f, KLMD, RRE, MSA, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KLMD) >>>>> ... >>>>> and in theory other instructions might also have 114 we should at least check that this is ppno/prno. >>>>> Or we split out a prno helper from the msa helper. >>>>> >>>> >>>> Doesn't >>>> >>>> s390_get_feat_block(type, subfunc); >>>> if (!test_be_bit(fc, subfunc)) { >>>> tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); >>>> } >>>> >>>> check that? As long as we don't implement 114 for any other instruction. >>>> that should properly fence off the other instructions. >>> >>> Right that would help. We should still take care of the query function. >>> >> s390_get_feat_block() should already take care of that as well, no? > > Ah right, yes it fills subfunc. So yes, that should do the trick. Sorry for the noise. > I had to look at that 2 times as well ...
Hi David, Christian, While this thread has your attention, I thought I'd reiterate my offer in: https://lore.kernel.org/qemu-devel/YuEoUwzDzBqFFpxe@zx2c4.com/ Do either of you want to "take ownership" of this patch to bring it past the finish line, and I can provide whatever additional crypto code you need for that? Jason
On 02.08.22 17:28, Jason A. Donenfeld wrote: > Hi David, Christian, > > While this thread has your attention, I thought I'd reiterate my offer in: > https://lore.kernel.org/qemu-devel/YuEoUwzDzBqFFpxe@zx2c4.com/ > > Do either of you want to "take ownership" of this patch to bring it > past the finish line, and I can provide whatever additional crypto > code you need for that? For me the patch is good enough. But sure, having a SHA512 implementation would be nice ... Long story short, I'll wire up whatever crypto stuff you can come up with ;)
On Wed, Jul 20, 2022 at 02:08:59PM +0200, Jason A. Donenfeld wrote: > + case 114: > + if (r1 & 1 || !r1 || r2 & 1 || !r2) > + tcg_s390_program_interrupt(env, PGM_SPECIFICATION, ra); This is already handled in op_msa. I'm going to remove it for v4.
On Tue, Aug 02, 2022 at 05:32:26PM +0200, David Hildenbrand wrote: > On 02.08.22 17:28, Jason A. Donenfeld wrote: > > Hi David, Christian, > > > > While this thread has your attention, I thought I'd reiterate my offer in: > > https://lore.kernel.org/qemu-devel/YuEoUwzDzBqFFpxe@zx2c4.com/ > > > > Do either of you want to "take ownership" of this patch to bring it > > past the finish line, and I can provide whatever additional crypto > > code you need for that? > > For me the patch is good enough. But sure, having a SHA512 > implementation would be nice ... > > Long story short, I'll wire up whatever crypto stuff you can come up with ;) Long story short, I started to take you up on that offer, but because I am an insane person, before I knew it, the whole thing was done... Patch series incoming. Jason
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..afd29f9cf0 100644 --- a/target/s390x/tcg/crypto_helper.c +++ b/target/s390x/tcg/crypto_helper.c @@ -12,12 +12,38 @@ #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_reg, uint64_t *len_reg) +{ + uint8_t tmp[256]; + uint64_t len = *len_reg; + int reg_len = 64; + + if (!(env->psw.mask & PSW_MASK_64)) { + len = (uint32_t)len; + reg_len = (env->psw.mask & PSW_MASK_32) ? 32 : 24; + } + + 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_reg), tmp[i], ra); + *buf_reg = deposit64(*buf_reg, 0, reg_len, *buf_reg + 1); + --*len_reg; + } + len -= block; + } +} + uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t r3, uint32_t type) { @@ -52,6 +78,12 @@ 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); + fill_buf_random(env, ra, &env->regs[r1], &env->regs[r1 + 1]); + fill_buf_random(env, ra, &env->regs[r2], &env->regs[r2 + 1]); + 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(-)