qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [EXTERNAL] [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt


From: Nicholas Piggin
Subject: Re: [EXTERNAL] [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt delivery
Date: Thu, 15 Apr 2021 15:25:43 +1000

Excerpts from Cédric Le Goater's message of April 15, 2021 1:24 am:
> On 4/14/21 5:23 AM, Nicholas Piggin wrote:
>> The AIL logic is becoming unmanageable spread all over powerpc_excp(),
>> and it is slated to get even worse with POWER10 support.
>> 
>> Move it all to a new helper function.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Tested-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks for the effort and the documentation. One minor comment below,
> 
> C.
> 
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr_hcall.c            |   3 +-
>>  target/ppc/cpu.h                |   8 --
>>  target/ppc/excp_helper.c        | 161 ++++++++++++++++++++------------
>>  target/ppc/translate_init.c.inc |   2 +-
>>  4 files changed, 104 insertions(+), 70 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 7b5cd3553c..2fbe04a689 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1395,7 +1395,8 @@ static target_ulong 
>> h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>>          return H_P4;
>>      }
>> 
>> -    if (mflags == AIL_RESERVED) {
>> +    if (mflags == 1) {
>> +        /* AIL=1 is reserved */
>>          return H_UNSUPPORTED_FLAG;
>>      }
>> 
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index e73416da68..5200a16d23 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -2375,14 +2375,6 @@ enum {
>>      HMER_XSCOM_STATUS_MASK      = PPC_BITMASK(21, 23),
>>  };
>> 
>> -/* Alternate Interrupt Location (AIL) */
>> -enum {
>> -    AIL_NONE                = 0,
>> -    AIL_RESERVED            = 1,
>> -    AIL_0001_8000           = 2,
>> -    AIL_C000_0000_0000_4000 = 3,
>> -};
> 
> I kind of like these. No big deal.

My thinking was they actually are just a POWER8 model of the AIL bits 
(e.g., they don't represent scv properly or AIL=2 reserved in P10), and 
they spread the meaning over multiple files. After this patch it's all
just in that single function.

>> 
>> -    switch (ail) {
>> -    case AIL_NONE:
>> -        break;
>> -    case AIL_0001_8000:
>> -        offset = 0x18000;
>> -        break;
>> -    case AIL_C000_0000_0000_4000:
>> -        offset = 0xc000000000004000ull;
>> -        break;
>> -    default:
>> -        cpu_abort(cs, "Invalid AIL combination %d\n", ail);
> 
> Could we keep this abort ? 

Well the abort is no longer there because we explicitly handle all 
cases, the reserved ones by just ignoring them. I don't know what
the hardware actually does if you tried to set it (it should ignore)
but I think this is nicer to not abort.

Thanks,
Nick



reply via email to

[Prev in Thread] Current Thread [Next in Thread]