* Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
@ 2015-04-27 12:02 ` Michael Tokarev
0 siblings, 0 replies; 22+ messages in thread
From: Michael Tokarev @ 2015-04-27 12:02 UTC (permalink / raw)
To: Peter Crosthwaite, qemu-devel
Cc: qemu-trivial, peter.maydell, Peter Crosthwaite
27.04.2015 04:38, Peter Crosthwaite wrote:
> No code uses the cpu_pc_from_tb() function. Delete.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> target-arm/cpu.h | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 7069103..c9c5d30 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1874,15 +1874,6 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>
> #include "exec/exec-all.h"
>
> -static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
This function is also used in target-tricore/cpu.h, and is mentioned
in comments in tcg/tcg.h. Should these be removed as well?
Thanks,
/mjt
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-trivial] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
2015-04-27 12:02 ` [Qemu-devel] " Michael Tokarev
@ 2015-04-27 12:06 ` Michael Tokarev
-1 siblings, 0 replies; 22+ messages in thread
From: Michael Tokarev @ 2015-04-27 12:06 UTC (permalink / raw)
To: Peter Crosthwaite, qemu-devel
Cc: qemu-trivial, peter.maydell, Peter Crosthwaite
27.04.2015 15:02, Michael Tokarev wrote:
> 27.04.2015 04:38, Peter Crosthwaite wrote:
>> No code uses the cpu_pc_from_tb() function. Delete.
> This function is also used in target-tricore/cpu.h, and is mentioned
Not _used_ but defined, the same way as it is defined but not used in
target-arm/cpu.h.
> in comments in tcg/tcg.h. Should these be removed as well?
Thanks,
/mjt
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
@ 2015-04-27 12:06 ` Michael Tokarev
0 siblings, 0 replies; 22+ messages in thread
From: Michael Tokarev @ 2015-04-27 12:06 UTC (permalink / raw)
To: Peter Crosthwaite, qemu-devel
Cc: qemu-trivial, peter.maydell, Peter Crosthwaite
27.04.2015 15:02, Michael Tokarev wrote:
> 27.04.2015 04:38, Peter Crosthwaite wrote:
>> No code uses the cpu_pc_from_tb() function. Delete.
> This function is also used in target-tricore/cpu.h, and is mentioned
Not _used_ but defined, the same way as it is defined but not used in
target-arm/cpu.h.
> in comments in tcg/tcg.h. Should these be removed as well?
Thanks,
/mjt
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-trivial] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
2015-04-27 12:02 ` [Qemu-devel] " Michael Tokarev
@ 2015-04-27 12:58 ` Peter Maydell
-1 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2015-04-27 12:58 UTC (permalink / raw)
To: Michael Tokarev
Cc: QEMU Trivial, Peter Crosthwaite, QEMU Developers,
Andreas Färber, Peter Crosthwaite
On 27 April 2015 at 13:02, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 27.04.2015 04:38, Peter Crosthwaite wrote:
>> No code uses the cpu_pc_from_tb() function. Delete.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> target-arm/cpu.h | 9 ---------
>> 1 file changed, 9 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 7069103..c9c5d30 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1874,15 +1874,6 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>>
>> #include "exec/exec-all.h"
>>
>> -static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
>
> This function is also used in target-tricore/cpu.h, and is mentioned
> in comments in tcg/tcg.h. Should these be removed as well?
The tcg/tcg.h comment should be updated:
* Otherwise, we gave up on execution of this TB before it started, and
* the caller must fix up the CPU state by calling the CPU's
* synchronize_from_tb() method with the next-TB pointer we return.
* (falling back to calling the CPU's set_pc() method with tb->pc
* if no synchronize_from_tb() method exists.)
That's a bit clunky though, which suggests we should
have a cpu_synchronize_from_tb() inline function in qom/cpu.h
which does the
CPUClass *cc = CPU_GET_CLASS(cpu);
if (cc->synchronize_from_tb) {
cc->synchronize_from_tb(cpu, tb);
} else {
assert(cc->set_pc);
cc->set_pc(cpu, tb->pc);
}
bit that cpu-exec.c currently open-codes.
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
@ 2015-04-27 12:58 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2015-04-27 12:58 UTC (permalink / raw)
To: Michael Tokarev
Cc: QEMU Trivial, Peter Crosthwaite, QEMU Developers,
Andreas Färber, Peter Crosthwaite
On 27 April 2015 at 13:02, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 27.04.2015 04:38, Peter Crosthwaite wrote:
>> No code uses the cpu_pc_from_tb() function. Delete.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> target-arm/cpu.h | 9 ---------
>> 1 file changed, 9 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 7069103..c9c5d30 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1874,15 +1874,6 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>>
>> #include "exec/exec-all.h"
>>
>> -static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
>
> This function is also used in target-tricore/cpu.h, and is mentioned
> in comments in tcg/tcg.h. Should these be removed as well?
The tcg/tcg.h comment should be updated:
* Otherwise, we gave up on execution of this TB before it started, and
* the caller must fix up the CPU state by calling the CPU's
* synchronize_from_tb() method with the next-TB pointer we return.
* (falling back to calling the CPU's set_pc() method with tb->pc
* if no synchronize_from_tb() method exists.)
That's a bit clunky though, which suggests we should
have a cpu_synchronize_from_tb() inline function in qom/cpu.h
which does the
CPUClass *cc = CPU_GET_CLASS(cpu);
if (cc->synchronize_from_tb) {
cc->synchronize_from_tb(cpu, tb);
} else {
assert(cc->set_pc);
cc->set_pc(cpu, tb->pc);
}
bit that cpu-exec.c currently open-codes.
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-trivial] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
2015-04-27 12:58 ` [Qemu-devel] " Peter Maydell
@ 2015-04-27 19:00 ` Peter Maydell
-1 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2015-04-27 19:00 UTC (permalink / raw)
To: Michael Tokarev
Cc: QEMU Trivial, Peter Crosthwaite, QEMU Developers,
Andreas Färber, Peter Crosthwaite
On 27 April 2015 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> The tcg/tcg.h comment should be updated:
> * Otherwise, we gave up on execution of this TB before it started, and
> * the caller must fix up the CPU state by calling the CPU's
> * synchronize_from_tb() method with the next-TB pointer we return.
> * (falling back to calling the CPU's set_pc() method with tb->pc
> * if no synchronize_from_tb() method exists.)
>
> That's a bit clunky though, which suggests we should
> have a cpu_synchronize_from_tb() inline function in qom/cpu.h
> which does the
> CPUClass *cc = CPU_GET_CLASS(cpu);
> if (cc->synchronize_from_tb) {
> cc->synchronize_from_tb(cpu, tb);
> } else {
> assert(cc->set_pc);
> cc->set_pc(cpu, tb->pc);
> }
>
> bit that cpu-exec.c currently open-codes.
...except that qom/cpu.h doesn't have the definition of the
TranslationBlock struct it would need to be able to do that "tb->pc".
* we can't just include exec-all.h from here or otherwise get
the TranslationBlock struct definition, because it is target
CPU dependent
* we can't have the common baseclass in qom/cpu.c provide an
implementation of the synchronize_from_tb method which calls
set_pc, because qom/cpu.c is a common-obj-y sourcefile]
which leaves us with:
* have cpu_synchronize_from_tb() take both tb and tb->pc as args,
which is pretty yucky
* give up and just update the tcg.h comment as above
I think I go for "just update the comment"...
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
@ 2015-04-27 19:00 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2015-04-27 19:00 UTC (permalink / raw)
To: Michael Tokarev
Cc: QEMU Trivial, Peter Crosthwaite, QEMU Developers,
Andreas Färber, Peter Crosthwaite
On 27 April 2015 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> The tcg/tcg.h comment should be updated:
> * Otherwise, we gave up on execution of this TB before it started, and
> * the caller must fix up the CPU state by calling the CPU's
> * synchronize_from_tb() method with the next-TB pointer we return.
> * (falling back to calling the CPU's set_pc() method with tb->pc
> * if no synchronize_from_tb() method exists.)
>
> That's a bit clunky though, which suggests we should
> have a cpu_synchronize_from_tb() inline function in qom/cpu.h
> which does the
> CPUClass *cc = CPU_GET_CLASS(cpu);
> if (cc->synchronize_from_tb) {
> cc->synchronize_from_tb(cpu, tb);
> } else {
> assert(cc->set_pc);
> cc->set_pc(cpu, tb->pc);
> }
>
> bit that cpu-exec.c currently open-codes.
...except that qom/cpu.h doesn't have the definition of the
TranslationBlock struct it would need to be able to do that "tb->pc".
* we can't just include exec-all.h from here or otherwise get
the TranslationBlock struct definition, because it is target
CPU dependent
* we can't have the common baseclass in qom/cpu.c provide an
implementation of the synchronize_from_tb method which calls
set_pc, because qom/cpu.c is a common-obj-y sourcefile]
which leaves us with:
* have cpu_synchronize_from_tb() take both tb and tb->pc as args,
which is pretty yucky
* give up and just update the tcg.h comment as above
I think I go for "just update the comment"...
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-trivial] [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
2015-04-27 19:00 ` [Qemu-devel] " Peter Maydell
@ 2015-04-27 19:06 ` Peter Crosthwaite
-1 siblings, 0 replies; 22+ messages in thread
From: Peter Crosthwaite @ 2015-04-27 19:06 UTC (permalink / raw)
To: Peter Maydell
Cc: Peter Crosthwaite, QEMU Trivial, Michael Tokarev, QEMU Developers,
Peter Crosthwaite, Andreas Färber
On Mon, Apr 27, 2015 at 12:00 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 27 April 2015 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The tcg/tcg.h comment should be updated:
>> * Otherwise, we gave up on execution of this TB before it started, and
>> * the caller must fix up the CPU state by calling the CPU's
>> * synchronize_from_tb() method with the next-TB pointer we return.
>> * (falling back to calling the CPU's set_pc() method with tb->pc
>> * if no synchronize_from_tb() method exists.)
>>
>> That's a bit clunky though, which suggests we should
>> have a cpu_synchronize_from_tb() inline function in qom/cpu.h
>> which does the
>> CPUClass *cc = CPU_GET_CLASS(cpu);
>> if (cc->synchronize_from_tb) {
>> cc->synchronize_from_tb(cpu, tb);
>> } else {
>> assert(cc->set_pc);
>> cc->set_pc(cpu, tb->pc);
>> }
>>
>> bit that cpu-exec.c currently open-codes.
>
> ...except that qom/cpu.h doesn't have the definition of the
> TranslationBlock struct it would need to be able to do that "tb->pc".
> * we can't just include exec-all.h from here or otherwise get
> the TranslationBlock struct definition, because it is target
> CPU dependent
> * we can't have the common baseclass in qom/cpu.c provide an
> implementation of the synchronize_from_tb method which calls
> set_pc, because qom/cpu.c is a common-obj-y sourcefile]
> which leaves us with:
> * have cpu_synchronize_from_tb() take both tb and tb->pc as args,
> which is pretty yucky
> * give up and just update the tcg.h comment as above
>
> I think I go for "just update the comment"...
>
Does that mean a respin of this patch or should the tricore fix and
the comment be a follow up?
Regards,
Peter
> -- PMM
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
@ 2015-04-27 19:06 ` Peter Crosthwaite
0 siblings, 0 replies; 22+ messages in thread
From: Peter Crosthwaite @ 2015-04-27 19:06 UTC (permalink / raw)
To: Peter Maydell
Cc: Peter Crosthwaite, QEMU Trivial, Michael Tokarev, QEMU Developers,
Peter Crosthwaite, Andreas Färber
On Mon, Apr 27, 2015 at 12:00 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 27 April 2015 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The tcg/tcg.h comment should be updated:
>> * Otherwise, we gave up on execution of this TB before it started, and
>> * the caller must fix up the CPU state by calling the CPU's
>> * synchronize_from_tb() method with the next-TB pointer we return.
>> * (falling back to calling the CPU's set_pc() method with tb->pc
>> * if no synchronize_from_tb() method exists.)
>>
>> That's a bit clunky though, which suggests we should
>> have a cpu_synchronize_from_tb() inline function in qom/cpu.h
>> which does the
>> CPUClass *cc = CPU_GET_CLASS(cpu);
>> if (cc->synchronize_from_tb) {
>> cc->synchronize_from_tb(cpu, tb);
>> } else {
>> assert(cc->set_pc);
>> cc->set_pc(cpu, tb->pc);
>> }
>>
>> bit that cpu-exec.c currently open-codes.
>
> ...except that qom/cpu.h doesn't have the definition of the
> TranslationBlock struct it would need to be able to do that "tb->pc".
> * we can't just include exec-all.h from here or otherwise get
> the TranslationBlock struct definition, because it is target
> CPU dependent
> * we can't have the common baseclass in qom/cpu.c provide an
> implementation of the synchronize_from_tb method which calls
> set_pc, because qom/cpu.c is a common-obj-y sourcefile]
> which leaves us with:
> * have cpu_synchronize_from_tb() take both tb and tb->pc as args,
> which is pretty yucky
> * give up and just update the tcg.h comment as above
>
> I think I go for "just update the comment"...
>
Does that mean a respin of this patch or should the tricore fix and
the comment be a follow up?
Regards,
Peter
> -- PMM
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-trivial] [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
2015-04-27 19:06 ` Peter Crosthwaite
@ 2015-04-27 19:10 ` Peter Maydell
-1 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2015-04-27 19:10 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Crosthwaite, QEMU Trivial, Michael Tokarev, QEMU Developers,
Peter Crosthwaite, Andreas Färber
On 27 April 2015 at 20:06, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Mon, Apr 27, 2015 at 12:00 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> I think I go for "just update the comment"...
>>
>
> Does that mean a respin of this patch or should the tricore fix and
> the comment be a follow up?
I have no preference either way.
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
@ 2015-04-27 19:10 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2015-04-27 19:10 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Crosthwaite, QEMU Trivial, Michael Tokarev, QEMU Developers,
Peter Crosthwaite, Andreas Färber
On 27 April 2015 at 20:06, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Mon, Apr 27, 2015 at 12:00 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> I think I go for "just update the comment"...
>>
>
> Does that mean a respin of this patch or should the tricore fix and
> the comment be a follow up?
I have no preference either way.
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-trivial] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
2015-04-27 19:06 ` Peter Crosthwaite
@ 2015-04-29 5:43 ` Michael Tokarev
-1 siblings, 0 replies; 22+ messages in thread
From: Michael Tokarev @ 2015-04-29 5:43 UTC (permalink / raw)
To: Peter Crosthwaite, Peter Maydell
Cc: QEMU Trivial, Peter Crosthwaite, QEMU Developers,
Andreas Färber, Peter Crosthwaite
27.04.2015 22:06, Peter Crosthwaite wrote:
> Does that mean a respin of this patch or should the tricore fix and
> the comment be a follow up?
Can you please resend whole thing in one change, removing single
unused function from two places and updating the comment, to fit
all the pieces together? The whole thing is trivial enough to split
it up and the definition of "unused" in the subject will be true :)
I think that sometimes we make a bit "too trivial" patches which
aren't worth a patch by their own :) and the result is unnecessary
clutter in the git history.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()
@ 2015-04-29 5:43 ` Michael Tokarev
0 siblings, 0 replies; 22+ messages in thread
From: Michael Tokarev @ 2015-04-29 5:43 UTC (permalink / raw)
To: Peter Crosthwaite, Peter Maydell
Cc: QEMU Trivial, Peter Crosthwaite, QEMU Developers,
Andreas Färber, Peter Crosthwaite
27.04.2015 22:06, Peter Crosthwaite wrote:
> Does that mean a respin of this patch or should the tricore fix and
> the comment be a follow up?
Can you please resend whole thing in one change, removing single
unused function from two places and updating the comment, to fit
all the pieces together? The whole thing is trivial enough to split
it up and the definition of "unused" in the subject will be true :)
I think that sometimes we make a bit "too trivial" patches which
aren't worth a patch by their own :) and the result is unnecessary
clutter in the git history.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 22+ messages in thread