Hi Damian, Luc, Peter.

I've been debugging some odd issue with the clocks:
a clock created in the machine (IOW, not a qdev clock) isn't
always resetted, thus propagating its value.
"not always" is the odd part. In the MPS2 board, the machine
clock is propagated. Apparently because the peripherals are
created directly in the machine_init() handler. When moving
them out in a SoC QOM container, the clock isn't... I'm still
having hard time to understand what is going on.

Alternatively I tried to strengthen the clock API by reducing
the clock creation in 2 cases: machine/device. This way clocks
aren't left dangling around alone. The qdev clocks are properly
resetted, and for the machine clocks I register a generic reset
handler. This way is safer, but I don't think we want to keep
adding generic reset handlers, instead we'd like to remove them.

I'll keep debugging to understand. Meanwhile posting this series
as RFC to get feedback on the approach and start discussing on
this issue.

Regards,

Phil.

Philippe Mathieu-Daudé (9):
  hw/core/clock: Increase clock propagation trace events verbosity
  hw/core/machine: Add machine_create_constant_clock() helper
  hw/arm: Use new machine_create_constant_clock() helper
  hw/mips: Use new machine_create_constant_clock() helper
  hw/core/qdev-clock: Add qdev_ground_clock() helper
  hw/misc/bcm2835_cprman: Use qdev_ground_clock() helper
  hw/misc/bcm2835_cprman: Feed 'xosc' from the board
  hw/clock: Declare clock_new() internally
  hw/core/machine: Reset machine clocks using qemu_register_reset()

 hw/core/clock-internal.h         | 32 ++++++++++++++++++++++++++++++++
 include/hw/boards.h              | 17 +++++++++++++++++
 include/hw/clock.h               | 13 -------------
 include/hw/misc/bcm2835_cprman.h |  2 --
 include/hw/qdev-clock.h          |  9 +++++++++
 hw/arm/bcm2835_peripherals.c     |  1 +
 hw/arm/bcm2836.c                 |  1 +
 hw/arm/mps2-tz.c                 |  6 ++----
 hw/arm/mps2.c                    |  3 +--
 hw/arm/musca.c                   |  6 ++----
 hw/arm/raspi.c                   |  4 ++++
 hw/core/clock.c                  |  9 ++++++++-
 hw/core/machine.c                | 20 ++++++++++++++++++++
 hw/core/qdev-clock.c             | 12 ++++++++++++
 hw/mips/fuloong2e.c              |  4 ++--
 hw/mips/jazz.c                   |  6 +++---
 hw/mips/loongson3_virt.c         |  4 ++--
 hw/mips/mipssim.c                |  7 ++++---
 hw/misc/bcm2835_cprman.c         | 12 +++---------
 MAINTAINERS                      |  1 +
 hw/core/trace-events             |  3 ++-
 21 files changed, 126 insertions(+), 46 deletions(-)
 create mode 100644 hw/core/clock-internal.h

-- 
2.26.3

On 4/9/21 8:23 AM, Philippe Mathieu-Daudé wrote:
> Hi Damian, Luc, Peter.
> 
> I've been debugging some odd issue with the clocks:
> a clock created in the machine (IOW, not a qdev clock) isn't
> always resetted, thus propagating its value.
> "not always" is the odd part. In the MPS2 board, the machine
> clock is propagated. Apparently because the peripherals are
> created directly in the machine_init() handler. When moving
> them out in a SoC QOM container, the clock isn't... I'm still
> having hard time to understand what is going on.
> 
> Alternatively I tried to strengthen the clock API by reducing
> the clock creation in 2 cases: machine/device. This way clocks
> aren't left dangling around alone. The qdev clocks are properly
> resetted, and for the machine clocks I register a generic reset
> handler. This way is safer, but I don't think we want to keep
> adding generic reset handlers, instead we'd like to remove them.
> 
> I'll keep debugging to understand. Meanwhile posting this series
> as RFC to get feedback on the approach and start discussing on
> this issue.

I wonder if this could be the culprit:

  commit 96250eab904261b31d9d1ac3abbdb36737635ffa
  Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
  Date:   Fri Aug 28 10:02:44 2020 +0100

      hw/clock: Only propagate clock changes if the clock is changed

      Avoid propagating the clock change when the clock does not change.

  diff --git a/include/hw/clock.h b/include/hw/clock.h
  index d85af45c967..9ecd78b2c30 100644
  --- a/include/hw/clock.h
  +++ b/include/hw/clock.h
  @@ -165,8 +165,9 @@ void clock_propagate(Clock *clk);
    */
   static inline void clock_update(Clock *clk, uint64_t value)
   {
  -    clock_set(clk, value);
  -    clock_propagate(clk);
  +    if (clock_set(clk, value)) {
  +        clock_propagate(clk);
  +    }
   }

I.e.:

- first use clock_set() to set the new period
- then call clock_update() with the same "new period"

-> the clock parent already has the new period, so the
   children are not updated.

On 4/9/21 3:12 PM, Philippe Mathieu-Daudé wrote:
> On 4/9/21 8:23 AM, Philippe Mathieu-Daudé wrote:
>> Hi Damian, Luc, Peter.
>>
>> I've been debugging some odd issue with the clocks:
>> a clock created in the machine (IOW, not a qdev clock) isn't
>> always resetted, thus propagating its value.
>> "not always" is the odd part. In the MPS2 board, the machine
>> clock is propagated. Apparently because the peripherals are
>> created directly in the machine_init() handler. When moving
>> them out in a SoC QOM container, the clock isn't... I'm still
>> having hard time to understand what is going on.
>>
>> Alternatively I tried to strengthen the clock API by reducing
>> the clock creation in 2 cases: machine/device. This way clocks
>> aren't left dangling around alone. The qdev clocks are properly
>> resetted, and for the machine clocks I register a generic reset
>> handler. This way is safer, but I don't think we want to keep
>> adding generic reset handlers, instead we'd like to remove them.
>>
>> I'll keep debugging to understand. Meanwhile posting this series
>> as RFC to get feedback on the approach and start discussing on
>> this issue.
> 
> I wonder if this could be the culprit:

No (same reverting it) :(

>   commit 96250eab904261b31d9d1ac3abbdb36737635ffa
>   Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
>   Date:   Fri Aug 28 10:02:44 2020 +0100
> 
>       hw/clock: Only propagate clock changes if the clock is changed
> 
>       Avoid propagating the clock change when the clock does not change.
> 
>   diff --git a/include/hw/clock.h b/include/hw/clock.h
>   index d85af45c967..9ecd78b2c30 100644
>   --- a/include/hw/clock.h
>   +++ b/include/hw/clock.h
>   @@ -165,8 +165,9 @@ void clock_propagate(Clock *clk);
>     */
>    static inline void clock_update(Clock *clk, uint64_t value)
>    {
>   -    clock_set(clk, value);
>   -    clock_propagate(clk);
>   +    if (clock_set(clk, value)) {
>   +        clock_propagate(clk);
>   +    }
>    }
> 
> I.e.:
> 
> - first use clock_set() to set the new period
> - then call clock_update() with the same "new period"
> 
> -> the clock parent already has the new period, so the
>    children are not updated.

This is actually what clock_set_source() does:

  void clock_set_source(Clock *clk, Clock *src)
  {
      ...

      clk->period = src->period; // <------------------------------
      QLIST_INSERT_HEAD(&src->children, clk, sibling);
      clk->source = src;
      clock_propagate_period(clk, false);
  }

So indeed if we use qdev_connect_clock_in() in DeviceRealize(),
it calls clock_set_source() and set the period, does not propagate,
then later when clock_propagate_period() is called:

static void clock_propagate_period(Clock *clk, bool call_callbacks)
{
    ...
    QLIST_FOREACH(child, &clk->children, sibling) {
        if (child->period != clk->period) {
            //           ^^^^ this condition is false
            ...
            clock_propagate_period(child, call_callbacks);
            // ^^^ children never get clock propagated
        }
    }
}

Does it make sense?

On 4/9/21 4:11 PM, Philippe Mathieu-Daudé wrote:
> On 4/9/21 3:12 PM, Philippe Mathieu-Daudé wrote:
>> On 4/9/21 8:23 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Damian, Luc, Peter.
>>>
>>> I've been debugging some odd issue with the clocks:
>>> a clock created in the machine (IOW, not a qdev clock) isn't
>>> always resetted, thus propagating its value.
>>> "not always" is the odd part. In the MPS2 board, the machine
>>> clock is propagated. Apparently because the peripherals are
>>> created directly in the machine_init() handler. When moving
>>> them out in a SoC QOM container, the clock isn't... I'm still
>>> having hard time to understand what is going on.
>>>
>>> Alternatively I tried to strengthen the clock API by reducing
>>> the clock creation in 2 cases: machine/device. This way clocks
>>> aren't left dangling around alone. The qdev clocks are properly
>>> resetted, and for the machine clocks I register a generic reset
>>> handler. This way is safer, but I don't think we want to keep
>>> adding generic reset handlers, instead we'd like to remove them.
>>>
>>> I'll keep debugging to understand. Meanwhile posting this series
>>> as RFC to get feedback on the approach and start discussing on
>>> this issue.
>>
>> I wonder if this could be the culprit:
> 
> No (same reverting it) :(
> 
>>   commit 96250eab904261b31d9d1ac3abbdb36737635ffa
>>   Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>   Date:   Fri Aug 28 10:02:44 2020 +0100
>>
>>       hw/clock: Only propagate clock changes if the clock is changed
>>
>>       Avoid propagating the clock change when the clock does not change.
>>
>>   diff --git a/include/hw/clock.h b/include/hw/clock.h
>>   index d85af45c967..9ecd78b2c30 100644
>>   --- a/include/hw/clock.h
>>   +++ b/include/hw/clock.h
>>   @@ -165,8 +165,9 @@ void clock_propagate(Clock *clk);
>>     */
>>    static inline void clock_update(Clock *clk, uint64_t value)
>>    {
>>   -    clock_set(clk, value);
>>   -    clock_propagate(clk);
>>   +    if (clock_set(clk, value)) {
>>   +        clock_propagate(clk);
>>   +    }
>>    }
>>
>> I.e.:
>>
>> - first use clock_set() to set the new period
>> - then call clock_update() with the same "new period"
>>
>> -> the clock parent already has the new period, so the
>>    children are not updated.
> 
> This is actually what clock_set_source() does:
> 
>   void clock_set_source(Clock *clk, Clock *src)
>   {
>       ...
> 
>       clk->period = src->period; // <------------------------------
>       QLIST_INSERT_HEAD(&src->children, clk, sibling);
>       clk->source = src;
>       clock_propagate_period(clk, false);
>   }
> 
> So indeed if we use qdev_connect_clock_in() in DeviceRealize(),
> it calls clock_set_source() and set the period, does not propagate,
> then later when clock_propagate_period() is called:
> 
> static void clock_propagate_period(Clock *clk, bool call_callbacks)
> {
>     ...
>     QLIST_FOREACH(child, &clk->children, sibling) {
>         if (child->period != clk->period) {
>             //           ^^^^ this condition is false
>             ...
>             clock_propagate_period(child, call_callbacks);
>             // ^^^ children never get clock propagated
>         }
>     }
> }
> 
> Does it make sense?

FWIW this fixes the problem I'm having:

-- >8 --
diff --git a/hw/core/clock.c b/hw/core/clock.c
index 23c0c372b5d..4ecb51b1465 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -87,7 +87,7 @@ static void clock_propagate_period(Clock *clk, bool
call_callbacks)
                                        CLOCK_PERIOD_TO_HZ(clk->period),
                                        CLOCK_PATH(child),
                                        CLOCK_PERIOD_TO_HZ(child->period));
-        if (child->period != clk->period) {
+        if (1) {//child->period != clk->period) {
             if (call_callbacks) {
                 clock_call_callback(child, ClockPreUpdate);
             }
---

At least this confirm my hypothesis and reduces the scope of
the problem.

I'm not sure what is the best way to fix this (yet?) so I'll
wait here for feedback. At least I can keep going :)

Regards,

Phil.

Hi Philippe,

On 08:23 Fri 09 Apr     , Philippe Mathieu-Daudé wrote:
> Hi Damian, Luc, Peter.
> 
> I've been debugging some odd issue with the clocks:
> a clock created in the machine (IOW, not a qdev clock) isn't
> always resetted, thus propagating its value.
> "not always" is the odd part. In the MPS2 board, the machine
> clock is propagated. Apparently because the peripherals are
> created directly in the machine_init() handler. When moving
> them out in a SoC QOM container, the clock isn't... I'm still
> having hard time to understand what is going on.

I think there is a misunderstanding on how the clock API works. If I
understand correctly your issue, you expect the callback of an input
clock connected to your constant "main oscillator" clock to be called on
machine reset.

If I'm not mistaken this is not the way the API has been designed. The
callback is called only when the clock period changes. A constant clock
does not change on reset, so the callback of child clocks should not be
called.

However devices that care about this clock value (e.g. a device that
has a clock input connected to this constant clock) should see their
standard reset callback called during reset. And they can effectively read
the clock value here and do what they need to do.

Note that clock propagation during reset has always been a complicated
problem. Calling clock_propagate is forbidden during the reset's enter
phase because of the side effects it can introduce.

> 
> Alternatively I tried to strengthen the clock API by reducing
> the clock creation in 2 cases: machine/device. This way clocks
> aren't left dangling around alone. The qdev clocks are properly
> resetted, and for the machine clocks I register a generic reset
> handler. This way is safer, but I don't think we want to keep
> adding generic reset handlers, instead we'd like to remove them.

I find your API modification a bit restrictive. I think creating a
standalone clock can be useful, e.g. in complicated devices that may
want to use internal "intermediate" clocks. I would not remove this
possibility to the API users.

Thanks,

-- 
Luc

Hi Luc,

On 4/10/21 3:19 PM, Luc Michel wrote:
> On 08:23 Fri 09 Apr     , Philippe Mathieu-Daudé wrote:
>> I've been debugging some odd issue with the clocks:
>> a clock created in the machine (IOW, not a qdev clock) isn't
>> always resetted, thus propagating its value.
>> "not always" is the odd part. In the MPS2 board, the machine
>> clock is propagated. Apparently because the peripherals are
>> created directly in the machine_init() handler. When moving
>> them out in a SoC QOM container, the clock isn't... I'm still
>> having hard time to understand what is going on.
> 
> I think there is a misunderstanding on how the clock API works. If I
> understand correctly your issue, you expect the callback of an input
> clock connected to your constant "main oscillator" clock to be called on
> machine reset.
> 
> If I'm not mistaken this is not the way the API has been designed. The
> callback is called only when the clock period changes. A constant clock
> does not change on reset, so the callback of child clocks should not be
> called.

They why the children of a clock tree fed with constant clock stay with
a clock of 0? Who is responsible of setting their clock to the constant
value?

> However devices that care about this clock value (e.g. a device that
> has a clock input connected to this constant clock) should see their
> standard reset callback called during reset. And they can effectively read
> the clock value here and do what they need to do.
> 
> Note that clock propagation during reset has always been a complicated
> problem. Calling clock_propagate is forbidden during the reset's enter
> phase because of the side effects it can introduce.

Ah... Maybe this is related to the generic reset problem in QEMU :(

>> Alternatively I tried to strengthen the clock API by reducing
>> the clock creation in 2 cases: machine/device. This way clocks
>> aren't left dangling around alone. The qdev clocks are properly
>> resetted, and for the machine clocks I register a generic reset
>> handler. This way is safer, but I don't think we want to keep
>> adding generic reset handlers, instead we'd like to remove them.
> 
> I find your API modification a bit restrictive. I think creating a
> standalone clock can be useful, e.g. in complicated devices that may
> want to use internal "intermediate" clocks. I would not remove this
> possibility to the API users.

Well, this is the point. I can't see a justification to have a clock
on a non-qdev object. We should be able to model complicated devices
with qdev.

We are having various problems with the CPUs which are non-qdev devices,
or recently even with the LED model...:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg798031.html

Phil.

On Sat, 10 Apr 2021 at 14:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Luc,
>
> On 4/10/21 3:19 PM, Luc Michel wrote:
> > Note that clock propagation during reset has always been a complicated
> > problem. Calling clock_propagate is forbidden during the reset's enter
> > phase because of the side effects it can introduce.
>
> Ah... Maybe this is related to the generic reset problem in QEMU :(

I do wonder if we got the clock-propagation-during-reset part of this
wrong -- it seemed right to me at the time but trying to use the
clock API recently I did run into some unhelpful-seeming results
(I forget the details now, though).

> > I find your API modification a bit restrictive. I think creating a
> > standalone clock can be useful, e.g. in complicated devices that may
> > want to use internal "intermediate" clocks. I would not remove this
> > possibility to the API users.
>
> Well, this is the point. I can't see a justification to have a clock
> on a non-qdev object. We should be able to model complicated devices
> with qdev.

The obvious reason is that machine objects are not qdev devices (ie
TYPE_MACHINE inherits directly from TYPE_OBJECT, not from TYPE_DEVICE),
but it's a reasonable thing to say "this machine has a fixed frequency
clock which it connects to the SoC".

I do wonder if the right fix to that would be to make TYPE_MACHINE
be a subtype of TYPE_DEVICE, though -- machines not being subtypes
of device has other annoying effects, like their not having reset
methods or being able to register vmstate structs. There might be
some unanticipated side effects of making that change, though.

thanks
-- PMM

+Mark & Igor

On 4/10/21 5:15 PM, Peter Maydell wrote:
> On Sat, 10 Apr 2021 at 14:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 4/10/21 3:19 PM, Luc Michel wrote:
>>> Note that clock propagation during reset has always been a complicated
>>> problem. Calling clock_propagate is forbidden during the reset's enter
>>> phase because of the side effects it can introduce.
>>
>> Ah... Maybe this is related to the generic reset problem in QEMU :(
> 
> I do wonder if we got the clock-propagation-during-reset part of this
> wrong -- it seemed right to me at the time but trying to use the
> clock API recently I did run into some unhelpful-seeming results
> (I forget the details now, though).
> 
>>> I find your API modification a bit restrictive. I think creating a
>>> standalone clock can be useful, e.g. in complicated devices that may
>>> want to use internal "intermediate" clocks. I would not remove this
>>> possibility to the API users.
>>
>> Well, this is the point. I can't see a justification to have a clock
>> on a non-qdev object. We should be able to model complicated devices
>> with qdev.
> 
> The obvious reason is that machine objects are not qdev devices (ie
> TYPE_MACHINE inherits directly from TYPE_OBJECT, not from TYPE_DEVICE),
> but it's a reasonable thing to say "this machine has a fixed frequency
> clock which it connects to the SoC".

In this series I restrict Clocks to 1/ qdev and 2/ MachineState (which
is the case you described). I tried to think about all the hardware I
worked with and all could be somehow modeled as qdev.

> I do wonder if the right fix to that would be to make TYPE_MACHINE
> be a subtype of TYPE_DEVICE, though -- machines not being subtypes
> of device has other annoying effects, like their not having reset
> methods or being able to register vmstate structs. There might be
> some unanticipated side effects of making that change, though.

Yes, that would simplify few things. I might try it.

Expanding the topic, this reminds me a discussion between Igor and Mark
about MemoryRegion...

One was about we can not change the NULL owner to MachineState
because the region could be migrated and there is a mismatch.

Another was about preparing the design for multi-arch machines, Mark was
confuse by having owner for memory regions such DRAM because on a board
they aren't owned, can be used by various masters (CPUs, DMA). So the
machine should be the owner somehow. Maybe the problem was with
migration indeed, I can't remember :S

Regards,

Phil.

On Sat, 10 Apr 2021 at 16:15, Peter Maydell <peter.maydell@linaro.org> wrote:
> I do wonder if the right fix to that would be to make TYPE_MACHINE
> be a subtype of TYPE_DEVICE, though -- machines not being subtypes
> of device has other annoying effects, like their not having reset
> methods or being able to register vmstate structs.

I wasn't quite right about this -- turns out that machines can
have a reset method, but it is a weird special case method
MachineClass::reset that isn't like device reset. (We use it
in a few machine types; if implemented, it is responsible for
calling qemu_devices_reset() to kick off device reset. Probably
MachineClass ought to implement TYPE_RESETTABLE_INTERFACE
instead.)

thanks
-- PMM

Hi Peter,

On 4/12/21 12:11 PM, Peter Maydell wrote:
> On Sat, 10 Apr 2021 at 16:15, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I do wonder if the right fix to that would be to make TYPE_MACHINE
>> be a subtype of TYPE_DEVICE, though -- machines not being subtypes
>> of device has other annoying effects, like their not having reset
>> methods or being able to register vmstate structs.
> 
> I wasn't quite right about this -- turns out that machines can
> have a reset method, but it is a weird special case method
> MachineClass::reset that isn't like device reset. (We use it
> in a few machine types; if implemented, it is responsible for
> calling qemu_devices_reset() to kick off device reset. Probably
> MachineClass ought to implement TYPE_RESETTABLE_INTERFACE
> instead.)

TIL MachineClass::reset().

- hw/hppa/machine.c
- hw/i386/pc.c

  Used to reset CPUs manually because CPUs aren't sysbus-reset.

- hw/i386/microvm.c

  Removing the microvm_fix_kernel_cmdline() call which is suspicious
  at reset (should be enough once in realize time), the is the same
  previous "Used to reset CPUs manually ..."

- hw/ppc/pnv.c

  "Reset BMC simulator" seems a legitimate case of machine reset.

  Next is 'fdt = pnv_dt_create()'. So guest has changed hardware state
  and we need to refresh the DT for the next guest boots. Could be.

- hw/ppc/spapr.c

  Similar previous "Used to reset CPUs manually ..."

  Similar previous "update DT after hardware physically changed".


Am I correct than half of these handlers code is to kludge the fact
that CPU aren't reset such device reset?

Regards,

Phil.

On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> TIL MachineClass::reset().
>
> - hw/hppa/machine.c
> - hw/i386/pc.c
>
>   Used to reset CPUs manually because CPUs aren't sysbus-reset.

pc_machine_reset() is not resetting the CPUs -- it is
re-resetting the APIC devices, which looks like it is a
workaround for a reset-ordering or other problem. I'm not
sure where the CPUs are being reset...

-- PMM

On 4/12/21 12:44 PM, Peter Maydell wrote:
> On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> TIL MachineClass::reset().
>>
>> - hw/hppa/machine.c
>> - hw/i386/pc.c
>>
>>   Used to reset CPUs manually because CPUs aren't sysbus-reset.
> 
> pc_machine_reset() is not resetting the CPUs -- it is
> re-resetting the APIC devices, which looks like it is a
> workaround for a reset-ordering or other problem. I'm not
> sure where the CPUs are being reset...

Ah, I got confused by the CPU_FOREACH(cs) macro.

On Mon, Apr 12, 2021 at 11:44:29AM +0100, Peter Maydell wrote:
> On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > TIL MachineClass::reset().
> >
> > - hw/hppa/machine.c
> > - hw/i386/pc.c
> >
> >   Used to reset CPUs manually because CPUs aren't sysbus-reset.
> 
> pc_machine_reset() is not resetting the CPUs -- it is
> re-resetting the APIC devices, which looks like it is a
> workaround for a reset-ordering or other problem. I'm not
> sure where the CPUs are being reset...

CPU reset code was moved from pc.c:pc_cpu_reset() to
cpu.c:x86_cpu_machine_reset_cb() in commit 65dee3805259
("target-i386: move cpu_reset and reset callback to cpu.c").
It's not clear to me why.

-- 
Eduardo

On Tue, 13 Apr 2021 15:43:19 -0400
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Apr 12, 2021 at 11:44:29AM +0100, Peter Maydell wrote:
> > On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
> > > TIL MachineClass::reset().
> > >
> > > - hw/hppa/machine.c
> > > - hw/i386/pc.c
> > >
> > >   Used to reset CPUs manually because CPUs aren't sysbus-reset.  
> > 
> > pc_machine_reset() is not resetting the CPUs -- it is
> > re-resetting the APIC devices, which looks like it is a
> > workaround for a reset-ordering or other problem. I'm not
> > sure where the CPUs are being reset...  
> 
> CPU reset code was moved from pc.c:pc_cpu_reset() to
> cpu.c:x86_cpu_machine_reset_cb() in commit 65dee3805259
> ("target-i386: move cpu_reset and reset callback to cpu.c").
> It's not clear to me why.

it was for cpu hotplug support, so that is we would have
CPU in well know initial state after realize is complete.

On 5/3/21 5:20 PM, Igor Mammedov wrote:
> On Tue, 13 Apr 2021 15:43:19 -0400
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Mon, Apr 12, 2021 at 11:44:29AM +0100, Peter Maydell wrote:
>>> On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
>>>> TIL MachineClass::reset().
>>>>
>>>> - hw/hppa/machine.c
>>>> - hw/i386/pc.c
>>>>
>>>>   Used to reset CPUs manually because CPUs aren't sysbus-reset.  
>>>
>>> pc_machine_reset() is not resetting the CPUs -- it is
>>> re-resetting the APIC devices, which looks like it is a
>>> workaround for a reset-ordering or other problem. I'm not
>>> sure where the CPUs are being reset...  
>>
>> CPU reset code was moved from pc.c:pc_cpu_reset() to
>> cpu.c:x86_cpu_machine_reset_cb() in commit 65dee3805259
>> ("target-i386: move cpu_reset and reset callback to cpu.c").
>> It's not clear to me why.
> 
> it was for cpu hotplug support, so that is we would have
> CPU in well know initial state after realize is complete.

It makes sense, but I don't see why this is considered x86 specific.

On 15:53 Sat 10 Apr     , Philippe Mathieu-Daudé wrote:
> Hi Luc,
> 
> On 4/10/21 3:19 PM, Luc Michel wrote:
> > On 08:23 Fri 09 Apr     , Philippe Mathieu-Daudé wrote:
> >> I've been debugging some odd issue with the clocks:
> >> a clock created in the machine (IOW, not a qdev clock) isn't
> >> always resetted, thus propagating its value.
> >> "not always" is the odd part. In the MPS2 board, the machine
> >> clock is propagated. Apparently because the peripherals are
> >> created directly in the machine_init() handler. When moving
> >> them out in a SoC QOM container, the clock isn't... I'm still
> >> having hard time to understand what is going on.
> > 
> > I think there is a misunderstanding on how the clock API works. If I
> > understand correctly your issue, you expect the callback of an input
> > clock connected to your constant "main oscillator" clock to be called on
> > machine reset.
> > 
> > If I'm not mistaken this is not the way the API has been designed. The
> > callback is called only when the clock period changes. A constant clock
> > does not change on reset, so the callback of child clocks should not be
> > called.
> 
> They why the children of a clock tree fed with constant clock stay with
> a clock of 0? Who is responsible of setting their clock to the constant
> value?

I think we expect the child to be set when we call clock_set_source. In
this function the child period is set to the parent one. Maybe you have
a case where clock_set_source is called before clock_set on the parent?

-- 
Luc