RNSBG is handled via the op_rosbg() helper function. But RNSBG has
the opcode 0xEC54, i.e. 0x54 as second byte, while op_rosbg() currently
checks for 0x55. This seems to be a typo, fix it to use 0x54 instead,
so that op_rosbg() does not abort() anymore if a program uses RNSBG.

I've checked with a simply test function that I now get the same results
with KVM and with TCG:

 static void test_rnsbg(void)
 {
	uint64_t r1, r2;

	r2 = 0xffff000000000000UL;
	r1 = 0x123456789bdfaaaaUL;
	asm volatile (" rnsbg %0,%1,12,61,16 " : "+r"(r1) : "r"(r2));

	printf("r1 afterwards: 0x%lx\n", r1);
 }

Buglink: https://bugs.launchpad.net/qemu/+bug/1860920
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/s390x/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 0bd2073718..4f6f1e31cd 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3874,7 +3874,7 @@ static DisasJumpType op_rosbg(DisasContext *s, DisasOps *o)
 
     /* Operate.  */
     switch (s->fields.op2) {
-    case 0x55: /* AND */
+    case 0x54: /* AND */
         tcg_gen_ori_i64(o->in2, o->in2, ~mask);
         tcg_gen_and_i64(o->out, o->out, o->in2);
         break;
-- 
2.18.1

On Thu, 30 Jan 2020 14:34:17 +0100
Thomas Huth <thuth@redhat.com> wrote:

> RNSBG is handled via the op_rosbg() helper function. But RNSBG has
> the opcode 0xEC54, i.e. 0x54 as second byte, while op_rosbg() currently
> checks for 0x55. This seems to be a typo, fix it to use 0x54 instead,
> so that op_rosbg() does not abort() anymore if a program uses RNSBG.
> 
> I've checked with a simply test function that I now get the same results

s/simply/simple/

> with KVM and with TCG:
> 
>  static void test_rnsbg(void)
>  {
> 	uint64_t r1, r2;
> 
> 	r2 = 0xffff000000000000UL;
> 	r1 = 0x123456789bdfaaaaUL;
> 	asm volatile (" rnsbg %0,%1,12,61,16 " : "+r"(r1) : "r"(r2));
> 
> 	printf("r1 afterwards: 0x%lx\n", r1);
>  }
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1860920
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/s390x/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 0bd2073718..4f6f1e31cd 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -3874,7 +3874,7 @@ static DisasJumpType op_rosbg(DisasContext *s, DisasOps *o)
>  
>      /* Operate.  */
>      switch (s->fields.op2) {
> -    case 0x55: /* AND */
> +    case 0x54: /* AND */
>          tcg_gen_ori_i64(o->in2, o->in2, ~mask);
>          tcg_gen_and_i64(o->out, o->out, o->in2);
>          break;

Thanks, applied.

On 30.01.20 14:34, Thomas Huth wrote:
> RNSBG is handled via the op_rosbg() helper function. But RNSBG has
> the opcode 0xEC54, i.e. 0x54 as second byte, while op_rosbg() currently
> checks for 0x55. This seems to be a typo, fix it to use 0x54 instead,
> so that op_rosbg() does not abort() anymore if a program uses RNSBG.
> 
> I've checked with a simply test function that I now get the same results
> with KVM and with TCG:
> 
>  static void test_rnsbg(void)
>  {
> 	uint64_t r1, r2;
> 
> 	r2 = 0xffff000000000000UL;
> 	r1 = 0x123456789bdfaaaaUL;
> 	asm volatile (" rnsbg %0,%1,12,61,16 " : "+r"(r1) : "r"(r2));
> 
> 	printf("r1 afterwards: 0x%lx\n", r1);
>  }

You could add a tcg test case for that :)

> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1860920
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/s390x/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 0bd2073718..4f6f1e31cd 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -3874,7 +3874,7 @@ static DisasJumpType op_rosbg(DisasContext *s, DisasOps *o)
>  
>      /* Operate.  */
>      switch (s->fields.op2) {
> -    case 0x55: /* AND */
> +    case 0x54: /* AND */
>          tcg_gen_ori_i64(o->in2, o->in2, ~mask);
>          tcg_gen_and_i64(o->out, o->out, o->in2);
>          break;
> 

Fixes: d6c6372e186e ("target-s390: Implement R[NOX]SBG")

Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

-- 
Thanks,

David / dhildenb

On 30/01/2020 14.45, David Hildenbrand wrote:
> On 30.01.20 14:34, Thomas Huth wrote:
>> RNSBG is handled via the op_rosbg() helper function. But RNSBG has
>> the opcode 0xEC54, i.e. 0x54 as second byte, while op_rosbg() currently
>> checks for 0x55. This seems to be a typo, fix it to use 0x54 instead,
>> so that op_rosbg() does not abort() anymore if a program uses RNSBG.
>>
>> I've checked with a simply test function that I now get the same results
>> with KVM and with TCG:
>>
>>  static void test_rnsbg(void)
>>  {
>> 	uint64_t r1, r2;
>>
>> 	r2 = 0xffff000000000000UL;
>> 	r1 = 0x123456789bdfaaaaUL;
>> 	asm volatile (" rnsbg %0,%1,12,61,16 " : "+r"(r1) : "r"(r2));
>>
>> 	printf("r1 afterwards: 0x%lx\n", r1);
>>  }
> 
> You could add a tcg test case for that :)

I already thought about it ... I'll have a try when I've got some spare
time.

 Thomas