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 <c...@kaod.org> > Tested-by: Cédric Le Goater <c...@kaod.org> > > Thanks for the effort and the documentation. One minor comment below, > > C. > >> >> Signed-off-by: Nicholas Piggin <npig...@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