diff mbox series

[1/2] hw/core: Add bql_interrupt flag to CPUClass

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

Commit Message

Robert Foley July 31, 2020, 12:51 p.m. UTC
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(+)

Comments

Eduardo Habkost July 31, 2020, 5:43 p.m. UTC | #1
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.
Robert Foley July 31, 2020, 7:14 p.m. UTC | #2
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
>
Eduardo Habkost July 31, 2020, 7:24 p.m. UTC | #3
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.
Alex Bennée Aug. 2, 2020, 4:05 p.m. UTC | #4
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.
Eduardo Habkost Aug. 4, 2020, 8:36 p.m. UTC | #5
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 mbox series

Patch

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.