Re: [Qemu-devel] [PATCH 2/5] target/s390x: Enforce instruction features

[Aurelien Jarno]

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 <address@hidden>
> >> ---
> >>  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
address@hidden                 http://www.aurel32.net