diff mbox

gdbstub: Synchronize CPU state unconditionally in gdb_set_cpu_pc

Message ID 1331569485-3559-1-git-send-email-peter.maydell@linaro.org
State Accepted
Commit a896d03bb5f70ed76b3e9e1acba864115cfb5e01
Headers show

Commit Message

Peter Maydell March 12, 2012, 4:24 p.m. UTC
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(-)

Comments

Peter Maydell April 3, 2012, 11:57 a.m. UTC | #1
Ping?

-- PMM

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
>
>
Christoffer Dall April 3, 2012, 2:58 p.m. UTC | #2
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
Peter Maydell April 10, 2012, 12:43 p.m. UTC | #3
Combination ping^2 and apology for forgetting to cc qemu-ppc.

-- PMM

On 3 April 2012 12:57, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ping?
>
> -- PMM
>
> 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
>>
>>
Alexander Graf April 16, 2012, 5:49 p.m. UTC | #4
Looks great to me, push it :)

Alex

On 10.04.2012, at 14:43, Peter Maydell wrote:

> Combination ping^2 and apology for forgetting to cc qemu-ppc.
> 
> -- PMM
> 
> On 3 April 2012 12:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Ping?
>> 
>> -- PMM
>> 
>> 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
>>> 
>>>
Peter Maydell April 16, 2012, 5:54 p.m. UTC | #5
gdbstub isn't my domain, patches should go via Anthony or Blue
I guess. Patchwork url:
 http://patchwork.ozlabs.org/patch/146153/

thanks
-- PMM

On 16 April 2012 18:49, Alexander Graf <agraf@suse.de> wrote:
> Looks great to me, push it :)
>
> Alex
>
> On 10.04.2012, at 14:43, Peter Maydell wrote:
>
>> Combination ping^2 and apology for forgetting to cc qemu-ppc.
>>
>> -- PMM
>>
>> On 3 April 2012 12:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Ping?
>>>
>>> -- PMM
>>>
>>> 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
>>>>
>>>>
>
Blue Swirl April 21, 2012, 1:46 p.m. UTC | #6
On Mon, Apr 16, 2012 at 17:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> gdbstub isn't my domain, patches should go via Anthony or Blue
> I guess. Patchwork url:
>  http://patchwork.ozlabs.org/patch/146153/

Thanks, applied.

>
> thanks
> -- PMM
>
> On 16 April 2012 18:49, Alexander Graf <agraf@suse.de> wrote:
>> Looks great to me, push it :)
>>
>> Alex
>>
>> On 10.04.2012, at 14:43, Peter Maydell wrote:
>>
>>> Combination ping^2 and apology for forgetting to cc qemu-ppc.
>>>
>>> -- PMM
>>>
>>> On 3 April 2012 12:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> Ping?
>>>>
>>>> -- PMM
>>>>
>>>> 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
>>>>>
>>>>>
>>
>
>
>
> --
> 12345678901234567890123456789012345678901234567890123456789012345678901234567890
>          1         2         3         4         5         6         7         8
diff mbox

Patch

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;