[Qemu-devel] [PATCH target-arm 0/2] Unused code cleanups

[Qemu-devel] [PATCH target-arm 0/2] Unused code cleanups

[Peter Crosthwaite]

Hi Peter,

Two unused code cleanups for target-arm/cpu.h

Regards,
Peter

Peter Crosthwaite (2):
  arm: cpu.h: Remove unused typdefs
  arm: cpu.h: Delete unused cpu_pc_from_tb()

 target-arm/cpu.h | 14 --------------
 1 file changed, 14 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH target-arm 1/2] arm: cpu.h: Remove unused typdefs

[Peter Crosthwaite]

These CP accessor function prototypes are unused. Remove them.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 target-arm/cpu.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 083211c..7069103 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -93,11 +93,6 @@
 #define ARM_CPU_VIRQ 2
 #define ARM_CPU_VFIQ 3
 
-typedef void ARMWriteCPFunc(void *opaque, int cp_info,
-                            int srcreg, int operand, uint32_t value);
-typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
-                               int dstreg, int operand);
-
 struct arm_boot_info;
 
 #define NB_MMU_MODES 7
-- 
1.9.1

Re: [Qemu-devel] [PATCH target-arm 1/2] arm: cpu.h: Remove unused typdefs

[Michael Tokarev]

27.04.2015 04:38, Peter Crosthwaite wrote:
> These CP accessor function prototypes are unused. Remove them.

Applied to -trivial, thank you!

/mjt

[Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()

[Peter Crosthwaite]

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)
-{
-    if (ARM_TBFLAG_AARCH64_STATE(tb->flags)) {
-        env->pc = tb->pc;
-    } else {
-        env->regs[15] = tb->pc;
-    }
-}
-
 enum {
     QEMU_PSCI_CONDUIT_DISABLED = 0,
     QEMU_PSCI_CONDUIT_SMC = 1,
-- 
1.9.1

Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()

[Peter Crosthwaite]

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
>

Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH target-arm 2/2] arm: cpu.h: Delete unused cpu_pc_from_tb()

[Michael Tokarev]

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