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é <f4...@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é <f4...@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
