| Message ID | ee7c07146e8e2e5a3d1d52aaf5a4eeef695c359d.1686776990.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Misc clean ups to target/ppc exception handling | expand |
On Thu Jun 15, 2023 at 7:34 AM AEST, BALATON Zoltan wrote: > After previous changes the hypercall handling in 7xx and 74xx > exception handlers can be folded into one if statement to simpilfy > this code. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > target/ppc/excp_helper.c | 26 ++++++++++---------------- > 1 file changed, 10 insertions(+), 16 deletions(-) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 1682b988ba..662457f342 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -740,26 +740,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp) > break; > case POWERPC_EXCP_SYSCALL: /* System call exception */ > { > - int lev = env->error_code; I would still keep lev. Self documenting and consistent with books handler. > + PowerPCCPU *cpu = env_archcpu(env); Is this necessary? Thanks, Nick > > - if (lev == 1 && cpu->vhyp) { > - dump_hcall(env); > - } else { > - dump_syscall(env); > - } > /* > * The Virtual Open Firmware (VOF) relies on the 'sc 1' > * instruction to communicate with QEMU. The pegasos2 machine > * uses VOF and the 7xx CPUs, so although the 7xx don't have > * HV mode, we need to keep hypercall support. > */ > - if (lev == 1 && cpu->vhyp) { > + if (unlikely(env->error_code == 1 && cpu->vhyp)) { > PPCVirtualHypervisorClass *vhc = > PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); > + dump_hcall(env); > vhc->hypercall(cpu->vhyp, cpu); > return; > + } else { > + dump_syscall(env); > } > - > break; > } > case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */ > @@ -884,26 +881,23 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp) > break; > case POWERPC_EXCP_SYSCALL: /* System call exception */ > { > - int lev = env->error_code; > + PowerPCCPU *cpu = env_archcpu(env); > > - if (lev == 1 && cpu->vhyp) { > - dump_hcall(env); > - } else { > - dump_syscall(env); > - } > /* > * The Virtual Open Firmware (VOF) relies on the 'sc 1' > * instruction to communicate with QEMU. The pegasos2 machine > * uses VOF and the 74xx CPUs, so although the 74xx don't have > * HV mode, we need to keep hypercall support. > */ > - if (lev == 1 && cpu->vhyp) { > + if (unlikely(env->error_code == 1 && cpu->vhyp)) { > PPCVirtualHypervisorClass *vhc = > PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); > + dump_hcall(env); > vhc->hypercall(cpu->vhyp, cpu); > return; > + } else { > + dump_syscall(env); > } > - > break; > } > case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */ > -- > 2.30.9
On Thu, 15 Jun 2023, Nicholas Piggin wrote: > On Thu Jun 15, 2023 at 7:34 AM AEST, BALATON Zoltan wrote: >> After previous changes the hypercall handling in 7xx and 74xx >> exception handlers can be folded into one if statement to simpilfy >> this code. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> target/ppc/excp_helper.c | 26 ++++++++++---------------- >> 1 file changed, 10 insertions(+), 16 deletions(-) >> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index 1682b988ba..662457f342 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -740,26 +740,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp) >> break; >> case POWERPC_EXCP_SYSCALL: /* System call exception */ >> { >> - int lev = env->error_code; > > I would still keep lev. Self documenting and consistent with books > handler. lev is still there in the books version, but probably not really needed in these 7xx versions which does not really have level parameter. This hack should likely go away and replaced with something else on the long run as this won't work with KVM but that needs some support from VOF or compiling a different version for pegasos2 which wasn't considered so far. I can add the local back if you really insist but I don't think it really makes much sense in these cases for 7xx and 74xx. >> + PowerPCCPU *cpu = env_archcpu(env); > > Is this necessary? Yes, for cpu->vhyp below. Regards, BALATON Zoltan > Thanks, > Nick > >> >> - if (lev == 1 && cpu->vhyp) { >> - dump_hcall(env); >> - } else { >> - dump_syscall(env); >> - } >> /* >> * The Virtual Open Firmware (VOF) relies on the 'sc 1' >> * instruction to communicate with QEMU. The pegasos2 machine >> * uses VOF and the 7xx CPUs, so although the 7xx don't have >> * HV mode, we need to keep hypercall support. >> */ >> - if (lev == 1 && cpu->vhyp) { >> + if (unlikely(env->error_code == 1 && cpu->vhyp)) { >> PPCVirtualHypervisorClass *vhc = >> PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); >> + dump_hcall(env); >> vhc->hypercall(cpu->vhyp, cpu); >> return; >> + } else { >> + dump_syscall(env); >> } >> - >> break; >> } >> case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */ >> @@ -884,26 +881,23 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp) >> break; >> case POWERPC_EXCP_SYSCALL: /* System call exception */ >> { >> - int lev = env->error_code; >> + PowerPCCPU *cpu = env_archcpu(env); >> >> - if (lev == 1 && cpu->vhyp) { >> - dump_hcall(env); >> - } else { >> - dump_syscall(env); >> - } >> /* >> * The Virtual Open Firmware (VOF) relies on the 'sc 1' >> * instruction to communicate with QEMU. The pegasos2 machine >> * uses VOF and the 74xx CPUs, so although the 74xx don't have >> * HV mode, we need to keep hypercall support. >> */ >> - if (lev == 1 && cpu->vhyp) { >> + if (unlikely(env->error_code == 1 && cpu->vhyp)) { >> PPCVirtualHypervisorClass *vhc = >> PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); >> + dump_hcall(env); >> vhc->hypercall(cpu->vhyp, cpu); >> return; >> + } else { >> + dump_syscall(env); >> } >> - >> break; >> } >> case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */ >> -- >> 2.30.9 > > >
On Thu Jun 15, 2023 at 7:25 PM AEST, BALATON Zoltan wrote: > On Thu, 15 Jun 2023, Nicholas Piggin wrote: > > On Thu Jun 15, 2023 at 7:34 AM AEST, BALATON Zoltan wrote: > >> After previous changes the hypercall handling in 7xx and 74xx > >> exception handlers can be folded into one if statement to simpilfy > >> this code. > >> > >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > >> --- > >> target/ppc/excp_helper.c | 26 ++++++++++---------------- > >> 1 file changed, 10 insertions(+), 16 deletions(-) > >> > >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > >> index 1682b988ba..662457f342 100644 > >> --- a/target/ppc/excp_helper.c > >> +++ b/target/ppc/excp_helper.c > >> @@ -740,26 +740,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp) > >> break; > >> case POWERPC_EXCP_SYSCALL: /* System call exception */ > >> { > >> - int lev = env->error_code; > > > > I would still keep lev. Self documenting and consistent with books > > handler. > > lev is still there in the books version, but probably not really needed in > these 7xx versions which does not really have level parameter. This hack > should likely go away and replaced with something else on the long run as > this won't work with KVM but that needs some support from VOF or compiling > a different version for pegasos2 which wasn't considered so far. I can add > the local back if you really insist but I don't think it really makes much > sense in these cases for 7xx and 74xx. It is using the sc 1 instruction which does have a lev field though? The hardware might not have such a thing but what is being emulatd here does, so I think lev makes sense. Removing this would be fine, but while you have it yes please just leave it as lev. > >> + PowerPCCPU *cpu = env_archcpu(env); > > > > Is this necessary? > > Yes, for cpu->vhyp below. cpu->vhyp was there before your patch... Thanks, Nick
On Thu, 15 Jun 2023, Nicholas Piggin wrote: > On Thu Jun 15, 2023 at 7:25 PM AEST, BALATON Zoltan wrote: >> On Thu, 15 Jun 2023, Nicholas Piggin wrote: >>> On Thu Jun 15, 2023 at 7:34 AM AEST, BALATON Zoltan wrote: >>>> After previous changes the hypercall handling in 7xx and 74xx >>>> exception handlers can be folded into one if statement to simpilfy >>>> this code. >>>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> target/ppc/excp_helper.c | 26 ++++++++++---------------- >>>> 1 file changed, 10 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >>>> index 1682b988ba..662457f342 100644 >>>> --- a/target/ppc/excp_helper.c >>>> +++ b/target/ppc/excp_helper.c >>>> @@ -740,26 +740,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp) >>>> break; >>>> case POWERPC_EXCP_SYSCALL: /* System call exception */ >>>> { >>>> - int lev = env->error_code; >>> >>> I would still keep lev. Self documenting and consistent with books >>> handler. >> >> lev is still there in the books version, but probably not really needed in >> these 7xx versions which does not really have level parameter. This hack >> should likely go away and replaced with something else on the long run as >> this won't work with KVM but that needs some support from VOF or compiling >> a different version for pegasos2 which wasn't considered so far. I can add >> the local back if you really insist but I don't think it really makes much >> sense in these cases for 7xx and 74xx. > > It is using the sc 1 instruction which does have a lev field though? The > hardware might not have such a thing but what is being emulatd here > does, so I think lev makes sense. > > Removing this would be fine, but while you have it yes please just leave > it as lev. > >>>> + PowerPCCPU *cpu = env_archcpu(env); >>> >>> Is this necessary? >> >> Yes, for cpu->vhyp below. > > cpu->vhyp was there before your patch... Originally I had another patch that chnaged these functions to take an env pointer and since cpu is only needed here this was declared local to where it's used. Now that we don't have those patches that change more functions parameters to env then it's indeed not needed. I've added back lev in v3 instead. Regards, BALATON Zoltan
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 1682b988ba..662457f342 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -740,26 +740,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp) break; case POWERPC_EXCP_SYSCALL: /* System call exception */ { - int lev = env->error_code; + PowerPCCPU *cpu = env_archcpu(env); - if (lev == 1 && cpu->vhyp) { - dump_hcall(env); - } else { - dump_syscall(env); - } /* * The Virtual Open Firmware (VOF) relies on the 'sc 1' * instruction to communicate with QEMU. The pegasos2 machine * uses VOF and the 7xx CPUs, so although the 7xx don't have * HV mode, we need to keep hypercall support. */ - if (lev == 1 && cpu->vhyp) { + if (unlikely(env->error_code == 1 && cpu->vhyp)) { PPCVirtualHypervisorClass *vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); + dump_hcall(env); vhc->hypercall(cpu->vhyp, cpu); return; + } else { + dump_syscall(env); } - break; } case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */ @@ -884,26 +881,23 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp) break; case POWERPC_EXCP_SYSCALL: /* System call exception */ { - int lev = env->error_code; + PowerPCCPU *cpu = env_archcpu(env); - if (lev == 1 && cpu->vhyp) { - dump_hcall(env); - } else { - dump_syscall(env); - } /* * The Virtual Open Firmware (VOF) relies on the 'sc 1' * instruction to communicate with QEMU. The pegasos2 machine * uses VOF and the 74xx CPUs, so although the 74xx don't have * HV mode, we need to keep hypercall support. */ - if (lev == 1 && cpu->vhyp) { + if (unlikely(env->error_code == 1 && cpu->vhyp)) { PPCVirtualHypervisorClass *vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); + dump_hcall(env); vhc->hypercall(cpu->vhyp, cpu); return; + } else { + dump_syscall(env); } - break; } case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */
After previous changes the hypercall handling in 7xx and 74xx exception handlers can be folded into one if statement to simpilfy this code. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- target/ppc/excp_helper.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)