Re: [Qemu-devel] [PATCH 2/4] s390x: remove duplicate definitions of DIAG 501

[Christian Borntraeger]

On 30/05/14 10:21, Alexander Graf wrote:
> 
> On 12.05.14 14:15, Jens Freimann wrote:
>> From: David Hildenbrand <address@hidden>
>>
>> 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 <address@hidden>
>> Reviewed-by: Cornelia Huck <address@hidden>
>> Signed-off-by: Jens Freimann <address@hidden>
> 
> 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;
>>       }
>>   
>