Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Alexander Graf]

On 10/01/2013 05:52 PM, Richard Henderson wrote:
> On 10/01/2013 08:48 AM, Alexander Graf wrote:
> > On 09/30/2013 09:15 PM, Richard Henderson wrote:
> > > On 09/30/2013 11:03 AM, Alexander Graf wrote:
> > > > On 09/23/2013 04:04 PM, Richard Henderson wrote:
> > > > > Rather than simply hard-coding them in STFL instruction.
> > > > >
> > > > > Signed-off-by: Richard Henderson<address@hidden>
> > > > > ---
> > > > >     target-s390x/cpu.c       |  3 +++
> > > > >     target-s390x/cpu.h       |  1 +
> > > > >     target-s390x/translate.c | 10 +++++-----
> > > > >     3 files changed, 9 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> > > > > index 3c89f8a..ff691df 100644
> > > > > --- a/target-s390x/cpu.c
> > > > > +++ b/target-s390x/cpu.c
> > > > > @@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj)
> > > > >         env->cpu_num = cpu_num++;
> > > > >         env->ext_index = -1;
> > > > >
> > > > > +    env->facilities[0] = 0xc000000000000000ull;
> > > > > +    env->facilities[1] = 0;
> > > > Could we add CPU definitions along the way here? I'm fine with making z9 the
> > > > default CPU type, but we should make this explicit :).
> > > Certainly that's what we should do.  I just hadn't yet researched the
> > > currently correct way to do that.  I know there's some amount of out-of-date
> > > examples in the current source base.
> > I'll leave the answer to that to Andreas :).
> Can we leave that for a separate patch series then?

Just make sure you actually check for feature bits on every instruction 
(which I think you do, but the current code is way too magical to me to 
really understand it anymore) so that we can always implement a z900 cpu 
type later on.

> > > > > -    TCGv_i64 f, a;
> > > > > -    /* We really ought to have more complete indication of facilities
> > > > > -       that we implement.  Address this when STFLE is implemented.  */
> > > > > +    TCGv_i64 f = tcg_temp_new_i64();
> > > > > +    TCGv_i64 a = tcg_const_i64(200);
> > > > > +
> > > > >         check_privileged(s);
> > > > > -    f = tcg_const_i64(0xc0000000);
> > > > > -    a = tcg_const_i64(200);
> > > > > +    tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0]));
> > > > > +    tcg_gen_shri_i64(f, f, 32);
> > > > IMHO the facility list should be stored in DisasContext. That way we can check
> > > > whether we're generating code against the correct target.
> > > See patch 4.
> > >
> > > As for the code we generate here, does it really matter if we load the value
> > > from env, or have it encoded as a constant?  It still has to get stored to
> > > memory, so it's not like the TCG optimizer is going to do anything with the
> > > constant.
> > No, it only seemed more straight forward to me from a "single source of
> > information" point of view. But it really doesn't matter. Shifting in C seems
> > to be easier to read :).
> Fair enough.  I'll rearrange the order of the patches so that we can
> update STFL to use the DisasContext data.

Thanks :)


Alex