Re: [PATCH v9 00/10] PMU-EBB support for PPC64 TCG

[Fabiano Rosas]

Cédric Le Goater <clg@kaod.org> writes:

> Hello,
>
> On 12/1/21 16:17, Daniel Henrique Barboza wrote:
>> Hi,
>> 
>> In this new version the most significant change is in patch 6,
>> where a new hflag allows us to not call the instruction helper
>> inside translate.c unless we're absolutely certain that there
>> is an instruction count event being sampled and active in the
>> PMU. This change turned out to be a big boost in performance
>> in the PMU emulation overall, most notably when dealing with
>> cycle events that were calling the helper needlessly.
>> 
>> This and all other changes were suggested by David in his review
>> of the previous version.
>
>
> patch 1-8 look good. I still have some questions on the exception
> handling and how EBB are gated.
>
> I am asking to get the model right for the next step which should
> be to modify the XIVE interrupt controller to generate External
> EBB exceptions.
>
> One more comment, not for now, since the EBB patchset is nearly
> ready.
>
> May be, it is time to think about introducing a per-CPU model
> excp_handlers[] array indexed by POWERPC_EXCP_* exception
> numbers and to duplicate some code for the sake of clarity.
>
> Fabiano, isn't it what you had in mind ?

I had basically changed env->excp_vectors to be an array of objects of
the kind:

  struct PPCInterrupt {
      Object parent;
  
      int id;
      const char *name;
      target_ulong addr;
      ppc_intr_fn_t setup_regs;
  };

we would access it from powerpc_excp() with:

  intr = &env->excp_vectors[excp];
  if (intr->setup_regs) {
      intr->setup_regs(cpu, intr, excp_model, &regs, &ignore);
  }

I also had another series to move the exception models into QOM like
this:

  struct PPCIntrModel {
      Object parent;
  
      int id;
      const char *name;
      target_ulong hreset_vector;
      target_ulong ivor_mask;
      target_ulong ivpr_mask;
      target_ulong excp_prefix;
      PPCInterrupt excp_vectors[POWERPC_EXCP_NB];
  };

  struct PPCIntrModelClass {
      ObjectClass parent_class;
  
      bool (*intr_little_endian)(CPUPPCState *env, bool hv);
      bool (*lpar_env_selection)(CPUPPCState *env);
      target_ulong (*filter_msr)(CPUPPCState *env);
      bool (*set_sixty_four_bit_mode)(CPUPPCState *env, target_ulong *msr);
      bool (*set_ail)(CPUPPCState *env, bool mmu_all_on, bool hv_escalation,
                      bool hv, int *_ail);
      void (*prepare_tlb_miss)(PowerPCCPU *cpu, int excp, target_ulong *new_msr,
                               target_ulong *msr);
      void (*debug_software_tlb)(CPUPPCState *env, int excp);
      void (*init_excp)(PPCIntrModel *im);
  };

So the powerpc_excp() code would become:

    PPCIntrModel *intr_model = &env->im;
    PPCInterrupt *intr;
    ...

    intr = &intr_model->entry_points[excp];
    if (!intr->setup_regs) {
        cpu_abort(cs, "Raised an exception without defined vector %d\n",
                  excp);
    }

    regs.new_nip = intr->addr | intr_model->excp_prefix;
    intr->setup_regs(cpu, intr, intr_model, &regs, &ignore);

I'll rebase it all and work on reducing some of the complexity around
QOM, which was pointed out by David in the previous version:

https://lists.nongnu.org/archive/html/qemu-ppc/2021-06/msg00140.html

Any other suggestions are welcome.

>
> Thanks,
>
> C.