Re: [Qemu-devel] [PATCH] gdbstub: Synchronize CPU state unconditionally in gdb_set_cpu_pc

[Christoffer Dall <c.dall@virtualopensystems.com>]

On Tue, Apr 3, 2012 at 7:57 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ping?
>
sorry about late response.

>
> On 12 March 2012 16:24, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Synchronize the CPU state via cpu_sychronize_state() unconditionally
>> in gdb_set_cpu_pc() rather than only in some of the target ifdef
>> ladder cases.
>>
>> We can divide the CPUs into three categories:
>>  * non-KVM targets: no change of behaviour since we will use the
>>   kvm-stub.c no-op function.
>>  * i386 and s390: no change of behaviour since they were already
>>   calling this function
>>  * PPC (in KVM mode): this fixes an error: failing to synchronise
>>   was accidental and probably a bug.
>>
>> This also paves the way for other targets (specifically ARM) which
>> can add KVM support in future without having to add another target
>> specific change to this bit of code.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Alex: could you test the KVM PPC case, please, since that's the only
>> one where we actually change behaviour here?
>>
>>  gdbstub.c |    3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index ef95ac2..776dcc5 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1904,8 +1904,8 @@ static void gdb_breakpoint_remove_all(void)
>>
>>  static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
>>  {
>> -#if defined(TARGET_I386)
>>     cpu_synchronize_state(s->c_cpu);
>> +#if defined(TARGET_I386)
>>     s->c_cpu->eip = pc;
>>  #elif defined (TARGET_PPC)
>>     s->c_cpu->nip = pc;
>> @@ -1930,7 +1930,6 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
>>  #elif defined (TARGET_ALPHA)
>>     s->c_cpu->pc = pc;
>>  #elif defined (TARGET_S390X)
>> -    cpu_synchronize_state(s->c_cpu);
>>     s->c_cpu->psw.addr = pc;
>>  #elif defined (TARGET_LM32)
>>     s->c_cpu->pc = pc;
>> --
>> 1.7.1
>>

looks good to me - much cleaner than what I had to do for ARM otherwise.

-Christoffer