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