Re: [Qemu-devel] [RFC v2 22/34] microblaze: enable multi-arch

[Paolo Bonzini <pbonzini@redhat.com>]

On 31/05/2015 08:11, Peter Crosthwaite wrote:
> Multi-arch conversion consisting of:
>  * configury
>     - defining CONFIG_ARCH_MULTI
>     - adding to MULTI_TARGETS
>     - enabling disas for MULTI_ARCH
>  * Converting target-microblaze to arch-obj-y
>  * cpu.h
>    - Compiling out all target-microblaze private contents
>      when doing multi-arch build
>    - Redefining target_ulong and cpu-def typenames with arch prefix
>    - Undeffing possibly previously defined macros
>  * Defining the QOM cpu hooks
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> I guess I could split to multi patches but it will bloat this series!
> ---
>  configure                       |  6 ++++++
>  target-microblaze/Makefile.objs |  6 +++---
>  target-microblaze/cpu-qom.h     |  2 ++
>  target-microblaze/cpu.c         |  1 +
>  target-microblaze/cpu.h         | 40 ++++++++++++++++++++++++++++++++++++----
>  5 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/configure b/configure
> index 1acafcd..3dbfd3e 100755
> --- a/configure
> +++ b/configure
> @@ -5440,6 +5440,9 @@ for i in $ARCH $TARGET_BASE_ARCH ; do
>    microblaze*)
>      echo "CONFIG_MICROBLAZE_DIS=y"  >> $config_target_mak
>      echo "CONFIG_MICROBLAZE_DIS=y"  >> config-all-disas.mak
> +    if [ -e $config_target_multi_mak ]; then
> +        echo "CONFIG_MICROBLAZE_DIS=y"  >> $config_target_multi_mak
> +    fi

-e looks dangerous if you have a previous build around.  Use a variable
that you can also print at the end of configure?

>    ;;
>    mips*)
>      echo "CONFIG_MIPS_DIS=y"  >> $config_target_mak
> @@ -5481,6 +5484,9 @@ if test "$tcg_interpreter" = "yes" ; then
>  fi
>  
>  case "$TARGET_BASE_ARCH" in
> +microblaze)
> +  echo "CONFIG_ARCH_MULTI=y" >> $config_target_mak
> +;;
>  *)
>    echo "CONFIG_ARCH_SINGLE=y"  >> $config_target_mak
>  ;;

How is CONFIG_ARCH_SINGLE different from $(call lnot,
$(CONFIG_ARCH_MULTI))?  And can CONFIG_ARCH_MULTI be defined in
default-configs/ rather than here?

> diff --git a/target-microblaze/Makefile.objs b/target-microblaze/Makefile.objs
> index f3d7b44..f70163d 100644
> --- a/target-microblaze/Makefile.objs
> +++ b/target-microblaze/Makefile.objs
> @@ -1,3 +1,3 @@
> -obj-y += translate.o op_helper.o helper.o cpu.o
> -obj-y += gdbstub.o
> -obj-$(CONFIG_SOFTMMU) += mmu.o
> +arch-obj-y += translate.o op_helper.o helper.o cpu.o
> +arch-obj-y += gdbstub.o
> +arch-obj-$(CONFIG_SOFTMMU) += mmu.o
> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
> index e3e0701..88526fa 100644
> --- a/target-microblaze/cpu-qom.h
> +++ b/target-microblaze/cpu-qom.h
> @@ -67,9 +67,11 @@ static inline MicroBlazeCPU *mb_env_get_cpu(CPUMBState *env)
>      return container_of(env, MicroBlazeCPU, env);
>  }
>  
> +#ifndef TARGET_MULTI
>  #define ENV_GET_CPU(e) CPU(mb_env_get_cpu(e))
>  
>  #define ENV_OFFSET offsetof(MicroBlazeCPU, env)
> +#endif /* !TARGET_MULTI */
>  
>  void mb_cpu_do_interrupt(CPUState *cs);
>  bool mb_cpu_exec_interrupt(CPUState *cs, int int_req);
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 563ad46..135233a 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -135,6 +135,7 @@ static void mb_cpu_initfn(Object *obj)
>      CPUMBState *env = &cpu->env;
>      static bool tcg_initialized;
>  
> +    CPU_SET_QOM_HOOKS(cs);

Why are the hooks in the instance rather in the class?  Performance?

Also, why a macro and not a function?

>      cs->env_ptr = env;
>      cpu_exec_init(cs);
>  
> diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
> index 9068272..4ccbac5 100644
> --- a/target-microblaze/cpu.h
> +++ b/target-microblaze/cpu.h
> @@ -22,10 +22,21 @@
>  #include "config.h"
>  #include "qemu-common.h"
>  
> +#undef TARGET_LONG_BITS
>  #define TARGET_LONG_BITS 32
>  
> +#undef CPUArchState
>  #define CPUArchState struct CPUMBState
>  
> +#undef CPUTLBEntry
> +#undef CPUIOTLBEntry
> +#undef target_long
> +#undef target_ulong
> +#define CPUTLBEntry MBCPUTLBEntry
> +#define CPUIOTLBEntry MBCPUIOTLBEntry
> +#define target_long mb_target_long
> +#define target_ulong mb_target_ulong

Oh, this answers my previous question.  Please document it in the header.

Could the #undefs be moved to exec/cpu-defs.h and exec/target_long.h
(see softmmu_template.h for a precedent)?

Paolo

>  #include "exec/cpu-defs.h"
>  #include "fpu/softfloat.h"
>  struct CPUMBState;
> @@ -34,6 +45,7 @@ typedef struct CPUMBState CPUMBState;
>  #include "mmu.h"
>  #endif
>  
> +#ifndef TARGET_MULTI
>  #define ELF_MACHINE	EM_MICROBLAZE
>  
>  #define EXCP_MMU        1
> @@ -45,13 +57,19 @@ typedef struct CPUMBState CPUMBState;
>  /* MicroBlaze-specific interrupt pending bits.  */
>  #define CPU_INTERRUPT_NMI       CPU_INTERRUPT_TGT_EXT_3
>  
> +#endif /* TARGET_MULTI */
> +
>  /* Meanings of the MBCPU object's two inbound GPIO lines */
>  #define MB_CPU_IRQ 0
>  #define MB_CPU_FIR 1
>  
> -/* Register aliases. R0 - R15 */
> -#define R_SP     1
> +/* FIXME: Implement cpu::set_pc fn for microblaze and privatise this */
>  #define SR_PC    0
> +
> +#ifndef TARGET_MULTI
> +
> +/* Register aliases. R1 - R15 */
> +#define R_SP     1
>  #define SR_MSR   1
>  #define SR_EAR   3
>  #define SR_ESR   5
> @@ -112,6 +130,13 @@ typedef struct CPUMBState CPUMBState;
>  #define FSR_UF          (1<<1) /* Underflow */
>  #define FSR_DO          (1<<0) /* Denormalized operand error */
>  
> +#endif /* TARGET_MULTI */
> +
> +/* The Microblaze bootloader configures some of the PVRs in a board specific
> + * way as a reset process. This should go away with PVR property QOMification
> + * and then the PVRs can be made private to CPUs.
> + */
> +
>  /* Version reg.  */
>  /* Basic PVR mask */
>  #define PVR0_PVR_FULL_MASK              0x80000000
> @@ -212,6 +237,7 @@ typedef struct CPUMBState CPUMBState;
>  #define PVR11_MSR_RESET_VALUE_MASK      0x000007FF
>  
>  
> +#ifndef TARGET_MULTI
>  
>  /* CPU flags.  */
>  
> @@ -223,14 +249,17 @@ typedef struct CPUMBState CPUMBState;
>  #define CC_NE  1
>  #define CC_EQ  0
>  
> -#define NB_MMU_MODES    3
> -
>  #define STREAM_EXCEPTION (1 << 0)
>  #define STREAM_ATOMIC    (1 << 1)
>  #define STREAM_TEST      (1 << 2)
>  #define STREAM_CONTROL   (1 << 3)
>  #define STREAM_NONBLOCK  (1 << 4)
>  
> +#endif /* TARGET_MULTI */
> +
> +#undef NB_MMU_MODES
> +#define NB_MMU_MODES    3
> +
>  struct CPUMBState {
>      uint32_t debug;
>      uint32_t btaken;
> @@ -274,6 +303,8 @@ struct CPUMBState {
>  
>  #include "cpu-qom.h"
>  
> +#ifndef TARGET_MULTI
> +
>  void mb_tcg_init(void);
>  MicroBlazeCPU *cpu_mb_init(const char *cpu_model);
>  int cpu_mb_exec(CPUState *cpu);
> @@ -337,4 +368,5 @@ void mb_cpu_unassigned_access(CPUState *cpu, hwaddr addr,
>  
>  #include "exec/exec-all.h"
>  
> +#endif /* !TARGET_MULTI */
>  #endif
>