Re: [Qemu-devel] [PATCH 3/4] target-ppc: ppc can be either endian

[Andreas Färber]

[fixing Bharata's address]

Am 28.04.2014 13:29, schrieb Greg Kurz:
> POWER7, POWER7+ and POWER8 families use the ILE bit of the LPCR
> special purpose register to decide the endianness to use when
> entering interrupt handlers. When running a linux guest, this
> provides a hint on the endianness used by the kernel. From a
> qemu point of view, the information is needed for legacy virtio

"QEMU" :) - and "Linux" while at it?

> support and crash dump support as well.
> 
> Suggested-by: Benjamin Herrenschmidt <address@hidden>
> Suggested-by: Alexander Graf <address@hidden>
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  target-ppc/cpu-qom.h        |    2 ++
>  target-ppc/misc_helper.c    |    7 +++++++
>  target-ppc/translate_init.c |   16 ++++++++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 47dc8e6..c20473a 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -76,6 +76,7 @@ typedef struct PowerPCCPUClass {
>      int (*handle_mmu_fault)(PowerPCCPU *cpu, target_ulong eaddr, int rwx,
>                              int mmu_idx);
>  #endif
> +    bool (*interrupts_big_endian)(CPUPPCState *env);

Please do not add *any* new functions with a CPUPPCState argument,
especially not in my shiny new PowerPCCPUClass. ;)

>  } PowerPCCPUClass;
>  
>  /**
> @@ -118,6 +119,7 @@ int ppc64_cpu_write_elf64_qemunote(WriteCoreDumpFunction 
> f,
>                                     CPUState *cpu, void *opaque);
>  int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>                                 int cpuid, void *opaque);
> +bool ppc_cpu_interrupts_big_endian(CPUState *cs);
>  #ifndef CONFIG_USER_ONLY
>  extern const struct VMStateDescription vmstate_ppc_cpu;
>  #endif
> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
> index 2eb2fa6..0e548ff 100644
> --- a/target-ppc/misc_helper.c
> +++ b/target-ppc/misc_helper.c
> @@ -120,3 +120,10 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
>  {
>      hreg_store_msr(env, value, 0);
>  }
> +
> +bool ppc_cpu_interrupts_big_endian(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);

White line after variable declaration block, please.

> +    return pcc->interrupts_big_endian(&cpu->env);
> +}
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 823c63c..0d5492f 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -3102,6 +3102,18 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
>      return 0;
>  }
>  
> +static bool interrupts_big_endian_always(CPUPPCState *env)

ppc_cpu_... as a convention for a function accepting PowerPCCPU *cpu as
main argument.

Further, note that we are working towards compiling multiple targets
into one binary, so at least ppc_... for non-static ones please.

> +{
> +    return true;
> +}
> +
> +#ifdef TARGET_PPC64
> +static bool interrupts_big_endian_POWER7(CPUPPCState *env)

Alex, do we prefer lowercase for new code?

Otherwise looks okay to me - we had a lengthy discussion about the hook
naming, but I admit some name is better than no functionality.

I would like to re-raise the question though of whether it may make
sense to do this at CPUClass level for the benefit of ARM? Could be
refactored as follow-up, of course.

Regards,
Andreas

> +{
> +    return !(env->spr[SPR_LPCR] & LPCR_ILE);
> +}
> +#endif
> +
>  
> /*****************************************************************************/
>  /* PowerPC implementations definitions                                       
> */
>  
> @@ -7097,6 +7109,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>                   POWERPC_FLAG_VSX;
>      pcc->l1_dcache_size = 0x8000;
>      pcc->l1_icache_size = 0x8000;
> +    pcc->interrupts_big_endian = interrupts_big_endian_POWER7;
>  }
>  
>  POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data)
> @@ -7140,6 +7153,7 @@ POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data)
>                   POWERPC_FLAG_VSX;
>      pcc->l1_dcache_size = 0x8000;
>      pcc->l1_icache_size = 0x8000;
> +    pcc->interrupts_big_endian = interrupts_big_endian_POWER7;
>  }
>  
>  static void init_proc_POWER8(CPUPPCState *env)
> @@ -7197,6 +7211,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>                   POWERPC_FLAG_VSX;
>      pcc->l1_dcache_size = 0x8000;
>      pcc->l1_icache_size = 0x8000;
> +    pcc->interrupts_big_endian = interrupts_big_endian_POWER7;
>  }
>  #endif /* defined (TARGET_PPC64) */
>  
> @@ -8523,6 +8538,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void 
> *data)
>      pcc->parent_realize = dc->realize;
>      pcc->pvr = CPU_POWERPC_DEFAULT_MASK;
>      pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK;
> +    pcc->interrupts_big_endian = interrupts_big_endian_always;
>      dc->realize = ppc_cpu_realizefn;
>      dc->unrealize = ppc_cpu_unrealizefn;
>  

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg