| Message ID | 20211220181903.3456898-3-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: powerpc_excp improvements | expand |
Adding Nicholas. On 12/20/21 19:18, Fabiano Rosas wrote: > None of the interrupt setup code touches 'vector', so we can move it > earlier in the function. This will allow us to later move the System > Call Vectored setup that is on the top level into the > POWERPC_EXCP_SYSCALL_VECTORED code block. > > This patch also moves the verification for when 'excp' does not have > an address associated with it. We now bail a little earlier when that > is the case. This should not cause any visible effects. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > target/ppc/excp_helper.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 8b9c6bc5a8..14fd0213a0 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > } > #endif > > + vector = env->excp_vectors[excp]; > + if (vector == (target_ulong)-1ULL) { > + cpu_abort(cs, "Raised an exception without defined vector %d\n", > + excp); > + } > + > + vector |= env->excp_prefix; > + > switch (excp) { > case POWERPC_EXCP_NONE: > /* Should never happen */ > @@ -831,14 +839,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > } > #endif > > - vector = env->excp_vectors[excp]; > - if (vector == (target_ulong)-1ULL) { > - cpu_abort(cs, "Raised an exception without defined vector %d\n", > - excp); > - } > - > - vector |= env->excp_prefix; > - > #if defined(TARGET_PPC64) > if (excp_model == POWERPC_EXCP_BOOKE) { > if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) { >
On 12/20/21 10:18 AM, Fabiano Rosas wrote: > None of the interrupt setup code touches 'vector', so we can move it > earlier in the function. This will allow us to later move the System > Call Vectored setup that is on the top level into the > POWERPC_EXCP_SYSCALL_VECTORED code block. > > This patch also moves the verification for when 'excp' does not have > an address associated with it. We now bail a little earlier when that > is the case. This should not cause any visible effects. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > --- > target/ppc/excp_helper.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 8b9c6bc5a8..14fd0213a0 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > } > #endif > > + vector = env->excp_vectors[excp]; > + if (vector == (target_ulong)-1ULL) { > + cpu_abort(cs, "Raised an exception without defined vector %d\n", > + excp); > + } > + > + vector |= env->excp_prefix; > + > switch (excp) { > case POWERPC_EXCP_NONE: > /* Should never happen */ You've moved the cpu_abort above the excp check above the early return for NONE (which possibly shouldn't exist) and above the excp default cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp); I would certainly expect invalid excp to not have a defined vector either. I'll also note that the excp_vectors[] index is no longer bounds checked by the switch. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 12/20/21 10:18 AM, Fabiano Rosas wrote: >> None of the interrupt setup code touches 'vector', so we can move it >> earlier in the function. This will allow us to later move the System >> Call Vectored setup that is on the top level into the >> POWERPC_EXCP_SYSCALL_VECTORED code block. >> >> This patch also moves the verification for when 'excp' does not have >> an address associated with it. We now bail a little earlier when that >> is the case. This should not cause any visible effects. >> >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >> --- >> target/ppc/excp_helper.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index 8b9c6bc5a8..14fd0213a0 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> } >> #endif >> >> + vector = env->excp_vectors[excp]; >> + if (vector == (target_ulong)-1ULL) { >> + cpu_abort(cs, "Raised an exception without defined vector %d\n", >> + excp); >> + } >> + >> + vector |= env->excp_prefix; >> + >> switch (excp) { >> case POWERPC_EXCP_NONE: >> /* Should never happen */ > > You've moved the cpu_abort above the excp check above the early return for NONE (which > possibly shouldn't exist) and above the excp default Right, I think I had this patch initially after patch 7 which moves the NONE check ealier in the function. I'll reorganize. > cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp); > > I would certainly expect invalid excp to not have a defined vector > either. One thing we always had that this series makes more explicit is the distinction between what interrupts QEMU knows about and what vectors a processor uses. I have played around with using the same data structure to hold both, which would perhaps match the expectation more closely, but nothing came out of it. I think there's still space after this series for further improvement in that regard. > I'll also note that the excp_vectors[] index is no longer bounds checked by the switch. Ack.
Excerpts from Cédric Le Goater's message of December 22, 2021 4:48 pm: > Adding Nicholas. These look fine by me. The whole series looks like a really nice cleanup actually. Thanks, Nick > > On 12/20/21 19:18, Fabiano Rosas wrote: >> None of the interrupt setup code touches 'vector', so we can move it >> earlier in the function. This will allow us to later move the System >> Call Vectored setup that is on the top level into the >> POWERPC_EXCP_SYSCALL_VECTORED code block. >> >> This patch also moves the verification for when 'excp' does not have >> an address associated with it. We now bail a little earlier when that >> is the case. This should not cause any visible effects. >> >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > Thanks, > > C. > >> --- >> target/ppc/excp_helper.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index 8b9c6bc5a8..14fd0213a0 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> } >> #endif >> >> + vector = env->excp_vectors[excp]; >> + if (vector == (target_ulong)-1ULL) { >> + cpu_abort(cs, "Raised an exception without defined vector %d\n", >> + excp); >> + } >> + >> + vector |= env->excp_prefix; >> + >> switch (excp) { >> case POWERPC_EXCP_NONE: >> /* Should never happen */ >> @@ -831,14 +839,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> } >> #endif >> >> - vector = env->excp_vectors[excp]; >> - if (vector == (target_ulong)-1ULL) { >> - cpu_abort(cs, "Raised an exception without defined vector %d\n", >> - excp); >> - } >> - >> - vector |= env->excp_prefix; >> - >> #if defined(TARGET_PPC64) >> if (excp_model == POWERPC_EXCP_BOOKE) { >> if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) { >> > >
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 8b9c6bc5a8..14fd0213a0 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) } #endif + vector = env->excp_vectors[excp]; + if (vector == (target_ulong)-1ULL) { + cpu_abort(cs, "Raised an exception without defined vector %d\n", + excp); + } + + vector |= env->excp_prefix; + switch (excp) { case POWERPC_EXCP_NONE: /* Should never happen */ @@ -831,14 +839,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) } #endif - vector = env->excp_vectors[excp]; - if (vector == (target_ulong)-1ULL) { - cpu_abort(cs, "Raised an exception without defined vector %d\n", - excp); - } - - vector |= env->excp_prefix; - #if defined(TARGET_PPC64) if (excp_model == POWERPC_EXCP_BOOKE) { if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) {
None of the interrupt setup code touches 'vector', so we can move it earlier in the function. This will allow us to later move the System Call Vectored setup that is on the top level into the POWERPC_EXCP_SYSCALL_VECTORED code block. This patch also moves the verification for when 'excp' does not have an address associated with it. We now bail a little earlier when that is the case. This should not cause any visible effects. Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- target/ppc/excp_helper.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)