[2/4] s390x: remove duplicate definitions of DIAG 501

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

When restoring the previously saved instruction in 
kvm_arch_remove_sw_breakpoint(), we only restored one byte. Let's use
the sizeof() operator to make sure we restore the entire instruction.

While we are at it, let's remove the duplicate definitions of DIAG 501
and replace its size (used when reading/writing the instruction) with
a sizeof() operator to make the code self explaining and less error-prone.


Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 target-s390x/kvm.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

On 12.05.14 14:15, Jens Freimann wrote:
> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>
> When restoring the previously saved instruction in
> kvm_arch_remove_sw_breakpoint(), we only restored one byte. Let's use
> the sizeof() operator to make sure we restore the entire instruction.
>
> While we are at it, let's remove the duplicate definitions of DIAG 501
> and replace its size (used when reading/writing the instruction) with
> a sizeof() operator to make the code self explaining and less error-prone.
>
>
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>

If the instruction we replace is 2 or 6 bytes long, doesn't this break 
the code flow?


Alex

> ---
>   target-s390x/kvm.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index b7b0edc..70cfe74 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -347,12 +347,16 @@ static void *legacy_s390_alloc(size_t size)
>       return mem == MAP_FAILED ? NULL : mem;
>   }
>   
> +/* DIAG 501 is used for sw breakpoints */
> +static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
> +
>   int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>   {
> -    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
>   
> -    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> -        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)diag_501, 4, 1)) {
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> +                            sizeof(diag_501), 0) ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)diag_501,
> +                            sizeof(diag_501), 1)) {
>           return -EINVAL;
>       }
>       return 0;
> @@ -360,14 +364,14 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>   
>   int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>   {
> -    uint8_t t[4];
> -    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
> +    uint8_t t[sizeof(diag_501)];
>   
> -    if (cpu_memory_rw_debug(cs, bp->pc, t, 4, 0)) {
> +    if (cpu_memory_rw_debug(cs, bp->pc, t, sizeof(diag_501), 0)) {
>           return -EINVAL;
> -    } else if (memcmp(t, diag_501, 4)) {
> +    } else if (memcmp(t, diag_501, sizeof(diag_501))) {
>           return -EINVAL;
> -    } else if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1)) {
> +    } else if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> +                                   sizeof(diag_501), 1)) {
>           return -EINVAL;
>       }
>

On 30/05/14 10:21, Alexander Graf wrote:
> 
> On 12.05.14 14:15, Jens Freimann wrote:
>> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>
>> When restoring the previously saved instruction in
>> kvm_arch_remove_sw_breakpoint(), we only restored one byte. Let's use
>> the sizeof() operator to make sure we restore the entire instruction.
>>
>> While we are at it, let's remove the duplicate definitions of DIAG 501
>> and replace its size (used when reading/writing the instruction) with
>> a sizeof() operator to make the code self explaining and less error-prone.
>>
>>
>> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> 
> If the instruction we replace is 2 or 6 bytes long, doesn't this break the code flow?

This code is not worse than your original code, since the old code also overwrote 4 bytes.
Now, this patch fixes a real bug: The old code wrote 4 bytes, but only restored the first one.

6 bytes instructions are fine, since addr+2 or addr+4 will never be a jump target.
Same for 4 byte instructions.
2 byte instructions do have a problem as addr+2 could be a jump target. Andreas
Arnez (s390 gdb developer) already made us aware of this. A 2 byte opcode for break
points (like normal gdb on s390) will be safer. We are currently looking for the best
instruction that always intercepts that can replace diag501.

Now: this patch consolidates all users to one data structure and uses sizeof.
So replacing diag501 should become less error prone with this patch. 

(Its already merged anyway, so we will provide a followup improvement when ready).

Christian

> 
> 
> Alex
> 
>> ---
>>   target-s390x/kvm.c | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>> index b7b0edc..70cfe74 100644
>> --- a/target-s390x/kvm.c
>> +++ b/target-s390x/kvm.c
>> @@ -347,12 +347,16 @@ static void *legacy_s390_alloc(size_t size)
>>       return mem == MAP_FAILED ? NULL : mem;
>>   }
>>   +/* DIAG 501 is used for sw breakpoints */
>> +static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
>> +
>>   int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>>   {
>> -    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
>>   -    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
>> -        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)diag_501, 4, 1)) {
>> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
>> +                            sizeof(diag_501), 0) ||
>> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)diag_501,
>> +                            sizeof(diag_501), 1)) {
>>           return -EINVAL;
>>       }
>>       return 0;
>> @@ -360,14 +364,14 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>>     int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>>   {
>> -    uint8_t t[4];
>> -    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
>> +    uint8_t t[sizeof(diag_501)];
>>   -    if (cpu_memory_rw_debug(cs, bp->pc, t, 4, 0)) {
>> +    if (cpu_memory_rw_debug(cs, bp->pc, t, sizeof(diag_501), 0)) {
>>           return -EINVAL;
>> -    } else if (memcmp(t, diag_501, 4)) {
>> +    } else if (memcmp(t, diag_501, sizeof(diag_501))) {
>>           return -EINVAL;
>> -    } else if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1)) {
>> +    } else if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
>> +                                   sizeof(diag_501), 1)) {
>>           return -EINVAL;
>>       }
>>   
>