diff mbox

[2/5] target/s390x: Enforce instruction features

Message ID 20170615055356.20684-3-rth@twiddle.net (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Henderson June 15, 2017, 5:53 a.m. UTC 
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target/s390x/translate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Aurelien Jarno June 15, 2017, 7:01 a.m. UTC | #1 
On 2017-06-14 22:53, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <rth@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.

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
David Hildenbrand June 15, 2017, 11:28 a.m. UTC | #2 
On 15.06.2017 09:01, Aurelien Jarno wrote:
> On 2017-06-14 22:53, Richard Henderson wrote:
>> Signed-off-by: Richard Henderson <rth@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.

> 
> 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.

We should look continue implementing missing instructions to "unlock"
these features, so we can finally enable them in the CPU model, and
therefore indicate them to the guest.

> 
> Aurelien
>
Aurelien Jarno June 15, 2017, 12:53 p.m. UTC | #3 
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 <rth@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.
David Hildenbrand June 15, 2017, 1:10 p.m. UTC | #4 
>> 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.

That's true, glibc sometimes assumes a certain architecture level
without checking. So you're right, maybe we should defer this "big
hammer" change until we have all facilities as part of the qemu CPU
model. Then, e.g. runnning -cpu qemu will not break such stuff, however
e.g. -cpu z900 could correctly simulate that architecture level.

One option would be:

/* for now, we don't fake absence of features for the qemu model */
if (!object_dynamic_cast(cpu, "qemu-s390x-cpu") {
	dc.features = cpu->model->features;
}


...

if (s->features && !test_bit(insn->fac, s->features)) {
    gen_program_exception(s, PGM_OPERATION);
    return EXIT_NORETURN;
}

> 
> 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.

I think we don't even implement the PLO, so we're not even pre-z complete ;)
Aurelien Jarno June 15, 2017, 8:55 p.m. UTC | #5 
On 2017-06-15 15:10, David Hildenbrand wrote:
> 
> >> 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.
> 
> That's true, glibc sometimes assumes a certain architecture level
> without checking. So you're right, maybe we should defer this "big
> hammer" change until we have all facilities as part of the qemu CPU

Well the GNU libc itself correctly probe the facilities with stfl/stfle.
What happens is that newer instructions might be generated directly by
GCC if told to do so (with -march=xxx or the default architecture).


> model. Then, e.g. runnning -cpu qemu will not break such stuff, however
> e.g. -cpu z900 could correctly simulate that architecture level.
> 
> One option would be:
> 
> /* for now, we don't fake absence of features for the qemu model */
> if (!object_dynamic_cast(cpu, "qemu-s390x-cpu") {
> 	dc.features = cpu->model->features;
> }
> 
> 
> ...
> 
> if (s->features && !test_bit(insn->fac, s->features)) {
>     gen_program_exception(s, PGM_OPERATION);
>     return EXIT_NORETURN;
> }

I don't know that part of the code enough to tell if it is the good way
to do that, but certainly having a "qemu" CPU that supports all
instructions look like the way to go, especially for the linux-user
emulation.

Aurelien
diff mbox

Patch

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;