Re: [PATCH v2] target/s390x: support PRNO_TRNG instruction

[Jason A. Donenfeld]

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