[PATCH 00/16] hw/mips: Set CPU frequency

[PATCH 00/16] hw/mips: Set CPU frequency

[Philippe Mathieu-Daudé]

All the MIPS cores emulated by QEMU provides the Coproc#0
'Count' register which can be used as a free running timer.

Since it's introduction in 2005 this timer uses a fixed
frequency of 100 MHz (for a CPU freq of 200 MHz).
While this is not an issue with Linux guests, it makes
some firmwares behave incorrectly.

The Clock API allow propagating clocks. It is particularly
useful when hardware dynamicly changes clock frequencies.

To be able to model such MIPS hardware, we need to refactor
the MIPS hardware code to handle clocks.

This series is organized as follow:

- let all CPU have an input clock,
- MIPS CPU get an input clock
- when the clock is changed, CP0 timer is updated
- set correct CPU frequencies to all boards
- do not allow MIPS CPU without input clock

I used a MIPSsim test suggested by Thomas. It is also included
as bonus at the end.

Possible follow up:
- QOM'ify the GIC
- let the GIC handle dynamic clock changes

Regards,

Phil.

Philippe Mathieu-Daudé (16):
  hw/core/cpu: Let CPU object have a clock source
  target/mips: Move cpu_mips_get_random() with CP0 helpers
  target/mips/cp0_timer: Explicit unit in variable name
  target/mips/cpu: Introduce mips_cpu_properties[]
  target/mips/cpu: Set default CPU frequency to 200 MHz
  target/mips: Keep CP0 counter in sync with the CPU frequency
  hw/mips/r4k: Explicit CPU frequency is 200 MHz
  hw/mips/fuloong2e: Set CPU frequency to 533 MHz
  hw/mips/mipssim: Correct CPU frequency
  hw/mips/jazz: Correct CPU frequencies
  hw/mips/cps: Expose input clock and connect it to CPU cores
  hw/mips/boston: Set CPU frequency to 1 GHz
  hw/mips/malta: Set CPU frequency to 320 MHz
  hw/mips/cps: Do not allow use without input clock
  target/mips/cpu: Do not allow system-mode use without input clock
  tests/acceptance: Test the MIPSsim machine

 include/hw/core/cpu.h                    |  5 +++
 include/hw/mips/cps.h                    |  2 +
 target/mips/cpu.h                        |  9 ++++
 target/mips/internal.h                   |  2 +-
 hw/core/cpu.c                            | 12 +++++
 hw/mips/boston.c                         | 13 ++++++
 hw/mips/cps.c                            |  8 ++++
 hw/mips/fuloong2e.c                      |  8 +++-
 hw/mips/jazz.c                           | 16 ++++++-
 hw/mips/malta.c                          | 20 +++++++--
 hw/mips/mipssim.c                        | 12 ++++-
 hw/mips/r4k.c                            |  8 +++-
 target/mips/cp0_helper.c                 | 25 +++++++++++
 target/mips/cp0_timer.c                  | 51 ++++++---------------
 target/mips/cpu.c                        | 43 +++++++++++++++++-
 MAINTAINERS                              |  1 +
 tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
 17 files changed, 244 insertions(+), 47 deletions(-)
 create mode 100644 tests/acceptance/machine_mips_mipssim.py

-- 
2.26.2

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

[Philippe Mathieu-Daudé]

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(+)

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);
-- 
2.26.2

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

[Igor Mammedov]

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);

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

[Philippe Mathieu-Daudé]

+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);
> 

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

[Philippe Mathieu-Daudé]

+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);
> 

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

[Igor Mammedov]

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);  
> >   
> 

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

[Igor Mammedov]

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);  
> >   
> 

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

[Philippe Mathieu-Daudé]

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);  
>>>   
>>
> 

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

[Philippe Mathieu-Daudé]

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);  
>>>   
>>
> 

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

[Eduardo Habkost]

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

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

[Eduardo Habkost]

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

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

[Philippe Mathieu-Daudé]

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);  
>>>>   
>>>
>>
> 

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

[Philippe Mathieu-Daudé]

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);  
>>>>   
>>>
>>
> 

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

[Philippe Mathieu-Daudé]

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);  
>>>>>   
>>>>
>>>
>>
> 

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

[Philippe Mathieu-Daudé]

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);  
>>>>>   
>>>>
>>>
>>
> 

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

[Eduardo Habkost]

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

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

[Eduardo Habkost]

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

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

[Philippe Mathieu-Daudé]

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);  
>>>>>>>   
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

[Philippe Mathieu-Daudé]

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);  
>>>>>>>   
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

[Philippe Mathieu-Daudé]

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);  
>>>>>>>>   
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

[Philippe Mathieu-Daudé]

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);  
>>>>>>>>   
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

[Eduardo Habkost]

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

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

[Eduardo Habkost]

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

[PATCH 02/16] target/mips: Move cpu_mips_get_random() with CP0 helpers

[Philippe Mathieu-Daudé]

The get_random() helper uses the CP0_Wired register, which is
unrelated to the CP0_Count register use as timer.
Commit e16fe40c872 ("Move the MIPS CPU timer in a separate file")
incorrectly moved this get_random() helper with timer specific
code. Move it back to generic CP0 helpers.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/internal.h   |  2 +-
 target/mips/cp0_helper.c | 25 +++++++++++++++++++++++++
 target/mips/cp0_timer.c  | 25 -------------------------
 3 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/target/mips/internal.h b/target/mips/internal.h
index 7f159a9230c..087cabaa6d4 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -144,6 +144,7 @@ void r4k_helper_tlbr(CPUMIPSState *env);
 void r4k_helper_tlbinv(CPUMIPSState *env);
 void r4k_helper_tlbinvf(CPUMIPSState *env);
 void r4k_invalidate_tlb(CPUMIPSState *env, int idx, int use_extra);
+uint32_t cpu_mips_get_random(CPUMIPSState *env);
 
 void mips_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
                                     vaddr addr, unsigned size,
@@ -209,7 +210,6 @@ void cpu_state_reset(CPUMIPSState *s);
 void cpu_mips_realize_env(CPUMIPSState *env);
 
 /* cp0_timer.c */
-uint32_t cpu_mips_get_random(CPUMIPSState *env);
 uint32_t cpu_mips_get_count(CPUMIPSState *env);
 void cpu_mips_store_count(CPUMIPSState *env, uint32_t value);
 void cpu_mips_store_compare(CPUMIPSState *env, uint32_t value);
diff --git a/target/mips/cp0_helper.c b/target/mips/cp0_helper.c
index de64add038b..12143ac55b9 100644
--- a/target/mips/cp0_helper.c
+++ b/target/mips/cp0_helper.c
@@ -203,6 +203,31 @@ static void sync_c0_entryhi(CPUMIPSState *cpu, int tc)
     *tcst |= asid;
 }
 
+/* XXX: do not use a global */
+uint32_t cpu_mips_get_random(CPUMIPSState *env)
+{
+    static uint32_t seed = 1;
+    static uint32_t prev_idx;
+    uint32_t idx;
+    uint32_t nb_rand_tlb = env->tlb->nb_tlb - env->CP0_Wired;
+
+    if (nb_rand_tlb == 1) {
+        return env->tlb->nb_tlb - 1;
+    }
+
+    /* Don't return same value twice, so get another value */
+    do {
+        /*
+         * Use a simple algorithm of Linear Congruential Generator
+         * from ISO/IEC 9899 standard.
+         */
+        seed = 1103515245 * seed + 12345;
+        idx = (seed >> 16) % nb_rand_tlb + env->CP0_Wired;
+    } while (idx == prev_idx);
+    prev_idx = idx;
+    return idx;
+}
+
 /* CP0 helpers */
 target_ulong helper_mfc0_mvpcontrol(CPUMIPSState *env)
 {
diff --git a/target/mips/cp0_timer.c b/target/mips/cp0_timer.c
index bd7efb152dd..9c38e9da1c8 100644
--- a/target/mips/cp0_timer.c
+++ b/target/mips/cp0_timer.c
@@ -29,31 +29,6 @@
 
 #define TIMER_PERIOD 10 /* 10 ns period for 100 Mhz frequency */
 
-/* XXX: do not use a global */
-uint32_t cpu_mips_get_random(CPUMIPSState *env)
-{
-    static uint32_t seed = 1;
-    static uint32_t prev_idx = 0;
-    uint32_t idx;
-    uint32_t nb_rand_tlb = env->tlb->nb_tlb - env->CP0_Wired;
-
-    if (nb_rand_tlb == 1) {
-        return env->tlb->nb_tlb - 1;
-    }
-
-    /* Don't return same value twice, so get another value */
-    do {
-        /*
-         * Use a simple algorithm of Linear Congruential Generator
-         * from ISO/IEC 9899 standard.
-         */
-        seed = 1103515245 * seed + 12345;
-        idx = (seed >> 16) % nb_rand_tlb + env->CP0_Wired;
-    } while (idx == prev_idx);
-    prev_idx = idx;
-    return idx;
-}
-
 /* MIPS R4K timer */
 static void cpu_mips_timer_update(CPUMIPSState *env)
 {
-- 
2.26.2

Re: [PATCH 02/16] target/mips: Move cpu_mips_get_random() with CP0 helpers

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 4003 bytes --]

On Monday, September 28, 2020, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> The get_random() helper uses the CP0_Wired register, which is
> unrelated to the CP0_Count register use as timer.
> Commit e16fe40c872 ("Move the MIPS CPU timer in a separate file")
> incorrectly moved this get_random() helper with timer specific
> code. Move it back to generic CP0 helpers.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---


Reviewed-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>


>  target/mips/internal.h   |  2 +-
>  target/mips/cp0_helper.c | 25 +++++++++++++++++++++++++
>  target/mips/cp0_timer.c  | 25 -------------------------
>  3 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/target/mips/internal.h b/target/mips/internal.h
> index 7f159a9230c..087cabaa6d4 100644
> --- a/target/mips/internal.h
> +++ b/target/mips/internal.h
> @@ -144,6 +144,7 @@ void r4k_helper_tlbr(CPUMIPSState *env);
>  void r4k_helper_tlbinv(CPUMIPSState *env);
>  void r4k_helper_tlbinvf(CPUMIPSState *env);
>  void r4k_invalidate_tlb(CPUMIPSState *env, int idx, int use_extra);
> +uint32_t cpu_mips_get_random(CPUMIPSState *env);
>
>  void mips_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>                                      vaddr addr, unsigned size,
> @@ -209,7 +210,6 @@ void cpu_state_reset(CPUMIPSState *s);
>  void cpu_mips_realize_env(CPUMIPSState *env);
>
>  /* cp0_timer.c */
> -uint32_t cpu_mips_get_random(CPUMIPSState *env);
>  uint32_t cpu_mips_get_count(CPUMIPSState *env);
>  void cpu_mips_store_count(CPUMIPSState *env, uint32_t value);
>  void cpu_mips_store_compare(CPUMIPSState *env, uint32_t value);
> diff --git a/target/mips/cp0_helper.c b/target/mips/cp0_helper.c
> index de64add038b..12143ac55b9 100644
> --- a/target/mips/cp0_helper.c
> +++ b/target/mips/cp0_helper.c
> @@ -203,6 +203,31 @@ static void sync_c0_entryhi(CPUMIPSState *cpu, int tc)
>      *tcst |= asid;
>  }
>
> +/* XXX: do not use a global */
> +uint32_t cpu_mips_get_random(CPUMIPSState *env)
> +{
> +    static uint32_t seed = 1;
> +    static uint32_t prev_idx;
> +    uint32_t idx;
> +    uint32_t nb_rand_tlb = env->tlb->nb_tlb - env->CP0_Wired;
> +
> +    if (nb_rand_tlb == 1) {
> +        return env->tlb->nb_tlb - 1;
> +    }
> +
> +    /* Don't return same value twice, so get another value */
> +    do {
> +        /*
> +         * Use a simple algorithm of Linear Congruential Generator
> +         * from ISO/IEC 9899 standard.
> +         */
> +        seed = 1103515245 * seed + 12345;
> +        idx = (seed >> 16) % nb_rand_tlb + env->CP0_Wired;
> +    } while (idx == prev_idx);
> +    prev_idx = idx;
> +    return idx;
> +}
> +
>  /* CP0 helpers */
>  target_ulong helper_mfc0_mvpcontrol(CPUMIPSState *env)
>  {
> diff --git a/target/mips/cp0_timer.c b/target/mips/cp0_timer.c
> index bd7efb152dd..9c38e9da1c8 100644
> --- a/target/mips/cp0_timer.c
> +++ b/target/mips/cp0_timer.c
> @@ -29,31 +29,6 @@
>
>  #define TIMER_PERIOD 10 /* 10 ns period for 100 Mhz frequency */
>
> -/* XXX: do not use a global */
> -uint32_t cpu_mips_get_random(CPUMIPSState *env)
> -{
> -    static uint32_t seed = 1;
> -    static uint32_t prev_idx = 0;
> -    uint32_t idx;
> -    uint32_t nb_rand_tlb = env->tlb->nb_tlb - env->CP0_Wired;
> -
> -    if (nb_rand_tlb == 1) {
> -        return env->tlb->nb_tlb - 1;
> -    }
> -
> -    /* Don't return same value twice, so get another value */
> -    do {
> -        /*
> -         * Use a simple algorithm of Linear Congruential Generator
> -         * from ISO/IEC 9899 standard.
> -         */
> -        seed = 1103515245 * seed + 12345;
> -        idx = (seed >> 16) % nb_rand_tlb + env->CP0_Wired;
> -    } while (idx == prev_idx);
> -    prev_idx = idx;
> -    return idx;
> -}
> -
>  /* MIPS R4K timer */
>  static void cpu_mips_timer_update(CPUMIPSState *env)
>  {
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 5171 bytes --]

[PATCH 03/16] target/mips/cp0_timer: Explicit unit in variable name

[Philippe Mathieu-Daudé]

Name variables holding nanoseconds with the '_ns' suffix.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/cp0_timer.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/target/mips/cp0_timer.c b/target/mips/cp0_timer.c
index 9c38e9da1c8..5194c967ae3 100644
--- a/target/mips/cp0_timer.c
+++ b/target/mips/cp0_timer.c
@@ -32,13 +32,14 @@
 /* MIPS R4K timer */
 static void cpu_mips_timer_update(CPUMIPSState *env)
 {
-    uint64_t now, next;
+    uint64_t now_ns, next_ns;
     uint32_t wait;
 
-    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    wait = env->CP0_Compare - env->CP0_Count - (uint32_t)(now / TIMER_PERIOD);
-    next = now + (uint64_t)wait * TIMER_PERIOD;
-    timer_mod(env->timer, next);
+    now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    wait = env->CP0_Compare - env->CP0_Count -
+           (uint32_t)(now_ns / TIMER_PERIOD);
+    next_ns = now_ns + (uint64_t)wait * TIMER_PERIOD;
+    timer_mod(env->timer, next_ns);
 }
 
 /* Expire the timer.  */
@@ -56,16 +57,16 @@ uint32_t cpu_mips_get_count(CPUMIPSState *env)
     if (env->CP0_Cause & (1 << CP0Ca_DC)) {
         return env->CP0_Count;
     } else {
-        uint64_t now;
+        uint64_t now_ns;
 
-        now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         if (timer_pending(env->timer)
-            && timer_expired(env->timer, now)) {
+            && timer_expired(env->timer, now_ns)) {
             /* The timer has already expired.  */
             cpu_mips_timer_expire(env);
         }
 
-        return env->CP0_Count + (uint32_t)(now / TIMER_PERIOD);
+        return env->CP0_Count + (uint32_t)(now_ns / TIMER_PERIOD);
     }
 }
 
-- 
2.26.2

Re: [PATCH 03/16] target/mips/cp0_timer: Explicit unit in variable name

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

On Monday, September 28, 2020, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Name variables holding nanoseconds with the '_ns' suffix.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---


Reviewed-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>


>  target/mips/cp0_timer.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/target/mips/cp0_timer.c b/target/mips/cp0_timer.c
> index 9c38e9da1c8..5194c967ae3 100644
> --- a/target/mips/cp0_timer.c
> +++ b/target/mips/cp0_timer.c
> @@ -32,13 +32,14 @@
>  /* MIPS R4K timer */
>  static void cpu_mips_timer_update(CPUMIPSState *env)
>  {
> -    uint64_t now, next;
> +    uint64_t now_ns, next_ns;
>      uint32_t wait;
>
> -    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -    wait = env->CP0_Compare - env->CP0_Count - (uint32_t)(now /
> TIMER_PERIOD);
> -    next = now + (uint64_t)wait * TIMER_PERIOD;
> -    timer_mod(env->timer, next);
> +    now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    wait = env->CP0_Compare - env->CP0_Count -
> +           (uint32_t)(now_ns / TIMER_PERIOD);
> +    next_ns = now_ns + (uint64_t)wait * TIMER_PERIOD;
> +    timer_mod(env->timer, next_ns);
>  }
>
>  /* Expire the timer.  */
> @@ -56,16 +57,16 @@ uint32_t cpu_mips_get_count(CPUMIPSState *env)
>      if (env->CP0_Cause & (1 << CP0Ca_DC)) {
>          return env->CP0_Count;
>      } else {
> -        uint64_t now;
> +        uint64_t now_ns;
>
> -        now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>          if (timer_pending(env->timer)
> -            && timer_expired(env->timer, now)) {
> +            && timer_expired(env->timer, now_ns)) {
>              /* The timer has already expired.  */
>              cpu_mips_timer_expire(env);
>          }
>
> -        return env->CP0_Count + (uint32_t)(now / TIMER_PERIOD);
> +        return env->CP0_Count + (uint32_t)(now_ns / TIMER_PERIOD);
>      }
>  }
>
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 3077 bytes --]

[PATCH 04/16] target/mips/cpu: Introduce mips_cpu_properties[]

[Philippe Mathieu-Daudé]

To allow objects creating CPU objects to set non-default properties
before qdev-realizing() it, add an empty mips_cpu_properties array
in MIPSCPUClass.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/cpu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index e86cd065483..c1c0f2e12ba 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -26,7 +26,7 @@
 #include "qemu/module.h"
 #include "sysemu/kvm.h"
 #include "exec/exec-all.h"
-
+#include "hw/qdev-properties.h"
 
 static void mips_cpu_set_pc(CPUState *cs, vaddr value)
 {
@@ -181,6 +181,10 @@ static ObjectClass *mips_cpu_class_by_name(const char *cpu_model)
     return oc;
 }
 
+static Property mips_cpu_properties[] = {
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void mips_cpu_class_init(ObjectClass *c, void *data)
 {
     MIPSCPUClass *mcc = MIPS_CPU_CLASS(c);
@@ -190,6 +194,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
     device_class_set_parent_realize(dc, mips_cpu_realizefn,
                                     &mcc->parent_realize);
     device_class_set_parent_reset(dc, mips_cpu_reset, &mcc->parent_reset);
+    device_class_set_props(dc, mips_cpu_properties);
 
     cc->class_by_name = mips_cpu_class_by_name;
     cc->has_work = mips_cpu_has_work;
-- 
2.26.2

[PATCH 05/16] target/mips/cpu: Set default CPU frequency to 200 MHz

[Philippe Mathieu-Daudé]

Since the introduction of the 'r4k' machine in commit 6af0bf9c7c3,
the MIPS target assumes a CPU running at 200 MHz.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/cpu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index c1c0f2e12ba..8d07a293a2e 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -141,6 +141,14 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
+    if (!clock_get(cs->clock)) {
+        /*
+         * Initialize the frequency to 200MHz in case
+         * the clock remains unconnected.
+         */
+        clock_set_hz(cs->clock, 200000000);
+    }
+
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
-- 
2.26.2

[PATCH 06/16] target/mips: Keep CP0 counter in sync with the CPU frequency

[Philippe Mathieu-Daudé]

Since commit 6af0bf9c7c3 the CP0 counter is running at half the
frequency of a 200 MHz CPU: a static 100 MHz value is used.

By using the Clock API we can change the CPU frequency at runtime,
so the CP0 counter might run out of sync with the CPU clock.
Avoid that by using the recently introduced CPUClass::clock_update()
callback. If the CPU clock is dynamically changed, our CP0 counter
will be updated correspondingly.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/cpu.h       |  9 +++++++++
 target/mips/cp0_timer.c | 13 ++++++-------
 target/mips/cpu.c       | 20 ++++++++++++++++++++
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 7cf7f5239f7..66d6124ae02 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1145,6 +1145,7 @@ struct CPUMIPSState {
     struct MIPSITUState *itu;
     MemoryRegion *itc_tag; /* ITC Configuration Tags */
     target_ulong exception_base; /* ExceptionBase input to the core */
+    unsigned cp0_count_ns; /* CP0_Count clock period (in nanoseconds) */
 };
 
 /**
@@ -1160,6 +1161,14 @@ struct MIPSCPU {
 
     CPUNegativeOffsetState neg;
     CPUMIPSState env;
+    /*
+     * The Count register acts as a timer, incrementing at a constant rate,
+     * whether or not an instruction is executed, retired, or any forward
+     * progress is made through the pipeline. The rate at which the counter
+     * increments is implementation dependent, and is a function of the
+     * pipeline clock of the processor, not the issue width of the processor.
+     */
+    unsigned cp0_count_rate;
 };
 
 
diff --git a/target/mips/cp0_timer.c b/target/mips/cp0_timer.c
index 5194c967ae3..5ec0d6249e9 100644
--- a/target/mips/cp0_timer.c
+++ b/target/mips/cp0_timer.c
@@ -27,8 +27,6 @@
 #include "sysemu/kvm.h"
 #include "internal.h"
 
-#define TIMER_PERIOD 10 /* 10 ns period for 100 Mhz frequency */
-
 /* MIPS R4K timer */
 static void cpu_mips_timer_update(CPUMIPSState *env)
 {
@@ -37,8 +35,8 @@ static void cpu_mips_timer_update(CPUMIPSState *env)
 
     now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     wait = env->CP0_Compare - env->CP0_Count -
-           (uint32_t)(now_ns / TIMER_PERIOD);
-    next_ns = now_ns + (uint64_t)wait * TIMER_PERIOD;
+           (uint32_t)(now_ns / env->cp0_count_ns);
+    next_ns = now_ns + (uint64_t)wait * env->cp0_count_ns;
     timer_mod(env->timer, next_ns);
 }
 
@@ -66,7 +64,7 @@ uint32_t cpu_mips_get_count(CPUMIPSState *env)
             cpu_mips_timer_expire(env);
         }
 
-        return env->CP0_Count + (uint32_t)(now_ns / TIMER_PERIOD);
+        return env->CP0_Count + (uint32_t)(now_ns / env->cp0_count_ns);
     }
 }
 
@@ -82,7 +80,8 @@ void cpu_mips_store_count(CPUMIPSState *env, uint32_t count)
     } else {
         /* Store new count register */
         env->CP0_Count = count -
-               (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / TIMER_PERIOD);
+               (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) /
+                          env->cp0_count_ns);
         /* Update timer timer */
         cpu_mips_timer_update(env);
     }
@@ -109,7 +108,7 @@ void cpu_mips_stop_count(CPUMIPSState *env)
 {
     /* Store the current value */
     env->CP0_Count += (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) /
-                                 TIMER_PERIOD);
+                                 env->cp0_count_ns);
 }
 
 static void mips_timer_cb(void *opaque)
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 8d07a293a2e..2f75216c324 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -26,6 +26,7 @@
 #include "qemu/module.h"
 #include "sysemu/kvm.h"
 #include "exec/exec-all.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 
 static void mips_cpu_set_pc(CPUState *cs, vaddr value)
@@ -134,6 +135,22 @@ static void mips_cpu_disas_set_info(CPUState *s, disassemble_info *info)
     }
 }
 
+static void mips_cp0_period_set(MIPSCPU *cpu)
+{
+    CPUMIPSState *env = &cpu->env;
+
+    /* Recompute CP0's period on clock change */
+    env->cp0_count_ns = cpu->cp0_count_rate * clock_get_ns(CPU(cpu)->clock);
+}
+
+static void mips_cpu_clk_update(CPUState *cs)
+{
+    MIPSCPU *cpu = MIPS_CPU(cs);
+
+    /* Recompute CP0's period on clock change */
+    mips_cp0_period_set(cpu);
+}
+
 static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
@@ -148,6 +165,7 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
          */
         clock_set_hz(cs->clock, 200000000);
     }
+    mips_cpu_clk_update(cs);
 
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
@@ -190,6 +208,7 @@ static ObjectClass *mips_cpu_class_by_name(const char *cpu_model)
 }
 
 static Property mips_cpu_properties[] = {
+    DEFINE_PROP_UINT32("CP0_Count-rate", MIPSCPU, cp0_count_rate, 2),
     DEFINE_PROP_END_OF_LIST()
 };
 
@@ -224,6 +243,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
     cc->tcg_initialize = mips_tcg_init;
     cc->tlb_fill = mips_cpu_tlb_fill;
 #endif
+    cc->clock_update = mips_cpu_clk_update;
 
     cc->gdb_num_core_regs = 73;
     cc->gdb_stop_before_watchpoint = true;
-- 
2.26.2

[PATCH 07/16] hw/mips/r4k: Explicit CPU frequency is 200 MHz

[Philippe Mathieu-Daudé]

Since its introduction in commit 6af0bf9c7c3,
the 'r4k' machine runs at 200 MHz.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/r4k.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/mips/r4k.c b/hw/mips/r4k.c
index 3487013a4a1..2aa18203f20 100644
--- a/hw/mips/r4k.c
+++ b/hw/mips/r4k.c
@@ -37,6 +37,7 @@
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
 #include "qemu/error-report.h"
+#include "hw/qdev-clock.h"
 
 #define MAX_IDE_BUS 2
 
@@ -184,6 +185,7 @@ void mips_r4k_init(MachineState *machine)
     int bios_size;
     MIPSCPU *cpu;
     CPUMIPSState *env;
+    Clock *cpuclk;
     ResetData *reset_info;
     int i;
     qemu_irq *i8259;
@@ -193,7 +195,11 @@ void mips_r4k_init(MachineState *machine)
     int be;
 
     /* init CPUs */
-    cpu = MIPS_CPU(cpu_create(machine->cpu_type));
+    cpu = MIPS_CPU(object_new(machine->cpu_type));
+    cpuclk = qdev_init_clock_out(DEVICE(cpu), "cpuclk");
+    clock_set_hz(cpuclk, 200000000); /* 200 MHz */
+    qdev_connect_clock_in(DEVICE(cpu), "clk", cpuclk);
+    qdev_realize(DEVICE(cpu), NULL, &error_abort);
     env = &cpu->env;
 
     reset_info = g_malloc0(sizeof(ResetData));
-- 
2.26.2

[PATCH 08/16] hw/mips/fuloong2e: Set CPU frequency to 533 MHz

[Philippe Mathieu-Daudé]

The CPU frequency is normally provided by the firmware in the
"cpuclock" environment variable. The 2E board can handles up
to 660MHz, but be conservative and take the same value used
by the Linux kernel: 533 MHz.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/fuloong2e.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index f28609976bf..1a925dcc201 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -28,6 +28,7 @@
 #include "hw/isa/superio.h"
 #include "net/net.h"
 #include "hw/boards.h"
+#include "hw/qdev-clock.h"
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/block/flash.h"
 #include "hw/mips/mips.h"
@@ -300,10 +301,15 @@ static void mips_fuloong2e_init(MachineState *machine)
     I2CBus *smbus;
     MIPSCPU *cpu;
     CPUMIPSState *env;
+    Clock *cpuclk;
     DeviceState *dev;
 
     /* init CPUs */
-    cpu = MIPS_CPU(cpu_create(machine->cpu_type));
+    cpu = MIPS_CPU(object_new(machine->cpu_type));
+    cpuclk = qdev_init_clock_out(DEVICE(cpu), "cpuclk");
+    clock_set_hz(cpuclk, 533080000); /* ~533 MHz */
+    qdev_connect_clock_in(DEVICE(cpu), "clk", cpuclk);
+    qdev_realize(DEVICE(cpu), NULL, &error_abort);
     env = &cpu->env;
 
     qemu_register_reset(main_cpu_reset, cpu);
-- 
2.26.2

[PATCH 09/16] hw/mips/mipssim: Correct CPU frequency

[Philippe Mathieu-Daudé]

The MIPSsim machine CPU frequency is too fast running at 200 MHz,
while it should be 12 MHz for the 24K and 6 MHz for the 5K core.

Ref: Linux commit c78cbf49c4ed
("Support for MIPSsim, the cycle accurate MIPS simulator.")

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/mipssim.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/mips/mipssim.c b/hw/mips/mipssim.c
index 1b3b7622035..6660af19c18 100644
--- a/hw/mips/mipssim.c
+++ b/hw/mips/mipssim.c
@@ -40,6 +40,7 @@
 #include "hw/loader.h"
 #include "elf.h"
 #include "hw/sysbus.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
@@ -152,11 +153,20 @@ mips_mipssim_init(MachineState *machine)
     MemoryRegion *bios = g_new(MemoryRegion, 1);
     MIPSCPU *cpu;
     CPUMIPSState *env;
+    Clock *cpuclk;
     ResetData *reset_info;
     int bios_size;
 
     /* Init CPUs. */
-    cpu = MIPS_CPU(cpu_create(machine->cpu_type));
+    cpu = MIPS_CPU(object_new(machine->cpu_type));
+    cpuclk = qdev_init_clock_out(DEVICE(cpu), "cpuclk");
+#ifdef TARGET_MIPS64
+    clock_set_hz(cpuclk, 6000000); /* 6 MHz */
+#else
+    clock_set_hz(cpuclk, 12000000); /* 12 MHz */
+#endif
+    qdev_connect_clock_in(DEVICE(cpu), "clk", cpuclk);
+    qdev_realize(DEVICE(cpu), NULL, &error_abort);
     env = &cpu->env;
 
     reset_info = g_malloc0(sizeof(ResetData));
-- 
2.26.2

[PATCH 10/16] hw/mips/jazz: Correct CPU frequencies

[Philippe Mathieu-Daudé]

The Magnum 4000PC CPU runs at 100 MHz, and the Acer PICA-61
CPU at ~134 MHz.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/jazz.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 47723093b63..eee79d5b47d 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -45,6 +45,7 @@
 #include "hw/audio/pcspk.h"
 #include "hw/input/i8042.h"
 #include "hw/sysbus.h"
+#include "hw/qdev-clock.h"
 #include "exec/address-spaces.h"
 #include "sysemu/qtest.h"
 #include "sysemu/reset.h"
@@ -145,6 +146,7 @@ static void mips_jazz_init(MachineState *machine,
     MIPSCPU *cpu;
     CPUClass *cc;
     CPUMIPSState *env;
+    Clock *cpuclk;
     qemu_irq *i8259;
     rc4030_dma *dmas;
     IOMMUMemoryRegion *rc4030_dma_mr;
@@ -163,6 +165,13 @@ static void mips_jazz_init(MachineState *machine,
     MemoryRegion *bios2 = g_new(MemoryRegion, 1);
     SysBusESPState *sysbus_esp;
     ESPState *esp;
+    static const struct {
+        unsigned freq_hz;
+        unsigned pll_mult;
+    } ext_clk[] = {
+        [JAZZ_MAGNUM] = {50000000, 2},
+        [JAZZ_PICA61] = {33333333, 4},
+    };
 
     if (machine->ram_size > 256 * MiB) {
         error_report("RAM size more than 256Mb is not supported");
@@ -170,7 +179,12 @@ static void mips_jazz_init(MachineState *machine,
     }
 
     /* init CPUs */
-    cpu = MIPS_CPU(cpu_create(machine->cpu_type));
+    cpu = MIPS_CPU(object_new(machine->cpu_type));
+    cpuclk = qdev_init_clock_out(DEVICE(cpu), "cpuclk");
+    clock_set_hz(cpuclk,
+                 ext_clk[jazz_model].freq_hz * ext_clk[jazz_model].pll_mult);
+    qdev_connect_clock_in(DEVICE(cpu), "clk", cpuclk);
+    qdev_realize(DEVICE(cpu), NULL, &error_abort);
     env = &cpu->env;
     qemu_register_reset(main_cpu_reset, cpu);
 
-- 
2.26.2

[PATCH 11/16] hw/mips/cps: Expose input clock and connect it to CPU cores

[Philippe Mathieu-Daudé]

Expose a qdev input clock named 'clk', and connect it to each
core.

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

diff --git a/include/hw/mips/cps.h b/include/hw/mips/cps.h
index 9e35a881366..859a8d4a674 100644
--- a/include/hw/mips/cps.h
+++ b/include/hw/mips/cps.h
@@ -21,6 +21,7 @@
 #define MIPS_CPS_H
 
 #include "hw/sysbus.h"
+#include "hw/clock.h"
 #include "hw/misc/mips_cmgcr.h"
 #include "hw/intc/mips_gic.h"
 #include "hw/misc/mips_cpc.h"
@@ -43,6 +44,7 @@ struct MIPSCPSState {
     MIPSGICState gic;
     MIPSCPCState cpc;
     MIPSITUState itu;
+    Clock *clock;
 };
 
 qemu_irq get_cps_irq(MIPSCPSState *cps, int pin_number);
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 23c0f87e41a..c332609f7b3 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -22,6 +22,7 @@
 #include "qemu/module.h"
 #include "hw/mips/cps.h"
 #include "hw/mips/mips.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "hw/mips/cpudevs.h"
 #include "sysemu/kvm.h"
@@ -38,6 +39,7 @@ static void mips_cps_init(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     MIPSCPSState *s = MIPS_CPS(obj);
 
+    s->clock = qdev_init_clock_in(DEVICE(obj), "clk", NULL, NULL);
     /*
      * Cover entire address space as there do not seem to be any
      * constraints for the base address of CPC and GIC.
@@ -80,6 +82,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
                                       errp)) {
             return;
         }
+        qdev_connect_clock_in(DEVICE(cpu), "clk", s->clock);
 
         if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) {
             return;
-- 
2.26.2

[PATCH 12/16] hw/mips/boston: Set CPU frequency to 1 GHz

[Philippe Mathieu-Daudé]

The I6400 can run at 1 GHz or more. Create a 'cpuclk'
output clock and connect it to the CPU input clock.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/boston.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 1b3f69e949c..5c541a0c5ea 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -30,6 +30,7 @@
 #include "hw/mips/cps.h"
 #include "hw/mips/cpudevs.h"
 #include "hw/pci-host/xilinx-pcie.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
@@ -54,6 +55,7 @@ struct BostonState {
     MachineState *mach;
     MIPSCPSState cps;
     SerialMM *uart;
+    Clock *cpuclk;
 
     CharBackend lcd_display;
     char lcd_content[8];
@@ -251,10 +253,19 @@ static const MemoryRegionOps boston_platreg_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static void mips_boston_instance_init(Object *obj)
+{
+    BostonState *s = BOSTON(obj);
+
+    s->cpuclk = qdev_init_clock_out(DEVICE(obj), "cpuclk");
+    clock_set_hz(s->cpuclk, 1000000000); /* 1 GHz */
+}
+
 static const TypeInfo boston_device = {
     .name          = TYPE_MIPS_BOSTON,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(BostonState),
+    .instance_init = mips_boston_instance_init,
 };
 
 static void boston_register_types(void)
@@ -462,6 +473,8 @@ static void boston_mach_init(MachineState *machine)
                             &error_fatal);
     object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus,
                             &error_fatal);
+    qdev_connect_clock_in(DEVICE(&s->cps), "clk",
+                          qdev_get_clock_out(dev, "cpuclk"));
     sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal);
 
     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
-- 
2.26.2

[PATCH 13/16] hw/mips/malta: Set CPU frequency to 320 MHz

[Philippe Mathieu-Daudé]

The CoreLV card with ID 0x420's CPU clocked at 320 MHz. Create
a 'cpuclk' output clock and connect it to the CPU input clock.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/malta.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 4019c9dc1a8..62ec543a610 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -57,6 +57,7 @@
 #include "sysemu/kvm.h"
 #include "hw/semihosting/semihost.h"
 #include "hw/mips/cps.h"
+#include "hw/qdev-clock.h"
 
 #define ENVP_ADDR           0x80002000l
 #define ENVP_NB_ENTRIES     16
@@ -94,6 +95,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(MaltaState, MIPS_MALTA)
 struct MaltaState {
     SysBusDevice parent_obj;
 
+    Clock *cpuclk;
     MIPSCPSState cps;
     qemu_irq i8259[ISA_NUM_IRQS];
 };
@@ -1159,7 +1161,7 @@ static void main_cpu_reset(void *opaque)
     }
 }
 
-static void create_cpu_without_cps(MachineState *ms,
+static void create_cpu_without_cps(MachineState *ms, MaltaState *s,
                                    qemu_irq *cbus_irq, qemu_irq *i8259_irq)
 {
     CPUMIPSState *env;
@@ -1167,7 +1169,9 @@ static void create_cpu_without_cps(MachineState *ms,
     int i;
 
     for (i = 0; i < ms->smp.cpus; i++) {
-        cpu = MIPS_CPU(cpu_create(ms->cpu_type));
+        cpu = MIPS_CPU(object_new(ms->cpu_type));
+        qdev_connect_clock_in(DEVICE(cpu), "clk", s->cpuclk);
+        qdev_realize(DEVICE(cpu), NULL, &error_abort);
 
         /* Init internal devices */
         cpu_mips_irq_init_cpu(cpu);
@@ -1189,6 +1193,7 @@ static void create_cps(MachineState *ms, MaltaState *s,
                             &error_fatal);
     object_property_set_int(OBJECT(&s->cps), "num-vp", ms->smp.cpus,
                             &error_fatal);
+    qdev_connect_clock_in(DEVICE(&s->cps), "clk", s->cpuclk);
     sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal);
 
     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
@@ -1203,7 +1208,7 @@ static void mips_create_cpu(MachineState *ms, MaltaState *s,
     if ((ms->smp.cpus > 1) && cpu_supports_cps_smp(ms->cpu_type)) {
         create_cps(ms, s, cbus_irq, i8259_irq);
     } else {
-        create_cpu_without_cps(ms, cbus_irq, i8259_irq);
+        create_cpu_without_cps(ms, s, cbus_irq, i8259_irq);
     }
 }
 
@@ -1421,10 +1426,19 @@ void mips_malta_init(MachineState *machine)
     pci_vga_init(pci_bus);
 }
 
+static void mips_malta_instance_init(Object *obj)
+{
+    MaltaState *s = MIPS_MALTA(obj);
+
+    s->cpuclk = qdev_init_clock_out(DEVICE(obj), "cpuclk");
+    clock_set_hz(s->cpuclk, 320000000); /* 320 MHz */
+}
+
 static const TypeInfo mips_malta_device = {
     .name          = TYPE_MIPS_MALTA,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(MaltaState),
+    .instance_init = mips_malta_instance_init,
 };
 
 static void mips_malta_machine_init(MachineClass *mc)
-- 
2.26.2

[PATCH 14/16] hw/mips/cps: Do not allow use without input clock

[Philippe Mathieu-Daudé]

Now than all QOM users provides the input clock, do not allow
using a CPS without input clock connected.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/cps.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index c332609f7b3..e4ff173f8c3 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -74,6 +74,11 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
     bool itu_present = false;
     bool saar_present = false;
 
+    if (!clock_get(s->clock)) {
+        error_setg(errp, "CPS clock must be connected to a clock source");
+        return;
+    }
+
     for (i = 0; i < s->num_vp; i++) {
         cpu = MIPS_CPU(object_new(s->cpu_type));
 
-- 
2.26.2

[PATCH 15/16] target/mips/cpu: Do not allow system-mode use without input clock

[Philippe Mathieu-Daudé]

Now than all QOM users provides the input clock, do not allow
using a CPU core without its input clock connected on system-mode
emulation. For user-mode, keep providing a fixed 200 MHz clock,
as it used by the RDHWR instruction (see commit cdfcad788394).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Igor Mammedov <imammedo@redhat.com>

We need the qtest check for tests/qtest/machine-none-test.c
which instanciate a CPU with the none machine. Igor, is it
better to remove the MIPS targets from the test cpus_map[]?
---
 target/mips/cpu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 2f75216c324..cc4ee75af30 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -25,6 +25,7 @@
 #include "kvm_mips.h"
 #include "qemu/module.h"
 #include "sysemu/kvm.h"
+#include "sysemu/qtest.h"
 #include "exec/exec-all.h"
 #include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
@@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
 
     if (!clock_get(cs->clock)) {
+#ifdef CONFIG_USER_ONLY
         /*
          * Initialize the frequency to 200MHz in case
          * the clock remains unconnected.
          */
         clock_set_hz(cs->clock, 200000000);
+#else
+        if (!qtest_enabled()) {
+            error_setg(errp, "CPU clock must be connected to a clock source");
+            return;
+        }
+#endif
     }
     mips_cpu_clk_update(cs);
 
-- 
2.26.2

Re: [PATCH 15/16] target/mips/cpu: Do not allow system-mode use without input clock

[Igor Mammedov]

On Mon, 28 Sep 2020 19:15:38 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Now than all QOM users provides the input clock, do not allow
> using a CPU core without its input clock connected on system-mode
> emulation. For user-mode, keep providing a fixed 200 MHz clock,
> as it used by the RDHWR instruction (see commit cdfcad788394).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Igor Mammedov <imammedo@redhat.com>
> 
> We need the qtest check for tests/qtest/machine-none-test.c
> which instanciate a CPU with the none machine. Igor, is it
> better to remove the MIPS targets from the test cpus_map[]?

I don't get idea, could you rephrase/elaborate?

> ---
>  target/mips/cpu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 2f75216c324..cc4ee75af30 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -25,6 +25,7 @@
>  #include "kvm_mips.h"
>  #include "qemu/module.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/qtest.h"
>  #include "exec/exec-all.h"
>  #include "hw/qdev-clock.h"
>  #include "hw/qdev-properties.h"
> @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>      Error *local_err = NULL;
>  
>      if (!clock_get(cs->clock)) {
> +#ifdef CONFIG_USER_ONLY
>          /*
>           * Initialize the frequency to 200MHz in case
>           * the clock remains unconnected.
>           */
>          clock_set_hz(cs->clock, 200000000);
> +#else
> +        if (!qtest_enabled()) {
> +            error_setg(errp, "CPU clock must be connected to a clock source");
> +            return;
> +        }
> +#endif
>      }
>      mips_cpu_clk_update(cs);
>  

Re: [PATCH 15/16] target/mips/cpu: Do not allow system-mode use without input clock

[Philippe Mathieu-Daudé]

On 9/29/20 3:01 PM, Igor Mammedov wrote:
> On Mon, 28 Sep 2020 19:15:38 +0200
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> Now than all QOM users provides the input clock, do not allow
>> using a CPU core without its input clock connected on system-mode
>> emulation. For user-mode, keep providing a fixed 200 MHz clock,
>> as it used by the RDHWR instruction (see commit cdfcad788394).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Cc: Igor Mammedov <imammedo@redhat.com>
>>
>> We need the qtest check for tests/qtest/machine-none-test.c
>> which instanciate a CPU with the none machine. Igor, is it
>> better to remove the MIPS targets from the test cpus_map[]?
> 
> I don't get idea, could you rephrase/elaborate?

cpu_class_init sets:

    /*
     * Reason: CPUs still need special care by board code: wiring up
     * IRQs, adding reset handlers, halting non-first CPUs, ...
     */
    dc->user_creatable = false;

but the CPUs are created via another path in vl.c:

        current_machine->cpu_type = parse_cpu_option(cpu_option);

The machine-none-test assumes CPU objects are user-creatable.

With this patch I enforce MIPS CPU to have an input clock
connected, so they are not user-creatable anymore.

I tried this code ...:

--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -229,7 +229,11 @@ static const TypeInfo mips_cpu_type_info = {
 static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data)
 {
     MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
     mcc->cpu_def = data;
+    /* Reason: clock need to be wired up */
+    dc->user_creatable = false;
 }

... but it is ignored, the CPU can still be created.

Not sure how to handle this.

> 
>> ---
>>  target/mips/cpu.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
>> index 2f75216c324..cc4ee75af30 100644
>> --- a/target/mips/cpu.c
>> +++ b/target/mips/cpu.c
>> @@ -25,6 +25,7 @@
>>  #include "kvm_mips.h"
>>  #include "qemu/module.h"
>>  #include "sysemu/kvm.h"
>> +#include "sysemu/qtest.h"
>>  #include "exec/exec-all.h"
>>  #include "hw/qdev-clock.h"
>>  #include "hw/qdev-properties.h"
>> @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>>      Error *local_err = NULL;
>>  
>>      if (!clock_get(cs->clock)) {
>> +#ifdef CONFIG_USER_ONLY
>>          /*
>>           * Initialize the frequency to 200MHz in case
>>           * the clock remains unconnected.
>>           */
>>          clock_set_hz(cs->clock, 200000000);
>> +#else
>> +        if (!qtest_enabled()) {
>> +            error_setg(errp, "CPU clock must be connected to a clock source");
>> +            return;
>> +        }
>> +#endif
>>      }
>>      mips_cpu_clk_update(cs);
>>  
> 

Re: [PATCH 15/16] target/mips/cpu: Do not allow system-mode use without input clock

[Philippe Mathieu-Daudé]

+Thomas for qtest

On 9/29/20 4:40 PM, Philippe Mathieu-Daudé wrote:
> On 9/29/20 3:01 PM, Igor Mammedov wrote:
>> On Mon, 28 Sep 2020 19:15:38 +0200
>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>>> Now than all QOM users provides the input clock, do not allow
>>> using a CPU core without its input clock connected on system-mode
>>> emulation. For user-mode, keep providing a fixed 200 MHz clock,
>>> as it used by the RDHWR instruction (see commit cdfcad788394).
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>
>>> We need the qtest check for tests/qtest/machine-none-test.c
>>> which instanciate a CPU with the none machine. Igor, is it
>>> better to remove the MIPS targets from the test cpus_map[]?
>>
>> I don't get idea, could you rephrase/elaborate?
> 
> cpu_class_init sets:
> 
>     /*
>      * Reason: CPUs still need special care by board code: wiring up
>      * IRQs, adding reset handlers, halting non-first CPUs, ...
>      */
>     dc->user_creatable = false;
> 
> but the CPUs are created via another path in vl.c:
> 
>         current_machine->cpu_type = parse_cpu_option(cpu_option);
> 
> The machine-none-test assumes CPU objects are user-creatable.
> 
> With this patch I enforce MIPS CPU to have an input clock
> connected, so they are not user-creatable anymore.
> 
> I tried this code ...:
> 
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -229,7 +229,11 @@ static const TypeInfo mips_cpu_type_info = {
>  static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>  {
>      MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
>      mcc->cpu_def = data;
> +    /* Reason: clock need to be wired up */
> +    dc->user_creatable = false;
>  }
> 
> ... but it is ignored, the CPU can still be created.
> 
> Not sure how to handle this.
> 
>>
>>> ---
>>>  target/mips/cpu.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
>>> index 2f75216c324..cc4ee75af30 100644
>>> --- a/target/mips/cpu.c
>>> +++ b/target/mips/cpu.c
>>> @@ -25,6 +25,7 @@
>>>  #include "kvm_mips.h"
>>>  #include "qemu/module.h"
>>>  #include "sysemu/kvm.h"
>>> +#include "sysemu/qtest.h"
>>>  #include "exec/exec-all.h"
>>>  #include "hw/qdev-clock.h"
>>>  #include "hw/qdev-properties.h"
>>> @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>>>      Error *local_err = NULL;
>>>  
>>>      if (!clock_get(cs->clock)) {
>>> +#ifdef CONFIG_USER_ONLY
>>>          /*
>>>           * Initialize the frequency to 200MHz in case
>>>           * the clock remains unconnected.
>>>           */
>>>          clock_set_hz(cs->clock, 200000000);
>>> +#else
>>> +        if (!qtest_enabled()) {
>>> +            error_setg(errp, "CPU clock must be connected to a clock source");
>>> +            return;
>>> +        }
>>> +#endif
>>>      }
>>>      mips_cpu_clk_update(cs);
>>>  
>>
> 

Re: [PATCH 15/16] target/mips/cpu: Do not allow system-mode use without input clock

[Igor Mammedov]

On Tue, 29 Sep 2020 16:40:24 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 9/29/20 3:01 PM, Igor Mammedov wrote:
> > On Mon, 28 Sep 2020 19:15:38 +0200
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> Now than all QOM users provides the input clock, do not allow
> >> using a CPU core without its input clock connected on system-mode
> >> emulation. For user-mode, keep providing a fixed 200 MHz clock,
> >> as it used by the RDHWR instruction (see commit cdfcad788394).
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >>
> >> We need the qtest check for tests/qtest/machine-none-test.c
> >> which instanciate a CPU with the none machine. Igor, is it
> >> better to remove the MIPS targets from the test cpus_map[]?
it will help to pass test but,

> > 
> > I don't get idea, could you rephrase/elaborate?  
> 
> cpu_class_init sets:
> 
>     /*
>      * Reason: CPUs still need special care by board code: wiring up
>      * IRQs, adding reset handlers, halting non-first CPUs, ...
>      */
>     dc->user_creatable = false;
> 
> but the CPUs are created via another path in vl.c:
> 
>         current_machine->cpu_type = parse_cpu_option(cpu_option);
> 
> The machine-none-test assumes CPU objects are user-creatable.

user_creatable -> able to create using -device, 
so test doesn't make this assumption.

problem is that enforcing would make machine-none not usable
for probing CPU features which is used by mgmt,
so that would break.

> 
> With this patch I enforce MIPS CPU to have an input clock
> connected, so they are not user-creatable anymore.
> 
> I tried this code ...:
> 
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -229,7 +229,11 @@ static const TypeInfo mips_cpu_type_info = {
>  static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>  {
>      MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
>      mcc->cpu_def = data;
> +    /* Reason: clock need to be wired up */
> +    dc->user_creatable = false;
>  }
> 
> ... but it is ignored, the CPU can still be created.
> 
> Not sure how to handle this.
> 
> >   
> >> ---
> >>  target/mips/cpu.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> >> index 2f75216c324..cc4ee75af30 100644
> >> --- a/target/mips/cpu.c
> >> +++ b/target/mips/cpu.c
> >> @@ -25,6 +25,7 @@
> >>  #include "kvm_mips.h"
> >>  #include "qemu/module.h"
> >>  #include "sysemu/kvm.h"
> >> +#include "sysemu/qtest.h"
> >>  #include "exec/exec-all.h"
> >>  #include "hw/qdev-clock.h"
> >>  #include "hw/qdev-properties.h"
> >> @@ -159,11 +160,18 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
> >>      Error *local_err = NULL;
> >>  
> >>      if (!clock_get(cs->clock)) {
> >> +#ifdef CONFIG_USER_ONLY
> >>          /*
> >>           * Initialize the frequency to 200MHz in case
> >>           * the clock remains unconnected.
> >>           */
> >>          clock_set_hz(cs->clock, 200000000);
> >> +#else
> >> +        if (!qtest_enabled()) {
> >> +            error_setg(errp, "CPU clock must be connected to a clock source");
> >> +            return;
> >> +        }
> >> +#endif
> >>      }
> >>      mips_cpu_clk_update(cs);
> >>    
> >   
> 

[PATCH 16/16] tests/acceptance: Test the MIPSsim machine

[Philippe Mathieu-Daudé]

Add a test for the mipssim machine, based on the recommended
test setup from Thomas Huth:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg606846.html

The test is quick and can be run as:

  $ avocado --show=console run -t machine:mipssim tests/acceptance/
   (1/1) tests/acceptance/machine_mips_mipssim.py:MipsSimMachine.test_mipssim_linux_console:
  console: Linux version 3.6.11 (root@711bb8ba16a7) (gcc version 4.8.3 (Buildroot 2014.11) ) #2 Sun Sep 27 13:39:35 UTC 2020
  console: Setting default memory size 0x02000000
  console: bootconsole [early0] enabled
  console: CPU revision is: 00019300 (MIPS 24Kc)
  console: FPU revision is: 00739300
  ...
  console: CPU frequency 12.00 MHz
  console: Calibrating delay loop... 950.27 BogoMIPS (lpj=4751360)
  ...
  console: MIPSNet Ethernet driver. Version: 2007-11-17. (c)2005 MIPS Technologies, Inc.
  ...
  console: Welcome to Buildroot
  console: buildroot login: root
  console: # root
  console: -sh: root: not found
  console: # ping -c 3 10.0.2.2
  console: PING 10.0.2.2 (10.0.2.2): 56 data bytes
  console: 64 bytes from 10.0.2.2: seq=0 ttl=255 time=48.231 ms
  console: 64 bytes from 10.0.2.2: seq=1 ttl=255 time=9.407 ms
  console: 64 bytes from 10.0.2.2: seq=2 ttl=255 time=2.298 ms
  console: --- 10.0.2.2 ping statistics ---
  console: 3 packets transmitted, 3 packets received, 0% packet loss
  PASS (7.99 s)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Thomas Huth <huth@tuxfamily.org>
---
 MAINTAINERS                              |  1 +
 tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
 2 files changed, 57 insertions(+)
 create mode 100644 tests/acceptance/machine_mips_mipssim.py

diff --git a/MAINTAINERS b/MAINTAINERS
index 5eed1e692b4..17d8a012b0e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -240,6 +240,7 @@ F: include/hw/misc/mips_*
 F: include/hw/timer/mips_gictimer.h
 F: tests/acceptance/linux_ssh_mips_malta.py
 F: tests/acceptance/machine_mips_malta.py
+F: tests/acceptance/machine_mips_mipssim.py
 F: tests/tcg/mips/
 K: ^Subject:.*(?i)mips
 
diff --git a/tests/acceptance/machine_mips_mipssim.py b/tests/acceptance/machine_mips_mipssim.py
new file mode 100644
index 00000000000..b2749917b08
--- /dev/null
+++ b/tests/acceptance/machine_mips_mipssim.py
@@ -0,0 +1,56 @@
+# Functional tests for the MIPS simulator (MIPSsim machine)
+#
+# Copyright (c) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+import logging
+import time
+
+from avocado import skipUnless
+from avocado_qemu import Test
+from avocado_qemu import exec_command_and_wait_for_pattern
+from avocado_qemu import interrupt_interactive_console_until_pattern
+from avocado_qemu import wait_for_console_pattern
+
+class MipsSimMachine(Test):
+
+    timeout = 30
+    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
+
+    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
+    def test_mipssim_linux_console(self):
+        """
+        Boots the Linux kernel and checks that the console is operational
+        :avocado: tags=arch:mipsel
+        :avocado: tags=machine:mipssim
+        :avocado: tags=device:mipsnet
+        """
+        kernel_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
+                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
+                      'mips/mipssim/mipsel/vmlinux')
+        kernel_hash = '0f9aeca3a2e25b5b0cc4999571f39a7ad58cdc43'
+        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
+        initrd_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
+                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
+                      'mips/mipssim/mipsel/rootfs.cpio')
+        initrd_hash = 'b20359bdfae66387e5a17d6692686d59c189417b'
+        initrd_path = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
+
+        self.vm.set_console()
+        self.vm.add_args('-kernel', kernel_path,
+                         '-initrd', initrd_path,
+                         '-append', self.KERNEL_COMMON_COMMAND_LINE)
+        self.vm.launch()
+
+        wait_for_console_pattern(self, 'Welcome to Buildroot')
+        interrupt_interactive_console_until_pattern(self,
+                                                    interrupt_string='root\r',
+                                                    success_message='#')
+        pattern = '3 packets transmitted, 3 packets received, 0% packet loss'
+        exec_command_and_wait_for_pattern(self, 'ping -c 3 10.0.2.2', pattern)
-- 
2.26.2

Re: [PATCH 16/16] tests/acceptance: Test the MIPSsim machine

[Willian Rampazzo]

On Mon, Sep 28, 2020 at 2:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Add a test for the mipssim machine, based on the recommended
> test setup from Thomas Huth:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg606846.html
>
> The test is quick and can be run as:
>
>   $ avocado --show=console run -t machine:mipssim tests/acceptance/
>    (1/1) tests/acceptance/machine_mips_mipssim.py:MipsSimMachine.test_mipssim_linux_console:
>   console: Linux version 3.6.11 (root@711bb8ba16a7) (gcc version 4.8.3 (Buildroot 2014.11) ) #2 Sun Sep 27 13:39:35 UTC 2020
>   console: Setting default memory size 0x02000000
>   console: bootconsole [early0] enabled
>   console: CPU revision is: 00019300 (MIPS 24Kc)
>   console: FPU revision is: 00739300
>   ...
>   console: CPU frequency 12.00 MHz
>   console: Calibrating delay loop... 950.27 BogoMIPS (lpj=4751360)
>   ...
>   console: MIPSNet Ethernet driver. Version: 2007-11-17. (c)2005 MIPS Technologies, Inc.
>   ...
>   console: Welcome to Buildroot
>   console: buildroot login: root
>   console: # root
>   console: -sh: root: not found
>   console: # ping -c 3 10.0.2.2
>   console: PING 10.0.2.2 (10.0.2.2): 56 data bytes
>   console: 64 bytes from 10.0.2.2: seq=0 ttl=255 time=48.231 ms
>   console: 64 bytes from 10.0.2.2: seq=1 ttl=255 time=9.407 ms
>   console: 64 bytes from 10.0.2.2: seq=2 ttl=255 time=2.298 ms
>   console: --- 10.0.2.2 ping statistics ---
>   console: 3 packets transmitted, 3 packets received, 0% packet loss
>   PASS (7.99 s)
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Thomas Huth <huth@tuxfamily.org>
> ---
>  MAINTAINERS                              |  1 +
>  tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
>  create mode 100644 tests/acceptance/machine_mips_mipssim.py
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5eed1e692b4..17d8a012b0e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -240,6 +240,7 @@ F: include/hw/misc/mips_*
>  F: include/hw/timer/mips_gictimer.h
>  F: tests/acceptance/linux_ssh_mips_malta.py
>  F: tests/acceptance/machine_mips_malta.py
> +F: tests/acceptance/machine_mips_mipssim.py
>  F: tests/tcg/mips/
>  K: ^Subject:.*(?i)mips
>
> diff --git a/tests/acceptance/machine_mips_mipssim.py b/tests/acceptance/machine_mips_mipssim.py
> new file mode 100644
> index 00000000000..b2749917b08
> --- /dev/null
> +++ b/tests/acceptance/machine_mips_mipssim.py
> @@ -0,0 +1,56 @@
> +# Functional tests for the MIPS simulator (MIPSsim machine)
> +#
> +# Copyright (c) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import os
> +import logging
> +import time
> +
> +from avocado import skipUnless
> +from avocado_qemu import Test
> +from avocado_qemu import exec_command_and_wait_for_pattern
> +from avocado_qemu import interrupt_interactive_console_until_pattern
> +from avocado_qemu import wait_for_console_pattern
> +
> +class MipsSimMachine(Test):
> +
> +    timeout = 30
> +    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
> +
> +    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
> +    def test_mipssim_linux_console(self):
> +        """
> +        Boots the Linux kernel and checks that the console is operational
> +        :avocado: tags=arch:mipsel
> +        :avocado: tags=machine:mipssim
> +        :avocado: tags=device:mipsnet
> +        """
> +        kernel_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
> +                      'mips/mipssim/mipsel/vmlinux')

So, are you willing to maintain some images on your GitHub to avoid
the image changes when they are not found?

> +        kernel_hash = '0f9aeca3a2e25b5b0cc4999571f39a7ad58cdc43'
> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +
> +        initrd_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
> +                      'mips/mipssim/mipsel/rootfs.cpio')
> +        initrd_hash = 'b20359bdfae66387e5a17d6692686d59c189417b'
> +        initrd_path = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
> +
> +        self.vm.set_console()
> +        self.vm.add_args('-kernel', kernel_path,
> +                         '-initrd', initrd_path,
> +                         '-append', self.KERNEL_COMMON_COMMAND_LINE)
> +        self.vm.launch()
> +
> +        wait_for_console_pattern(self, 'Welcome to Buildroot')
> +        interrupt_interactive_console_until_pattern(self,
> +                                                    interrupt_string='root\r',
> +                                                    success_message='#')
> +        pattern = '3 packets transmitted, 3 packets received, 0% packet loss'
> +        exec_command_and_wait_for_pattern(self, 'ping -c 3 10.0.2.2', pattern)
> --
> 2.26.2
>
>

IMHO, although the solution of spreading images over personal
repositories is not the best, I don't have a strong opinion to block
this, so

Reviewed-by: Willian Rampazzo (willianr@redhat.com)

Re: [PATCH 16/16] tests/acceptance: Test the MIPSsim machine

[Philippe Mathieu-Daudé]

+Alex/Daniel

On 9/28/20 10:33 PM, Willian Rampazzo wrote:
> On Mon, Sep 28, 2020 at 2:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Add a test for the mipssim machine, based on the recommended
>> test setup from Thomas Huth:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg606846.html
>>
>> The test is quick and can be run as:
>>
>>   $ avocado --show=console run -t machine:mipssim tests/acceptance/
>>    (1/1) tests/acceptance/machine_mips_mipssim.py:MipsSimMachine.test_mipssim_linux_console:
>>   console: Linux version 3.6.11 (root@711bb8ba16a7) (gcc version 4.8.3 (Buildroot 2014.11) ) #2 Sun Sep 27 13:39:35 UTC 2020
>>   console: Setting default memory size 0x02000000
>>   console: bootconsole [early0] enabled
>>   console: CPU revision is: 00019300 (MIPS 24Kc)
>>   console: FPU revision is: 00739300
>>   ...
>>   console: CPU frequency 12.00 MHz
>>   console: Calibrating delay loop... 950.27 BogoMIPS (lpj=4751360)
>>   ...
>>   console: MIPSNet Ethernet driver. Version: 2007-11-17. (c)2005 MIPS Technologies, Inc.
>>   ...
>>   console: Welcome to Buildroot
>>   console: buildroot login: root
>>   console: # root
>>   console: -sh: root: not found
>>   console: # ping -c 3 10.0.2.2
>>   console: PING 10.0.2.2 (10.0.2.2): 56 data bytes
>>   console: 64 bytes from 10.0.2.2: seq=0 ttl=255 time=48.231 ms
>>   console: 64 bytes from 10.0.2.2: seq=1 ttl=255 time=9.407 ms
>>   console: 64 bytes from 10.0.2.2: seq=2 ttl=255 time=2.298 ms
>>   console: --- 10.0.2.2 ping statistics ---
>>   console: 3 packets transmitted, 3 packets received, 0% packet loss
>>   PASS (7.99 s)
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Cc: Thomas Huth <huth@tuxfamily.org>
>> ---
>>  MAINTAINERS                              |  1 +
>>  tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
>>  2 files changed, 57 insertions(+)
>>  create mode 100644 tests/acceptance/machine_mips_mipssim.py
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5eed1e692b4..17d8a012b0e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -240,6 +240,7 @@ F: include/hw/misc/mips_*
>>  F: include/hw/timer/mips_gictimer.h
>>  F: tests/acceptance/linux_ssh_mips_malta.py
>>  F: tests/acceptance/machine_mips_malta.py
>> +F: tests/acceptance/machine_mips_mipssim.py
>>  F: tests/tcg/mips/
>>  K: ^Subject:.*(?i)mips
>>
>> diff --git a/tests/acceptance/machine_mips_mipssim.py b/tests/acceptance/machine_mips_mipssim.py
>> new file mode 100644
>> index 00000000000..b2749917b08
>> --- /dev/null
>> +++ b/tests/acceptance/machine_mips_mipssim.py
>> @@ -0,0 +1,56 @@
>> +# Functional tests for the MIPS simulator (MIPSsim machine)
>> +#
>> +# Copyright (c) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>> +# See the COPYING file in the top-level directory.
>> +#
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +import os
>> +import logging
>> +import time
>> +
>> +from avocado import skipUnless
>> +from avocado_qemu import Test
>> +from avocado_qemu import exec_command_and_wait_for_pattern
>> +from avocado_qemu import interrupt_interactive_console_until_pattern
>> +from avocado_qemu import wait_for_console_pattern
>> +
>> +class MipsSimMachine(Test):
>> +
>> +    timeout = 30
>> +    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>> +
>> +    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
>> +    def test_mipssim_linux_console(self):
>> +        """
>> +        Boots the Linux kernel and checks that the console is operational
>> +        :avocado: tags=arch:mipsel
>> +        :avocado: tags=machine:mipssim
>> +        :avocado: tags=device:mipsnet
>> +        """
>> +        kernel_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
>> +                      'mips/mipssim/mipsel/vmlinux')
> 
> So, are you willing to maintain some images on your GitHub to avoid
> the image changes when they are not found?

No, I am not willing to do that.

But I see it pointless to have everyone (including CI) to spend 1h
building this image, when the sources and build recipe is available,
making the built image reproducible.

I'm not sure we need to run all the tests all the time, but I'd like
to have the tests:

1/ provided with a series, so reviewer are free to rebuild the image
   and test this series works as expected,
2/ committed in the repository, so maintainers can verify future changes
   doesn't introduce regression.

> 
>> +        kernel_hash = '0f9aeca3a2e25b5b0cc4999571f39a7ad58cdc43'
>> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
>> +
>> +        initrd_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
>> +                      'mips/mipssim/mipsel/rootfs.cpio')
>> +        initrd_hash = 'b20359bdfae66387e5a17d6692686d59c189417b'
>> +        initrd_path = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
>> +
>> +        self.vm.set_console()
>> +        self.vm.add_args('-kernel', kernel_path,
>> +                         '-initrd', initrd_path,
>> +                         '-append', self.KERNEL_COMMON_COMMAND_LINE)
>> +        self.vm.launch()
>> +
>> +        wait_for_console_pattern(self, 'Welcome to Buildroot')
>> +        interrupt_interactive_console_until_pattern(self,
>> +                                                    interrupt_string='root\r',
>> +                                                    success_message='#')
>> +        pattern = '3 packets transmitted, 3 packets received, 0% packet loss'
>> +        exec_command_and_wait_for_pattern(self, 'ping -c 3 10.0.2.2', pattern)
>> --
>> 2.26.2
>>
>>
> 
> IMHO, although the solution of spreading images over personal
> repositories is not the best, I don't have a strong opinion to block
> this, so

If the URL is stable (stable commit or even better, tag) it should
be OK.

tag is used in the quanta-gsj tests, see commit 4fe986dd448
("tests/acceptance: console boot tests for quanta-gsj"):

  image_url = (
          'https://github.com/hskinnemoen/openbmc/releases/download/'
          '20200711-gsj-qemu-0/obmc-phosphor-image-gsj.static.mtd.gz')
  image_hash = '14895e634923345cb5c8776037ff7876df96f6b1'

> 
> Reviewed-by: Willian Rampazzo (willianr@redhat.com)
> 

Thanks!

Phil.

Re: [PATCH 16/16] tests/acceptance: Test the MIPSsim machine

[Alex Bennée]

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

> +Alex/Daniel
>
> On 9/28/20 10:33 PM, Willian Rampazzo wrote:
>> On Mon, Sep 28, 2020 at 2:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> Add a test for the mipssim machine, based on the recommended
>>> test setup from Thomas Huth:
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg606846.html
>>>
>>> The test is quick and can be run as:
>>>
>>>   $ avocado --show=console run -t machine:mipssim tests/acceptance/
>>>    (1/1) tests/acceptance/machine_mips_mipssim.py:MipsSimMachine.test_mipssim_linux_console:
>>>   console: Linux version 3.6.11 (root@711bb8ba16a7) (gcc version 4.8.3 (Buildroot 2014.11) ) #2 Sun Sep 27 13:39:35 UTC 2020
>>>   console: Setting default memory size 0x02000000
>>>   console: bootconsole [early0] enabled
>>>   console: CPU revision is: 00019300 (MIPS 24Kc)
>>>   console: FPU revision is: 00739300
>>>   ...
>>>   console: CPU frequency 12.00 MHz
>>>   console: Calibrating delay loop... 950.27 BogoMIPS (lpj=4751360)
>>>   ...
>>>   console: MIPSNet Ethernet driver. Version: 2007-11-17. (c)2005 MIPS Technologies, Inc.
>>>   ...
>>>   console: Welcome to Buildroot
>>>   console: buildroot login: root
>>>   console: # root
>>>   console: -sh: root: not found
>>>   console: # ping -c 3 10.0.2.2
>>>   console: PING 10.0.2.2 (10.0.2.2): 56 data bytes
>>>   console: 64 bytes from 10.0.2.2: seq=0 ttl=255 time=48.231 ms
>>>   console: 64 bytes from 10.0.2.2: seq=1 ttl=255 time=9.407 ms
>>>   console: 64 bytes from 10.0.2.2: seq=2 ttl=255 time=2.298 ms
>>>   console: --- 10.0.2.2 ping statistics ---
>>>   console: 3 packets transmitted, 3 packets received, 0% packet loss
>>>   PASS (7.99 s)
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> Cc: Thomas Huth <huth@tuxfamily.org>
>>> ---
>>>  MAINTAINERS                              |  1 +
>>>  tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
>>>  2 files changed, 57 insertions(+)
>>>  create mode 100644 tests/acceptance/machine_mips_mipssim.py
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 5eed1e692b4..17d8a012b0e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -240,6 +240,7 @@ F: include/hw/misc/mips_*
>>>  F: include/hw/timer/mips_gictimer.h
>>>  F: tests/acceptance/linux_ssh_mips_malta.py
>>>  F: tests/acceptance/machine_mips_malta.py
>>> +F: tests/acceptance/machine_mips_mipssim.py
>>>  F: tests/tcg/mips/
>>>  K: ^Subject:.*(?i)mips
>>>
>>> diff --git a/tests/acceptance/machine_mips_mipssim.py b/tests/acceptance/machine_mips_mipssim.py
>>> new file mode 100644
>>> index 00000000000..b2749917b08
>>> --- /dev/null
>>> +++ b/tests/acceptance/machine_mips_mipssim.py
>>> @@ -0,0 +1,56 @@
>>> +# Functional tests for the MIPS simulator (MIPSsim machine)
>>> +#
>>> +# Copyright (c) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> +#
>>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> +# See the COPYING file in the top-level directory.
>>> +#
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +import os
>>> +import logging
>>> +import time
>>> +
>>> +from avocado import skipUnless
>>> +from avocado_qemu import Test
>>> +from avocado_qemu import exec_command_and_wait_for_pattern
>>> +from avocado_qemu import interrupt_interactive_console_until_pattern
>>> +from avocado_qemu import wait_for_console_pattern
>>> +
>>> +class MipsSimMachine(Test):
>>> +
>>> +    timeout = 30
>>> +    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>>> +
>>> +    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
>>> +    def test_mipssim_linux_console(self):
>>> +        """
>>> +        Boots the Linux kernel and checks that the console is operational
>>> +        :avocado: tags=arch:mipsel
>>> +        :avocado: tags=machine:mipssim
>>> +        :avocado: tags=device:mipsnet
>>> +        """
>>> +        kernel_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
>>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
>>> +                      'mips/mipssim/mipsel/vmlinux')
>> 
>> So, are you willing to maintain some images on your GitHub to avoid
>> the image changes when they are not found?
>
> No, I am not willing to do that.
>
> But I see it pointless to have everyone (including CI) to spend 1h
> building this image, when the sources and build recipe is available,
> making the built image reproducible.

I agree we don't want to build from scratch each time. However as we
move from relying on third parties (which outside of the major distros
has proved very flaky) we are getting to the point of managing our GPL
liabilities.

After a bit of digging in your branches I see this is a buildroot
recipe running in Docker. Are they fully reproducible? Could we have a:

  make acceptance-image-mipsel

which built the same blob that gets inserted into the avocado cache
should your github url ever disappear?

> I'm not sure we need to run all the tests all the time, but I'd like
> to have the tests:
>
> 1/ provided with a series, so reviewer are free to rebuild the image
>    and test this series works as expected,
> 2/ committed in the repository, so maintainers can verify future changes
>    doesn't introduce regression.

Yes these are great goals - my only worry is the shifting sands some of
these images seem to exist on.

>> 
>>> +        kernel_hash = '0f9aeca3a2e25b5b0cc4999571f39a7ad58cdc43'
>>> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
>>> +
>>> +        initrd_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
>>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
>>> +                      'mips/mipssim/mipsel/rootfs.cpio')
>>> +        initrd_hash = 'b20359bdfae66387e5a17d6692686d59c189417b'
>>> +        initrd_path = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
>>> +
>>> +        self.vm.set_console()
>>> +        self.vm.add_args('-kernel', kernel_path,
>>> +                         '-initrd', initrd_path,
>>> +                         '-append', self.KERNEL_COMMON_COMMAND_LINE)
>>> +        self.vm.launch()
>>> +
>>> +        wait_for_console_pattern(self, 'Welcome to Buildroot')
>>> +        interrupt_interactive_console_until_pattern(self,
>>> +                                                    interrupt_string='root\r',
>>> +                                                    success_message='#')
>>> +        pattern = '3 packets transmitted, 3 packets received, 0% packet loss'
>>> +        exec_command_and_wait_for_pattern(self, 'ping -c 3 10.0.2.2', pattern)
>>> --
>>> 2.26.2
>>>
>>>
>> 
>> IMHO, although the solution of spreading images over personal
>> repositories is not the best, I don't have a strong opinion to block
>> this, so
>
> If the URL is stable (stable commit or even better, tag) it should
> be OK.
>
> tag is used in the quanta-gsj tests, see commit 4fe986dd448
> ("tests/acceptance: console boot tests for quanta-gsj"):
>
>   image_url = (
>           'https://github.com/hskinnemoen/openbmc/releases/download/'
>           '20200711-gsj-qemu-0/obmc-phosphor-image-gsj.static.mtd.gz')
>   image_hash = '14895e634923345cb5c8776037ff7876df96f6b1'
>
>> 
>> Reviewed-by: Willian Rampazzo (willianr@redhat.com)
>> 
>
> Thanks!
>
> Phil.


-- 
Alex Bennée

Re: [PATCH 16/16] tests/acceptance: Test the MIPSsim machine

[Philippe Mathieu-Daudé]

On 9/29/20 11:38 AM, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> +Alex/Daniel
>>
>> On 9/28/20 10:33 PM, Willian Rampazzo wrote:
>>> On Mon, Sep 28, 2020 at 2:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> Add a test for the mipssim machine, based on the recommended
>>>> test setup from Thomas Huth:
>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg606846.html
>>>>
>>>> The test is quick and can be run as:
>>>>
>>>>   $ avocado --show=console run -t machine:mipssim tests/acceptance/
>>>>    (1/1) tests/acceptance/machine_mips_mipssim.py:MipsSimMachine.test_mipssim_linux_console:
>>>>   console: Linux version 3.6.11 (root@711bb8ba16a7) (gcc version 4.8.3 (Buildroot 2014.11) ) #2 Sun Sep 27 13:39:35 UTC 2020
>>>>   console: Setting default memory size 0x02000000
>>>>   console: bootconsole [early0] enabled
>>>>   console: CPU revision is: 00019300 (MIPS 24Kc)
>>>>   console: FPU revision is: 00739300
>>>>   ...
>>>>   console: CPU frequency 12.00 MHz
>>>>   console: Calibrating delay loop... 950.27 BogoMIPS (lpj=4751360)
>>>>   ...
>>>>   console: MIPSNet Ethernet driver. Version: 2007-11-17. (c)2005 MIPS Technologies, Inc.
>>>>   ...
>>>>   console: Welcome to Buildroot
>>>>   console: buildroot login: root
>>>>   console: # root
>>>>   console: -sh: root: not found
>>>>   console: # ping -c 3 10.0.2.2
>>>>   console: PING 10.0.2.2 (10.0.2.2): 56 data bytes
>>>>   console: 64 bytes from 10.0.2.2: seq=0 ttl=255 time=48.231 ms
>>>>   console: 64 bytes from 10.0.2.2: seq=1 ttl=255 time=9.407 ms
>>>>   console: 64 bytes from 10.0.2.2: seq=2 ttl=255 time=2.298 ms
>>>>   console: --- 10.0.2.2 ping statistics ---
>>>>   console: 3 packets transmitted, 3 packets received, 0% packet loss
>>>>   PASS (7.99 s)
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>> Cc: Thomas Huth <huth@tuxfamily.org>
>>>> ---
>>>>  MAINTAINERS                              |  1 +
>>>>  tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
>>>>  2 files changed, 57 insertions(+)
>>>>  create mode 100644 tests/acceptance/machine_mips_mipssim.py
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 5eed1e692b4..17d8a012b0e 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -240,6 +240,7 @@ F: include/hw/misc/mips_*
>>>>  F: include/hw/timer/mips_gictimer.h
>>>>  F: tests/acceptance/linux_ssh_mips_malta.py
>>>>  F: tests/acceptance/machine_mips_malta.py
>>>> +F: tests/acceptance/machine_mips_mipssim.py
>>>>  F: tests/tcg/mips/
>>>>  K: ^Subject:.*(?i)mips
>>>>
>>>> diff --git a/tests/acceptance/machine_mips_mipssim.py b/tests/acceptance/machine_mips_mipssim.py
>>>> new file mode 100644
>>>> index 00000000000..b2749917b08
>>>> --- /dev/null
>>>> +++ b/tests/acceptance/machine_mips_mipssim.py
>>>> @@ -0,0 +1,56 @@
>>>> +# Functional tests for the MIPS simulator (MIPSsim machine)
>>>> +#
>>>> +# Copyright (c) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> +#
>>>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>> +# See the COPYING file in the top-level directory.
>>>> +#
>>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>>> +
>>>> +import os
>>>> +import logging
>>>> +import time
>>>> +
>>>> +from avocado import skipUnless
>>>> +from avocado_qemu import Test
>>>> +from avocado_qemu import exec_command_and_wait_for_pattern
>>>> +from avocado_qemu import interrupt_interactive_console_until_pattern
>>>> +from avocado_qemu import wait_for_console_pattern
>>>> +
>>>> +class MipsSimMachine(Test):
>>>> +
>>>> +    timeout = 30
>>>> +    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>>>> +
>>>> +    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
>>>> +    def test_mipssim_linux_console(self):
>>>> +        """
>>>> +        Boots the Linux kernel and checks that the console is operational
>>>> +        :avocado: tags=arch:mipsel
>>>> +        :avocado: tags=machine:mipssim
>>>> +        :avocado: tags=device:mipsnet
>>>> +        """
>>>> +        kernel_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
>>>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
>>>> +                      'mips/mipssim/mipsel/vmlinux')
>>>
>>> So, are you willing to maintain some images on your GitHub to avoid
>>> the image changes when they are not found?
>>
>> No, I am not willing to do that.
>>
>> But I see it pointless to have everyone (including CI) to spend 1h
>> building this image, when the sources and build recipe is available,
>> making the built image reproducible.
> 
> I agree we don't want to build from scratch each time. However as we
> move from relying on third parties (which outside of the major distros
> has proved very flaky) we are getting to the point of managing our GPL
> liabilities.
> 
> After a bit of digging in your branches I see this is a buildroot
> recipe running in Docker. Are they fully reproducible?

Yes, I wrote this Dockerfile to 1/ allow others to use/test but also
2/ so I don't have to manually do this again.

BTW Thomas suggested to add a 'make legal-info' final step, since
buildroot provides it (I was not aware of that):
https://buildroot.org/downloads/manual/manual.html#legal-info

> Could we have a:
> 
>   make acceptance-image-mipsel
> 
> which built the same blob that gets inserted into the avocado cache
> should your github url ever disappear?

Yes but that would mean add this Dockerfile into the QEMU repository,
right? Where to put the .config, QEMU repo too? Then keep the *built*
image + sources + legal-info out of tree. Finally the Avocado test
containing the image SHA1 in QEMU tree. Is that OK?

> 
>> I'm not sure we need to run all the tests all the time, but I'd like
>> to have the tests:
>>
>> 1/ provided with a series, so reviewer are free to rebuild the image
>>    and test this series works as expected,
>> 2/ committed in the repository, so maintainers can verify future changes
>>    doesn't introduce regression.
> 
> Yes these are great goals - my only worry is the shifting sands some of
> these images seem to exist on.
> 
>>>
>>>> +        kernel_hash = '0f9aeca3a2e25b5b0cc4999571f39a7ad58cdc43'
>>>> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
>>>> +
>>>> +        initrd_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
>>>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
>>>> +                      'mips/mipssim/mipsel/rootfs.cpio')
>>>> +        initrd_hash = 'b20359bdfae66387e5a17d6692686d59c189417b'
>>>> +        initrd_path = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
>>>> +
>>>> +        self.vm.set_console()
>>>> +        self.vm.add_args('-kernel', kernel_path,
>>>> +                         '-initrd', initrd_path,
>>>> +                         '-append', self.KERNEL_COMMON_COMMAND_LINE)
>>>> +        self.vm.launch()
>>>> +
>>>> +        wait_for_console_pattern(self, 'Welcome to Buildroot')
>>>> +        interrupt_interactive_console_until_pattern(self,
>>>> +                                                    interrupt_string='root\r',
>>>> +                                                    success_message='#')
>>>> +        pattern = '3 packets transmitted, 3 packets received, 0% packet loss'
>>>> +        exec_command_and_wait_for_pattern(self, 'ping -c 3 10.0.2.2', pattern)
>>>> --
>>>> 2.26.2
>>>>
>>>>
>>>
>>> IMHO, although the solution of spreading images over personal
>>> repositories is not the best, I don't have a strong opinion to block
>>> this, so
>>
>> If the URL is stable (stable commit or even better, tag) it should
>> be OK.
>>
>> tag is used in the quanta-gsj tests, see commit 4fe986dd448
>> ("tests/acceptance: console boot tests for quanta-gsj"):
>>
>>   image_url = (
>>           'https://github.com/hskinnemoen/openbmc/releases/download/'
>>           '20200711-gsj-qemu-0/obmc-phosphor-image-gsj.static.mtd.gz')
>>   image_hash = '14895e634923345cb5c8776037ff7876df96f6b1'
>>
>>>
>>> Reviewed-by: Willian Rampazzo (willianr@redhat.com)
>>>
>>
>> Thanks!
>>
>> Phil.
> 
> 

Re: [PATCH 16/16] tests/acceptance: Test the MIPSsim machine

[Daniel P. Berrangé]

On Tue, Sep 29, 2020 at 10:38:30AM +0100, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
> > +Alex/Daniel
> >
> > On 9/28/20 10:33 PM, Willian Rampazzo wrote:
> >> On Mon, Sep 28, 2020 at 2:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>>
> >>> Add a test for the mipssim machine, based on the recommended
> >>> test setup from Thomas Huth:
> >>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg606846.html
> >>>
> >>> The test is quick and can be run as:
> >>>
> >>>   $ avocado --show=console run -t machine:mipssim tests/acceptance/
> >>>    (1/1) tests/acceptance/machine_mips_mipssim.py:MipsSimMachine.test_mipssim_linux_console:
> >>>   console: Linux version 3.6.11 (root@711bb8ba16a7) (gcc version 4.8.3 (Buildroot 2014.11) ) #2 Sun Sep 27 13:39:35 UTC 2020
> >>>   console: Setting default memory size 0x02000000
> >>>   console: bootconsole [early0] enabled
> >>>   console: CPU revision is: 00019300 (MIPS 24Kc)
> >>>   console: FPU revision is: 00739300
> >>>   ...
> >>>   console: CPU frequency 12.00 MHz
> >>>   console: Calibrating delay loop... 950.27 BogoMIPS (lpj=4751360)
> >>>   ...
> >>>   console: MIPSNet Ethernet driver. Version: 2007-11-17. (c)2005 MIPS Technologies, Inc.
> >>>   ...
> >>>   console: Welcome to Buildroot
> >>>   console: buildroot login: root
> >>>   console: # root
> >>>   console: -sh: root: not found
> >>>   console: # ping -c 3 10.0.2.2
> >>>   console: PING 10.0.2.2 (10.0.2.2): 56 data bytes
> >>>   console: 64 bytes from 10.0.2.2: seq=0 ttl=255 time=48.231 ms
> >>>   console: 64 bytes from 10.0.2.2: seq=1 ttl=255 time=9.407 ms
> >>>   console: 64 bytes from 10.0.2.2: seq=2 ttl=255 time=2.298 ms
> >>>   console: --- 10.0.2.2 ping statistics ---
> >>>   console: 3 packets transmitted, 3 packets received, 0% packet loss
> >>>   PASS (7.99 s)
> >>>
> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> ---
> >>> Cc: Thomas Huth <huth@tuxfamily.org>
> >>> ---
> >>>  MAINTAINERS                              |  1 +
> >>>  tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
> >>>  2 files changed, 57 insertions(+)
> >>>  create mode 100644 tests/acceptance/machine_mips_mipssim.py
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index 5eed1e692b4..17d8a012b0e 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -240,6 +240,7 @@ F: include/hw/misc/mips_*
> >>>  F: include/hw/timer/mips_gictimer.h
> >>>  F: tests/acceptance/linux_ssh_mips_malta.py
> >>>  F: tests/acceptance/machine_mips_malta.py
> >>> +F: tests/acceptance/machine_mips_mipssim.py
> >>>  F: tests/tcg/mips/
> >>>  K: ^Subject:.*(?i)mips
> >>>
> >>> diff --git a/tests/acceptance/machine_mips_mipssim.py b/tests/acceptance/machine_mips_mipssim.py
> >>> new file mode 100644
> >>> index 00000000000..b2749917b08
> >>> --- /dev/null
> >>> +++ b/tests/acceptance/machine_mips_mipssim.py
> >>> @@ -0,0 +1,56 @@
> >>> +# Functional tests for the MIPS simulator (MIPSsim machine)
> >>> +#
> >>> +# Copyright (c) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> +#
> >>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>> +# See the COPYING file in the top-level directory.
> >>> +#
> >>> +# SPDX-License-Identifier: GPL-2.0-or-later
> >>> +
> >>> +import os
> >>> +import logging
> >>> +import time
> >>> +
> >>> +from avocado import skipUnless
> >>> +from avocado_qemu import Test
> >>> +from avocado_qemu import exec_command_and_wait_for_pattern
> >>> +from avocado_qemu import interrupt_interactive_console_until_pattern
> >>> +from avocado_qemu import wait_for_console_pattern
> >>> +
> >>> +class MipsSimMachine(Test):
> >>> +
> >>> +    timeout = 30
> >>> +    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
> >>> +
> >>> +    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
> >>> +    def test_mipssim_linux_console(self):
> >>> +        """
> >>> +        Boots the Linux kernel and checks that the console is operational
> >>> +        :avocado: tags=arch:mipsel
> >>> +        :avocado: tags=machine:mipssim
> >>> +        :avocado: tags=device:mipsnet
> >>> +        """
> >>> +        kernel_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
> >>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
> >>> +                      'mips/mipssim/mipsel/vmlinux')
> >> 
> >> So, are you willing to maintain some images on your GitHub to avoid
> >> the image changes when they are not found?
> >
> > No, I am not willing to do that.
> >
> > But I see it pointless to have everyone (including CI) to spend 1h
> > building this image, when the sources and build recipe is available,
> > making the built image reproducible.
> 
> I agree we don't want to build from scratch each time. However as we
> move from relying on third parties (which outside of the major distros
> has proved very flaky) we are getting to the point of managing our GPL
> liabilities.

I'm wondering what the 1 hour time to build the images is
spent on, and what does the image need to actually do ?

Is it anything more than boot the kernel successfully ?

I've not tried a timed build, but surely a minimal / tiny
Linx kernel build is much less than an hour ? Combine with
a tiny C program that simply prints "OK" as /bin/init and
powers off the VM. Or we need more advanced then a minimal
busybox build.

I think it'd be great if we could self-build a minimal
kernel+init for all platforms we target, using distro
provided cross-compilers. 


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 16/16] tests/acceptance: Test the MIPSsim machine

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Sep 29, 2020 at 10:38:30AM +0100, Alex Bennée wrote:
>> 
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> 
>> > +Alex/Daniel
>> >
>> > On 9/28/20 10:33 PM, Willian Rampazzo wrote:
>> >> On Mon, Sep 28, 2020 at 2:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> >>>
>> >>> Add a test for the mipssim machine, based on the recommended
>> >>> test setup from Thomas Huth:
>> >>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg606846.html
>> >>>
>> >>> The test is quick and can be run as:
>> >>>
>> >>>   $ avocado --show=console run -t machine:mipssim tests/acceptance/
>> >>>    (1/1) tests/acceptance/machine_mips_mipssim.py:MipsSimMachine.test_mipssim_linux_console:
>> >>>   console: Linux version 3.6.11 (root@711bb8ba16a7) (gcc version 4.8.3 (Buildroot 2014.11) ) #2 Sun Sep 27 13:39:35 UTC 2020
>> >>>   console: Setting default memory size 0x02000000
>> >>>   console: bootconsole [early0] enabled
>> >>>   console: CPU revision is: 00019300 (MIPS 24Kc)
>> >>>   console: FPU revision is: 00739300
>> >>>   ...
>> >>>   console: CPU frequency 12.00 MHz
>> >>>   console: Calibrating delay loop... 950.27 BogoMIPS (lpj=4751360)
>> >>>   ...
>> >>>   console: MIPSNet Ethernet driver. Version: 2007-11-17. (c)2005 MIPS Technologies, Inc.
>> >>>   ...
>> >>>   console: Welcome to Buildroot
>> >>>   console: buildroot login: root
>> >>>   console: # root
>> >>>   console: -sh: root: not found
>> >>>   console: # ping -c 3 10.0.2.2
>> >>>   console: PING 10.0.2.2 (10.0.2.2): 56 data bytes
>> >>>   console: 64 bytes from 10.0.2.2: seq=0 ttl=255 time=48.231 ms
>> >>>   console: 64 bytes from 10.0.2.2: seq=1 ttl=255 time=9.407 ms
>> >>>   console: 64 bytes from 10.0.2.2: seq=2 ttl=255 time=2.298 ms
>> >>>   console: --- 10.0.2.2 ping statistics ---
>> >>>   console: 3 packets transmitted, 3 packets received, 0% packet loss
>> >>>   PASS (7.99 s)
>> >>>
>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> >>> ---
>> >>> Cc: Thomas Huth <huth@tuxfamily.org>
>> >>> ---
>> >>>  MAINTAINERS                              |  1 +
>> >>>  tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
>> >>>  2 files changed, 57 insertions(+)
>> >>>  create mode 100644 tests/acceptance/machine_mips_mipssim.py
>> >>>
>> >>> diff --git a/MAINTAINERS b/MAINTAINERS
>> >>> index 5eed1e692b4..17d8a012b0e 100644
>> >>> --- a/MAINTAINERS
>> >>> +++ b/MAINTAINERS
>> >>> @@ -240,6 +240,7 @@ F: include/hw/misc/mips_*
>> >>>  F: include/hw/timer/mips_gictimer.h
>> >>>  F: tests/acceptance/linux_ssh_mips_malta.py
>> >>>  F: tests/acceptance/machine_mips_malta.py
>> >>> +F: tests/acceptance/machine_mips_mipssim.py
>> >>>  F: tests/tcg/mips/
>> >>>  K: ^Subject:.*(?i)mips
>> >>>
>> >>> diff --git a/tests/acceptance/machine_mips_mipssim.py b/tests/acceptance/machine_mips_mipssim.py
>> >>> new file mode 100644
>> >>> index 00000000000..b2749917b08
>> >>> --- /dev/null
>> >>> +++ b/tests/acceptance/machine_mips_mipssim.py
>> >>> @@ -0,0 +1,56 @@
>> >>> +# Functional tests for the MIPS simulator (MIPSsim machine)
>> >>> +#
>> >>> +# Copyright (c) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
>> >>> +#
>> >>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>> >>> +# See the COPYING file in the top-level directory.
>> >>> +#
>> >>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> >>> +
>> >>> +import os
>> >>> +import logging
>> >>> +import time
>> >>> +
>> >>> +from avocado import skipUnless
>> >>> +from avocado_qemu import Test
>> >>> +from avocado_qemu import exec_command_and_wait_for_pattern
>> >>> +from avocado_qemu import interrupt_interactive_console_until_pattern
>> >>> +from avocado_qemu import wait_for_console_pattern
>> >>> +
>> >>> +class MipsSimMachine(Test):
>> >>> +
>> >>> +    timeout = 30
>> >>> +    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>> >>> +
>> >>> +    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
>> >>> +    def test_mipssim_linux_console(self):
>> >>> +        """
>> >>> +        Boots the Linux kernel and checks that the console is operational
>> >>> +        :avocado: tags=arch:mipsel
>> >>> +        :avocado: tags=machine:mipssim
>> >>> +        :avocado: tags=device:mipsnet
>> >>> +        """
>> >>> +        kernel_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
>> >>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
>> >>> +                      'mips/mipssim/mipsel/vmlinux')
>> >> 
>> >> So, are you willing to maintain some images on your GitHub to avoid
>> >> the image changes when they are not found?
>> >
>> > No, I am not willing to do that.
>> >
>> > But I see it pointless to have everyone (including CI) to spend 1h
>> > building this image, when the sources and build recipe is available,
>> > making the built image reproducible.
>> 
>> I agree we don't want to build from scratch each time. However as we
>> move from relying on third parties (which outside of the major distros
>> has proved very flaky) we are getting to the point of managing our GPL
>> liabilities.
>
> I'm wondering what the 1 hour time to build the images is
> spent on, and what does the image need to actually do ?

Buildroot will also build the toolchain and libc of chouce before
building it's rootfs. To be honest the rootfs part of the build is
fairly minimal if it's mostly busybox.

> Is it anything more than boot the kernel successfully ?
>
> I've not tried a timed build, but surely a minimal / tiny
> Linx kernel build is much less than an hour ? Combine with
> a tiny C program that simply prints "OK" as /bin/init and
> powers off the VM. Or we need more advanced then a minimal
> busybox build.

Starting from a buildroot is useful for development certainly because
it's a lot easier to add stuff to it if you need to.

> I think it'd be great if we could self-build a minimal
> kernel+init for all platforms we target, using distro
> provided cross-compilers.

We've been down this round before haven't we? Wasn't that what the JeOS
builds where all about?

Anyway things have moved on from the bad old days of crosstool and the
distros are better at packaging sane cross compilers that can build
kernels and basic user space. However leaving everyone to use their own
distro compiler is going to lead to repeatability problems - or maybe we
just treat it like we do with the existing TCG tests?

>
>
> Regards,
> Daniel


-- 
Alex Bennée

Re: [PATCH 16/16] tests/acceptance: Test the MIPSsim machine

[Philippe Mathieu-Daudé]

On 9/30/20 11:49 AM, Alex Bennée wrote:
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Tue, Sep 29, 2020 at 10:38:30AM +0100, Alex Bennée wrote:
>>>
>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>>
>>>> +Alex/Daniel
>>>>
>>>> On 9/28/20 10:33 PM, Willian Rampazzo wrote:
>>>>> On Mon, Sep 28, 2020 at 2:31 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>>>
>>>>>> Add a test for the mipssim machine, based on the recommended
>>>>>> test setup from Thomas Huth:
>>>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg606846.html
>>>>>>
>>>>>> The test is quick and can be run as:
>>>>>>
>>>>>>   $ avocado --show=console run -t machine:mipssim tests/acceptance/
>>>>>>    (1/1) tests/acceptance/machine_mips_mipssim.py:MipsSimMachine.test_mipssim_linux_console:
>>>>>>   console: Linux version 3.6.11 (root@711bb8ba16a7) (gcc version 4.8.3 (Buildroot 2014.11) ) #2 Sun Sep 27 13:39:35 UTC 2020
>>>>>>   console: Setting default memory size 0x02000000
>>>>>>   console: bootconsole [early0] enabled
>>>>>>   console: CPU revision is: 00019300 (MIPS 24Kc)
>>>>>>   console: FPU revision is: 00739300
>>>>>>   ...
>>>>>>   console: CPU frequency 12.00 MHz
>>>>>>   console: Calibrating delay loop... 950.27 BogoMIPS (lpj=4751360)
>>>>>>   ...
>>>>>>   console: MIPSNet Ethernet driver. Version: 2007-11-17. (c)2005 MIPS Technologies, Inc.
>>>>>>   ...
>>>>>>   console: Welcome to Buildroot
>>>>>>   console: buildroot login: root
>>>>>>   console: # root
>>>>>>   console: -sh: root: not found
>>>>>>   console: # ping -c 3 10.0.2.2
>>>>>>   console: PING 10.0.2.2 (10.0.2.2): 56 data bytes
>>>>>>   console: 64 bytes from 10.0.2.2: seq=0 ttl=255 time=48.231 ms
>>>>>>   console: 64 bytes from 10.0.2.2: seq=1 ttl=255 time=9.407 ms
>>>>>>   console: 64 bytes from 10.0.2.2: seq=2 ttl=255 time=2.298 ms
>>>>>>   console: --- 10.0.2.2 ping statistics ---
>>>>>>   console: 3 packets transmitted, 3 packets received, 0% packet loss
>>>>>>   PASS (7.99 s)
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> ---
>>>>>> Cc: Thomas Huth <huth@tuxfamily.org>
>>>>>> ---
>>>>>>  MAINTAINERS                              |  1 +
>>>>>>  tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
>>>>>>  2 files changed, 57 insertions(+)
>>>>>>  create mode 100644 tests/acceptance/machine_mips_mipssim.py
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index 5eed1e692b4..17d8a012b0e 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -240,6 +240,7 @@ F: include/hw/misc/mips_*
>>>>>>  F: include/hw/timer/mips_gictimer.h
>>>>>>  F: tests/acceptance/linux_ssh_mips_malta.py
>>>>>>  F: tests/acceptance/machine_mips_malta.py
>>>>>> +F: tests/acceptance/machine_mips_mipssim.py
>>>>>>  F: tests/tcg/mips/
>>>>>>  K: ^Subject:.*(?i)mips
>>>>>>
>>>>>> diff --git a/tests/acceptance/machine_mips_mipssim.py b/tests/acceptance/machine_mips_mipssim.py
>>>>>> new file mode 100644
>>>>>> index 00000000000..b2749917b08
>>>>>> --- /dev/null
>>>>>> +++ b/tests/acceptance/machine_mips_mipssim.py
>>>>>> @@ -0,0 +1,56 @@
>>>>>> +# Functional tests for the MIPS simulator (MIPSsim machine)
>>>>>> +#
>>>>>> +# Copyright (c) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> +#
>>>>>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>>>> +# See the COPYING file in the top-level directory.
>>>>>> +#
>>>>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>>>>> +
>>>>>> +import os
>>>>>> +import logging
>>>>>> +import time
>>>>>> +
>>>>>> +from avocado import skipUnless
>>>>>> +from avocado_qemu import Test
>>>>>> +from avocado_qemu import exec_command_and_wait_for_pattern
>>>>>> +from avocado_qemu import interrupt_interactive_console_until_pattern
>>>>>> +from avocado_qemu import wait_for_console_pattern
>>>>>> +
>>>>>> +class MipsSimMachine(Test):
>>>>>> +
>>>>>> +    timeout = 30
>>>>>> +    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>>>>>> +
>>>>>> +    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
>>>>>> +    def test_mipssim_linux_console(self):
>>>>>> +        """
>>>>>> +        Boots the Linux kernel and checks that the console is operational
>>>>>> +        :avocado: tags=arch:mipsel
>>>>>> +        :avocado: tags=machine:mipssim
>>>>>> +        :avocado: tags=device:mipsnet
>>>>>> +        """
>>>>>> +        kernel_url = ('https://github.com/philmd/qemu-testing-blob/raw/'
>>>>>> +                      '32ea5764e1de8fffa0d59366c44822cd06d7c8e0/'
>>>>>> +                      'mips/mipssim/mipsel/vmlinux')
>>>>>
>>>>> So, are you willing to maintain some images on your GitHub to avoid
>>>>> the image changes when they are not found?
>>>>
>>>> No, I am not willing to do that.
>>>>
>>>> But I see it pointless to have everyone (including CI) to spend 1h
>>>> building this image, when the sources and build recipe is available,
>>>> making the built image reproducible.
>>>
>>> I agree we don't want to build from scratch each time. However as we
>>> move from relying on third parties (which outside of the major distros
>>> has proved very flaky) we are getting to the point of managing our GPL
>>> liabilities.
>>
>> I'm wondering what the 1 hour time to build the images is
>> spent on, and what does the image need to actually do ?
> 
> Buildroot will also build the toolchain and libc of chouce before
> building it's rootfs. To be honest the rootfs part of the build is
> fairly minimal if it's mostly busybox.

Exactly, half of the time is spent building binutils and gcc.

> 
>> Is it anything more than boot the kernel successfully ?
>>
>> I've not tried a timed build, but surely a minimal / tiny
>> Linx kernel build is much less than an hour ? Combine with
>> a tiny C program that simply prints "OK" as /bin/init and
>> powers off the VM. Or we need more advanced then a minimal
>> busybox build.
> 
> Starting from a buildroot is useful for development certainly because
> it's a lot easier to add stuff to it if you need to.
> 
>> I think it'd be great if we could self-build a minimal
>> kernel+init for all platforms we target, using distro
>> provided cross-compilers.

We can't because MIPSsim has been dropped from the Linux kernel v3.18,
and trying to build a such old kernel produce a *lot* of errors with
a current (cross) compiler. See:
https://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg04071.html

What provides buildroot is old enough toolchain to be able to build
old code.

> 
> We've been down this round before haven't we? Wasn't that what the JeOS
> builds where all about?
> 
> Anyway things have moved on from the bad old days of crosstool and the
> distros are better at packaging sane cross compilers that can build
> kernels and basic user space. However leaving everyone to use their own
> distro compiler is going to lead to repeatability problems - or maybe we
> just treat it like we do with the existing TCG tests?
> 
>>
>>
>> Regards,
>> Daniel
> 
> 

Re: [PATCH 00/16] hw/mips: Set CPU frequency

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200928171539.788309-1-f4bug@amsat.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

C linker for the host machine: cc ld.bfd 2.27-43
Host machine cpu family: x86_64
Host machine cpu: x86_64
../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
Program sh found: YES
Program python3 found: YES (/usr/bin/python3)
Configuring ninjatool using configuration
---
qemu-system-aarch64: falling back to tcg
socket_accept failed: Resource temporarily unavailable
**
ERROR:../src/tests/qtest/libqtest.c:301:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
../src/tests/qtest/libqtest.c:166: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
  TEST    iotest-qcow2: 013
ERROR qtest-x86_64: pxe-test - Bail out! ERROR:../src/tests/qtest/libqtest.c:301:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
make: *** [run-test-110] Error 1
make: *** Waiting for unfinished jobs....

Looking for expected file 'tests/data/acpi/virt/FACP.numamem'
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=e2cdd463ef8a4c2f9b9143e69d8ac437', '-u', '1003', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-9dln7e0v/src/docker-src.2020-09-28-22.27.36.11269:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=e2cdd463ef8a4c2f9b9143e69d8ac437
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-9dln7e0v/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    18m49.678s
user    0m13.756s


The full log is available at
http://patchew.org/logs/20200928171539.788309-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 00/16] hw/mips: Set CPU frequency

[Philippe Mathieu-Daudé]

On 9/29/20 4:46 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200928171539.788309-1-f4bug@amsat.org/
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> ---
> qemu-system-aarch64: falling back to tcg
> socket_accept failed: Resource temporarily unavailable
> **
> ERROR:../src/tests/qtest/libqtest.c:301:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)

Unrelated to this series, as it is only MIPS.

> ../src/tests/qtest/libqtest.c:166: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
>   TEST    iotest-qcow2: 013
> ERROR qtest-x86_64: pxe-test - Bail out! ERROR:../src/tests/qtest/libqtest.c:301:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
> make: *** [run-test-110] Error 1
> make: *** Waiting for unfinished jobs....
> 
> Looking for expected file 'tests/data/acpi/virt/FACP.numamem'
> ---
> real    18m49.678s
> user    0m13.756s
> 
> 
> The full log is available at
> http://patchew.org/logs/20200928171539.788309-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
> ---

Re: [PATCH 00/16] hw/mips: Set CPU frequency

[Igor Mammedov]

On Mon, 28 Sep 2020 19:15:23 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> All the MIPS cores emulated by QEMU provides the Coproc#0
> 'Count' register which can be used as a free running timer.
> 
> Since it's introduction in 2005 this timer uses a fixed
> frequency of 100 MHz (for a CPU freq of 200 MHz).
> While this is not an issue with Linux guests, it makes
> some firmwares behave incorrectly.
> 
> The Clock API allow propagating clocks. It is particularly
> useful when hardware dynamicly changes clock frequencies.
> 
> To be able to model such MIPS hardware, we need to refactor
> the MIPS hardware code to handle clocks.
> 
> This series is organized as follow:
> 
> - let all CPU have an input clock,
> - MIPS CPU get an input clock
> - when the clock is changed, CP0 timer is updated
> - set correct CPU frequencies to all boards
> - do not allow MIPS CPU without input clock

is this clock an integral part of MIPS cpus or it's an external device?

> I used a MIPSsim test suggested by Thomas. It is also included
> as bonus at the end.
> 
> Possible follow up:
> - QOM'ify the GIC
> - let the GIC handle dynamic clock changes
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (16):
>   hw/core/cpu: Let CPU object have a clock source
>   target/mips: Move cpu_mips_get_random() with CP0 helpers
>   target/mips/cp0_timer: Explicit unit in variable name
>   target/mips/cpu: Introduce mips_cpu_properties[]
>   target/mips/cpu: Set default CPU frequency to 200 MHz
>   target/mips: Keep CP0 counter in sync with the CPU frequency
>   hw/mips/r4k: Explicit CPU frequency is 200 MHz
>   hw/mips/fuloong2e: Set CPU frequency to 533 MHz
>   hw/mips/mipssim: Correct CPU frequency
>   hw/mips/jazz: Correct CPU frequencies
>   hw/mips/cps: Expose input clock and connect it to CPU cores
>   hw/mips/boston: Set CPU frequency to 1 GHz
>   hw/mips/malta: Set CPU frequency to 320 MHz
>   hw/mips/cps: Do not allow use without input clock
>   target/mips/cpu: Do not allow system-mode use without input clock
>   tests/acceptance: Test the MIPSsim machine
> 
>  include/hw/core/cpu.h                    |  5 +++
>  include/hw/mips/cps.h                    |  2 +
>  target/mips/cpu.h                        |  9 ++++
>  target/mips/internal.h                   |  2 +-
>  hw/core/cpu.c                            | 12 +++++
>  hw/mips/boston.c                         | 13 ++++++
>  hw/mips/cps.c                            |  8 ++++
>  hw/mips/fuloong2e.c                      |  8 +++-
>  hw/mips/jazz.c                           | 16 ++++++-
>  hw/mips/malta.c                          | 20 +++++++--
>  hw/mips/mipssim.c                        | 12 ++++-
>  hw/mips/r4k.c                            |  8 +++-
>  target/mips/cp0_helper.c                 | 25 +++++++++++
>  target/mips/cp0_timer.c                  | 51 ++++++---------------
>  target/mips/cpu.c                        | 43 +++++++++++++++++-
>  MAINTAINERS                              |  1 +
>  tests/acceptance/machine_mips_mipssim.py | 56 ++++++++++++++++++++++++
>  17 files changed, 244 insertions(+), 47 deletions(-)
>  create mode 100644 tests/acceptance/machine_mips_mipssim.py
> 

Re: [PATCH 00/16] hw/mips: Set CPU frequency

[Philippe Mathieu-Daudé]

On 9/30/20 9:40 AM, Igor Mammedov wrote:
> On Mon, 28 Sep 2020 19:15:23 +0200
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> All the MIPS cores emulated by QEMU provides the Coproc#0
>> 'Count' register which can be used as a free running timer.
>>
>> Since it's introduction in 2005 this timer uses a fixed
>> frequency of 100 MHz (for a CPU freq of 200 MHz).
>> While this is not an issue with Linux guests, it makes
>> some firmwares behave incorrectly.
>>
>> The Clock API allow propagating clocks. It is particularly
>> useful when hardware dynamicly changes clock frequencies.
>>
>> To be able to model such MIPS hardware, we need to refactor
>> the MIPS hardware code to handle clocks.
>>
>> This series is organized as follow:
>>
>> - let all CPU have an input clock,
>> - MIPS CPU get an input clock
>> - when the clock is changed, CP0 timer is updated
>> - set correct CPU frequencies to all boards
>> - do not allow MIPS CPU without input clock
> 
> is this clock an integral part of MIPS cpus or it's an external device?

CPU cores are clocked via an external clock.
This clock can be on the board (from a crystal oscillator to
complex PLL) or on-chip for some system-on-chip.

In all the (current) QEMU MIPS machines it is external although.

Regards,

Phil.

Re: [PATCH 00/16] hw/mips: Set CPU frequency

[Philippe Mathieu-Daudé]

On 9/28/20 7:15 PM, Philippe Mathieu-Daudé wrote:
> All the MIPS cores emulated by QEMU provides the Coproc#0
> 'Count' register which can be used as a free running timer.
> 
> Since it's introduction in 2005 this timer uses a fixed
> frequency of 100 MHz (for a CPU freq of 200 MHz).
> While this is not an issue with Linux guests, it makes
> some firmwares behave incorrectly.
> 
> The Clock API allow propagating clocks. It is particularly
> useful when hardware dynamicly changes clock frequencies.
> 
> To be able to model such MIPS hardware, we need to refactor
> the MIPS hardware code to handle clocks.
> 
> This series is organized as follow:
> 
> - let all CPU have an input clock,
> - MIPS CPU get an input clock
> - when the clock is changed, CP0 timer is updated
> - set correct CPU frequencies to all boards
> - do not allow MIPS CPU without input clock
> 
> I used a MIPSsim test suggested by Thomas. It is also included
> as bonus at the end.
> 
> Possible follow up:
> - QOM'ify the GIC
> - let the GIC handle dynamic clock changes
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (16):
>    hw/core/cpu: Let CPU object have a clock source
>    target/mips: Move cpu_mips_get_random() with CP0 helpers
>    target/mips/cp0_timer: Explicit unit in variable name
>    target/mips/cpu: Introduce mips_cpu_properties[]
>    target/mips/cpu: Set default CPU frequency to 200 MHz
>    target/mips: Keep CP0 counter in sync with the CPU frequency
>    hw/mips/r4k: Explicit CPU frequency is 200 MHz
>    hw/mips/fuloong2e: Set CPU frequency to 533 MHz
>    hw/mips/mipssim: Correct CPU frequency
>    hw/mips/jazz: Correct CPU frequencies
>    hw/mips/cps: Expose input clock and connect it to CPU cores
>    hw/mips/boston: Set CPU frequency to 1 GHz
>    hw/mips/malta: Set CPU frequency to 320 MHz
>    hw/mips/cps: Do not allow use without input clock
>    target/mips/cpu: Do not allow system-mode use without input clock
>    tests/acceptance: Test the MIPSsim machine

I'm queuing patches 2 and 3 to mips-next. The others depend
of #1 which has been asked for changes.

Thanks,

Phil.