Re: [PATCH 1/2] target/ppc: Introduce ppc_interrupts_little_endian()

[Fabiano Rosas <farosas@linux.ibm.com>]

Greg Kurz <groug@kaod.org> writes:

> PowerPC CPUs use big endian by default but starting with POWER7,
> server grade CPUs use the ILE bit of the LPCR special purpose
> register to decide on the endianness to use when handling
> interrupts. This gives a clue to QEMU on the endianness the
> guest kernel is running, which is needed when generating an
> ELF dump of the guest or when delivering an FWNMI machine
> check interrupt.
>
> Commit 382d2db62bcb ("target-ppc: Introduce callback for interrupt
> endianness") added a class method to PowerPCCPUClass to modelize
> this : default implementation returns a fixed "big endian" value,
> while POWER7 and newer do the LPCR_ILE check. This is suboptimal
> as it forces to implement the method for every new CPU family, and
> it is very unlikely that this will ever be different than what we
> have today.
>
> We basically only have three cases to consider:
> a) CPU doesn't have an LPCR => big endian
> b) CPU has an LPCR but doesn't support the ILE bit => big endian
> c) CPU has an LPCR and supports the ILE bit => little or big endian
>
> Instead of class methods, introduce an inline helper that checks the
> ILE bit in the LPCR_MASK to decide on the outcome. The new helper
> words little endian instead of big endian. This allows to drop a !
> operator in ppc_cpu_do_fwnmi_machine_check().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

> ---
>  target/ppc/cpu.h         | 15 +++++++++++++++
>  target/ppc/arch_dump.c   |  8 +++-----
>  target/ppc/excp_helper.c |  3 +--
>  3 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index b4de0db7ff5c..93d308ac8f2d 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2643,6 +2643,21 @@ static inline bool ppc_has_spr(PowerPCCPU *cpu, int spr)
>      return cpu->env.spr_cb[spr].name != NULL;
>  }
>  
> +static inline bool ppc_interrupts_little_endian(PowerPCCPU *cpu)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +    /*
> +     * Only models that have an LPCR and know about LPCR_ILE can do little
> +     * endian.
> +     */
> +    if (pcc->lpcr_mask & LPCR_ILE) {
> +        return !!(cpu->env.spr[SPR_LPCR] & LPCR_ILE);
> +    }
> +
> +    return false;
> +}
> +
>  void dump_mmu(CPUPPCState *env);
>  
>  void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> index 9210e61ef463..bb392f6d8885 100644
> --- a/target/ppc/arch_dump.c
> +++ b/target/ppc/arch_dump.c
> @@ -227,22 +227,20 @@ int cpu_get_dump_info(ArchDumpInfo *info,
>                        const struct GuestPhysBlockList *guest_phys_blocks)
>  {
>      PowerPCCPU *cpu;
> -    PowerPCCPUClass *pcc;
>  
>      if (first_cpu == NULL) {
>          return -1;
>      }
>  
>      cpu = POWERPC_CPU(first_cpu);
> -    pcc = POWERPC_CPU_GET_CLASS(cpu);
>  
>      info->d_machine = PPC_ELF_MACHINE;
>      info->d_class = ELFCLASS;
>  
> -    if ((*pcc->interrupts_big_endian)(cpu)) {
> -        info->d_endian = ELFDATA2MSB;
> -    } else {
> +    if (ppc_interrupts_little_endian(cpu)) {
>          info->d_endian = ELFDATA2LSB;
> +    } else {
> +        info->d_endian = ELFDATA2MSB;
>      }
>      /* 64KB is the max page size for pseries kernel */
>      if (strncmp(object_get_typename(qdev_get_machine()),
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index fd147e2a3766..a79a0ed465e5 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1099,7 +1099,6 @@ void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      target_ulong msr = 0;
>  
>      /*
> @@ -1108,7 +1107,7 @@ void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
>       */
>      msr = (1ULL << MSR_ME);
>      msr |= env->msr & (1ULL << MSR_SF);
> -    if (!(*pcc->interrupts_big_endian)(cpu)) {
> +    if (ppc_interrupts_little_endian(cpu)) {
>          msr |= (1ULL << MSR_LE);
>      }