| Message ID | 20200731125127.30866-2-robert.foley@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path | expand |
On Fri, Jul 31, 2020 at 08:51:26AM -0400, Robert Foley wrote: > The new flag bql_interrupt, allows the CPUClass to > determine if the BQL should be held during calls to > cpu_exec_interrupt or do_interrupt. > > This is being added in preparation for changes in > cpu_handle_interrupt, which will use this flag. > > Signed-off-by: Robert Foley <robert.foley@linaro.org> > --- > hw/core/cpu.c | 1 + > include/hw/core/cpu.h | 8 ++++++++ > 2 files changed, 9 insertions(+) > > diff --git a/hw/core/cpu.c b/hw/core/cpu.c > index 8707ce2c34..7ab88caa97 100644 > --- a/hw/core/cpu.c > +++ b/hw/core/cpu.c > @@ -425,6 +425,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > k->cpu_exec_exit = cpu_common_noop; > k->cpu_exec_interrupt = cpu_common_exec_interrupt; > k->adjust_watchpoint_address = cpu_adjust_watchpoint_address; > + k->bql_interrupt = true; > set_bit(DEVICE_CATEGORY_CPU, dc->categories); > dc->realize = cpu_common_realizefn; > dc->unrealize = cpu_common_unrealizefn; > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 6a2c77682f..d2c426ee5d 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -157,6 +157,8 @@ struct TranslationBlock; > * @disas_set_info: Setup architecture specific components of disassembly info > * @adjust_watchpoint_address: Perform a target-specific adjustment to an > * address before attempting to match it against watchpoints. > + * @bql_interrupt: Hold BQL while performing the cpu_exec_interrupt > + * or do_interrupt call. > * > * Represents a CPU family or model. > */ > @@ -227,6 +229,7 @@ typedef struct CPUClass { > /* Keep non-pointer data at the end to minimize holes. */ > int gdb_num_core_regs; > bool gdb_stop_before_watchpoint; > + bool bql_interrupt; > } CPUClass; > > /* > @@ -589,6 +592,11 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu) > } > } > > +static inline void cpu_class_enable_bql_interrupt(CPUClass *cc) > +{ > + cc->bql_interrupt = true; > +} Class data is not supposed to change outside class_init. Why do you need this function? I don't see it being used anywhere in this series.
On Fri, 31 Jul 2020 at 13:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > +static inline void cpu_class_disable_bql_interrupt(CPUClass *cc) > > +{ > > + cc->bql_interrupt = false; > > +} > > Class data is not supposed to change outside class_init. Why do > you need this function? I don't see it being used anywhere in > this series. This function was to be called from changes in a later patch series that depend on these changes. BTW, I added a correction above, it should be disable, not enable. The idea is that it is initialized to true, but then the per arch changes would use this call at init time to set it to false as needed. We can remove this function from this series and add it in later when it gets used, it might make things more clear. Thanks, -Rob > -- > Eduardo >
On Fri, Jul 31, 2020 at 03:14:02PM -0400, Robert Foley wrote: > On Fri, 31 Jul 2020 at 13:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > +static inline void cpu_class_disable_bql_interrupt(CPUClass *cc) > > > +{ > > > + cc->bql_interrupt = false; > > > +} > > > > Class data is not supposed to change outside class_init. Why do > > you need this function? I don't see it being used anywhere in > > this series. > > This function was to be called from changes in a later patch series > that depend on these changes. BTW, I added a correction above, > it should be disable, not enable. The idea is that it is initialized to true, > but then the per arch changes would use this call at init time to set > it to false > as needed. If you plan to call it from class_init, I don't think you need a wrapper. You can simply set cc->bql_interrupt=false directly inside arch-specific class_init functions. If you plan to call it from somewhere else, then maybe the field doesn't belong to CPUClass. > > We can remove this function from this series and add it in later when > it gets used, > it might make things more clear. Makes sense to me.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Fri, Jul 31, 2020 at 03:14:02PM -0400, Robert Foley wrote: >> On Fri, 31 Jul 2020 at 13:44, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > > >> > > +static inline void cpu_class_disable_bql_interrupt(CPUClass *cc) >> > > +{ >> > > + cc->bql_interrupt = false; >> > > +} >> > >> > Class data is not supposed to change outside class_init. Why do >> > you need this function? I don't see it being used anywhere in >> > this series. >> >> This function was to be called from changes in a later patch series >> that depend on these changes. BTW, I added a correction above, >> it should be disable, not enable. The idea is that it is initialized to true, >> but then the per arch changes would use this call at init time to set >> it to false >> as needed. > > If you plan to call it from class_init, I don't think you need a > wrapper. You can simply set cc->bql_interrupt=false directly > inside arch-specific class_init functions. We just need to be careful of the ordering so the base class init goes first. Is that always the case? > > If you plan to call it from somewhere else, then maybe the field > doesn't belong to CPUClass. > >> >> We can remove this function from this series and add it in later when >> it gets used, >> it might make things more clear. > > Makes sense to me.
On Sun, Aug 02, 2020 at 05:05:04PM +0100, Alex Bennée wrote: > > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Fri, Jul 31, 2020 at 03:14:02PM -0400, Robert Foley wrote: > >> On Fri, 31 Jul 2020 at 13:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > > > >> > > +static inline void cpu_class_disable_bql_interrupt(CPUClass *cc) > >> > > +{ > >> > > + cc->bql_interrupt = false; > >> > > +} > >> > > >> > Class data is not supposed to change outside class_init. Why do > >> > you need this function? I don't see it being used anywhere in > >> > this series. > >> > >> This function was to be called from changes in a later patch series > >> that depend on these changes. BTW, I added a correction above, > >> it should be disable, not enable. The idea is that it is initialized to true, > >> but then the per arch changes would use this call at init time to set > >> it to false > >> as needed. > > > > If you plan to call it from class_init, I don't think you need a > > wrapper. You can simply set cc->bql_interrupt=false directly > > inside arch-specific class_init functions. > > We just need to be careful of the ordering so the base class init goes > first. Is that always the case? Absolutely. Subclasses overriding class data previously initialized by the base class is a very common pattern in QOM code.
diff --git a/hw/core/cpu.c b/hw/core/cpu.c index 8707ce2c34..7ab88caa97 100644 --- a/hw/core/cpu.c +++ b/hw/core/cpu.c @@ -425,6 +425,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) k->cpu_exec_exit = cpu_common_noop; k->cpu_exec_interrupt = cpu_common_exec_interrupt; k->adjust_watchpoint_address = cpu_adjust_watchpoint_address; + k->bql_interrupt = true; set_bit(DEVICE_CATEGORY_CPU, dc->categories); dc->realize = cpu_common_realizefn; dc->unrealize = cpu_common_unrealizefn; diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 6a2c77682f..d2c426ee5d 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -157,6 +157,8 @@ struct TranslationBlock; * @disas_set_info: Setup architecture specific components of disassembly info * @adjust_watchpoint_address: Perform a target-specific adjustment to an * address before attempting to match it against watchpoints. + * @bql_interrupt: Hold BQL while performing the cpu_exec_interrupt + * or do_interrupt call. * * Represents a CPU family or model. */ @@ -227,6 +229,7 @@ typedef struct CPUClass { /* Keep non-pointer data at the end to minimize holes. */ int gdb_num_core_regs; bool gdb_stop_before_watchpoint; + bool bql_interrupt; } CPUClass; /* @@ -589,6 +592,11 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu) } } +static inline void cpu_class_enable_bql_interrupt(CPUClass *cc) +{ + cc->bql_interrupt = true; +} + /** * qemu_tcg_mttcg_enabled: * Check whether we are running MultiThread TCG or not.
The new flag bql_interrupt, allows the CPUClass to determine if the BQL should be held during calls to cpu_exec_interrupt or do_interrupt. This is being added in preparation for changes in cpu_handle_interrupt, which will use this flag. Signed-off-by: Robert Foley <robert.foley@linaro.org> --- hw/core/cpu.c | 1 + include/hw/core/cpu.h | 8 ++++++++ 2 files changed, 9 insertions(+)