| Message ID | 20211208230650.2125095-1-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG | expand |
On 12/9/21 00:06, Fabiano Rosas wrote: > We cannot have TCG code in powerpc_excp because the function is called > from kvm-only code via ppc_cpu_do_interrupt: > > ../target/ppc/excp_helper.c:463:29: error: implicit declaration of > function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] > > Fortunately, the Alignment interrupt is not among the ones dispatched > from kvm-only code, so we can keep it out of the disable-tcg build for > now. > > Fixes: 336e91f853 ("target/ppc: Move SPR_DSISR setting to powerpc_excp") > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > > --- > > Perhaps we could make powerpc_excp TCG only and have a separate > function that only knows the two interrupts that we use with KVM > (Program, Machine check). But for now this fix will do, I think. If KVM only uses 2 exception vectors, you could guard the enum in target/ppc/cpu.h using #ifdef'ry. While making the include uglier, it will helps to catch vector misused at compile time. > --- > target/ppc/excp_helper.c | 2 ++ > 1 file changed, 2 insertions(+)
On 12/9/21 00:06, Fabiano Rosas wrote: > We cannot have TCG code in powerpc_excp because the function is called > from kvm-only code via ppc_cpu_do_interrupt: > > ../target/ppc/excp_helper.c:463:29: error: implicit declaration of > function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] > > Fortunately, the Alignment interrupt is not among the ones dispatched > from kvm-only code, so we can keep it out of the disable-tcg build for > now. > > Fixes: 336e91f853 ("target/ppc: Move SPR_DSISR setting to powerpc_excp") > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > > --- > > Perhaps we could make powerpc_excp TCG only and have a separate > function that only knows the two interrupts that we use with KVM > (Program, Machine check). But for now this fix will do, I think. > --- > target/ppc/excp_helper.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 17607adbe4..dcf22440cc 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -453,6 +453,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > } > break; > } > +#ifdef CONFIG_TCG > case POWERPC_EXCP_ALIGN: /* Alignment exception */ > /* > * Get rS/rD and rA from faulting opcode. > @@ -464,6 +465,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) > env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16; > } > break; > +#endif > case POWERPC_EXCP_PROGRAM: /* Program exception */ > switch (env->error_code & ~0xF) { > case POWERPC_EXCP_FP: > Shouldn't we move that code under ppc_cpu_do_unaligned_access ? Thanks, C.
Cédric Le Goater <clg@kaod.org> writes: > On 12/9/21 00:06, Fabiano Rosas wrote: >> We cannot have TCG code in powerpc_excp because the function is called >> from kvm-only code via ppc_cpu_do_interrupt: >> >> ../target/ppc/excp_helper.c:463:29: error: implicit declaration of >> function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] >> >> Fortunately, the Alignment interrupt is not among the ones dispatched >> from kvm-only code, so we can keep it out of the disable-tcg build for >> now. >> >> Fixes: 336e91f853 ("target/ppc: Move SPR_DSISR setting to powerpc_excp") >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >> >> --- >> >> Perhaps we could make powerpc_excp TCG only and have a separate >> function that only knows the two interrupts that we use with KVM >> (Program, Machine check). But for now this fix will do, I think. >> --- >> target/ppc/excp_helper.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index 17607adbe4..dcf22440cc 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -453,6 +453,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> } >> break; >> } >> +#ifdef CONFIG_TCG >> case POWERPC_EXCP_ALIGN: /* Alignment exception */ >> /* >> * Get rS/rD and rA from faulting opcode. >> @@ -464,6 +465,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >> env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16; >> } >> break; >> +#endif >> case POWERPC_EXCP_PROGRAM: /* Program exception */ >> switch (env->error_code & ~0xF) { >> case POWERPC_EXCP_FP: >> > > Shouldn't we move that code under ppc_cpu_do_unaligned_access ? Well, it came from there initially. We could revert 336e91f853 and that would fix the issue as well. > > Thanks, > > C.
Richard, On 12/9/21 16:05, Fabiano Rosas wrote: > Cédric Le Goater <clg@kaod.org> writes: > >> On 12/9/21 00:06, Fabiano Rosas wrote: >>> We cannot have TCG code in powerpc_excp because the function is called >>> from kvm-only code via ppc_cpu_do_interrupt: >>> >>> ../target/ppc/excp_helper.c:463:29: error: implicit declaration of >>> function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] >>> >>> Fortunately, the Alignment interrupt is not among the ones dispatched >>> from kvm-only code, so we can keep it out of the disable-tcg build for >>> now. >>> >>> Fixes: 336e91f853 ("target/ppc: Move SPR_DSISR setting to powerpc_excp") >>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >>> >>> --- >>> >>> Perhaps we could make powerpc_excp TCG only and have a separate >>> function that only knows the two interrupts that we use with KVM >>> (Program, Machine check). But for now this fix will do, I think. >>> --- >>> target/ppc/excp_helper.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >>> index 17607adbe4..dcf22440cc 100644 >>> --- a/target/ppc/excp_helper.c >>> +++ b/target/ppc/excp_helper.c >>> @@ -453,6 +453,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >>> } >>> break; >>> } >>> +#ifdef CONFIG_TCG >>> case POWERPC_EXCP_ALIGN: /* Alignment exception */ >>> /* >>> * Get rS/rD and rA from faulting opcode. >>> @@ -464,6 +465,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >>> env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16; >>> } >>> break; >>> +#endif >>> case POWERPC_EXCP_PROGRAM: /* Program exception */ >>> switch (env->error_code & ~0xF) { >>> case POWERPC_EXCP_FP: >>> >> >> Shouldn't we move that code under ppc_cpu_do_unaligned_access ? > > Well, it came from there initially. We could revert 336e91f853 and that > would fix the issue as well. What would you prefer ? Thanks, C.
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 12/9/21 00:06, Fabiano Rosas wrote: >> We cannot have TCG code in powerpc_excp because the function is called >> from kvm-only code via ppc_cpu_do_interrupt: >> >> ../target/ppc/excp_helper.c:463:29: error: implicit declaration of >> function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] >> >> Fortunately, the Alignment interrupt is not among the ones dispatched >> from kvm-only code, so we can keep it out of the disable-tcg build for >> now. >> >> Fixes: 336e91f853 ("target/ppc: Move SPR_DSISR setting to powerpc_excp") >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >> >> --- >> >> Perhaps we could make powerpc_excp TCG only and have a separate >> function that only knows the two interrupts that we use with KVM >> (Program, Machine check). But for now this fix will do, I think. > > If KVM only uses 2 exception vectors, you could guard the > enum in target/ppc/cpu.h using #ifdef'ry. While making the > include uglier, it will helps to catch vector misused at > compile time. Yes, good point. I just noticed that we also use System Reset with KVM. The other two are kvm-only, but this one is in code shared with TCG, so it will need a bit more work to disentangle. But should still be doable. >> --- >> target/ppc/excp_helper.c | 2 ++ >> 1 file changed, 2 insertions(+)
Cédric Le Goater <clg@kaod.org> writes: > Richard, > > On 12/9/21 16:05, Fabiano Rosas wrote: >> Cédric Le Goater <clg@kaod.org> writes: >> >>> On 12/9/21 00:06, Fabiano Rosas wrote: >>>> We cannot have TCG code in powerpc_excp because the function is called >>>> from kvm-only code via ppc_cpu_do_interrupt: >>>> >>>> ../target/ppc/excp_helper.c:463:29: error: implicit declaration of >>>> function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] >>>> >>>> Fortunately, the Alignment interrupt is not among the ones dispatched >>>> from kvm-only code, so we can keep it out of the disable-tcg build for >>>> now. >>>> >>>> Fixes: 336e91f853 ("target/ppc: Move SPR_DSISR setting to powerpc_excp") >>>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >>>> >>>> --- >>>> >>>> Perhaps we could make powerpc_excp TCG only and have a separate >>>> function that only knows the two interrupts that we use with KVM >>>> (Program, Machine check). But for now this fix will do, I think. >>>> --- >>>> target/ppc/excp_helper.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >>>> index 17607adbe4..dcf22440cc 100644 >>>> --- a/target/ppc/excp_helper.c >>>> +++ b/target/ppc/excp_helper.c >>>> @@ -453,6 +453,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >>>> } >>>> break; >>>> } >>>> +#ifdef CONFIG_TCG >>>> case POWERPC_EXCP_ALIGN: /* Alignment exception */ >>>> /* >>>> * Get rS/rD and rA from faulting opcode. >>>> @@ -464,6 +465,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >>>> env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16; >>>> } >>>> break; >>>> +#endif >>>> case POWERPC_EXCP_PROGRAM: /* Program exception */ >>>> switch (env->error_code & ~0xF) { >>>> case POWERPC_EXCP_FP: >>>> >>> >>> Shouldn't we move that code under ppc_cpu_do_unaligned_access ? >> >> Well, it came from there initially. We could revert 336e91f853 and that >> would fix the issue as well. > > What would you prefer ? Well none of this interfere with the work I'm doing, so it really makes no difference. I guess reverting the patch is cleaner than having an ifdef loose in the middle of the code. I'll send a v2 with the revert. > > Thanks, > > C.
Fabiano Rosas <farosas@linux.ibm.com> writes: > Cédric Le Goater <clg@kaod.org> writes: > >> Richard, >> >> On 12/9/21 16:05, Fabiano Rosas wrote: >>> Cédric Le Goater <clg@kaod.org> writes: >>> >>>> On 12/9/21 00:06, Fabiano Rosas wrote: >>>>> We cannot have TCG code in powerpc_excp because the function is called >>>>> from kvm-only code via ppc_cpu_do_interrupt: >>>>> >>>>> ../target/ppc/excp_helper.c:463:29: error: implicit declaration of >>>>> function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] >>>>> >>>>> Fortunately, the Alignment interrupt is not among the ones dispatched >>>>> from kvm-only code, so we can keep it out of the disable-tcg build for >>>>> now. >>>>> >>>>> Fixes: 336e91f853 ("target/ppc: Move SPR_DSISR setting to powerpc_excp") >>>>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >>>>> >>>>> --- >>>>> >>>>> Perhaps we could make powerpc_excp TCG only and have a separate >>>>> function that only knows the two interrupts that we use with KVM >>>>> (Program, Machine check). But for now this fix will do, I think. >>>>> --- >>>>> target/ppc/excp_helper.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >>>>> index 17607adbe4..dcf22440cc 100644 >>>>> --- a/target/ppc/excp_helper.c >>>>> +++ b/target/ppc/excp_helper.c >>>>> @@ -453,6 +453,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >>>>> } >>>>> break; >>>>> } >>>>> +#ifdef CONFIG_TCG >>>>> case POWERPC_EXCP_ALIGN: /* Alignment exception */ >>>>> /* >>>>> * Get rS/rD and rA from faulting opcode. >>>>> @@ -464,6 +465,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) >>>>> env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16; >>>>> } >>>>> break; >>>>> +#endif >>>>> case POWERPC_EXCP_PROGRAM: /* Program exception */ >>>>> switch (env->error_code & ~0xF) { >>>>> case POWERPC_EXCP_FP: >>>>> >>>> >>>> Shouldn't we move that code under ppc_cpu_do_unaligned_access ? >>> >>> Well, it came from there initially. We could revert 336e91f853 and that >>> would fix the issue as well. >> >> What would you prefer ? > > Well none of this interfere with the work I'm doing, so it really makes > no difference. I guess reverting the patch is cleaner than having an > ifdef loose in the middle of the code. I'll send a v2 with the revert. > Ah I missed that you were talking to Richard! That first line got kind of hidden. I already sent a v2, but as I said, I have no preference either way. Let's hear from Richard. Sorry for the confusion =) >> >> Thanks, >> >> C.
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 17607adbe4..dcf22440cc 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -453,6 +453,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) } break; } +#ifdef CONFIG_TCG case POWERPC_EXCP_ALIGN: /* Alignment exception */ /* * Get rS/rD and rA from faulting opcode. @@ -464,6 +465,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16; } break; +#endif case POWERPC_EXCP_PROGRAM: /* Program exception */ switch (env->error_code & ~0xF) { case POWERPC_EXCP_FP:
We cannot have TCG code in powerpc_excp because the function is called from kvm-only code via ppc_cpu_do_interrupt: ../target/ppc/excp_helper.c:463:29: error: implicit declaration of function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] Fortunately, the Alignment interrupt is not among the ones dispatched from kvm-only code, so we can keep it out of the disable-tcg build for now. Fixes: 336e91f853 ("target/ppc: Move SPR_DSISR setting to powerpc_excp") Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- Perhaps we could make powerpc_excp TCG only and have a separate function that only knows the two interrupts that we use with KVM (Program, Machine check). But for now this fix will do, I think. --- target/ppc/excp_helper.c | 2 ++ 1 file changed, 2 insertions(+)