| Message ID | 20211220181903.3456898-1-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | target/ppc: powerpc_excp improvements | expand |
Hello Fabiano, On 12/20/21 19:18, Fabiano Rosas wrote: > This changed a lot since v1, basically what remains is the idea that > we want to have some sort of array of interrupts and some sort of > separation between processors. > > At the end of this series we'll have: > > - One file with all interrupt implementations (interrupts.c); > > - Separate files for each major group of CPUs (book3s, booke, > 32bits). Only interrupt code for now, but we could bring pieces of > cpu_init into them; > > - Four separate interrupt arrays, one for each of the above groups > plus KVM. > > - powerpc_excp calls into the individual files and from there we > dispatch according to what is available in the interrupts array. This is going in the good direction. I think we need more steps for the reviewers, for tests and bisectability. First 4 patches are OK and I hope to merge them ASAP. The powerpc_excp() routine has grown nearly out of control these last years and it is becoming difficult to maintain. The goal is to clarify what it is going on for each CPU or each CPU family. The first step consists basically in duplicating the code and moving the exceptions handlers in specific routines. 1. cleanups should come first as usual. 2. isolate large chunks, like Nick did with ppc_excp_apply_ail(). We could do easily the same for : 2.1 ILE 2.2 unimplemeted ones doing a cpu abort: cpu_abort(cs, ".... " "is not implemented yet !\n"); 2.3 6x TLBS This should reduce considerably powerpc_excp() without changing too much the execution path. 3. Cleanup the use of excp_model, like in dcbz_common() and kvm. This is not critical but some are shortcuts. 4. Introduce a new powerpc_excp() handler : static void powerpc_excp(PowerPCCPU *cpu, int excp) { switch(env->excp_model) { case POWERPC_EXCP_FOO1: case POWERPC_EXCP_FOO2: powerpc_excp_foo(cpu, excp); break; case POWERPC_EXCP_BAR: powerpc_excp_legacy(cpu, excp); break; default: g_assert_not_reached(); } } and start duplicating code cpu per cpu in specific excp handlers, avoiding as much as possible the use of excp_model in the powerpc_excp_*() routines. That's for the theory. I suppose these can be grouped in the following way : * 405 CPU POWERPC_EXCP_40x, * 6xx CPUs POWERPC_EXCP_601, POWERPC_EXCP_602, POWERPC_EXCP_603, POWERPC_EXCP_G2, POWERPC_EXCP_604, * 7xx CPUs POWERPC_EXCP_7x0, POWERPC_EXCP_7x5, POWERPC_EXCP_74xx, * BOOKE CPUs POWERPC_EXCP_BOOKE, * BOOKS CPUs POWERPC_EXCP_970, /* could be special */ POWERPC_EXCP_POWER7, POWERPC_EXCP_POWER8, POWERPC_EXCP_POWER9, POWERPC_EXCP_POWER10, If not possible, then, we will duplicate more and that's not a problem. I would keep the routines in the same excp_helper.c file for now; we can move the code in different files but I would do it later and with other components in mind and not just the exception models. book3s, booke, 7xx, 6xx, 405 are the different groups. It fits what you did. 5. Once done, get rid of powerpc_excp_legacy() 6. Start looking at refactoring again. There might be a common prologue and epilogue. As a consequence we could change the args passed to powerpc_excp_*(). There could be common handlers and that's why an array of exception handlers looks good. this is what you are trying to address after patch 5 but I would prefer to do the above steps before. Thanks, C.
Cédric Le Goater <clg@kaod.org> writes: > Hello Fabiano, > > On 12/20/21 19:18, Fabiano Rosas wrote: >> This changed a lot since v1, basically what remains is the idea that >> we want to have some sort of array of interrupts and some sort of >> separation between processors. >> >> At the end of this series we'll have: >> >> - One file with all interrupt implementations (interrupts.c); >> >> - Separate files for each major group of CPUs (book3s, booke, >> 32bits). Only interrupt code for now, but we could bring pieces of >> cpu_init into them; >> >> - Four separate interrupt arrays, one for each of the above groups >> plus KVM. >> >> - powerpc_excp calls into the individual files and from there we >> dispatch according to what is available in the interrupts array. > > > This is going in the good direction. I think we need more steps for > the reviewers, for tests and bisectability. First 4 patches are OK > and I hope to merge them ASAP. Ok, I'm sending another series with just these 4 + the bounds check Richard mentioned. > > The powerpc_excp() routine has grown nearly out of control these last > years and it is becoming difficult to maintain. The goal is to clarify > what it is going on for each CPU or each CPU family. The first step > consists basically in duplicating the code and moving the exceptions > handlers in specific routines. > > 1. cleanups should come first as usual. > > 2. isolate large chunks, like Nick did with ppc_excp_apply_ail(). > We could do easily the same for : > > 2.1 ILE > 2.2 unimplemeted ones doing a cpu abort: > > cpu_abort(cs, ".... " "is not implemented yet !\n"); > 2.3 6x TLBS > > This should reduce considerably powerpc_excp() without changing too > much the execution path. Agreed. > > 3. Cleanup the use of excp_model, like in dcbz_common() and kvm. > This is not critical but some are shortcuts. The issue here is that we would probably be switching one arbitrary identifier for another. I don't think we have a lightweight canonical way of identifying a CPU or group of CPUs. But maybe having these conditionals on a specific CPU should be considered a hack to begin with. > > 4. Introduce a new powerpc_excp() handler : > > static void powerpc_excp(PowerPCCPU *cpu, int excp) > { > switch(env->excp_model) { > case POWERPC_EXCP_FOO1: > case POWERPC_EXCP_FOO2: > powerpc_excp_foo(cpu, excp); > break; > case POWERPC_EXCP_BAR: > powerpc_excp_legacy(cpu, excp); > break; > default: > g_assert_not_reached(); > } > } > > and start duplicating code cpu per cpu in specific excp handlers, avoiding > as much as possible the use of excp_model in the powerpc_excp_*() routines. > That's for the theory. > > I suppose these can be grouped in the following way : > > * 405 CPU > POWERPC_EXCP_40x, > > * 6xx CPUs > POWERPC_EXCP_601, > POWERPC_EXCP_602, > POWERPC_EXCP_603, > POWERPC_EXCP_G2, > POWERPC_EXCP_604, > > * 7xx CPUs > POWERPC_EXCP_7x0, > POWERPC_EXCP_7x5, > POWERPC_EXCP_74xx, > > * BOOKE CPUs > POWERPC_EXCP_BOOKE, > > * BOOKS CPUs > POWERPC_EXCP_970, /* could be special */ > POWERPC_EXCP_POWER7, > POWERPC_EXCP_POWER8, > POWERPC_EXCP_POWER9, > POWERPC_EXCP_POWER10, > > If not possible, then, we will duplicate more and that's not a problem. > > I would keep the routines in the same excp_helper.c file for now; we > can move the code in different files but I would do it later and with > other components in mind and not just the exception models. book3s, > booke, 7xx, 6xx, 405 are the different groups. It fits what you did. > > 5. Once done, get rid of powerpc_excp_legacy() > > 6. Start looking at refactoring again. > > There might be a common prologue and epilogue. As a consequence we could > change the args passed to powerpc_excp_*(). > > There could be common handlers and that's why an array of exception > handlers looks good. this is what you are trying to address after patch 5 > but I would prefer to do the above steps before. Ack all of this. I'm working on it. Thank you for the inputs.
This changed a lot since v1, basically what remains is the idea that we want to have some sort of array of interrupts and some sort of separation between processors. At the end of this series we'll have: - One file with all interrupt implementations (interrupts.c); - Separate files for each major group of CPUs (book3s, booke, 32bits). Only interrupt code for now, but we could bring pieces of cpu_init into them; - Four separate interrupt arrays, one for each of the above groups plus KVM. - powerpc_excp calls into the individual files and from there we dispatch according to what is available in the interrupts array. Please comment, Thanks. v1: https://lists.nongnu.org/archive/html/qemu-ppc/2021-06/msg00026.html Fabiano Rosas (12): target/ppc: powerpc_excp: Set alternate SRRs directly target/ppc: powerpc_excp: Set vector earlier target/ppc: powerpc_excp: Move system call vectored code together target/ppc: powerpc_excp: Stop passing excp_model around target/ppc: powerpc_excp: Standardize arguments to interrupt code target/ppc: Extract interrupt routines into a new file target/ppc: Introduce PPCInterrupt target/ppc: Remove unimplemented interrupt code target/ppc: Use common code for Hypervisor interrupts target/ppc: Split powerpc_excp into book3s, booke and 32 bit target/ppc: Create new files for book3s, booke and ppc32 exception code target/ppc: Do not enable all interrupts when running KVM target/ppc/cpu.h | 2 + target/ppc/excp_helper.c | 862 ++------------------------------------- target/ppc/interrupts.c | 521 +++++++++++++++++++++++ target/ppc/intr-book3s.c | 383 +++++++++++++++++ target/ppc/intr-booke.c | 152 +++++++ target/ppc/intr-ppc32.c | 159 ++++++++ target/ppc/meson.build | 4 + target/ppc/ppc_intr.h | 62 +++ target/ppc/tcg-stub.c | 6 + 9 files changed, 1323 insertions(+), 828 deletions(-) create mode 100644 target/ppc/interrupts.c create mode 100644 target/ppc/intr-book3s.c create mode 100644 target/ppc/intr-booke.c create mode 100644 target/ppc/intr-ppc32.c create mode 100644 target/ppc/ppc_intr.h