Re: [PATCH v2 4/7] target/ppc: Add HV support to ppc_interrupts_little_endian

From: David Gibson <david@gibson.dropbear.id.au>
To: Fabiano Rosas <farosas@linux.ibm.com>
Cc: richard.henderson@linaro.org, danielhb413@gmail.com,
	qemu-ppc@nongnu.org, qemu-devel@nongnu.org, clg@kaod.org
Subject: Re: [PATCH v2 4/7] target/ppc: Add HV support to ppc_interrupts_little_endian
Date: Thu, 6 Jan 2022 16:30:10 +1100	[thread overview]
Message-ID: <YdZ+YtvZSUhkFvR/@yekko> (raw)
In-Reply-To: <20220105204029.4058500-5-farosas@linux.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 3601 bytes --]

On Wed, Jan 05, 2022 at 05:40:26PM -0300, Fabiano Rosas wrote:
> The ppc_interrupts_little_endian function could be used for interrupts
> delivered in Hypervisor mode, so add support for powernv8 and powernv9
> to it.
> 
> Also drop the comment because it is inaccurate, all CPUs that can run
> little endian can have interrupts in little endian. The point is
> whether they can take interrupts in an endianness different from
> MSR_LE.
> 
> This change has no functional impact.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

With one nit you might want to look at later:

> ---
>  target/ppc/arch_dump.c   |  2 +-
>  target/ppc/cpu.h         | 23 +++++++++++++++--------
>  target/ppc/excp_helper.c |  2 +-
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> index bb392f6d88..12cde198a3 100644
> --- a/target/ppc/arch_dump.c
> +++ b/target/ppc/arch_dump.c
> @@ -237,7 +237,7 @@ int cpu_get_dump_info(ArchDumpInfo *info,
>      info->d_machine = PPC_ELF_MACHINE;
>      info->d_class = ELFCLASS;
>  
> -    if (ppc_interrupts_little_endian(cpu)) {
> +    if (ppc_interrupts_little_endian(cpu, false)) {

I'm wondering if using hv==false here is actually correct, and AFAICT
it probably is for spapr, but not for powernv.  So I'm wondering if we
should actually test cpu->vhyp here to get the right value for powernv
as well.

>          info->d_endian = ELFDATA2LSB;
>      } else {
>          info->d_endian = ELFDATA2MSB;
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f20d4ffa6d..a6fc857ad4 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2728,20 +2728,27 @@ 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)
> +#if !defined(CONFIG_USER_ONLY)
> +static inline bool ppc_interrupts_little_endian(PowerPCCPU *cpu, bool hv)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    bool ile = false;
>  
> -    /*
> -     * 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);
> +    if (hv && env->has_hv_mode) {
> +        if (is_isa300(pcc)) {
> +            ile = !!(env->spr[SPR_HID0] & HID0_POWER9_HILE);
> +        } else {
> +            ile = !!(env->spr[SPR_HID0] & HID0_HILE);
> +        }
> +
> +    } else if (pcc->lpcr_mask & LPCR_ILE) {
> +        ile = !!(env->spr[SPR_LPCR] & LPCR_ILE);
>      }
>  
> -    return false;
> +    return ile;
>  }
> +#endif
>  
>  void dump_mmu(CPUPPCState *env);
>  
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index fa41f8048d..92953af896 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1071,7 +1071,7 @@ void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
>       */
>      msr = (1ULL << MSR_ME);
>      msr |= env->msr & (1ULL << MSR_SF);
> -    if (ppc_interrupts_little_endian(cpu)) {
> +    if (ppc_interrupts_little_endian(cpu, false)) {
>          msr |= (1ULL << MSR_LE);
>      }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]