| Message ID | 1379945085-29086-2-git-send-email-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
On 09/23/2013 04:04 PM, Richard Henderson wrote: > Rather than simply hard-coding them in STFL instruction. > > Signed-off-by: Richard Henderson<rth@twiddle.net> > --- > 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 :). > + > if (tcg_enabled()&& !inited) { > inited = true; > s390x_translate_init(); > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h > index 8be5648..746aec8 100644 > --- a/target-s390x/cpu.h > +++ b/target-s390x/cpu.h > @@ -136,6 +136,7 @@ typedef struct CPUS390XState { > CPU_COMMON > > /* reset does memset(0) up to here */ > + uint64_t facilities[2]; > > int cpu_num; > uint8_t *storage_keys; > diff --git a/target-s390x/translate.c b/target-s390x/translate.c > index afe90eb..d4dc8ea 100644 > --- a/target-s390x/translate.c > +++ b/target-s390x/translate.c > @@ -3230,12 +3230,12 @@ static ExitStatus op_spt(DisasContext *s, DisasOps *o) > > static ExitStatus op_stfl(DisasContext *s, DisasOps *o) > { > - 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. Alex
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<rth@twiddle.net> >> --- >> 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. Pointers? >> static ExitStatus op_stfl(DisasContext *s, DisasOps *o) >> { >> - 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. r~
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<rth@twiddle.net> >>> --- >>> 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 :). > > Pointers? > >>> static ExitStatus op_stfl(DisasContext *s, DisasOps *o) >>> { >>> - 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 :). Alex
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<rth@twiddle.net> >>>> --- >>>> 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? >>>> - 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. r~
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<rth@twiddle.net> >>>>> --- >>>>> 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
On 10/01/2013 08:54 AM, Alexander Graf wrote: > 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. Yes, the code to check is in "translate_one". Which, as you might imagine, translates one instruction. ;-) r~
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; + if (tcg_enabled() && !inited) { inited = true; s390x_translate_init(); diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h index 8be5648..746aec8 100644 --- a/target-s390x/cpu.h +++ b/target-s390x/cpu.h @@ -136,6 +136,7 @@ typedef struct CPUS390XState { CPU_COMMON /* reset does memset(0) up to here */ + uint64_t facilities[2]; int cpu_num; uint8_t *storage_keys; diff --git a/target-s390x/translate.c b/target-s390x/translate.c index afe90eb..d4dc8ea 100644 --- a/target-s390x/translate.c +++ b/target-s390x/translate.c @@ -3230,12 +3230,12 @@ static ExitStatus op_spt(DisasContext *s, DisasOps *o) static ExitStatus op_stfl(DisasContext *s, DisasOps *o) { - 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); tcg_gen_qemu_st32(f, a, get_mem_index(s)); tcg_temp_free_i64(f); tcg_temp_free_i64(a);
Rather than simply hard-coding them in STFL instruction. Signed-off-by: Richard Henderson <rth@twiddle.net> --- target-s390x/cpu.c | 3 +++ target-s390x/cpu.h | 1 + target-s390x/translate.c | 10 +++++----- 3 files changed, 9 insertions(+), 5 deletions(-)