[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