Re: [RFC PATCH 5/5] target/ppc: powerpc_excp: Move interrupt raising code to QOM

[Fabiano Rosas]

Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:

> On 01/06/2021 18:46, Fabiano Rosas wrote:

<snip>

>> +struct ppc_intr_args {
>> +    target_ulong nip;
>> +    target_ulong msr;
>> +    target_ulong new_nip;
>> +    target_ulong new_msr;
>> +    int sprn_srr0;
>> +    int sprn_srr1;
>> +    int sprn_asrr0;
>> +    int sprn_asrr1;
>> +    int lev;
>> +};
>> +
> This part also has me a bit confused. Why define it first in 
> excp_helper.c in the last patch just to move it to here now?

Because back then it wasn't used outside of excp_helper.c. People would
probably ask: "why put this in a common header if it is not used
anywhere else?" =)

>> +struct PPCInterrupt {
>> +    Object parent;
>> +
>> +    int id;
>> +    const char *name;
>> +    target_ulong addr;
>> +    ppc_intr_fn_t setup_regs;
>> +};
>> +
>>   #define PPC_INPUT(env) ((env)->bus_model)
>>   
>>   
>> /*****************************************************************************/
>> @@ -1115,7 +1142,7 @@ struct CPUPPCState {
>>       uint32_t irq_input_state;
>>       void **irq_inputs;
>>   
>> -    target_ulong excp_vectors[POWERPC_EXCP_NB]; /* Exception vectors */
>> +    PPCInterrupt entry_points[POWERPC_EXCP_NB];
>>       target_ulong excp_prefix;
>>       target_ulong ivor_mask;
>>       target_ulong ivpr_mask;
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index d0411e7302..d91183357d 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -46,6 +46,7 @@
>>   #include "helper_regs.h"
>>   #include "internal.h"
>>   #include "spr_tcg.h"
>> +#include "ppc_intr.h"
>>   
>>   /* #define PPC_DEBUG_SPR */
>>   /* #define USE_APPLE_GDB */
>> @@ -2132,16 +2133,16 @@ static void register_8xx_sprs(CPUPPCState *env)
>>   static void init_excp_4xx_real(CPUPPCState *env)
>>   {
>>   #if !defined(CONFIG_USER_ONLY)
>> -    env->excp_vectors[POWERPC_EXCP_CRITICAL] = 0x00000100;
>> -    env->excp_vectors[POWERPC_EXCP_MCHECK]   = 0x00000200;
>> -    env->excp_vectors[POWERPC_EXCP_EXTERNAL] = 0x00000500;
>> -    env->excp_vectors[POWERPC_EXCP_ALIGN]    = 0x00000600;
>> -    env->excp_vectors[POWERPC_EXCP_PROGRAM]  = 0x00000700;
>> -    env->excp_vectors[POWERPC_EXCP_SYSCALL]  = 0x00000C00;
>> -    env->excp_vectors[POWERPC_EXCP_PIT]      = 0x00001000;
>> -    env->excp_vectors[POWERPC_EXCP_FIT]      = 0x00001010;
>> -    env->excp_vectors[POWERPC_EXCP_WDT]      = 0x00001020;
>> -    env->excp_vectors[POWERPC_EXCP_DEBUG]    = 0x00002000;
>> +    ppc_intr_add(env, 0x00000100, POWERPC_EXCP_CRITICAL);
>> +    ppc_intr_add(env, 0x00000200, POWERPC_EXCP_MCHECK);
>> +    ppc_intr_add(env, 0x00000500, POWERPC_EXCP_EXTERNAL);
>> +    ppc_intr_add(env, 0x00000600, POWERPC_EXCP_ALIGN);
>> +    ppc_intr_add(env, 0x00000700, POWERPC_EXCP_PROGRAM);
>> +    ppc_intr_add(env, 0x00000C00, POWERPC_EXCP_SYSCALL);
>> +    ppc_intr_add(env, 0x00001000, POWERPC_EXCP_PIT);
>> +    ppc_intr_add(env, 0x00001010, POWERPC_EXCP_FIT);
>> +    ppc_intr_add(env, 0x00001020, POWERPC_EXCP_WDT);
>> +    ppc_intr_add(env, 0x00002000, POWERPC_EXCP_DEBUG);
>>       env->ivor_mask = 0x0000FFF0UL;
>>       env->ivpr_mask = 0xFFFF0000UL;
>>       /* Hardware reset vector */
> <snip>
>> @@ -2624,8 +2625,8 @@ static void init_excp_POWER9(CPUPPCState *env)
>>       init_excp_POWER8(env);
>>   
>>   #if !defined(CONFIG_USER_ONLY)
>> -    env->excp_vectors[POWERPC_EXCP_HVIRT]    = 0x00000EA0;
>> -    env->excp_vectors[POWERPC_EXCP_SYSCALL_VECTORED] = 0x00017000;
>> +    ppc_intr_add(env, 0x00000EA0, POWERPC_EXCP_HVIRT);
>> +    ppc_intr_add(env, 0x00017000, POWERPC_EXCP_SYSCALL_VECTORED);
>>   #endif
>>   }
> Not sure if this is possible, but if this bit can be done separately as 
> an earlier patch, it would make reviewing a lot easier.

It could, but then it would be a synthetic change with not much purpose
on its own. We probably wouldn't want to merge a change that adds a
function that only writes an array position.

But I agree that this patch is on the verge of being too large. I had
another version split into more patches and it felt that we'd need to
keep going back and forth to understand the real impact of the
change. I'll think some more about it for a v2.

>>   
>> @@ -8375,13 +8376,8 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>>       PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>       CPUPPCState *env = &cpu->env;
>>   #if !defined(CONFIG_USER_ONLY)
>> -    int i;
>>   
>>       env->irq_inputs = NULL;
>> -    /* Set all exception vectors to an invalid address */
>> -    for (i = 0; i < POWERPC_EXCP_NB; i++) {
>> -        env->excp_vectors[i] = (target_ulong)(-1ULL);
>> -    }
> We don't need to use this to set invalid values?

I'm now using the interrupt callback pointer for this. So if the
processor has not registered the interrupt, the pointer will be NULL.