Re: [Qemu-devel] [PATCH v3 09/22] target/arm: Add pre-EL change hooks

[Aaron Lindsay]

On Apr 12 17:49, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <address@hidden> wrote:
> > Because the design of the PMU requires that the counter values be
> > converted between their delta and guest-visible forms for mode
> > filtering, an additional hook which occurs before the EL is changed is
> > necessary.
> >
> > Signed-off-by: Aaron Lindsay <address@hidden>
> > ---
> >  target/arm/cpu.c       | 13 +++++++++++++
> >  target/arm/cpu.h       | 12 ++++++++----
> >  target/arm/helper.c    | 14 ++++++++------
> >  target/arm/internals.h |  7 +++++++
> >  target/arm/op_helper.c |  8 ++++++++
> >  5 files changed, 44 insertions(+), 10 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 5f782bf..a2cb21e 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -55,6 +55,18 @@ static bool arm_cpu_has_work(CPUState *cs)
> >           | CPU_INTERRUPT_EXITTB);
> >  }
> >
> > +void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> > +                                 void *opaque)
> > +{
> > +    ARMELChangeHook *entry;
> > +    entry = g_malloc0(sizeof (*entry));
> 
> g_new0().
> 
> > +
> > +    entry->hook = hook;
> > +    entry->opaque = opaque;
> > +
> > +    QLIST_INSERT_HEAD(&cpu->pre_el_change_hooks, entry, node);
> > +}
> > +
> >  void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> >                                   void *opaque)
> >  {
> > @@ -747,6 +759,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >          return;
> >      }
> >
> > +    QLIST_INIT(&cpu->pre_el_change_hooks);
> >      QLIST_INIT(&cpu->el_change_hooks);
> >
> >      /* Some features automatically imply others: */
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 3b45d3d..b0ef727 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -832,6 +832,7 @@ struct ARMCPU {
> >       */
> >      bool cfgend;
> >
> > +    QLIST_HEAD(, ARMELChangeHook) pre_el_change_hooks;
> >      QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
> >
> >      int32_t node_id; /* NUMA node this CPU belongs to */
> > @@ -2895,12 +2896,15 @@ static inline AddressSpace 
> > *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
> >  #endif
> >
> >  /**
> > + * arm_register_pre_el_change_hook:
> >   * arm_register_el_change_hook:
> > - * Register a hook function which will be called back whenever this
> > - * CPU changes exception level or mode. The hook function will be
> > - * passed a pointer to the ARMCPU and the opaque data pointer passed
> > - * to this function when the hook was registered.
> > + * Register a hook function which will be called back before or after this 
> > CPU
> > + * changes exception level or mode. The hook function will be passed a 
> > pointer
> > + * to the ARMCPU and the opaque data pointer passed to this function when 
> > the
> > + * hook was registered.
> >   */
> 
> I would just have one doc comment for each function, rather than
> trying to share. (Some day we may actually autogenerate HTML docs
> from these comments...)
> 
> Do we make the guarantee that if we call the pre-change hook
> then we will definitely subsequently call the post-change hook?
> (ie does the PMU hook rely on that?)

Yes, the PMU relies on the pre- and post- hooks being called in pairs
since they drive the state machine of sorts that exists in the variables
holding the counter values/deltas. And unless I've really screwed up the
implementation, I believe the change hooks in my patchset make that
guarantee.

-Aaron

> 
> Otherwise looks OK.
> 
> thanks
> -- PMM

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.