[PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[Fabiano Rosas]

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:
-- 
2.33.1

Re: [PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[Philippe Mathieu-Daudé]

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(+)

Re: [PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[Cédric Le Goater]

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.

Re: [PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[Fabiano Rosas]

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.

Re: [PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[Cédric Le Goater]

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.

Re: [PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[Fabiano Rosas]

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(+)

Re: [PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[Fabiano Rosas]

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.

Re: [PATCH] target/ppc: powerpc_excp: Guard ALIGNMENT interrupt with CONFIG_TCG

[Fabiano Rosas]

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.