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

[Cédric Le Goater]

On 1/6/22 06:30, David Gibson wrote:
> 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.

yes. 'cpu->vhyp' or 'env->has_hv_mode' or 'env->msr_mask & MSR_HVB'

The use of 'env->msr_mask & MSR_HVB' would need a cleanup. env->has_hv_mode
is equivalent. May be a helper to rule them both would be better.

Thanks,

C.

> >           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);
> >       }
> >