On 2017-06-15 13:28, David Hildenbrand wrote: > On 15.06.2017 09:01, Aurelien Jarno wrote: > > On 2017-06-14 22:53, Richard Henderson wrote: > >> Signed-off-by: Richard Henderson <r...@twiddle.net> > >> --- > >> target/s390x/translate.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/target/s390x/translate.c b/target/s390x/translate.c > >> index af18ffb..48cee25 100644 > >> --- a/target/s390x/translate.c > >> +++ b/target/s390x/translate.c > >> @@ -55,6 +55,7 @@ typedef struct DisasFields DisasFields; > >> > >> struct DisasContext { > >> struct TranslationBlock *tb; > >> + const unsigned long *features; > >> const DisasInsn *insn; > >> DisasFields *fields; > >> uint64_t ex_value; > >> @@ -5600,6 +5601,12 @@ static ExitStatus translate_one(CPUS390XState *env, > >> DisasContext *s) > >> } > >> #endif > >> > >> + /* Check for insn feature enabled. */ > >> + if (!test_bit(insn->fac, s->features)) { > >> + gen_program_exception(s, PGM_OPERATION); > >> + return EXIT_NORETURN; > >> + } > >> + > >> /* Check for insn specification exceptions. */ > >> if (insn->spec) { > >> int spec = insn->spec, excp = 0, r; > >> @@ -5726,6 +5733,7 @@ void gen_intermediate_code(CPUS390XState *env, > >> struct TranslationBlock *tb) > >> } > >> > >> dc.tb = tb; > >> + dc.features = cpu->model->features; > >> dc.pc = pc_start; > >> dc.cc_op = CC_OP_DYNAMIC; > >> dc.ex_value = tb->cs_base; > > > > It looks correct technically. Now I am not sure we want to do that, > > without at least provided a user to enable those instructions. There are > > a lot facilities that are partially implemented, usually because only > > the really used instructions are implemented, but also because some > > facilities are grouped in a single feature bit. This includes for > > example FPE, LPP, MIE, LAT, IEEEE_SIM and more. > > A "sane" guest (e.g. Linux) will only use an instruction if the > corresponding stfl(e) bit is set. So in my opinion, this should be just > fine. If the bit is not set currently, the guest will not use it == dead > code.
Not necessarily. Depending on the distribution, gcc and hence binaries default to a different ISA. Over the time people have added the corresponding instructions to QEMU so that these binaries work. Now given that GCC does not necessarily use all the instructions from a given facility, we end up with missing instructions. Taking this to its logical extreme, given we don't fully implement the Z facility (for example the HFP instructions are missing), we should prevent all the programs to run until that is fixed. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net