| Message ID | 1521232280-13089-9-git-send-email-alindsay@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | More fully implement ARM PMUv3 | expand |
On 03/16/2018 09:31 PM, Aaron Lindsay wrote: > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > --- > 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. > + > + 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(...); } > /* Some features automatically imply others: */ > if (arm_feature(env, ARM_FEATURE_V8)) { > set_feature(env, ARM_FEATURE_V7); > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index f17592b..3b45d3d 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -633,11 +633,17 @@ typedef struct CPUARMState { > > /** > * ARMELChangeHook: > - * type of a function which can be registered via arm_register_el_change_hook() > - * to get callbacks when the CPU changes its exception level or mode. > + * Support registering functions with ARMELChangeHookFn's signature via > + * arm_register_el_change_hook() to get callbacks when the CPU changes its > + * exception level or mode. > */ > -typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque); > - > +typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque); > +typedef struct ARMELChangeHook ARMELChangeHook; > +struct ARMELChangeHook { > + ARMELChangeHookFn *hook; > + void *opaque; > + QLIST_ENTRY(ARMELChangeHook) node; > +}; > > /* These values map onto the return values for > * QEMU_PSCI_0_2_FN_AFFINITY_INFO */ > @@ -826,8 +832,7 @@ struct ARMCPU { > */ > bool cfgend; > > - ARMELChangeHook *el_change_hook; > - void *el_change_hook_opaque; > + QLIST_HEAD(, ARMELChangeHook) el_change_hooks; > > int32_t node_id; /* NUMA node this CPU belongs to */ > > @@ -2895,12 +2900,8 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs) > * 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. > - * > - * Note that we currently only support registering a single hook function, > - * and will assert if this function is called twice. > - * This facility is intended for the use of the GICv3 emulation. > */ > -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook, > +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook, > void *opaque); > > /** > diff --git a/target/arm/internals.h b/target/arm/internals.h > index 47cc224..7df3eda 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, > int mmu_idx, MemTxAttrs attrs, > MemTxResult response, uintptr_t retaddr); > > -/* Call the EL change hook if one has been registered */ > +/* Call any registered EL change hooks */ > static inline void arm_call_el_change_hook(ARMCPU *cpu) > { > - if (cpu->el_change_hook) { > - cpu->el_change_hook(cpu, cpu->el_change_hook_opaque); > + ARMELChangeHook *hook, *next; > + QLIST_FOREACH_SAFE(hook, &cpu->el_change_hooks, node, next) { > + hook->hook(cpu, hook->opaque); > } > } > >
On Mar 18 23:41, Philippe Mathieu-Daudé wrote: > On 03/16/2018 09:31 PM, Aaron Lindsay wrote: > > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > > --- > > 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? > > > + > > + 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()? -Aaron
Le 20 mars 2018 9:45 PM, "Aaron Lindsay" <alindsay@codeaurora.org> 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 <alindsay@codeaurora.org> > > --- > > 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.
On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote: > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > --- > 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)); g_new0(ARMELChangeHook, 1) is nicer for initializing a thing of known size. > + > + 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); > + If you put this in arm_cpu_initfn() you don't have to wonder whether anybody's calling arm_register_el_change_hook() after the CPU object has been created but before it is realized... > /* Some features automatically imply others: */ > if (arm_feature(env, ARM_FEATURE_V8)) { > set_feature(env, ARM_FEATURE_V7); > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index f17592b..3b45d3d 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -633,11 +633,17 @@ typedef struct CPUARMState { > > /** > * ARMELChangeHook: > - * type of a function which can be registered via arm_register_el_change_hook() > - * to get callbacks when the CPU changes its exception level or mode. > + * Support registering functions with ARMELChangeHookFn's signature via > + * arm_register_el_change_hook() to get callbacks when the CPU changes its > + * exception level or mode. > */ This is supposed to be the doc comment for the type, which I think was fine as it is, apart from the name change. If you want to also have a doc comment for the struct type that's fine but would be something different (and is less necessary, because the struct type is internal to the CPU whereas the function type is part of its API to the rest of the system). > -typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque); > - > +typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque); > +typedef struct ARMELChangeHook ARMELChangeHook; > +struct ARMELChangeHook { > + ARMELChangeHookFn *hook; > + void *opaque; > + QLIST_ENTRY(ARMELChangeHook) node; > +}; > > /* These values map onto the return values for > * QEMU_PSCI_0_2_FN_AFFINITY_INFO */ > @@ -826,8 +832,7 @@ struct ARMCPU { > */ > bool cfgend; > > - ARMELChangeHook *el_change_hook; > - void *el_change_hook_opaque; > + QLIST_HEAD(, ARMELChangeHook) el_change_hooks; > > int32_t node_id; /* NUMA node this CPU belongs to */ > > @@ -2895,12 +2900,8 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs) > * 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. > - * > - * Note that we currently only support registering a single hook function, > - * and will assert if this function is called twice. > - * This facility is intended for the use of the GICv3 emulation. > */ > -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook, > +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook, > void *opaque); > > /** > diff --git a/target/arm/internals.h b/target/arm/internals.h > index 47cc224..7df3eda 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, > int mmu_idx, MemTxAttrs attrs, > MemTxResult response, uintptr_t retaddr); > > -/* Call the EL change hook if one has been registered */ > +/* Call any registered EL change hooks */ > static inline void arm_call_el_change_hook(ARMCPU *cpu) > { > - if (cpu->el_change_hook) { > - cpu->el_change_hook(cpu, cpu->el_change_hook_opaque); > + ARMELChangeHook *hook, *next; > + QLIST_FOREACH_SAFE(hook, &cpu->el_change_hooks, node, next) { > + hook->hook(cpu, hook->opaque); > } > } > thanks -- PMM
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)); + + 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); + /* Some features automatically imply others: */ if (arm_feature(env, ARM_FEATURE_V8)) { set_feature(env, ARM_FEATURE_V7); diff --git a/target/arm/cpu.h b/target/arm/cpu.h index f17592b..3b45d3d 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -633,11 +633,17 @@ typedef struct CPUARMState { /** * ARMELChangeHook: - * type of a function which can be registered via arm_register_el_change_hook() - * to get callbacks when the CPU changes its exception level or mode. + * Support registering functions with ARMELChangeHookFn's signature via + * arm_register_el_change_hook() to get callbacks when the CPU changes its + * exception level or mode. */ -typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque); - +typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque); +typedef struct ARMELChangeHook ARMELChangeHook; +struct ARMELChangeHook { + ARMELChangeHookFn *hook; + void *opaque; + QLIST_ENTRY(ARMELChangeHook) node; +}; /* These values map onto the return values for * QEMU_PSCI_0_2_FN_AFFINITY_INFO */ @@ -826,8 +832,7 @@ struct ARMCPU { */ bool cfgend; - ARMELChangeHook *el_change_hook; - void *el_change_hook_opaque; + QLIST_HEAD(, ARMELChangeHook) el_change_hooks; int32_t node_id; /* NUMA node this CPU belongs to */ @@ -2895,12 +2900,8 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs) * 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. - * - * Note that we currently only support registering a single hook function, - * and will assert if this function is called twice. - * This facility is intended for the use of the GICv3 emulation. */ -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook, +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook, void *opaque); /** diff --git a/target/arm/internals.h b/target/arm/internals.h index 47cc224..7df3eda 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, int mmu_idx, MemTxAttrs attrs, MemTxResult response, uintptr_t retaddr); -/* Call the EL change hook if one has been registered */ +/* Call any registered EL change hooks */ static inline void arm_call_el_change_hook(ARMCPU *cpu) { - if (cpu->el_change_hook) { - cpu->el_change_hook(cpu, cpu->el_change_hook_opaque); + ARMELChangeHook *hook, *next; + QLIST_FOREACH_SAFE(hook, &cpu->el_change_hooks, node, next) { + hook->hook(cpu, hook->opaque); } }
Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> --- target/arm/cpu.c | 15 ++++++++++----- target/arm/cpu.h | 23 ++++++++++++----------- target/arm/internals.h | 7 ++++--- 3 files changed, 26 insertions(+), 19 deletions(-)