Re: [Qemu-devel] [PATCH v3 2/8] target/s390x: Implement CONVERT UNICODE insns

[Richard Henderson]

On 07/14/2017 10:25 PM, Aurelien Jarno wrote:

That said I still wonder if we can get generators like that:

| static void in1_r1n(DisasContext *s, DisasFields *f, DisasOps *o)
| {
| int r1 = get_field(s->fields, r1);
| o->in1 = tcg_const_i32(r1);
|
| #define SPEC_in1_r1n 0

and

| static void in1_r1n_even(DisasContext *s, DisasFields *f, DisasOps *o)
| {
| int r1 = get_field(s->fields, r1);
| o->in1 = tcg_const_i32(r1);
|
| #define SPEC_in1_r1n_even SPEC_r1_even


Well, not in to o->in1, obviously, since that's TCGv_i32 not TCGv_i64.
I suppose we could add something of the sort though.


r~

Re: [Qemu-devel] [PATCH v3 2/8] target/s390x: Implement CONVERT UNICODE insns

[Aurelien Jarno]

On 2017-07-14 14:23, Richard Henderson wrote:
> On 07/14/2017 11:08 AM, Aurelien Jarno wrote:
> > On 2017-07-11 17:18, Thomas Huth wrote:
> > > On 10.07.2017 22:45, Richard Henderson wrote:
> > > > Signed-off-by: Richard Henderson 
> > > > ---
> > > >   target/s390x/helper.h  |   6 +
> > > >   target/s390x/mem_helper.c  | 310 
> > > > +
> > > >   target/s390x/translate.c   |  43 +++
> > > >   target/s390x/insn-data.def |  13 ++
> > > >   4 files changed, 372 insertions(+)
> > > > 
> > > > diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> > > > index 23e8d1d..2793cf3 100644
> > > > --- a/target/s390x/helper.h
> > > > +++ b/target/s390x/helper.h
> > > > @@ -107,6 +107,12 @@ DEF_HELPER_2(stfle, i32, env, i64)
> > > >   DEF_HELPER_FLAGS_2(lpq, TCG_CALL_NO_WG, i64, env, i64)
> > > >   DEF_HELPER_FLAGS_4(stpq, TCG_CALL_NO_WG, void, env, i64, i64, i64)
> > > >   DEF_HELPER_4(mvcos, i32, env, i64, i64, i64)
> > > > +DEF_HELPER_4(cu12, i32, env, i32, i32, i32)
> > > > +DEF_HELPER_4(cu14, i32, env, i32, i32, i32)
> > > > +DEF_HELPER_4(cu21, i32, env, i32, i32, i32)
> > > > +DEF_HELPER_4(cu24, i32, env, i32, i32, i32)
> > > > +DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
> > > > +DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
> > > >   #ifndef CONFIG_USER_ONLY
> > > >   DEF_HELPER_3(servc, i32, env, i64, i64)
> > > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> > > > index 513b402..0b18560 100644
> > > > --- a/target/s390x/mem_helper.c
> > > > +++ b/target/s390x/mem_helper.c
> > > [...]
> > > > +static inline uint32_t convert_unicode(CPUS390XState *env, uint32_t r1,
> > > > +   uint32_t r2, uint32_t m3, 
> > > > uintptr_t ra,
> > > > +   decode_unicode_fn decode,
> > > > +   encode_unicode_fn encode)
> > > > +{
> > > > +uint64_t dst = get_address(env, r1);
> > > > +uint64_t dlen = get_length(env, r1 + 1);
> > > > +uint64_t src = get_address(env, r2);
> > > > +uint64_t slen = get_length(env, r2 + 1);
> > > > +bool enh_check = m3 & 1;
> > > > +int cc, i;
> > > 
> > > According to the PoP:
> > > 
> > > "The R1 and R2 fields [...] must designate an even-
> > >   numbered register; otherwise, a specification excep-
> > >   tion is recognized."
> > > 
> > > I think you should add such a check for even-numbered registers here.
> > 
> > Actually it should not be done here, but at translation time in
> > translate.c.
> > 
> > There are a few places where the register number is checked to be even
> > and later loaded into a temp. I guess that can be replaced by generators
> > instead?
> 
> Yes, that's done in a v3.5 patch that's a part of this thread.

Sorry I have some backlogs in all those mails, and I haven't seen it.
This is correct now.

That said I still wonder if we can get generators like that:

| static void in1_r1n(DisasContext *s, DisasFields *f, DisasOps *o)
| {   
| int r1 = get_field(s->fields, r1);
| o->in1 = tcg_const_i32(r1);
|
| #define SPEC_in1_r1n 0

and

| static void in1_r1n_even(DisasContext *s, DisasFields *f, DisasOps *o)
| {   
| int r1 = get_field(s->fields, r1);
| o->in1 = tcg_const_i32(r1);
|
| #define SPEC_in1_r1n_even SPEC_r1_even


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

Re: [Qemu-devel] [PATCH v3 2/8] target/s390x: Implement CONVERT UNICODE insns

[Richard Henderson]

On 07/14/2017 11:08 AM, Aurelien Jarno wrote:

On 2017-07-11 17:18, Thomas Huth wrote:

On 10.07.2017 22:45, Richard Henderson wrote:

Signed-off-by: Richard Henderson 
---
  target/s390x/helper.h  |   6 +
  target/s390x/mem_helper.c  | 310 +
  target/s390x/translate.c   |  43 +++
  target/s390x/insn-data.def |  13 ++
  4 files changed, 372 insertions(+)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 23e8d1d..2793cf3 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -107,6 +107,12 @@ DEF_HELPER_2(stfle, i32, env, i64)
  DEF_HELPER_FLAGS_2(lpq, TCG_CALL_NO_WG, i64, env, i64)
  DEF_HELPER_FLAGS_4(stpq, TCG_CALL_NO_WG, void, env, i64, i64, i64)
  DEF_HELPER_4(mvcos, i32, env, i64, i64, i64)
+DEF_HELPER_4(cu12, i32, env, i32, i32, i32)
+DEF_HELPER_4(cu14, i32, env, i32, i32, i32)
+DEF_HELPER_4(cu21, i32, env, i32, i32, i32)
+DEF_HELPER_4(cu24, i32, env, i32, i32, i32)
+DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
+DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
  
  #ifndef CONFIG_USER_ONLY

  DEF_HELPER_3(servc, i32, env, i64, i64)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 513b402..0b18560 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c

[...]

+static inline uint32_t convert_unicode(CPUS390XState *env, uint32_t r1,
+   uint32_t r2, uint32_t m3, uintptr_t ra,
+   decode_unicode_fn decode,
+   encode_unicode_fn encode)
+{
+uint64_t dst = get_address(env, r1);
+uint64_t dlen = get_length(env, r1 + 1);
+uint64_t src = get_address(env, r2);
+uint64_t slen = get_length(env, r2 + 1);
+bool enh_check = m3 & 1;
+int cc, i;


According to the PoP:

"The R1 and R2 fields [...] must designate an even-
  numbered register; otherwise, a specification excep-
  tion is recognized."

I think you should add such a check for even-numbered registers here.


Actually it should not be done here, but at translation time in
translate.c.

There are a few places where the register number is checked to be even
and later loaded into a temp. I guess that can be replaced by generators
instead?


Yes, that's done in a v3.5 patch that's a part of this thread.


r~

Re: [Qemu-devel] [PATCH v3 2/8] target/s390x: Implement CONVERT UNICODE insns

[Aurelien Jarno]

On 2017-07-11 17:18, Thomas Huth wrote:
> On 10.07.2017 22:45, Richard Henderson wrote:
> > Signed-off-by: Richard Henderson 
> > ---
> >  target/s390x/helper.h  |   6 +
> >  target/s390x/mem_helper.c  | 310 
> > +
> >  target/s390x/translate.c   |  43 +++
> >  target/s390x/insn-data.def |  13 ++
> >  4 files changed, 372 insertions(+)
> > 
> > diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> > index 23e8d1d..2793cf3 100644
> > --- a/target/s390x/helper.h
> > +++ b/target/s390x/helper.h
> > @@ -107,6 +107,12 @@ DEF_HELPER_2(stfle, i32, env, i64)
> >  DEF_HELPER_FLAGS_2(lpq, TCG_CALL_NO_WG, i64, env, i64)
> >  DEF_HELPER_FLAGS_4(stpq, TCG_CALL_NO_WG, void, env, i64, i64, i64)
> >  DEF_HELPER_4(mvcos, i32, env, i64, i64, i64)
> > +DEF_HELPER_4(cu12, i32, env, i32, i32, i32)
> > +DEF_HELPER_4(cu14, i32, env, i32, i32, i32)
> > +DEF_HELPER_4(cu21, i32, env, i32, i32, i32)
> > +DEF_HELPER_4(cu24, i32, env, i32, i32, i32)
> > +DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
> > +DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
> >  
> >  #ifndef CONFIG_USER_ONLY
> >  DEF_HELPER_3(servc, i32, env, i64, i64)
> > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> > index 513b402..0b18560 100644
> > --- a/target/s390x/mem_helper.c
> > +++ b/target/s390x/mem_helper.c
> [...]
> > +static inline uint32_t convert_unicode(CPUS390XState *env, uint32_t r1,
> > +   uint32_t r2, uint32_t m3, uintptr_t 
> > ra,
> > +   decode_unicode_fn decode,
> > +   encode_unicode_fn encode)
> > +{
> > +uint64_t dst = get_address(env, r1);
> > +uint64_t dlen = get_length(env, r1 + 1);
> > +uint64_t src = get_address(env, r2);
> > +uint64_t slen = get_length(env, r2 + 1);
> > +bool enh_check = m3 & 1;
> > +int cc, i;
> 
> According to the PoP:
> 
> "The R1 and R2 fields [...] must designate an even-
>  numbered register; otherwise, a specification excep-
>  tion is recognized."
> 
> I think you should add such a check for even-numbered registers here.

Actually it should not be done here, but at translation time in
translate.c.

There are a few places where the register number is checked to be even
and later loaded into a temp. I guess that can be replaced by generators
instead?

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

Re: [Qemu-devel] [PATCH v3 2/8] target/s390x: Implement CONVERT UNICODE insns

[Aurelien Jarno]

On 2017-07-10 10:45, Richard Henderson wrote:
> Signed-off-by: Richard Henderson 
> ---
>  target/s390x/helper.h  |   6 +
>  target/s390x/mem_helper.c  | 310 
> +
>  target/s390x/translate.c   |  43 +++
>  target/s390x/insn-data.def |  13 ++
>  4 files changed, 372 insertions(+)
> 

Besides the check for even r1 and r3, this now looks good.

Reviewed-by: Aurelien Jarno 

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

Re: [Qemu-devel] [PATCH v3 2/8] target/s390x: Implement CONVERT UNICODE insns

[Thomas Huth]

On 10.07.2017 22:45, Richard Henderson wrote:
> Signed-off-by: Richard Henderson 
> ---
>  target/s390x/helper.h  |   6 +
>  target/s390x/mem_helper.c  | 310 
> +
>  target/s390x/translate.c   |  43 +++
>  target/s390x/insn-data.def |  13 ++
>  4 files changed, 372 insertions(+)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 23e8d1d..2793cf3 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -107,6 +107,12 @@ DEF_HELPER_2(stfle, i32, env, i64)
>  DEF_HELPER_FLAGS_2(lpq, TCG_CALL_NO_WG, i64, env, i64)
>  DEF_HELPER_FLAGS_4(stpq, TCG_CALL_NO_WG, void, env, i64, i64, i64)
>  DEF_HELPER_4(mvcos, i32, env, i64, i64, i64)
> +DEF_HELPER_4(cu12, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu14, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu21, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu24, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>  
>  #ifndef CONFIG_USER_ONLY
>  DEF_HELPER_3(servc, i32, env, i64, i64)
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 513b402..0b18560 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
[...]
> +static inline uint32_t convert_unicode(CPUS390XState *env, uint32_t r1,
> +   uint32_t r2, uint32_t m3, uintptr_t 
> ra,
> +   decode_unicode_fn decode,
> +   encode_unicode_fn encode)
> +{
> +uint64_t dst = get_address(env, r1);
> +uint64_t dlen = get_length(env, r1 + 1);
> +uint64_t src = get_address(env, r2);
> +uint64_t slen = get_length(env, r2 + 1);
> +bool enh_check = m3 & 1;
> +int cc, i;

According to the PoP:

"The R1 and R2 fields [...] must designate an even-
 numbered register; otherwise, a specification excep-
 tion is recognized."

I think you should add such a check for even-numbered registers here.

> +/* Lest we fail to service interrupts in a timely manner, limit the
> +   amount of work we're willing to do.  For now, let's cap at 256.  */
> +for (i = 0; i < 256; ++i) {
> +uint32_t c, ilen, olen;
> +
> +cc = decode(env, src, slen, enh_check, ra, , );
> +if (unlikely(cc >= 0)) {
> +break;
> +}
> +cc = encode(env, dst, dlen, ra, c, );
> +if (unlikely(cc >= 0)) {
> +break;
> +}
> +
> +src += ilen;
> +slen -= ilen;
> +dst += olen;
> +dlen -= olen;
> +cc = 3;
> +}
> +
> +set_address(env, r1, dst);
> +set_length(env, r1 + 1, dlen);
> +set_address(env, r2, src);
> +set_length(env, r2 + 1, slen);
> +
> +return cc;
> +}

 Thomas