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
