Re: [Qemu-arm] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Philippe Mathieu-Daudé]

Le 20 mars 2018 9:45 PM, "Aaron Lindsay" <address@hidden> a écrit :

On Mar 18 23:41, Philippe Mathieu-Daudé wrote:
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > Signed-off-by: Aaron Lindsay <address@hidden>
> > ---
> >  target/arm/cpu.c       | 15 ++++++++++-----
> >  target/arm/cpu.h       | 23 ++++++++++++-----------
> >  target/arm/internals.h |  7 ++++---
> >  3 files changed, 26 insertions(+), 19 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 072cbbf..5f782bf 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
> >           | CPU_INTERRUPT_EXITTB);
> >  }
> >
> > -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> > +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> >                                   void *opaque)
> >  {
> > -    /* We currently only support registering a single hook function */
> > -    assert(!cpu->el_change_hook);
> > -    cpu->el_change_hook = hook;
> > -    cpu->el_change_hook_opaque = opaque;
> > +    ARMELChangeHook *entry;
> > +    entry = g_malloc0(sizeof (*entry));
>
> imho g_malloc() is enough.

It seems like the only difference is between initializing it to zero
(g_malloc0) and making it as uninitialized (g_malloc) for coverity. Are
there coding standards for when we should choose which?

Since you initialize all members, bzero is not necessary; until someone add another member to the structure. So your way is correct and safer, with a ridiculous performance penalty. 

>
> > +
> > +    entry->hook = hook;
> > +    entry->opaque = opaque;
> > +
> > +    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
> >  }
> >
> >  static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
> > @@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >
> > +    QLIST_INIT(&cpu->el_change_hooks);
> > +
>
> You missed to fill arm_cpu_unrealizefn() with:
>
>     QLIST_FOREACH(...) {
>         QLIST_REMOVE(...);
>         g_free(...);
>     }

Do you mean arm_cpu_finalizefn()?

Yes :)

-Aaron

--
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.