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

From: "Cédric Le Goater" <clg@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>,
	Fabiano Rosas <farosas@linux.ibm.com>
Cc: richard.henderson@linaro.org, danielhb413@gmail.com,
	qemu-ppc@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 4/7] target/ppc: Add HV support to ppc_interrupts_little_endian
Date: Thu, 6 Jan 2022 14:05:08 +0100	[thread overview]
Message-ID: <0be0b54a-e9fd-ac35-1c73-b840d8f34093@kaod.org> (raw)
In-Reply-To: <YdZ+YtvZSUhkFvR/@yekko>

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