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

[Aurelien Jarno]

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.

We could maybe provide a way to override the check, or only enable
enforcement for fully implemented facilities. Failing to do so means we
just introduce a regression from the user point of view (many binaries
will stop working) and also that we change thousand of lines of code
into dead code.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net