Re: [PATCH v2] target/s390x: support PRNO_TRNG instruction
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
Cc: David Hildenbrand
Cc: Richard Henderson
Cc: Cornelia Huck
Cc: Harald Freudenberger
Cc: Holger Dengler
Signed-off-by: Jason A. Donenfeld
---
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
Re: [PATCH v2] target/s390x: support PRNO_TRNG instruction
>> 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! -- Thanks, David / dhildenb
Re: [PATCH v2] target/s390x: support PRNO_TRNG instruction
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
> Cc: David Hildenbrand
> Cc: Richard Henderson
> Cc: Cornelia Huck
> Cc: Harald Freudenberger
> Cc: Holger Dengler
> Signed-off-by: Jason A. Donenfeld
> ---
> 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
Re: [PATCH v2] target/s390x: support PRNO_TRNG instruction
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
[PATCH v2] target/s390x: support PRNO_TRNG instruction
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
Cc: David Hildenbrand
Cc: Richard Henderson
Cc: Cornelia Huck
Cc: Harald Freudenberger
Cc: Holger Dengler
Signed-off-by: Jason A. Donenfeld
---
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();
--
2.35.1
