diff mbox series

[01/16] hw/core/cpu: Let CPU object have a clock source

Message ID 20200928171539.788309-2-f4bug@amsat.org
State New
Headers show
Series hw/mips: Set CPU frequency | expand

Commit Message

Philippe Mathieu-Daudé Sept. 28, 2020, 5:15 p.m. UTC
Let CPUState have a clock source (named 'clk') and CPUClass
have a clock_update() callback. The clock can be optionally
set Using qdev_connect_clock_in() from the Clock API.
If the clock changes, the optional clock_update() will be
called.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/core/cpu.h |  5 +++++
 hw/core/cpu.c         | 12 ++++++++++++
 2 files changed, 17 insertions(+)

Comments

Igor Mammedov Sept. 30, 2020, 7:43 a.m. UTC | #1
On Mon, 28 Sep 2020 19:15:24 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Let CPUState have a clock source (named 'clk') and CPUClass

> have a clock_update() callback. The clock can be optionally

> set Using qdev_connect_clock_in() from the Clock API.

> If the clock changes, the optional clock_update() will be

> called.


the sole user of it is mips cpu, so question is why
you are making it part of generic CPUm instead of
MIPSCPUClass/MIPSCPU?

> 

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---

>  include/hw/core/cpu.h |  5 +++++

>  hw/core/cpu.c         | 12 ++++++++++++

>  2 files changed, 17 insertions(+)

> 

> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

> index 6c34798c8b3..6989d90c193 100644

> --- a/include/hw/core/cpu.h

> +++ b/include/hw/core/cpu.h

> @@ -31,6 +31,7 @@

>  #include "qemu/thread.h"

>  #include "qemu/plugin.h"

>  #include "qom/object.h"

> +#include "hw/clock.h"

>  

>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

>                                       void *opaque);

> @@ -155,6 +156,7 @@ 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.

> + * @clock_update: Callback for input clock changes

>   *

>   * Represents a CPU family or model.

>   */

> @@ -176,6 +178,7 @@ struct CPUClass {

>                                    unsigned size, MMUAccessType access_type,

>                                    int mmu_idx, MemTxAttrs attrs,

>                                    MemTxResult response, uintptr_t retaddr);

> +    void (*clock_update)(CPUState *cpu);

>      bool (*virtio_is_big_endian)(CPUState *cpu);

>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

>                             uint8_t *buf, int len, bool is_write);

> @@ -316,6 +319,7 @@ struct qemu_work_item;

>   *   QOM parent.

>   * @nr_cores: Number of cores within this CPU package.

>   * @nr_threads: Number of threads within this CPU.

> + * @clock: this CPU source clock (an output clock of another device)

>   * @running: #true if CPU is currently running (lockless).

>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

>   * valid under cpu_list_lock.

> @@ -400,6 +404,7 @@ struct CPUState {

>      int num_ases;

>      AddressSpace *as;

>      MemoryRegion *memory;

> +    Clock *clock;

>  

>      void *env_ptr; /* CPUArchState */

>      IcountDecr *icount_decr_ptr;

> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

> index c55c09f734c..37fcff3ec64 100644

> --- a/hw/core/cpu.c

> +++ b/hw/core/cpu.c

> @@ -30,6 +30,7 @@

>  #include "qemu/qemu-print.h"

>  #include "sysemu/tcg.h"

>  #include "hw/boards.h"

> +#include "hw/qdev-clock.h"

>  #include "hw/qdev-properties.h"

>  #include "trace/trace-root.h"

>  #include "qemu/plugin.h"

> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

>      trace_guest_cpu_reset(cpu);

>  }

>  

> +static void cpu_clk_update(void *opaque)

> +{

> +    CPUState *cpu = opaque;

> +    CPUClass *cc = CPU_GET_CLASS(cpu);

> +

> +    if (cc->clock_update) {

> +        cc->clock_update(cpu);

> +    }

> +}

> +

>  static void cpu_common_reset(DeviceState *dev)

>  {

>      CPUState *cpu = CPU(dev);

> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

>      /* the default value is changed by qemu_init_vcpu() for softmmu */

>      cpu->nr_cores = 1;

>      cpu->nr_threads = 1;

> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

>  

>      qemu_mutex_init(&cpu->work_mutex);

>      QSIMPLEQ_INIT(&cpu->work_list);
Philippe Mathieu-Daudé Sept. 30, 2020, 10:16 a.m. UTC | #2
+arm/ppc/riscv folks

On 9/30/20 9:43 AM, Igor Mammedov wrote:
> On Mon, 28 Sep 2020 19:15:24 +0200

> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> 

>> Let CPUState have a clock source (named 'clk') and CPUClass

>> have a clock_update() callback. The clock can be optionally

>> set Using qdev_connect_clock_in() from the Clock API.

>> If the clock changes, the optional clock_update() will be

>> called.

> 

> the sole user of it is mips cpu, so question is why

> you are making it part of generic CPUm instead of

> MIPSCPUClass/MIPSCPU?


This is a feature of the CPU, regardless its architecture.

I expect the other archs to start using it soon.

> 

>>

>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>> ---

>>  include/hw/core/cpu.h |  5 +++++

>>  hw/core/cpu.c         | 12 ++++++++++++

>>  2 files changed, 17 insertions(+)

>>

>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

>> index 6c34798c8b3..6989d90c193 100644

>> --- a/include/hw/core/cpu.h

>> +++ b/include/hw/core/cpu.h

>> @@ -31,6 +31,7 @@

>>  #include "qemu/thread.h"

>>  #include "qemu/plugin.h"

>>  #include "qom/object.h"

>> +#include "hw/clock.h"

>>  

>>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

>>                                       void *opaque);

>> @@ -155,6 +156,7 @@ 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.

>> + * @clock_update: Callback for input clock changes

>>   *

>>   * Represents a CPU family or model.

>>   */

>> @@ -176,6 +178,7 @@ struct CPUClass {

>>                                    unsigned size, MMUAccessType access_type,

>>                                    int mmu_idx, MemTxAttrs attrs,

>>                                    MemTxResult response, uintptr_t retaddr);

>> +    void (*clock_update)(CPUState *cpu);

>>      bool (*virtio_is_big_endian)(CPUState *cpu);

>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

>>                             uint8_t *buf, int len, bool is_write);

>> @@ -316,6 +319,7 @@ struct qemu_work_item;

>>   *   QOM parent.

>>   * @nr_cores: Number of cores within this CPU package.

>>   * @nr_threads: Number of threads within this CPU.

>> + * @clock: this CPU source clock (an output clock of another device)

>>   * @running: #true if CPU is currently running (lockless).

>>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

>>   * valid under cpu_list_lock.

>> @@ -400,6 +404,7 @@ struct CPUState {

>>      int num_ases;

>>      AddressSpace *as;

>>      MemoryRegion *memory;

>> +    Clock *clock;

>>  

>>      void *env_ptr; /* CPUArchState */

>>      IcountDecr *icount_decr_ptr;

>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

>> index c55c09f734c..37fcff3ec64 100644

>> --- a/hw/core/cpu.c

>> +++ b/hw/core/cpu.c

>> @@ -30,6 +30,7 @@

>>  #include "qemu/qemu-print.h"

>>  #include "sysemu/tcg.h"

>>  #include "hw/boards.h"

>> +#include "hw/qdev-clock.h"

>>  #include "hw/qdev-properties.h"

>>  #include "trace/trace-root.h"

>>  #include "qemu/plugin.h"

>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

>>      trace_guest_cpu_reset(cpu);

>>  }

>>  

>> +static void cpu_clk_update(void *opaque)

>> +{

>> +    CPUState *cpu = opaque;

>> +    CPUClass *cc = CPU_GET_CLASS(cpu);

>> +

>> +    if (cc->clock_update) {

>> +        cc->clock_update(cpu);

>> +    }

>> +}

>> +

>>  static void cpu_common_reset(DeviceState *dev)

>>  {

>>      CPUState *cpu = CPU(dev);

>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

>>      /* the default value is changed by qemu_init_vcpu() for softmmu */

>>      cpu->nr_cores = 1;

>>      cpu->nr_threads = 1;

>> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

>>  

>>      qemu_mutex_init(&cpu->work_mutex);

>>      QSIMPLEQ_INIT(&cpu->work_list);

>
Igor Mammedov Oct. 5, 2020, 4:40 p.m. UTC | #3
On Wed, 30 Sep 2020 12:16:53 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> +arm/ppc/riscv folks

> 

> On 9/30/20 9:43 AM, Igor Mammedov wrote:

> > On Mon, 28 Sep 2020 19:15:24 +0200

> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> >   

> >> Let CPUState have a clock source (named 'clk') and CPUClass

> >> have a clock_update() callback. The clock can be optionally

> >> set Using qdev_connect_clock_in() from the Clock API.

> >> If the clock changes, the optional clock_update() will be

> >> called.  

> > 

> > the sole user of it is mips cpu, so question is why

> > you are making it part of generic CPUm instead of

> > MIPSCPUClass/MIPSCPU?  

> 

> This is a feature of the CPU, regardless its architecture.

> 

> I expect the other archs to start using it soon.


if there aren't any plans to actually to do that,
I'd keep it to MIPS class and generalize later when there is demand.
 
> 

> >   

> >>

> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> >> ---

> >>  include/hw/core/cpu.h |  5 +++++

> >>  hw/core/cpu.c         | 12 ++++++++++++

> >>  2 files changed, 17 insertions(+)

> >>

> >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

> >> index 6c34798c8b3..6989d90c193 100644

> >> --- a/include/hw/core/cpu.h

> >> +++ b/include/hw/core/cpu.h

> >> @@ -31,6 +31,7 @@

> >>  #include "qemu/thread.h"

> >>  #include "qemu/plugin.h"

> >>  #include "qom/object.h"

> >> +#include "hw/clock.h"

> >>  

> >>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

> >>                                       void *opaque);

> >> @@ -155,6 +156,7 @@ 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.

> >> + * @clock_update: Callback for input clock changes

> >>   *

> >>   * Represents a CPU family or model.

> >>   */

> >> @@ -176,6 +178,7 @@ struct CPUClass {

> >>                                    unsigned size, MMUAccessType access_type,

> >>                                    int mmu_idx, MemTxAttrs attrs,

> >>                                    MemTxResult response, uintptr_t retaddr);

> >> +    void (*clock_update)(CPUState *cpu);

> >>      bool (*virtio_is_big_endian)(CPUState *cpu);

> >>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

> >>                             uint8_t *buf, int len, bool is_write);

> >> @@ -316,6 +319,7 @@ struct qemu_work_item;

> >>   *   QOM parent.

> >>   * @nr_cores: Number of cores within this CPU package.

> >>   * @nr_threads: Number of threads within this CPU.

> >> + * @clock: this CPU source clock (an output clock of another device)

> >>   * @running: #true if CPU is currently running (lockless).

> >>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

> >>   * valid under cpu_list_lock.

> >> @@ -400,6 +404,7 @@ struct CPUState {

> >>      int num_ases;

> >>      AddressSpace *as;

> >>      MemoryRegion *memory;

> >> +    Clock *clock;

> >>  

> >>      void *env_ptr; /* CPUArchState */

> >>      IcountDecr *icount_decr_ptr;

> >> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

> >> index c55c09f734c..37fcff3ec64 100644

> >> --- a/hw/core/cpu.c

> >> +++ b/hw/core/cpu.c

> >> @@ -30,6 +30,7 @@

> >>  #include "qemu/qemu-print.h"

> >>  #include "sysemu/tcg.h"

> >>  #include "hw/boards.h"

> >> +#include "hw/qdev-clock.h"

> >>  #include "hw/qdev-properties.h"

> >>  #include "trace/trace-root.h"

> >>  #include "qemu/plugin.h"

> >> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

> >>      trace_guest_cpu_reset(cpu);

> >>  }

> >>  

> >> +static void cpu_clk_update(void *opaque)

> >> +{

> >> +    CPUState *cpu = opaque;

> >> +    CPUClass *cc = CPU_GET_CLASS(cpu);

> >> +

> >> +    if (cc->clock_update) {

> >> +        cc->clock_update(cpu);

> >> +    }

> >> +}

> >> +

> >>  static void cpu_common_reset(DeviceState *dev)

> >>  {

> >>      CPUState *cpu = CPU(dev);

> >> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

> >>      /* the default value is changed by qemu_init_vcpu() for softmmu */

> >>      cpu->nr_cores = 1;

> >>      cpu->nr_threads = 1;

> >> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

> >>  

> >>      qemu_mutex_init(&cpu->work_mutex);

> >>      QSIMPLEQ_INIT(&cpu->work_list);  

> >   

>
Philippe Mathieu-Daudé Oct. 5, 2020, 5:19 p.m. UTC | #4
On 10/5/20 6:40 PM, Igor Mammedov wrote:
> On Wed, 30 Sep 2020 12:16:53 +0200

> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> 

>> +arm/ppc/riscv folks

>>

>> On 9/30/20 9:43 AM, Igor Mammedov wrote:

>>> On Mon, 28 Sep 2020 19:15:24 +0200

>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>>   

>>>> Let CPUState have a clock source (named 'clk') and CPUClass

>>>> have a clock_update() callback. The clock can be optionally

>>>> set Using qdev_connect_clock_in() from the Clock API.

>>>> If the clock changes, the optional clock_update() will be

>>>> called.  

>>>

>>> the sole user of it is mips cpu, so question is why

>>> you are making it part of generic CPUm instead of

>>> MIPSCPUClass/MIPSCPU?  

>>

>> This is a feature of the CPU, regardless its architecture.

>>

>> I expect the other archs to start using it soon.

> 

> if there aren't any plans to actually to do that,

> I'd keep it to MIPS class and generalize later when there is demand.


OK will update the patch.

>  

>>

>>>   

>>>>

>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>>> ---

>>>>  include/hw/core/cpu.h |  5 +++++

>>>>  hw/core/cpu.c         | 12 ++++++++++++

>>>>  2 files changed, 17 insertions(+)

>>>>

>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

>>>> index 6c34798c8b3..6989d90c193 100644

>>>> --- a/include/hw/core/cpu.h

>>>> +++ b/include/hw/core/cpu.h

>>>> @@ -31,6 +31,7 @@

>>>>  #include "qemu/thread.h"

>>>>  #include "qemu/plugin.h"

>>>>  #include "qom/object.h"

>>>> +#include "hw/clock.h"

>>>>  

>>>>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

>>>>                                       void *opaque);

>>>> @@ -155,6 +156,7 @@ 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.

>>>> + * @clock_update: Callback for input clock changes

>>>>   *

>>>>   * Represents a CPU family or model.

>>>>   */

>>>> @@ -176,6 +178,7 @@ struct CPUClass {

>>>>                                    unsigned size, MMUAccessType access_type,

>>>>                                    int mmu_idx, MemTxAttrs attrs,

>>>>                                    MemTxResult response, uintptr_t retaddr);

>>>> +    void (*clock_update)(CPUState *cpu);

>>>>      bool (*virtio_is_big_endian)(CPUState *cpu);

>>>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

>>>>                             uint8_t *buf, int len, bool is_write);

>>>> @@ -316,6 +319,7 @@ struct qemu_work_item;

>>>>   *   QOM parent.

>>>>   * @nr_cores: Number of cores within this CPU package.

>>>>   * @nr_threads: Number of threads within this CPU.

>>>> + * @clock: this CPU source clock (an output clock of another device)

>>>>   * @running: #true if CPU is currently running (lockless).

>>>>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

>>>>   * valid under cpu_list_lock.

>>>> @@ -400,6 +404,7 @@ struct CPUState {

>>>>      int num_ases;

>>>>      AddressSpace *as;

>>>>      MemoryRegion *memory;

>>>> +    Clock *clock;

>>>>  

>>>>      void *env_ptr; /* CPUArchState */

>>>>      IcountDecr *icount_decr_ptr;

>>>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

>>>> index c55c09f734c..37fcff3ec64 100644

>>>> --- a/hw/core/cpu.c

>>>> +++ b/hw/core/cpu.c

>>>> @@ -30,6 +30,7 @@

>>>>  #include "qemu/qemu-print.h"

>>>>  #include "sysemu/tcg.h"

>>>>  #include "hw/boards.h"

>>>> +#include "hw/qdev-clock.h"

>>>>  #include "hw/qdev-properties.h"

>>>>  #include "trace/trace-root.h"

>>>>  #include "qemu/plugin.h"

>>>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

>>>>      trace_guest_cpu_reset(cpu);

>>>>  }

>>>>  

>>>> +static void cpu_clk_update(void *opaque)

>>>> +{

>>>> +    CPUState *cpu = opaque;

>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);

>>>> +

>>>> +    if (cc->clock_update) {

>>>> +        cc->clock_update(cpu);

>>>> +    }

>>>> +}

>>>> +

>>>>  static void cpu_common_reset(DeviceState *dev)

>>>>  {

>>>>      CPUState *cpu = CPU(dev);

>>>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

>>>>      /* the default value is changed by qemu_init_vcpu() for softmmu */

>>>>      cpu->nr_cores = 1;

>>>>      cpu->nr_threads = 1;

>>>> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

>>>>  

>>>>      qemu_mutex_init(&cpu->work_mutex);

>>>>      QSIMPLEQ_INIT(&cpu->work_list);  

>>>   

>>

>
Eduardo Habkost Oct. 5, 2020, 5:44 p.m. UTC | #5
On Mon, Oct 05, 2020 at 06:40:09PM +0200, Igor Mammedov wrote:
> On Wed, 30 Sep 2020 12:16:53 +0200

> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> 

> > +arm/ppc/riscv folks

> > 

> > On 9/30/20 9:43 AM, Igor Mammedov wrote:

> > > On Mon, 28 Sep 2020 19:15:24 +0200

> > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> > >   

> > >> Let CPUState have a clock source (named 'clk') and CPUClass


The language here confuses me: is this a clock source inside the
CPU, or just a clock input that can be connected to a clock
source somewhere?

See also comment below[1].

> > >> have a clock_update() callback. The clock can be optionally

> > >> set Using qdev_connect_clock_in() from the Clock API.

> > >> If the clock changes, the optional clock_update() will be

> > >> called.  


What does "clock change" means?  Is this just about the
frequency, or something else?

(By reading the Clock API documentation, it looks like it only
represents the clock frequency, but I'm not sure)

> > > 

> > > the sole user of it is mips cpu, so question is why

> > > you are making it part of generic CPUm instead of

> > > MIPSCPUClass/MIPSCPU?  

> > 

> > This is a feature of the CPU, regardless its architecture.

> > 

> > I expect the other archs to start using it soon.

> 

> if there aren't any plans to actually to do that,

> I'd keep it to MIPS class and generalize later when there is demand.


I normally don't mind if a feature is generic from the beginning.
But in this case I'm inclined to agree with Igor.  Unless we
expect to see arch-independent code to use CPUState.clock soon
(do we?), having CPUState.clock existing but unused by most
architectures would be misleading.

Also, at least on x86 there are so many different clock sources,
that I'm not sure it would be a desirable to have a generic clock
input named "clk".

>  

> > 

> > >   

> > >>

> > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> > >> ---

> > >>  include/hw/core/cpu.h |  5 +++++

> > >>  hw/core/cpu.c         | 12 ++++++++++++

> > >>  2 files changed, 17 insertions(+)

> > >>

> > >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

> > >> index 6c34798c8b3..6989d90c193 100644

> > >> --- a/include/hw/core/cpu.h

> > >> +++ b/include/hw/core/cpu.h

> > >> @@ -31,6 +31,7 @@

> > >>  #include "qemu/thread.h"

> > >>  #include "qemu/plugin.h"

> > >>  #include "qom/object.h"

> > >> +#include "hw/clock.h"

> > >>  

> > >>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

> > >>                                       void *opaque);

> > >> @@ -155,6 +156,7 @@ 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.

> > >> + * @clock_update: Callback for input clock changes

> > >>   *

> > >>   * Represents a CPU family or model.

> > >>   */

> > >> @@ -176,6 +178,7 @@ struct CPUClass {

> > >>                                    unsigned size, MMUAccessType access_type,

> > >>                                    int mmu_idx, MemTxAttrs attrs,

> > >>                                    MemTxResult response, uintptr_t retaddr);

> > >> +    void (*clock_update)(CPUState *cpu);

> > >>      bool (*virtio_is_big_endian)(CPUState *cpu);

> > >>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

> > >>                             uint8_t *buf, int len, bool is_write);

> > >> @@ -316,6 +319,7 @@ struct qemu_work_item;

> > >>   *   QOM parent.

> > >>   * @nr_cores: Number of cores within this CPU package.

> > >>   * @nr_threads: Number of threads within this CPU.

> > >> + * @clock: this CPU source clock (an output clock of another device)


[1]

What does "source clock" means?  Is this the same as "clock input"?


> > >>   * @running: #true if CPU is currently running (lockless).

> > >>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

> > >>   * valid under cpu_list_lock.

> > >> @@ -400,6 +404,7 @@ struct CPUState {

> > >>      int num_ases;

> > >>      AddressSpace *as;

> > >>      MemoryRegion *memory;

> > >> +    Clock *clock;

> > >>  

> > >>      void *env_ptr; /* CPUArchState */

> > >>      IcountDecr *icount_decr_ptr;

> > >> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

> > >> index c55c09f734c..37fcff3ec64 100644

> > >> --- a/hw/core/cpu.c

> > >> +++ b/hw/core/cpu.c

> > >> @@ -30,6 +30,7 @@

> > >>  #include "qemu/qemu-print.h"

> > >>  #include "sysemu/tcg.h"

> > >>  #include "hw/boards.h"

> > >> +#include "hw/qdev-clock.h"

> > >>  #include "hw/qdev-properties.h"

> > >>  #include "trace/trace-root.h"

> > >>  #include "qemu/plugin.h"

> > >> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

> > >>      trace_guest_cpu_reset(cpu);

> > >>  }

> > >>  

> > >> +static void cpu_clk_update(void *opaque)

> > >> +{

> > >> +    CPUState *cpu = opaque;

> > >> +    CPUClass *cc = CPU_GET_CLASS(cpu);

> > >> +

> > >> +    if (cc->clock_update) {

> > >> +        cc->clock_update(cpu);

> > >> +    }

> > >> +}

> > >> +

> > >>  static void cpu_common_reset(DeviceState *dev)

> > >>  {

> > >>      CPUState *cpu = CPU(dev);

> > >> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

> > >>      /* the default value is changed by qemu_init_vcpu() for softmmu */

> > >>      cpu->nr_cores = 1;

> > >>      cpu->nr_threads = 1;

> > >> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

> > >>  

> > >>      qemu_mutex_init(&cpu->work_mutex);

> > >>      QSIMPLEQ_INIT(&cpu->work_list);  

> > >   

> > 

> 


-- 
Eduardo
Philippe Mathieu-Daudé Oct. 5, 2020, 6:09 p.m. UTC | #6
On 10/5/20 7:44 PM, Eduardo Habkost wrote:
> On Mon, Oct 05, 2020 at 06:40:09PM +0200, Igor Mammedov wrote:

>> On Wed, 30 Sep 2020 12:16:53 +0200

>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>

>>> +arm/ppc/riscv folks

>>>

>>> On 9/30/20 9:43 AM, Igor Mammedov wrote:

>>>> On Mon, 28 Sep 2020 19:15:24 +0200

>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>>>   

>>>>> Let CPUState have a clock source (named 'clk') and CPUClass

> 

> The language here confuses me: is this a clock source inside the

> CPU, or just a clock input that can be connected to a clock

> source somewhere?


2nd description, "somewhere". I'll reword.

> 

> See also comment below[1].

> 

>>>>> have a clock_update() callback. The clock can be optionally

>>>>> set Using qdev_connect_clock_in() from the Clock API.

>>>>> If the clock changes, the optional clock_update() will be

>>>>> called.  

> 

> What does "clock change" means?  Is this just about the

> frequency, or something else?


A frequency changes -- which can be because a parent (in the
clock tree) changed its source using a MUX.

> 

> (By reading the Clock API documentation, it looks like it only

> represents the clock frequency, but I'm not sure)

> 

>>>>

>>>> the sole user of it is mips cpu, so question is why

>>>> you are making it part of generic CPUm instead of

>>>> MIPSCPUClass/MIPSCPU?  

>>>

>>> This is a feature of the CPU, regardless its architecture.

>>>

>>> I expect the other archs to start using it soon.

>>

>> if there aren't any plans to actually to do that,

>> I'd keep it to MIPS class and generalize later when there is demand.


No problem.

> 

> I normally don't mind if a feature is generic from the beginning.

> But in this case I'm inclined to agree with Igor.  Unless we

> expect to see arch-independent code to use CPUState.clock soon

> (do we?), having CPUState.clock existing but unused by most

> architectures would be misleading.

> 

> Also, at least on x86 there are so many different clock sources,

> that I'm not sure it would be a desirable to have a generic clock

> input named "clk".


Well X86 is the arch I'm less confident with. Anyhow if it has
multiple clock sources, I'd expect a Clock MUX block to select
an unique clock to feed the CPU.

> 

>>  

>>>

>>>>   

>>>>>

>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>>>> ---

>>>>>  include/hw/core/cpu.h |  5 +++++

>>>>>  hw/core/cpu.c         | 12 ++++++++++++

>>>>>  2 files changed, 17 insertions(+)

>>>>>

>>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

>>>>> index 6c34798c8b3..6989d90c193 100644

>>>>> --- a/include/hw/core/cpu.h

>>>>> +++ b/include/hw/core/cpu.h

>>>>> @@ -31,6 +31,7 @@

>>>>>  #include "qemu/thread.h"

>>>>>  #include "qemu/plugin.h"

>>>>>  #include "qom/object.h"

>>>>> +#include "hw/clock.h"

>>>>>  

>>>>>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

>>>>>                                       void *opaque);

>>>>> @@ -155,6 +156,7 @@ 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.

>>>>> + * @clock_update: Callback for input clock changes

>>>>>   *

>>>>>   * Represents a CPU family or model.

>>>>>   */

>>>>> @@ -176,6 +178,7 @@ struct CPUClass {

>>>>>                                    unsigned size, MMUAccessType access_type,

>>>>>                                    int mmu_idx, MemTxAttrs attrs,

>>>>>                                    MemTxResult response, uintptr_t retaddr);

>>>>> +    void (*clock_update)(CPUState *cpu);

>>>>>      bool (*virtio_is_big_endian)(CPUState *cpu);

>>>>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

>>>>>                             uint8_t *buf, int len, bool is_write);

>>>>> @@ -316,6 +319,7 @@ struct qemu_work_item;

>>>>>   *   QOM parent.

>>>>>   * @nr_cores: Number of cores within this CPU package.

>>>>>   * @nr_threads: Number of threads within this CPU.

>>>>> + * @clock: this CPU source clock (an output clock of another device)

> 

> [1]

> 

> What does "source clock" means?  Is this the same as "clock input"?


Yes, for clocks it is common to use source/sink instead of input/output.
I'll try to reword.

> 

> 

>>>>>   * @running: #true if CPU is currently running (lockless).

>>>>>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

>>>>>   * valid under cpu_list_lock.

>>>>> @@ -400,6 +404,7 @@ struct CPUState {

>>>>>      int num_ases;

>>>>>      AddressSpace *as;

>>>>>      MemoryRegion *memory;

>>>>> +    Clock *clock;

>>>>>  

>>>>>      void *env_ptr; /* CPUArchState */

>>>>>      IcountDecr *icount_decr_ptr;

>>>>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

>>>>> index c55c09f734c..37fcff3ec64 100644

>>>>> --- a/hw/core/cpu.c

>>>>> +++ b/hw/core/cpu.c

>>>>> @@ -30,6 +30,7 @@

>>>>>  #include "qemu/qemu-print.h"

>>>>>  #include "sysemu/tcg.h"

>>>>>  #include "hw/boards.h"

>>>>> +#include "hw/qdev-clock.h"

>>>>>  #include "hw/qdev-properties.h"

>>>>>  #include "trace/trace-root.h"

>>>>>  #include "qemu/plugin.h"

>>>>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

>>>>>      trace_guest_cpu_reset(cpu);

>>>>>  }

>>>>>  

>>>>> +static void cpu_clk_update(void *opaque)

>>>>> +{

>>>>> +    CPUState *cpu = opaque;

>>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);

>>>>> +

>>>>> +    if (cc->clock_update) {

>>>>> +        cc->clock_update(cpu);

>>>>> +    }

>>>>> +}

>>>>> +

>>>>>  static void cpu_common_reset(DeviceState *dev)

>>>>>  {

>>>>>      CPUState *cpu = CPU(dev);

>>>>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

>>>>>      /* the default value is changed by qemu_init_vcpu() for softmmu */

>>>>>      cpu->nr_cores = 1;

>>>>>      cpu->nr_threads = 1;

>>>>> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

>>>>>  

>>>>>      qemu_mutex_init(&cpu->work_mutex);

>>>>>      QSIMPLEQ_INIT(&cpu->work_list);  

>>>>   

>>>

>>

>
Philippe Mathieu-Daudé Oct. 5, 2020, 6:29 p.m. UTC | #7
On 10/5/20 8:09 PM, Philippe Mathieu-Daudé wrote:
> On 10/5/20 7:44 PM, Eduardo Habkost wrote:

>> On Mon, Oct 05, 2020 at 06:40:09PM +0200, Igor Mammedov wrote:

>>> On Wed, 30 Sep 2020 12:16:53 +0200

>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>>

>>>> +arm/ppc/riscv folks

>>>>

>>>> On 9/30/20 9:43 AM, Igor Mammedov wrote:

>>>>> On Mon, 28 Sep 2020 19:15:24 +0200

>>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>>>>   

>>>>>> Let CPUState have a clock source (named 'clk') and CPUClass

>>

>> The language here confuses me: is this a clock source inside the

>> CPU, or just a clock input that can be connected to a clock

>> source somewhere?

> 

> 2nd description, "somewhere". I'll reword.

> 

>>

>> See also comment below[1].

>>

>>>>>> have a clock_update() callback. The clock can be optionally

>>>>>> set Using qdev_connect_clock_in() from the Clock API.

>>>>>> If the clock changes, the optional clock_update() will be

>>>>>> called.  

>>

>> What does "clock change" means?  Is this just about the

>> frequency, or something else?

> 

> A frequency changes -- which can be because a parent (in the

> clock tree) changed its source using a MUX.

> 

>>

>> (By reading the Clock API documentation, it looks like it only

>> represents the clock frequency, but I'm not sure)

>>

>>>>>

>>>>> the sole user of it is mips cpu, so question is why

>>>>> you are making it part of generic CPUm instead of

>>>>> MIPSCPUClass/MIPSCPU?  

>>>>

>>>> This is a feature of the CPU, regardless its architecture.

>>>>

>>>> I expect the other archs to start using it soon.

>>>

>>> if there aren't any plans to actually to do that,

>>> I'd keep it to MIPS class and generalize later when there is demand.

> 

> No problem.

> 

>>

>> I normally don't mind if a feature is generic from the beginning.

>> But in this case I'm inclined to agree with Igor.  Unless we

>> expect to see arch-independent code to use CPUState.clock soon

>> (do we?), having CPUState.clock existing but unused by most

>> architectures would be misleading.

>>

>> Also, at least on x86 there are so many different clock sources,

>> that I'm not sure it would be a desirable to have a generic clock

>> input named "clk".

> 

> Well X86 is the arch I'm less confident with. Anyhow if it has

> multiple clock sources, I'd expect a Clock MUX block to select

> an unique clock to feed the CPU.

> 

>>

>>>  

>>>>

>>>>>   

>>>>>>

>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>>>>> ---

>>>>>>  include/hw/core/cpu.h |  5 +++++

>>>>>>  hw/core/cpu.c         | 12 ++++++++++++

>>>>>>  2 files changed, 17 insertions(+)

>>>>>>

>>>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

>>>>>> index 6c34798c8b3..6989d90c193 100644

>>>>>> --- a/include/hw/core/cpu.h

>>>>>> +++ b/include/hw/core/cpu.h

>>>>>> @@ -31,6 +31,7 @@

>>>>>>  #include "qemu/thread.h"

>>>>>>  #include "qemu/plugin.h"

>>>>>>  #include "qom/object.h"

>>>>>> +#include "hw/clock.h"

>>>>>>  

>>>>>>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

>>>>>>                                       void *opaque);

>>>>>> @@ -155,6 +156,7 @@ 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.

>>>>>> + * @clock_update: Callback for input clock changes

>>>>>>   *

>>>>>>   * Represents a CPU family or model.

>>>>>>   */

>>>>>> @@ -176,6 +178,7 @@ struct CPUClass {

>>>>>>                                    unsigned size, MMUAccessType access_type,

>>>>>>                                    int mmu_idx, MemTxAttrs attrs,

>>>>>>                                    MemTxResult response, uintptr_t retaddr);

>>>>>> +    void (*clock_update)(CPUState *cpu);

>>>>>>      bool (*virtio_is_big_endian)(CPUState *cpu);

>>>>>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

>>>>>>                             uint8_t *buf, int len, bool is_write);

>>>>>> @@ -316,6 +319,7 @@ struct qemu_work_item;

>>>>>>   *   QOM parent.

>>>>>>   * @nr_cores: Number of cores within this CPU package.

>>>>>>   * @nr_threads: Number of threads within this CPU.

>>>>>> + * @clock: this CPU source clock (an output clock of another device)

>>

>> [1]

>>

>> What does "source clock" means?  Is this the same as "clock input"?

> 

> Yes, for clocks it is common to use source/sink instead of input/output.

> I'll try to reword.


Hard to reword when it looks clear to oneself...

@clock is the source, @cpu is the sink.
@clock clocks @cpu at some frequency.

One output from @clock is the @cpu.
The @cpu has an unique input: @clock.

Damien/Peter/Luc, do you have better description suggestions?

> 

>>

>>

>>>>>>   * @running: #true if CPU is currently running (lockless).

>>>>>>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

>>>>>>   * valid under cpu_list_lock.

>>>>>> @@ -400,6 +404,7 @@ struct CPUState {

>>>>>>      int num_ases;

>>>>>>      AddressSpace *as;

>>>>>>      MemoryRegion *memory;

>>>>>> +    Clock *clock;

>>>>>>  

>>>>>>      void *env_ptr; /* CPUArchState */

>>>>>>      IcountDecr *icount_decr_ptr;

>>>>>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

>>>>>> index c55c09f734c..37fcff3ec64 100644

>>>>>> --- a/hw/core/cpu.c

>>>>>> +++ b/hw/core/cpu.c

>>>>>> @@ -30,6 +30,7 @@

>>>>>>  #include "qemu/qemu-print.h"

>>>>>>  #include "sysemu/tcg.h"

>>>>>>  #include "hw/boards.h"

>>>>>> +#include "hw/qdev-clock.h"

>>>>>>  #include "hw/qdev-properties.h"

>>>>>>  #include "trace/trace-root.h"

>>>>>>  #include "qemu/plugin.h"

>>>>>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

>>>>>>      trace_guest_cpu_reset(cpu);

>>>>>>  }

>>>>>>  

>>>>>> +static void cpu_clk_update(void *opaque)

>>>>>> +{

>>>>>> +    CPUState *cpu = opaque;

>>>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);

>>>>>> +

>>>>>> +    if (cc->clock_update) {

>>>>>> +        cc->clock_update(cpu);

>>>>>> +    }

>>>>>> +}

>>>>>> +

>>>>>>  static void cpu_common_reset(DeviceState *dev)

>>>>>>  {

>>>>>>      CPUState *cpu = CPU(dev);

>>>>>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

>>>>>>      /* the default value is changed by qemu_init_vcpu() for softmmu */

>>>>>>      cpu->nr_cores = 1;

>>>>>>      cpu->nr_threads = 1;

>>>>>> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

>>>>>>  

>>>>>>      qemu_mutex_init(&cpu->work_mutex);

>>>>>>      QSIMPLEQ_INIT(&cpu->work_list);  

>>>>>   

>>>>

>>>

>>

>
Eduardo Habkost Oct. 5, 2020, 6:43 p.m. UTC | #8
On Mon, Oct 05, 2020 at 08:09:31PM +0200, Philippe Mathieu-Daudé wrote:
> On 10/5/20 7:44 PM, Eduardo Habkost wrote:

> > On Mon, Oct 05, 2020 at 06:40:09PM +0200, Igor Mammedov wrote:

> >> On Wed, 30 Sep 2020 12:16:53 +0200

> >> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> >>

> >>> +arm/ppc/riscv folks

> >>>

> >>> On 9/30/20 9:43 AM, Igor Mammedov wrote:

> >>>> On Mon, 28 Sep 2020 19:15:24 +0200

> >>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> >>>>   

> >>>>> Let CPUState have a clock source (named 'clk') and CPUClass

> > 

> > The language here confuses me: is this a clock source inside the

> > CPU, or just a clock input that can be connected to a clock

> > source somewhere?

> 

> 2nd description, "somewhere". I'll reword.

> 

> > 

> > See also comment below[1].

> > 

> >>>>> have a clock_update() callback. The clock can be optionally

> >>>>> set Using qdev_connect_clock_in() from the Clock API.

> >>>>> If the clock changes, the optional clock_update() will be

> >>>>> called.  

> > 

> > What does "clock change" means?  Is this just about the

> > frequency, or something else?

> 

> A frequency changes -- which can be because a parent (in the

> clock tree) changed its source using a MUX.

> 

> > 

> > (By reading the Clock API documentation, it looks like it only

> > represents the clock frequency, but I'm not sure)

> > 

> >>>>

> >>>> the sole user of it is mips cpu, so question is why

> >>>> you are making it part of generic CPUm instead of

> >>>> MIPSCPUClass/MIPSCPU?  

> >>>

> >>> This is a feature of the CPU, regardless its architecture.

> >>>

> >>> I expect the other archs to start using it soon.

> >>

> >> if there aren't any plans to actually to do that,

> >> I'd keep it to MIPS class and generalize later when there is demand.

> 

> No problem.

> 

> > 

> > I normally don't mind if a feature is generic from the beginning.

> > But in this case I'm inclined to agree with Igor.  Unless we

> > expect to see arch-independent code to use CPUState.clock soon

> > (do we?), having CPUState.clock existing but unused by most

> > architectures would be misleading.

> > 

> > Also, at least on x86 there are so many different clock sources,

> > that I'm not sure it would be a desirable to have a generic clock

> > input named "clk".

> 

> Well X86 is the arch I'm less confident with. Anyhow if it has

> multiple clock sources, I'd expect a Clock MUX block to select

> an unique clock to feed the CPU.


I don't mean there are different clocks that can feed the CPU,
but that there are multiple software interfaces that report a
clock frequency to the guest OS, and they can disagree with each
other.  The phrase "the actual frequency of the CPU clock" is
going to be ambiguous.

> 

[...]
> >>>>>   *   QOM parent.

> >>>>>   * @nr_cores: Number of cores within this CPU package.

> >>>>>   * @nr_threads: Number of threads within this CPU.

> >>>>> + * @clock: this CPU source clock (an output clock of another device)

> > 

> > [1]

> > 

> > What does "source clock" means?  Is this the same as "clock input"?

> 

> Yes, for clocks it is common to use source/sink instead of input/output.

> I'll try to reword.


If we use source/sink terminology, wouldn't CPUState.clock be a
sink, which can be connected to a source somewhere else?

> 

> > 

> > 

> >>>>>   * @running: #true if CPU is currently running (lockless).

> >>>>>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

> >>>>>   * valid under cpu_list_lock.

> >>>>> @@ -400,6 +404,7 @@ struct CPUState {

> >>>>>      int num_ases;

> >>>>>      AddressSpace *as;

> >>>>>      MemoryRegion *memory;

> >>>>> +    Clock *clock;

> >>>>>  

> >>>>>      void *env_ptr; /* CPUArchState */

> >>>>>      IcountDecr *icount_decr_ptr;

> >>>>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

> >>>>> index c55c09f734c..37fcff3ec64 100644

> >>>>> --- a/hw/core/cpu.c

> >>>>> +++ b/hw/core/cpu.c

> >>>>> @@ -30,6 +30,7 @@

> >>>>>  #include "qemu/qemu-print.h"

> >>>>>  #include "sysemu/tcg.h"

> >>>>>  #include "hw/boards.h"

> >>>>> +#include "hw/qdev-clock.h"

> >>>>>  #include "hw/qdev-properties.h"

> >>>>>  #include "trace/trace-root.h"

> >>>>>  #include "qemu/plugin.h"

> >>>>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

> >>>>>      trace_guest_cpu_reset(cpu);

> >>>>>  }

> >>>>>  

> >>>>> +static void cpu_clk_update(void *opaque)

> >>>>> +{

> >>>>> +    CPUState *cpu = opaque;

> >>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);

> >>>>> +

> >>>>> +    if (cc->clock_update) {

> >>>>> +        cc->clock_update(cpu);

> >>>>> +    }

> >>>>> +}

> >>>>> +

> >>>>>  static void cpu_common_reset(DeviceState *dev)

> >>>>>  {

> >>>>>      CPUState *cpu = CPU(dev);

> >>>>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

> >>>>>      /* the default value is changed by qemu_init_vcpu() for softmmu */

> >>>>>      cpu->nr_cores = 1;

> >>>>>      cpu->nr_threads = 1;

> >>>>> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

> >>>>>  

> >>>>>      qemu_mutex_init(&cpu->work_mutex);

> >>>>>      QSIMPLEQ_INIT(&cpu->work_list);  

> >>>>   

> >>>

> >>

> > 

> 


-- 
Eduardo
Eduardo Habkost Oct. 5, 2020, 7:22 p.m. UTC | #9
On Mon, Oct 05, 2020 at 08:29:24PM +0200, Philippe Mathieu-Daudé wrote:
> On 10/5/20 8:09 PM, Philippe Mathieu-Daudé wrote:

> > On 10/5/20 7:44 PM, Eduardo Habkost wrote:

> >> On Mon, Oct 05, 2020 at 06:40:09PM +0200, Igor Mammedov wrote:

> >>> On Wed, 30 Sep 2020 12:16:53 +0200

> >>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> >>>

> >>>> +arm/ppc/riscv folks

> >>>>

> >>>> On 9/30/20 9:43 AM, Igor Mammedov wrote:

> >>>>> On Mon, 28 Sep 2020 19:15:24 +0200

> >>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> >>>>>   

> >>>>>> Let CPUState have a clock source (named 'clk') and CPUClass

> >>

> >> The language here confuses me: is this a clock source inside the

> >> CPU, or just a clock input that can be connected to a clock

> >> source somewhere?

> > 

> > 2nd description, "somewhere". I'll reword.

> > 

> >>

> >> See also comment below[1].

> >>

> >>>>>> have a clock_update() callback. The clock can be optionally

> >>>>>> set Using qdev_connect_clock_in() from the Clock API.

> >>>>>> If the clock changes, the optional clock_update() will be

> >>>>>> called.  

> >>

> >> What does "clock change" means?  Is this just about the

> >> frequency, or something else?

> > 

> > A frequency changes -- which can be because a parent (in the

> > clock tree) changed its source using a MUX.

> > 

> >>

> >> (By reading the Clock API documentation, it looks like it only

> >> represents the clock frequency, but I'm not sure)

> >>

> >>>>>

> >>>>> the sole user of it is mips cpu, so question is why

> >>>>> you are making it part of generic CPUm instead of

> >>>>> MIPSCPUClass/MIPSCPU?  

> >>>>

> >>>> This is a feature of the CPU, regardless its architecture.

> >>>>

> >>>> I expect the other archs to start using it soon.

> >>>

> >>> if there aren't any plans to actually to do that,

> >>> I'd keep it to MIPS class and generalize later when there is demand.

> > 

> > No problem.

> > 

> >>

> >> I normally don't mind if a feature is generic from the beginning.

> >> But in this case I'm inclined to agree with Igor.  Unless we

> >> expect to see arch-independent code to use CPUState.clock soon

> >> (do we?), having CPUState.clock existing but unused by most

> >> architectures would be misleading.

> >>

> >> Also, at least on x86 there are so many different clock sources,

> >> that I'm not sure it would be a desirable to have a generic clock

> >> input named "clk".

> > 

> > Well X86 is the arch I'm less confident with. Anyhow if it has

> > multiple clock sources, I'd expect a Clock MUX block to select

> > an unique clock to feed the CPU.

> > 

> >>

> >>>  

> >>>>

> >>>>>   

> >>>>>>

> >>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> >>>>>> ---

> >>>>>>  include/hw/core/cpu.h |  5 +++++

> >>>>>>  hw/core/cpu.c         | 12 ++++++++++++

> >>>>>>  2 files changed, 17 insertions(+)

> >>>>>>

> >>>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

> >>>>>> index 6c34798c8b3..6989d90c193 100644

> >>>>>> --- a/include/hw/core/cpu.h

> >>>>>> +++ b/include/hw/core/cpu.h

> >>>>>> @@ -31,6 +31,7 @@

> >>>>>>  #include "qemu/thread.h"

> >>>>>>  #include "qemu/plugin.h"

> >>>>>>  #include "qom/object.h"

> >>>>>> +#include "hw/clock.h"

> >>>>>>  

> >>>>>>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

> >>>>>>                                       void *opaque);

> >>>>>> @@ -155,6 +156,7 @@ 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.

> >>>>>> + * @clock_update: Callback for input clock changes

> >>>>>>   *

> >>>>>>   * Represents a CPU family or model.

> >>>>>>   */

> >>>>>> @@ -176,6 +178,7 @@ struct CPUClass {

> >>>>>>                                    unsigned size, MMUAccessType access_type,

> >>>>>>                                    int mmu_idx, MemTxAttrs attrs,

> >>>>>>                                    MemTxResult response, uintptr_t retaddr);

> >>>>>> +    void (*clock_update)(CPUState *cpu);

> >>>>>>      bool (*virtio_is_big_endian)(CPUState *cpu);

> >>>>>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

> >>>>>>                             uint8_t *buf, int len, bool is_write);

> >>>>>> @@ -316,6 +319,7 @@ struct qemu_work_item;

> >>>>>>   *   QOM parent.

> >>>>>>   * @nr_cores: Number of cores within this CPU package.

> >>>>>>   * @nr_threads: Number of threads within this CPU.

> >>>>>> + * @clock: this CPU source clock (an output clock of another device)

> >>

> >> [1]

> >>

> >> What does "source clock" means?  Is this the same as "clock input"?

> > 

> > Yes, for clocks it is common to use source/sink instead of input/output.

> > I'll try to reword.

> 

> Hard to reword when it looks clear to oneself...

> 

> @clock is the source, @cpu is the sink.

> @clock clocks @cpu at some frequency.

> 

> One output from @clock is the @cpu.

> The @cpu has an unique input: @clock.


The interchangeable usage of "clock source" and "clock input" is
what confuses me here.  CPUState.clock seems to be a clock input,
which may or may not be connected to a clock source.

You seem to imply that "clock source" and "clock input" are
synonymous, but that's not what I understand from the clock API
documentation.

> 

> Damien/Peter/Luc, do you have better description suggestions?

> 

> > 

> >>

> >>

> >>>>>>   * @running: #true if CPU is currently running (lockless).

> >>>>>>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

> >>>>>>   * valid under cpu_list_lock.

> >>>>>> @@ -400,6 +404,7 @@ struct CPUState {

> >>>>>>      int num_ases;

> >>>>>>      AddressSpace *as;

> >>>>>>      MemoryRegion *memory;

> >>>>>> +    Clock *clock;

> >>>>>>  

> >>>>>>      void *env_ptr; /* CPUArchState */

> >>>>>>      IcountDecr *icount_decr_ptr;

> >>>>>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

> >>>>>> index c55c09f734c..37fcff3ec64 100644

> >>>>>> --- a/hw/core/cpu.c

> >>>>>> +++ b/hw/core/cpu.c

> >>>>>> @@ -30,6 +30,7 @@

> >>>>>>  #include "qemu/qemu-print.h"

> >>>>>>  #include "sysemu/tcg.h"

> >>>>>>  #include "hw/boards.h"

> >>>>>> +#include "hw/qdev-clock.h"

> >>>>>>  #include "hw/qdev-properties.h"

> >>>>>>  #include "trace/trace-root.h"

> >>>>>>  #include "qemu/plugin.h"

> >>>>>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

> >>>>>>      trace_guest_cpu_reset(cpu);

> >>>>>>  }

> >>>>>>  

> >>>>>> +static void cpu_clk_update(void *opaque)

> >>>>>> +{

> >>>>>> +    CPUState *cpu = opaque;

> >>>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);

> >>>>>> +

> >>>>>> +    if (cc->clock_update) {

> >>>>>> +        cc->clock_update(cpu);

> >>>>>> +    }

> >>>>>> +}

> >>>>>> +

> >>>>>>  static void cpu_common_reset(DeviceState *dev)

> >>>>>>  {

> >>>>>>      CPUState *cpu = CPU(dev);

> >>>>>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

> >>>>>>      /* the default value is changed by qemu_init_vcpu() for softmmu */

> >>>>>>      cpu->nr_cores = 1;

> >>>>>>      cpu->nr_threads = 1;

> >>>>>> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

> >>>>>>  

> >>>>>>      qemu_mutex_init(&cpu->work_mutex);

> >>>>>>      QSIMPLEQ_INIT(&cpu->work_list);  

> >>>>>   

> >>>>

> >>>

> >>

> > 

> 


-- 
Eduardo
Philippe Mathieu-Daudé Oct. 6, 2020, 6:11 p.m. UTC | #10
On 10/5/20 9:22 PM, Eduardo Habkost wrote:
> On Mon, Oct 05, 2020 at 08:29:24PM +0200, Philippe Mathieu-Daudé wrote:

>> On 10/5/20 8:09 PM, Philippe Mathieu-Daudé wrote:

>>> On 10/5/20 7:44 PM, Eduardo Habkost wrote:

>>>> On Mon, Oct 05, 2020 at 06:40:09PM +0200, Igor Mammedov wrote:

>>>>> On Wed, 30 Sep 2020 12:16:53 +0200

>>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>>>>

>>>>>> +arm/ppc/riscv folks

>>>>>>

>>>>>> On 9/30/20 9:43 AM, Igor Mammedov wrote:

>>>>>>> On Mon, 28 Sep 2020 19:15:24 +0200

>>>>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>>>>>>   

>>>>>>>> Let CPUState have a clock source (named 'clk') and CPUClass

>>>>

>>>> The language here confuses me: is this a clock source inside the

>>>> CPU, or just a clock input that can be connected to a clock

>>>> source somewhere?

>>>

>>> 2nd description, "somewhere". I'll reword.

>>>

>>>>

>>>> See also comment below[1].

>>>>

>>>>>>>> have a clock_update() callback. The clock can be optionally

>>>>>>>> set Using qdev_connect_clock_in() from the Clock API.

>>>>>>>> If the clock changes, the optional clock_update() will be

>>>>>>>> called.  

>>>>

>>>> What does "clock change" means?  Is this just about the

>>>> frequency, or something else?

>>>

>>> A frequency changes -- which can be because a parent (in the

>>> clock tree) changed its source using a MUX.

>>>

>>>>

>>>> (By reading the Clock API documentation, it looks like it only

>>>> represents the clock frequency, but I'm not sure)

>>>>

>>>>>>>

>>>>>>> the sole user of it is mips cpu, so question is why

>>>>>>> you are making it part of generic CPUm instead of

>>>>>>> MIPSCPUClass/MIPSCPU?  

>>>>>>

>>>>>> This is a feature of the CPU, regardless its architecture.

>>>>>>

>>>>>> I expect the other archs to start using it soon.

>>>>>

>>>>> if there aren't any plans to actually to do that,

>>>>> I'd keep it to MIPS class and generalize later when there is demand.

>>>

>>> No problem.

>>>

>>>>

>>>> I normally don't mind if a feature is generic from the beginning.

>>>> But in this case I'm inclined to agree with Igor.  Unless we

>>>> expect to see arch-independent code to use CPUState.clock soon

>>>> (do we?), having CPUState.clock existing but unused by most

>>>> architectures would be misleading.

>>>>

>>>> Also, at least on x86 there are so many different clock sources,

>>>> that I'm not sure it would be a desirable to have a generic clock

>>>> input named "clk".

>>>

>>> Well X86 is the arch I'm less confident with. Anyhow if it has

>>> multiple clock sources, I'd expect a Clock MUX block to select

>>> an unique clock to feed the CPU.

>>>

>>>>

>>>>>  

>>>>>>

>>>>>>>   

>>>>>>>>

>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>>>>>>> ---

>>>>>>>>  include/hw/core/cpu.h |  5 +++++

>>>>>>>>  hw/core/cpu.c         | 12 ++++++++++++

>>>>>>>>  2 files changed, 17 insertions(+)

>>>>>>>>

>>>>>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

>>>>>>>> index 6c34798c8b3..6989d90c193 100644

>>>>>>>> --- a/include/hw/core/cpu.h

>>>>>>>> +++ b/include/hw/core/cpu.h

>>>>>>>> @@ -31,6 +31,7 @@

>>>>>>>>  #include "qemu/thread.h"

>>>>>>>>  #include "qemu/plugin.h"

>>>>>>>>  #include "qom/object.h"

>>>>>>>> +#include "hw/clock.h"

>>>>>>>>  

>>>>>>>>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

>>>>>>>>                                       void *opaque);

>>>>>>>> @@ -155,6 +156,7 @@ 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.

>>>>>>>> + * @clock_update: Callback for input clock changes

>>>>>>>>   *

>>>>>>>>   * Represents a CPU family or model.

>>>>>>>>   */

>>>>>>>> @@ -176,6 +178,7 @@ struct CPUClass {

>>>>>>>>                                    unsigned size, MMUAccessType access_type,

>>>>>>>>                                    int mmu_idx, MemTxAttrs attrs,

>>>>>>>>                                    MemTxResult response, uintptr_t retaddr);

>>>>>>>> +    void (*clock_update)(CPUState *cpu);

>>>>>>>>      bool (*virtio_is_big_endian)(CPUState *cpu);

>>>>>>>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

>>>>>>>>                             uint8_t *buf, int len, bool is_write);

>>>>>>>> @@ -316,6 +319,7 @@ struct qemu_work_item;

>>>>>>>>   *   QOM parent.

>>>>>>>>   * @nr_cores: Number of cores within this CPU package.

>>>>>>>>   * @nr_threads: Number of threads within this CPU.

>>>>>>>> + * @clock: this CPU source clock (an output clock of another device)

>>>>

>>>> [1]

>>>>

>>>> What does "source clock" means?  Is this the same as "clock input"?

>>>

>>> Yes, for clocks it is common to use source/sink instead of input/output.

>>> I'll try to reword.

>>

>> Hard to reword when it looks clear to oneself...

>>

>> @clock is the source, @cpu is the sink.

>> @clock clocks @cpu at some frequency.

>>

>> One output from @clock is the @cpu.

>> The @cpu has an unique input: @clock.

> 

> The interchangeable usage of "clock source" and "clock input" is

> what confuses me here.  CPUState.clock seems to be a clock input,

> which may or may not be connected to a clock source.

> 

> You seem to imply that "clock source" and "clock input" are

> synonymous, but that's not what I understand from the clock API

> documentation.


Hmm the concepts are different.

From a clock generator point of view, the "output" is
the point where the signal leaves the block, to be
eventually consumed by a sink. The generator doesn't
know what the sinks are made of:

+-----------+   +----------> Sink1
|           |   |
|           +---+ ClkOut1
|           |   |
|  ClkGen   |   +----------> Sink2
|           |
|           +--> ClkOut2
|           |
+-----------+

From a device point of view, the "input" is where
the external signal (generated by a source) is
connected. The device doesn't know what is the
source made of:

                     +-----------+
                     |           |
                     |           |
(Source)  ClkIn ----->  Device   |
                     |           |
                     |           |
                     +-----------+

So input/output refer to interface to connect the
clock signal.

Source/sink refer to the other object having a
relation with this signal.

> 

>>

>> Damien/Peter/Luc, do you have better description suggestions?

>>

>>>

>>>>

>>>>

>>>>>>>>   * @running: #true if CPU is currently running (lockless).

>>>>>>>>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

>>>>>>>>   * valid under cpu_list_lock.

>>>>>>>> @@ -400,6 +404,7 @@ struct CPUState {

>>>>>>>>      int num_ases;

>>>>>>>>      AddressSpace *as;

>>>>>>>>      MemoryRegion *memory;

>>>>>>>> +    Clock *clock;

>>>>>>>>  

>>>>>>>>      void *env_ptr; /* CPUArchState */

>>>>>>>>      IcountDecr *icount_decr_ptr;

>>>>>>>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

>>>>>>>> index c55c09f734c..37fcff3ec64 100644

>>>>>>>> --- a/hw/core/cpu.c

>>>>>>>> +++ b/hw/core/cpu.c

>>>>>>>> @@ -30,6 +30,7 @@

>>>>>>>>  #include "qemu/qemu-print.h"

>>>>>>>>  #include "sysemu/tcg.h"

>>>>>>>>  #include "hw/boards.h"

>>>>>>>> +#include "hw/qdev-clock.h"

>>>>>>>>  #include "hw/qdev-properties.h"

>>>>>>>>  #include "trace/trace-root.h"

>>>>>>>>  #include "qemu/plugin.h"

>>>>>>>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

>>>>>>>>      trace_guest_cpu_reset(cpu);

>>>>>>>>  }

>>>>>>>>  

>>>>>>>> +static void cpu_clk_update(void *opaque)

>>>>>>>> +{

>>>>>>>> +    CPUState *cpu = opaque;

>>>>>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);

>>>>>>>> +

>>>>>>>> +    if (cc->clock_update) {

>>>>>>>> +        cc->clock_update(cpu);

>>>>>>>> +    }

>>>>>>>> +}

>>>>>>>> +

>>>>>>>>  static void cpu_common_reset(DeviceState *dev)

>>>>>>>>  {

>>>>>>>>      CPUState *cpu = CPU(dev);

>>>>>>>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

>>>>>>>>      /* the default value is changed by qemu_init_vcpu() for softmmu */

>>>>>>>>      cpu->nr_cores = 1;

>>>>>>>>      cpu->nr_threads = 1;

>>>>>>>> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

>>>>>>>>  

>>>>>>>>      qemu_mutex_init(&cpu->work_mutex);

>>>>>>>>      QSIMPLEQ_INIT(&cpu->work_list);  

>>>>>>>   

>>>>>>

>>>>>

>>>>

>>>

>>

>
Philippe Mathieu-Daudé Oct. 6, 2020, 7:53 p.m. UTC | #11
On 10/6/20 8:11 PM, Philippe Mathieu-Daudé wrote:
> On 10/5/20 9:22 PM, Eduardo Habkost wrote:

>> On Mon, Oct 05, 2020 at 08:29:24PM +0200, Philippe Mathieu-Daudé wrote:

>>> On 10/5/20 8:09 PM, Philippe Mathieu-Daudé wrote:

>>>> On 10/5/20 7:44 PM, Eduardo Habkost wrote:

>>>>> On Mon, Oct 05, 2020 at 06:40:09PM +0200, Igor Mammedov wrote:

>>>>>> On Wed, 30 Sep 2020 12:16:53 +0200

>>>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>>>>>

>>>>>>> +arm/ppc/riscv folks

>>>>>>>

>>>>>>> On 9/30/20 9:43 AM, Igor Mammedov wrote:

>>>>>>>> On Mon, 28 Sep 2020 19:15:24 +0200

>>>>>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>>>>>>>>   

>>>>>>>>> Let CPUState have a clock source (named 'clk') and CPUClass

>>>>>

>>>>> The language here confuses me: is this a clock source inside the

>>>>> CPU, or just a clock input that can be connected to a clock

>>>>> source somewhere?

>>>>

>>>> 2nd description, "somewhere". I'll reword.

>>>>

>>>>>

>>>>> See also comment below[1].

>>>>>

>>>>>>>>> have a clock_update() callback. The clock can be optionally

>>>>>>>>> set Using qdev_connect_clock_in() from the Clock API.

>>>>>>>>> If the clock changes, the optional clock_update() will be

>>>>>>>>> called.  

>>>>>

>>>>> What does "clock change" means?  Is this just about the

>>>>> frequency, or something else?

>>>>

>>>> A frequency changes -- which can be because a parent (in the

>>>> clock tree) changed its source using a MUX.

>>>>

>>>>>

>>>>> (By reading the Clock API documentation, it looks like it only

>>>>> represents the clock frequency, but I'm not sure)

>>>>>

>>>>>>>>

>>>>>>>> the sole user of it is mips cpu, so question is why

>>>>>>>> you are making it part of generic CPUm instead of

>>>>>>>> MIPSCPUClass/MIPSCPU?  

>>>>>>>

>>>>>>> This is a feature of the CPU, regardless its architecture.

>>>>>>>

>>>>>>> I expect the other archs to start using it soon.

>>>>>>

>>>>>> if there aren't any plans to actually to do that,

>>>>>> I'd keep it to MIPS class and generalize later when there is demand.

>>>>

>>>> No problem.

>>>>

>>>>>

>>>>> I normally don't mind if a feature is generic from the beginning.

>>>>> But in this case I'm inclined to agree with Igor.  Unless we

>>>>> expect to see arch-independent code to use CPUState.clock soon

>>>>> (do we?), having CPUState.clock existing but unused by most

>>>>> architectures would be misleading.

>>>>>

>>>>> Also, at least on x86 there are so many different clock sources,

>>>>> that I'm not sure it would be a desirable to have a generic clock

>>>>> input named "clk".

>>>>

>>>> Well X86 is the arch I'm less confident with. Anyhow if it has

>>>> multiple clock sources, I'd expect a Clock MUX block to select

>>>> an unique clock to feed the CPU.

>>>>

>>>>>

>>>>>>  

>>>>>>>

>>>>>>>>   

>>>>>>>>>

>>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>>>>>>>> ---

>>>>>>>>>  include/hw/core/cpu.h |  5 +++++

>>>>>>>>>  hw/core/cpu.c         | 12 ++++++++++++

>>>>>>>>>  2 files changed, 17 insertions(+)

>>>>>>>>>

>>>>>>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h

>>>>>>>>> index 6c34798c8b3..6989d90c193 100644

>>>>>>>>> --- a/include/hw/core/cpu.h

>>>>>>>>> +++ b/include/hw/core/cpu.h

>>>>>>>>> @@ -31,6 +31,7 @@

>>>>>>>>>  #include "qemu/thread.h"

>>>>>>>>>  #include "qemu/plugin.h"

>>>>>>>>>  #include "qom/object.h"

>>>>>>>>> +#include "hw/clock.h"

>>>>>>>>>  

>>>>>>>>>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,

>>>>>>>>>                                       void *opaque);

>>>>>>>>> @@ -155,6 +156,7 @@ 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.

>>>>>>>>> + * @clock_update: Callback for input clock changes

>>>>>>>>>   *

>>>>>>>>>   * Represents a CPU family or model.

>>>>>>>>>   */

>>>>>>>>> @@ -176,6 +178,7 @@ struct CPUClass {

>>>>>>>>>                                    unsigned size, MMUAccessType access_type,

>>>>>>>>>                                    int mmu_idx, MemTxAttrs attrs,

>>>>>>>>>                                    MemTxResult response, uintptr_t retaddr);

>>>>>>>>> +    void (*clock_update)(CPUState *cpu);

>>>>>>>>>      bool (*virtio_is_big_endian)(CPUState *cpu);

>>>>>>>>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,

>>>>>>>>>                             uint8_t *buf, int len, bool is_write);

>>>>>>>>> @@ -316,6 +319,7 @@ struct qemu_work_item;

>>>>>>>>>   *   QOM parent.

>>>>>>>>>   * @nr_cores: Number of cores within this CPU package.

>>>>>>>>>   * @nr_threads: Number of threads within this CPU.

>>>>>>>>> + * @clock: this CPU source clock (an output clock of another device)

>>>>>

>>>>> [1]

>>>>>

>>>>> What does "source clock" means?  Is this the same as "clock input"?

>>>>

>>>> Yes, for clocks it is common to use source/sink instead of input/output.

>>>> I'll try to reword.

>>>

>>> Hard to reword when it looks clear to oneself...

>>>

>>> @clock is the source, @cpu is the sink.

>>> @clock clocks @cpu at some frequency.

>>>

>>> One output from @clock is the @cpu.

>>> The @cpu has an unique input: @clock.

>>

>> The interchangeable usage of "clock source" and "clock input" is

>> what confuses me here.  CPUState.clock seems to be a clock input,

>> which may or may not be connected to a clock source.

>>

>> You seem to imply that "clock source" and "clock input" are

>> synonymous, but that's not what I understand from the clock API

>> documentation.

> 

> Hmm the concepts are different.

> 

> From a clock generator point of view, the "output" is

> the point where the signal leaves the block, to be

> eventually consumed by a sink. The generator doesn't

> know what the sinks are made of:

> 

> +-----------+   +----------> Sink1

> |           |   |

> |           +---+ ClkOut1

> |           |   |

> |  ClkGen   |   +----------> Sink2

> |           |

> |           +--> ClkOut2

> |           |

> +-----------+

> 

> From a device point of view, the "input" is where

> the external signal (generated by a source) is

> connected. The device doesn't know what is the

> source made of:

> 

>                      +-----------+

>                      |           |

>                      |           |

> (Source)  ClkIn ----->  Device   |

>                      |           |

>                      |           |

>                      +-----------+

> 

> So input/output refer to interface to connect the

> clock signal.

> 

> Source/sink refer to the other object having a

> relation with this signal.


Well, we don't need source/sink complication for QEMU,
restricting to input/output should be sufficient and
simple enough.

> 

>>

>>>

>>> Damien/Peter/Luc, do you have better description suggestions?

>>>

>>>>

>>>>>

>>>>>

>>>>>>>>>   * @running: #true if CPU is currently running (lockless).

>>>>>>>>>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;

>>>>>>>>>   * valid under cpu_list_lock.

>>>>>>>>> @@ -400,6 +404,7 @@ struct CPUState {

>>>>>>>>>      int num_ases;

>>>>>>>>>      AddressSpace *as;

>>>>>>>>>      MemoryRegion *memory;

>>>>>>>>> +    Clock *clock;

>>>>>>>>>  

>>>>>>>>>      void *env_ptr; /* CPUArchState */

>>>>>>>>>      IcountDecr *icount_decr_ptr;

>>>>>>>>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c

>>>>>>>>> index c55c09f734c..37fcff3ec64 100644

>>>>>>>>> --- a/hw/core/cpu.c

>>>>>>>>> +++ b/hw/core/cpu.c

>>>>>>>>> @@ -30,6 +30,7 @@

>>>>>>>>>  #include "qemu/qemu-print.h"

>>>>>>>>>  #include "sysemu/tcg.h"

>>>>>>>>>  #include "hw/boards.h"

>>>>>>>>> +#include "hw/qdev-clock.h"

>>>>>>>>>  #include "hw/qdev-properties.h"

>>>>>>>>>  #include "trace/trace-root.h"

>>>>>>>>>  #include "qemu/plugin.h"

>>>>>>>>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu)

>>>>>>>>>      trace_guest_cpu_reset(cpu);

>>>>>>>>>  }

>>>>>>>>>  

>>>>>>>>> +static void cpu_clk_update(void *opaque)

>>>>>>>>> +{

>>>>>>>>> +    CPUState *cpu = opaque;

>>>>>>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);

>>>>>>>>> +

>>>>>>>>> +    if (cc->clock_update) {

>>>>>>>>> +        cc->clock_update(cpu);

>>>>>>>>> +    }

>>>>>>>>> +}

>>>>>>>>> +

>>>>>>>>>  static void cpu_common_reset(DeviceState *dev)

>>>>>>>>>  {

>>>>>>>>>      CPUState *cpu = CPU(dev);

>>>>>>>>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj)

>>>>>>>>>      /* the default value is changed by qemu_init_vcpu() for softmmu */

>>>>>>>>>      cpu->nr_cores = 1;

>>>>>>>>>      cpu->nr_threads = 1;

>>>>>>>>> +    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);

>>>>>>>>>  

>>>>>>>>>      qemu_mutex_init(&cpu->work_mutex);

>>>>>>>>>      QSIMPLEQ_INIT(&cpu->work_list);  

>>>>>>>>   

>>>>>>>

>>>>>>

>>>>>

>>>>

>>>

>>

>
diff mbox series

Patch

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 6c34798c8b3..6989d90c193 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -31,6 +31,7 @@ 
 #include "qemu/thread.h"
 #include "qemu/plugin.h"
 #include "qom/object.h"
+#include "hw/clock.h"
 
 typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,
                                      void *opaque);
@@ -155,6 +156,7 @@  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.
+ * @clock_update: Callback for input clock changes
  *
  * Represents a CPU family or model.
  */
@@ -176,6 +178,7 @@  struct CPUClass {
                                   unsigned size, MMUAccessType access_type,
                                   int mmu_idx, MemTxAttrs attrs,
                                   MemTxResult response, uintptr_t retaddr);
+    void (*clock_update)(CPUState *cpu);
     bool (*virtio_is_big_endian)(CPUState *cpu);
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
@@ -316,6 +319,7 @@  struct qemu_work_item;
  *   QOM parent.
  * @nr_cores: Number of cores within this CPU package.
  * @nr_threads: Number of threads within this CPU.
+ * @clock: this CPU source clock (an output clock of another device)
  * @running: #true if CPU is currently running (lockless).
  * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
  * valid under cpu_list_lock.
@@ -400,6 +404,7 @@  struct CPUState {
     int num_ases;
     AddressSpace *as;
     MemoryRegion *memory;
+    Clock *clock;
 
     void *env_ptr; /* CPUArchState */
     IcountDecr *icount_decr_ptr;
diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index c55c09f734c..37fcff3ec64 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -30,6 +30,7 @@ 
 #include "qemu/qemu-print.h"
 #include "sysemu/tcg.h"
 #include "hw/boards.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "trace/trace-root.h"
 #include "qemu/plugin.h"
@@ -247,6 +248,16 @@  void cpu_reset(CPUState *cpu)
     trace_guest_cpu_reset(cpu);
 }
 
+static void cpu_clk_update(void *opaque)
+{
+    CPUState *cpu = opaque;
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->clock_update) {
+        cc->clock_update(cpu);
+    }
+}
+
 static void cpu_common_reset(DeviceState *dev)
 {
     CPUState *cpu = CPU(dev);
@@ -367,6 +378,7 @@  static void cpu_common_initfn(Object *obj)
     /* the default value is changed by qemu_init_vcpu() for softmmu */
     cpu->nr_cores = 1;
     cpu->nr_threads = 1;
+    cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, cpu);
 
     qemu_mutex_init(&cpu->work_mutex);
     QSIMPLEQ_INIT(&cpu->work_list);