This series aims to add a way to model clock distribution in qemu. This allows
to model the clock tree of a platform allowing us to inspect clock
configuration and detect problems such as disabled clock or bad configured
pll.

The added clock api is very similar the the gpio api for devices. We can add
input and output and connect them together.

Very few changes since v5 in the core patches: we were waiting for multi phase
ability to allow proper initialization of the clock tree. So this is almost a
simple rebase on top of the current "Multi-phase reset mechanism" series.
Based-on: <20190821163341.16309-1-damien.hedde@greensocs.com>

Changes since v5:
 - drop the "-port" in file names
 - new patch 2, extracted from patch 1 (to fix some problem with linux-user builds)
 - patch 3, minor modification to better match gpios api and allow non device-related clock
   (I've dropped the reviewed-by, see the patch message for the details of what has changed).
 - patch 6, Philippe's comments and various improvement
 - patches 7/8/9, multi-phase reset addition and scope reduced to uart ref clocks

The patches are organised as follows:
+ Patches 1 to 5 adds the clock support in qemu (1, 4 and 5 are already reviewed and
  also a big part of the 3)
+ Patch 6 add some documentation in docs/devel
+ Patches 7 to 9 adds the uart's clocks to the xilinx_zynq platform as an
example for this framework. It updates the zynq's slcr clock controller, the 
cadence_uart device, and the zynq toplevel platform.

I've tested this patchset on the xilinx-zynq-a9 machine with the buildroot's
zynq_zc706_defconfig which package the Xilinx's Linux.
Clocks are correctly updated and we ends up with a configured baudrate of 115601
on the console uart (for a theoretical 115200) which is nice. "cadence_uart*" and
"clock*" traces can be enabled to see what's going on in this platform.

Any comments and suggestion are welcomed.

Thanks to the Xilinx QEMU team who sponsored this development.

Damien Hedde (9):
  hw/core/clock: introduce clock objects
  hw/core/clock-vmstate: define a vmstate entry for clock state
  qdev: add clock input&output support to devices.
  qdev-monitor: print the device's clock with info qtree
  qdev-clock: introduce an init array to ease the device construction
  docs/clocks: add device's clock documentation
  hw/misc/zynq_slcr: add clock generation for uarts
  hw/char/cadence_uart: add clock support
  hw/arm/xilinx_zynq: connect uart clocks to slcr

 Makefile.objs                  |   1 +
 docs/devel/clock.txt           | 246 +++++++++++++++++++++++++++++++++
 hw/arm/xilinx_zynq.c           |  64 +++++++--
 hw/char/cadence_uart.c         |  85 ++++++++++--
 hw/char/trace-events           |   3 +
 hw/core/Makefile.objs          |   4 +-
 hw/core/clock-vmstate.c        |  25 ++++
 hw/core/clock.c                | 144 +++++++++++++++++++
 hw/core/qdev-clock.c           | 181 ++++++++++++++++++++++++
 hw/core/qdev.c                 |  32 +++++
 hw/core/trace-events           |   6 +
 hw/misc/zynq_slcr.c            | 145 ++++++++++++++++++-
 include/hw/char/cadence_uart.h |   1 +
 include/hw/clock.h             | 133 ++++++++++++++++++
 include/hw/qdev-clock.h        | 134 ++++++++++++++++++
 include/hw/qdev-core.h         |  14 ++
 qdev-monitor.c                 |  13 ++
 tests/Makefile.include         |   1 +
 18 files changed, 1210 insertions(+), 22 deletions(-)
 create mode 100644 docs/devel/clock.txt
 create mode 100644 hw/core/clock-vmstate.c
 create mode 100644 hw/core/clock.c
 create mode 100644 hw/core/qdev-clock.c
 create mode 100644 include/hw/clock.h
 create mode 100644 include/hw/qdev-clock.h

-- 
2.22.0

On Wed, 4 Sep 2019 at 13:56, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This series aims to add a way to model clock distribution in qemu. This allows
> to model the clock tree of a platform allowing us to inspect clock
> configuration and detect problems such as disabled clock or bad configured
> pll.
>
> The added clock api is very similar the the gpio api for devices. We can add
> input and output and connect them together.
>
> Very few changes since v5 in the core patches: we were waiting for multi phase
> ability to allow proper initialization of the clock tree. So this is almost a
> simple rebase on top of the current "Multi-phase reset mechanism" series.
> Based-on: <20190821163341.16309-1-damien.hedde@greensocs.com>

I've now gone through and given review comments on the patchset.
I don't think there was anything particularly major -- overall
I like the structure and API (and also the documentation!).

The one topic I think we could do with discussing is whether
a simple uint64_t giving the frequency of the clock in Hz is
the right representation. In particular in your patch 9 the
board has a clock frequency that's not a nice integer number
of Hz. I think Philippe also mentioned on irc some board where
the UART clock ends up at a weird frequency. Since the
representation of the frequency is baked into the migration
format it's going to be easier to get it right first rather
than trying to change it later.

So what should the representation be? Some random thoughts:

1) ptimer internally uses a 'period plus fraction' representation:
 int64_t period is the integer part of the period in nanoseconds,
 uint32_t period_frac is the fractional part of the period
(if you like you can think of this as "96-bit integer
period measured in units of one-2^32nd of a nanosecond").
However its only public interfaces for setting the frequency
are (a) set the frequency in Hz (uint32_t) or (b) set
the period in nanoseconds (int64_t); the period_frac part
is used to handle frequencies which don't work out to
a nice whole number of nanoseconds per cycle.

2) I hear that SystemC uses "value plus a time unit", with
the smallest unit being a picosecond. (I think SystemC
also lets you specify the duty cycle, but we definitely
don't want to get into that!)

3) QEMUTimers are basically just nanosecond timers

4) The MAME emulator seems to work with periods of
96-bit attoseconds (represented internally by a
32-bit count of seconds plus a 64-bit count of
attoseconds). One attosecond is 1e-18 seconds.

Does anybody else have experience with other modelling
or emulator technology and how it represents clocks ?

I feel we should at least be able to represent clocks
with the same accuracy that ptimer has.

thanks
-- PMM

On 12/2/19 5:15 PM, Peter Maydell wrote:
> 
> The one topic I think we could do with discussing is whether
> a simple uint64_t giving the frequency of the clock in Hz is
> the right representation. In particular in your patch 9 the
> board has a clock frequency that's not a nice integer number
> of Hz. I think Philippe also mentioned on irc some board where
> the UART clock ends up at a weird frequency. Since the
> representation of the frequency is baked into the migration
> format it's going to be easier to get it right first rather
> than trying to change it later.
> 
> So what should the representation be? Some random thoughts:
> 
> 1) ptimer internally uses a 'period plus fraction' representation:
>  int64_t period is the integer part of the period in nanoseconds,
>  uint32_t period_frac is the fractional part of the period
> (if you like you can think of this as "96-bit integer
> period measured in units of one-2^32nd of a nanosecond").
> However its only public interfaces for setting the frequency
> are (a) set the frequency in Hz (uint32_t) or (b) set
> the period in nanoseconds (int64_t); the period_frac part
> is used to handle frequencies which don't work out to
> a nice whole number of nanoseconds per cycle.
> 
> 2) I hear that SystemC uses "value plus a time unit", with
> the smallest unit being a picosecond. (I think SystemC
> also lets you specify the duty cycle, but we definitely
> don't want to get into that!)

The "value" is internally stored in a 64bits unsigned integer.

> 
> 3) QEMUTimers are basically just nanosecond timers
> 
> 4) The MAME emulator seems to work with periods of
> 96-bit attoseconds (represented internally by a
> 32-bit count of seconds plus a 64-bit count of
> attoseconds). One attosecond is 1e-18 seconds.
> 
> Does anybody else have experience with other modelling
> or emulator technology and how it represents clocks ?

5) In linux, a clock rate is an "unsigned long" representing Hz.

> 
> I feel we should at least be able to represent clocks
> with the same accuracy that ptimer has.

Then is a maybe a good idea to store the period and not the frequency in
clocks so that we don't loose anything when we switch from a clock to a
ptimer ?

Regarding the clock, I don't see any strong obstacle to switch
internally to a period based value.
The only things we have to choose is how to represent a disabled clock.
Since putting a "0" period to a ptimer will disable the timer in
ptimer_reload(). We can choose that (and it's a good value because we
can multiply or divide it, it stays the same).

We could use the same representation as a ptimer. But if we don't keep a
C number representation, then computation of frequencies/periods will be
complicated at best and error prone.

From that point of view, if we could stick to a 64bits integer (or
floating point number) it would be great. Can we use a sub nanosecond
unit that fit our needs ?

I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
the unit of the ptimer fractional part ?) and if I'm not mistaken
+ we have a frequency range from ~0.2Hz up to 10^18Hz
+ the resolution is decreasing with the frequency (but at 100Mhz we have
a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
resolution). We hit 1Hz resolution around 2GHz.

So it sounds to me we have largely enough resolution to model clocks in
the range of frequencies we will have to handle. What do you think ?

--
Damien

On 12/4/19 5:40 PM, Damien Hedde wrote:
> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>
>> The one topic I think we could do with discussing is whether
>> a simple uint64_t giving the frequency of the clock in Hz is
>> the right representation. In particular in your patch 9 the
>> board has a clock frequency that's not a nice integer number
>> of Hz. I think Philippe also mentioned on irc some board where
>> the UART clock ends up at a weird frequency. Since the
>> representation of the frequency is baked into the migration
>> format it's going to be easier to get it right first rather
>> than trying to change it later.

Important precision for Damien, IIUC we can not migrate float/double types.

>> So what should the representation be? Some random thoughts:
>>
>> 1) ptimer internally uses a 'period plus fraction' representation:
>>   int64_t period is the integer part of the period in nanoseconds,
>>   uint32_t period_frac is the fractional part of the period
>> (if you like you can think of this as "96-bit integer
>> period measured in units of one-2^32nd of a nanosecond").
>> However its only public interfaces for setting the frequency
>> are (a) set the frequency in Hz (uint32_t) or (b) set
>> the period in nanoseconds (int64_t); the period_frac part
>> is used to handle frequencies which don't work out to
>> a nice whole number of nanoseconds per cycle.

This is very clear, thanks Peter!

The period+period_frac split allow us to migrate the 96 bits:

         VMSTATE_UINT32(period_frac, ptimer_state),
         VMSTATE_INT64(period, ptimer_state),

>> 2) I hear that SystemC uses "value plus a time unit", with
>> the smallest unit being a picosecond. (I think SystemC
>> also lets you specify the duty cycle, but we definitely
>> don't want to get into that!)
> 
> The "value" is internally stored in a 64bits unsigned integer.
> 
>>
>> 3) QEMUTimers are basically just nanosecond timers

Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:

#define SCALE_MS 1000000
#define SCALE_US 1000
#define SCALE_NS 1

>>
>> 4) The MAME emulator seems to work with periods of
>> 96-bit attoseconds (represented internally by a
>> 32-bit count of seconds plus a 64-bit count of
>> attoseconds). One attosecond is 1e-18 seconds.
>>
>> Does anybody else have experience with other modelling
>> or emulator technology and how it represents clocks ?
> 
> 5) In linux, a clock rate is an "unsigned long" representing Hz.
> 
>>
>> I feel we should at least be able to represent clocks
>> with the same accuracy that ptimer has.
> 
> Then is a maybe a good idea to store the period and not the frequency in
> clocks so that we don't loose anything when we switch from a clock to a
> ptimer ?

I think storing the period as an integer type is a good idea.

However if we store the period in nanoseconds, we get at most 1GHz 
frequency.

The attosecond granularity feels overkill.

If we use a 96-bit integer to store picoseconds and use similar SCALE 
macros we get to 1THz.

Regardless the unit chosen, as long it is integer, we can migrate it.
If can migrate the period, we don't need to migrate the frequency.
We can then use the float type in with the timer API to pass frequencies 
(which in the modeled hardware are ratios, likely not integers).

So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.

> Regarding the clock, I don't see any strong obstacle to switch
> internally to a period based value.
> The only things we have to choose is how to represent a disabled clock.
> Since putting a "0" period to a ptimer will disable the timer in
> ptimer_reload(). We can choose that (and it's a good value because we
> can multiply or divide it, it stays the same).
> 
> We could use the same representation as a ptimer. But if we don't keep a
> C number representation, then computation of frequencies/periods will be
> complicated at best and error prone.
> 
>  From that point of view, if we could stick to a 64bits integer (or
> floating point number) it would be great. Can we use a sub nanosecond
> unit that fit our needs ?
> 
> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
> the unit of the ptimer fractional part ?) and if I'm not mistaken
> + we have a frequency range from ~0.2Hz up to 10^18Hz
> + the resolution is decreasing with the frequency (but at 100Mhz we have
> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
> resolution). We hit 1Hz resolution around 2GHz.
> 
> So it sounds to me we have largely enough resolution to model clocks in
> the range of frequencies we will have to handle. What do you think ?

Back to your series, I wonder why you want to store the frequency in 
ClockIn. ClockIn shouldn't be aware at what frequency it is clocked. 
What matters is ClockOut, and each device exposing ClockOuts has a 
(migrated) state of the output frequencies (rather in fields, or encoded 
in registers). Once migrated, after the state is loaded back into the 
device, we call post_load(). Isn't it a good place to call 
clock_set_frequency(ClockOut[]) which will correctly set each ClockIn 
frequency.

IOW I don't think ClockIn/ClockOut require to migrate a frequency field.

On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
> On 12/4/19 5:40 PM, Damien Hedde wrote:
>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>
>>> The one topic I think we could do with discussing is whether
>>> a simple uint64_t giving the frequency of the clock in Hz is
>>> the right representation. In particular in your patch 9 the
>>> board has a clock frequency that's not a nice integer number
>>> of Hz. I think Philippe also mentioned on irc some board where
>>> the UART clock ends up at a weird frequency. Since the
>>> representation of the frequency is baked into the migration
>>> format it's going to be easier to get it right first rather
>>> than trying to change it later.
> 
> Important precision for Damien, IIUC we can not migrate float/double types.
> 
>>> So what should the representation be? Some random thoughts:
>>>
>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>>   int64_t period is the integer part of the period in nanoseconds,
>>>   uint32_t period_frac is the fractional part of the period
>>> (if you like you can think of this as "96-bit integer
>>> period measured in units of one-2^32nd of a nanosecond").
>>> However its only public interfaces for setting the frequency
>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>> the period in nanoseconds (int64_t); the period_frac part
>>> is used to handle frequencies which don't work out to
>>> a nice whole number of nanoseconds per cycle.
> 
> This is very clear, thanks Peter!
> 
> The period+period_frac split allow us to migrate the 96 bits:
> 
>         VMSTATE_UINT32(period_frac, ptimer_state),
>         VMSTATE_INT64(period, ptimer_state),
> 
>>> 2) I hear that SystemC uses "value plus a time unit", with
>>> the smallest unit being a picosecond. (I think SystemC
>>> also lets you specify the duty cycle, but we definitely
>>> don't want to get into that!)
>>
>> The "value" is internally stored in a 64bits unsigned integer.
>>
>>>
>>> 3) QEMUTimers are basically just nanosecond timers
> 
> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
> 
> #define SCALE_MS 1000000
> #define SCALE_US 1000
> #define SCALE_NS 1
> 
>>>
>>> 4) The MAME emulator seems to work with periods of
>>> 96-bit attoseconds (represented internally by a
>>> 32-bit count of seconds plus a 64-bit count of
>>> attoseconds). One attosecond is 1e-18 seconds.
>>>
>>> Does anybody else have experience with other modelling
>>> or emulator technology and how it represents clocks ?
>>
>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>
>>>
>>> I feel we should at least be able to represent clocks
>>> with the same accuracy that ptimer has.
>>
>> Then is a maybe a good idea to store the period and not the frequency in
>> clocks so that we don't loose anything when we switch from a clock to a
>> ptimer ?
> 
> I think storing the period as an integer type is a good idea.
> 
> However if we store the period in nanoseconds, we get at most 1GHz
> frequency.
> 
> The attosecond granularity feels overkill.
> 
> If we use a 96-bit integer to store picoseconds and use similar SCALE
> macros we get to 1THz.
> 
> Regardless the unit chosen, as long it is integer, we can migrate it.
> If can migrate the period, we don't need to migrate the frequency.
> We can then use the float type in with the timer API to pass frequencies
> (which in the modeled hardware are ratios, likely not integers).
> 
> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
> 
>> Regarding the clock, I don't see any strong obstacle to switch
>> internally to a period based value.
>> The only things we have to choose is how to represent a disabled clock.
>> Since putting a "0" period to a ptimer will disable the timer in
>> ptimer_reload(). We can choose that (and it's a good value because we
>> can multiply or divide it, it stays the same).
>>
>> We could use the same representation as a ptimer. But if we don't keep a
>> C number representation, then computation of frequencies/periods will be
>> complicated at best and error prone.
>>
>>  From that point of view, if we could stick to a 64bits integer (or
>> floating point number) it would be great. Can we use a sub nanosecond
>> unit that fit our needs ?
>>
>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>> resolution). We hit 1Hz resolution around 2GHz.
>>
>> So it sounds to me we have largely enough resolution to model clocks in
>> the range of frequencies we will have to handle. What do you think ?
> 
> Back to your series, I wonder why you want to store the frequency in
> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
> What matters is ClockOut, and each device exposing ClockOuts has a
> (migrated) state of the output frequencies (rather in fields, or encoded
> in registers). Once migrated, after the state is loaded back into the
> device, we call post_load(). Isn't it a good place to call
> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
> frequency.
> 
> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
> 

I agree it is more logical to store the frequency in clock out. But,
regarding migration constraints, we have no choice I think because a
device cannot rely on values that are migrated by another device for
restoring its state. (when I checked, I add the impression that
post_load()s are called on a per device migration basis not all at the
end of migration).

So we could store the frequency in clock out and migrate things there.
But since we have no way to ensure all clock out states are migrated
before some device fetch a ClockIn: we'll have to say "don't fetch one
of your ClockIn frequency during migration and migrate the value
yourself if you need it", pretty much like gpios.

So we will probably migrate all ClockOut and almost all ClockIn.

It would nice if we had a way to ensure clocks are migrated before
devices try to use them. But I don't think this is possible.

--
Damien

On 12/5/19 10:36 AM, Damien Hedde wrote:
> On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
>> On 12/4/19 5:40 PM, Damien Hedde wrote:
>>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>>
>>>> The one topic I think we could do with discussing is whether
>>>> a simple uint64_t giving the frequency of the clock in Hz is
>>>> the right representation. In particular in your patch 9 the
>>>> board has a clock frequency that's not a nice integer number
>>>> of Hz. I think Philippe also mentioned on irc some board where
>>>> the UART clock ends up at a weird frequency. Since the
>>>> representation of the frequency is baked into the migration
>>>> format it's going to be easier to get it right first rather
>>>> than trying to change it later.
>>
>> Important precision for Damien, IIUC we can not migrate float/double types.
>>
>>>> So what should the representation be? Some random thoughts:
>>>>
>>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>>>    int64_t period is the integer part of the period in nanoseconds,
>>>>    uint32_t period_frac is the fractional part of the period
>>>> (if you like you can think of this as "96-bit integer
>>>> period measured in units of one-2^32nd of a nanosecond").
>>>> However its only public interfaces for setting the frequency
>>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>>> the period in nanoseconds (int64_t); the period_frac part
>>>> is used to handle frequencies which don't work out to
>>>> a nice whole number of nanoseconds per cycle.
>>
>> This is very clear, thanks Peter!
>>
>> The period+period_frac split allow us to migrate the 96 bits:
>>
>>          VMSTATE_UINT32(period_frac, ptimer_state),
>>          VMSTATE_INT64(period, ptimer_state),
>>
>>>> 2) I hear that SystemC uses "value plus a time unit", with
>>>> the smallest unit being a picosecond. (I think SystemC
>>>> also lets you specify the duty cycle, but we definitely
>>>> don't want to get into that!)
>>>
>>> The "value" is internally stored in a 64bits unsigned integer.
>>>
>>>>
>>>> 3) QEMUTimers are basically just nanosecond timers
>>
>> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
>>
>> #define SCALE_MS 1000000
>> #define SCALE_US 1000
>> #define SCALE_NS 1
>>
>>>>
>>>> 4) The MAME emulator seems to work with periods of
>>>> 96-bit attoseconds (represented internally by a
>>>> 32-bit count of seconds plus a 64-bit count of
>>>> attoseconds). One attosecond is 1e-18 seconds.
>>>>
>>>> Does anybody else have experience with other modelling
>>>> or emulator technology and how it represents clocks ?
>>>
>>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>>
>>>>
>>>> I feel we should at least be able to represent clocks
>>>> with the same accuracy that ptimer has.
>>>
>>> Then is a maybe a good idea to store the period and not the frequency in
>>> clocks so that we don't loose anything when we switch from a clock to a
>>> ptimer ?
>>
>> I think storing the period as an integer type is a good idea.
>>
>> However if we store the period in nanoseconds, we get at most 1GHz
>> frequency.
>>
>> The attosecond granularity feels overkill.
>>
>> If we use a 96-bit integer to store picoseconds and use similar SCALE
>> macros we get to 1THz.
>>
>> Regardless the unit chosen, as long it is integer, we can migrate it.
>> If can migrate the period, we don't need to migrate the frequency.
>> We can then use the float type in with the timer API to pass frequencies
>> (which in the modeled hardware are ratios, likely not integers).
>>
>> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
>>
>>> Regarding the clock, I don't see any strong obstacle to switch
>>> internally to a period based value.
>>> The only things we have to choose is how to represent a disabled clock.
>>> Since putting a "0" period to a ptimer will disable the timer in
>>> ptimer_reload(). We can choose that (and it's a good value because we
>>> can multiply or divide it, it stays the same).
>>>
>>> We could use the same representation as a ptimer. But if we don't keep a
>>> C number representation, then computation of frequencies/periods will be
>>> complicated at best and error prone.
>>>
>>>   From that point of view, if we could stick to a 64bits integer (or
>>> floating point number) it would be great. Can we use a sub nanosecond
>>> unit that fit our needs ?
>>>
>>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>>> resolution). We hit 1Hz resolution around 2GHz.
>>>
>>> So it sounds to me we have largely enough resolution to model clocks in
>>> the range of frequencies we will have to handle. What do you think ?
>>
>> Back to your series, I wonder why you want to store the frequency in
>> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
>> What matters is ClockOut, and each device exposing ClockOuts has a
>> (migrated) state of the output frequencies (rather in fields, or encoded
>> in registers). Once migrated, after the state is loaded back into the
>> device, we call post_load(). Isn't it a good place to call
>> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
>> frequency.
>>
>> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
>>
> 
> I agree it is more logical to store the frequency in clock out. But,
> regarding migration constraints, we have no choice I think because a
> device cannot rely on values that are migrated by another device for
> restoring its state. (when I checked, I add the impression that
> post_load()s are called on a per device migration basis not all at the
> end of migration).

Cc'ing David to clear that out.

> So we could store the frequency in clock out and migrate things there.
> But since we have no way to ensure all clock out states are migrated
> before some device fetch a ClockIn: we'll have to say "don't fetch one
> of your ClockIn frequency during migration and migrate the value
> yourself if you need it", pretty much like gpios.
> 
> So we will probably migrate all ClockOut and almost all ClockIn.
> 
> It would nice if we had a way to ensure clocks are migrated before
> devices try to use them. But I don't think this is possible.
> 
> --
> Damien
> 

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> On 12/5/19 10:36 AM, Damien Hedde wrote:
> > On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
> > > On 12/4/19 5:40 PM, Damien Hedde wrote:
> > > > On 12/2/19 5:15 PM, Peter Maydell wrote:
> > > > > 
> > > > > The one topic I think we could do with discussing is whether
> > > > > a simple uint64_t giving the frequency of the clock in Hz is
> > > > > the right representation. In particular in your patch 9 the
> > > > > board has a clock frequency that's not a nice integer number
> > > > > of Hz. I think Philippe also mentioned on irc some board where
> > > > > the UART clock ends up at a weird frequency. Since the
> > > > > representation of the frequency is baked into the migration
> > > > > format it's going to be easier to get it right first rather
> > > > > than trying to change it later.
> > > 
> > > Important precision for Damien, IIUC we can not migrate float/double types.
> > > 
> > > > > So what should the representation be? Some random thoughts:
> > > > > 
> > > > > 1) ptimer internally uses a 'period plus fraction' representation:
> > > > >    int64_t period is the integer part of the period in nanoseconds,
> > > > >    uint32_t period_frac is the fractional part of the period
> > > > > (if you like you can think of this as "96-bit integer
> > > > > period measured in units of one-2^32nd of a nanosecond").
> > > > > However its only public interfaces for setting the frequency
> > > > > are (a) set the frequency in Hz (uint32_t) or (b) set
> > > > > the period in nanoseconds (int64_t); the period_frac part
> > > > > is used to handle frequencies which don't work out to
> > > > > a nice whole number of nanoseconds per cycle.
> > > 
> > > This is very clear, thanks Peter!
> > > 
> > > The period+period_frac split allow us to migrate the 96 bits:
> > > 
> > >          VMSTATE_UINT32(period_frac, ptimer_state),
> > >          VMSTATE_INT64(period, ptimer_state),
> > > 
> > > > > 2) I hear that SystemC uses "value plus a time unit", with
> > > > > the smallest unit being a picosecond. (I think SystemC
> > > > > also lets you specify the duty cycle, but we definitely
> > > > > don't want to get into that!)
> > > > 
> > > > The "value" is internally stored in a 64bits unsigned integer.
> > > > 
> > > > > 
> > > > > 3) QEMUTimers are basically just nanosecond timers
> > > 
> > > Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
> > > 
> > > #define SCALE_MS 1000000
> > > #define SCALE_US 1000
> > > #define SCALE_NS 1
> > > 
> > > > > 
> > > > > 4) The MAME emulator seems to work with periods of
> > > > > 96-bit attoseconds (represented internally by a
> > > > > 32-bit count of seconds plus a 64-bit count of
> > > > > attoseconds). One attosecond is 1e-18 seconds.
> > > > > 
> > > > > Does anybody else have experience with other modelling
> > > > > or emulator technology and how it represents clocks ?
> > > > 
> > > > 5) In linux, a clock rate is an "unsigned long" representing Hz.
> > > > 
> > > > > 
> > > > > I feel we should at least be able to represent clocks
> > > > > with the same accuracy that ptimer has.
> > > > 
> > > > Then is a maybe a good idea to store the period and not the frequency in
> > > > clocks so that we don't loose anything when we switch from a clock to a
> > > > ptimer ?
> > > 
> > > I think storing the period as an integer type is a good idea.
> > > 
> > > However if we store the period in nanoseconds, we get at most 1GHz
> > > frequency.
> > > 
> > > The attosecond granularity feels overkill.
> > > 
> > > If we use a 96-bit integer to store picoseconds and use similar SCALE
> > > macros we get to 1THz.
> > > 
> > > Regardless the unit chosen, as long it is integer, we can migrate it.
> > > If can migrate the period, we don't need to migrate the frequency.
> > > We can then use the float type in with the timer API to pass frequencies
> > > (which in the modeled hardware are ratios, likely not integers).
> > > 
> > > So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
> > > 
> > > > Regarding the clock, I don't see any strong obstacle to switch
> > > > internally to a period based value.
> > > > The only things we have to choose is how to represent a disabled clock.
> > > > Since putting a "0" period to a ptimer will disable the timer in
> > > > ptimer_reload(). We can choose that (and it's a good value because we
> > > > can multiply or divide it, it stays the same).
> > > > 
> > > > We could use the same representation as a ptimer. But if we don't keep a
> > > > C number representation, then computation of frequencies/periods will be
> > > > complicated at best and error prone.
> > > > 
> > > >   From that point of view, if we could stick to a 64bits integer (or
> > > > floating point number) it would be great. Can we use a sub nanosecond
> > > > unit that fit our needs ?
> > > > 
> > > > I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
> > > > the unit of the ptimer fractional part ?) and if I'm not mistaken
> > > > + we have a frequency range from ~0.2Hz up to 10^18Hz
> > > > + the resolution is decreasing with the frequency (but at 100Mhz we have
> > > > a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
> > > > resolution). We hit 1Hz resolution around 2GHz.
> > > > 
> > > > So it sounds to me we have largely enough resolution to model clocks in
> > > > the range of frequencies we will have to handle. What do you think ?
> > > 
> > > Back to your series, I wonder why you want to store the frequency in
> > > ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
> > > What matters is ClockOut, and each device exposing ClockOuts has a
> > > (migrated) state of the output frequencies (rather in fields, or encoded
> > > in registers). Once migrated, after the state is loaded back into the
> > > device, we call post_load(). Isn't it a good place to call
> > > clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
> > > frequency.
> > > 
> > > IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
> > > 
> > 
> > I agree it is more logical to store the frequency in clock out. But,
> > regarding migration constraints, we have no choice I think because a
> > device cannot rely on values that are migrated by another device for
> > restoring its state. (when I checked, I add the impression that
> > post_load()s are called on a per device migration basis not all at the
> > end of migration).
> 
> Cc'ing David to clear that out.


That's pretty much right; the 'post_load' is called on a structure at the end
of loading the data for that structure.

You can do things at the end of migration; one way is to register a
vm change state handler (search for qemu_add_vm_change_state_handler)
and that means you get a kick when the VM starts running or a timer set
in virtual time (not wall clock time because that becomes sensitive
to the speed of the host).

Somewhere ^^^ it says we can't migrate fp values; I'm not sure that's
true; we've got a VMSTATE_FLOAT64 macro but I don't see it's used
anywhere.

Dave

> > So we could store the frequency in clock out and migrate things there.
> > But since we have no way to ensure all clock out states are migrated
> > before some device fetch a ClockIn: we'll have to say "don't fetch one
> > of your ClockIn frequency during migration and migrate the value
> > yourself if you need it", pretty much like gpios.
> > 
> > So we will probably migrate all ClockOut and almost all ClockIn.
> > 
> > It would nice if we had a way to ensure clocks are migrated before
> > devices try to use them. But I don't think this is possible.
> > 
> > --
> > Damien
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 12/5/19 11:21 AM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> On 12/5/19 10:36 AM, Damien Hedde wrote:
>>> On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
>>>> On 12/4/19 5:40 PM, Damien Hedde wrote:
>>>>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>>>>
>>>>>> The one topic I think we could do with discussing is whether
>>>>>> a simple uint64_t giving the frequency of the clock in Hz is
>>>>>> the right representation. In particular in your patch 9 the
>>>>>> board has a clock frequency that's not a nice integer number
>>>>>> of Hz. I think Philippe also mentioned on irc some board where
>>>>>> the UART clock ends up at a weird frequency. Since the
>>>>>> representation of the frequency is baked into the migration
>>>>>> format it's going to be easier to get it right first rather
>>>>>> than trying to change it later.
>>>>
>>>> Important precision for Damien, IIUC we can not migrate float/double types.
>>>>
>>>>>> So what should the representation be? Some random thoughts:
>>>>>>
>>>>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>>>>>     int64_t period is the integer part of the period in nanoseconds,
>>>>>>     uint32_t period_frac is the fractional part of the period
>>>>>> (if you like you can think of this as "96-bit integer
>>>>>> period measured in units of one-2^32nd of a nanosecond").
>>>>>> However its only public interfaces for setting the frequency
>>>>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>>>>> the period in nanoseconds (int64_t); the period_frac part
>>>>>> is used to handle frequencies which don't work out to
>>>>>> a nice whole number of nanoseconds per cycle.
>>>>
>>>> This is very clear, thanks Peter!
>>>>
>>>> The period+period_frac split allow us to migrate the 96 bits:
>>>>
>>>>           VMSTATE_UINT32(period_frac, ptimer_state),
>>>>           VMSTATE_INT64(period, ptimer_state),
>>>>
>>>>>> 2) I hear that SystemC uses "value plus a time unit", with
>>>>>> the smallest unit being a picosecond. (I think SystemC
>>>>>> also lets you specify the duty cycle, but we definitely
>>>>>> don't want to get into that!)
>>>>>
>>>>> The "value" is internally stored in a 64bits unsigned integer.
>>>>>
>>>>>>
>>>>>> 3) QEMUTimers are basically just nanosecond timers
>>>>
>>>> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
>>>>
>>>> #define SCALE_MS 1000000
>>>> #define SCALE_US 1000
>>>> #define SCALE_NS 1
>>>>
>>>>>>
>>>>>> 4) The MAME emulator seems to work with periods of
>>>>>> 96-bit attoseconds (represented internally by a
>>>>>> 32-bit count of seconds plus a 64-bit count of
>>>>>> attoseconds). One attosecond is 1e-18 seconds.
>>>>>>
>>>>>> Does anybody else have experience with other modelling
>>>>>> or emulator technology and how it represents clocks ?
>>>>>
>>>>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>>>>
>>>>>>
>>>>>> I feel we should at least be able to represent clocks
>>>>>> with the same accuracy that ptimer has.
>>>>>
>>>>> Then is a maybe a good idea to store the period and not the frequency in
>>>>> clocks so that we don't loose anything when we switch from a clock to a
>>>>> ptimer ?
>>>>
>>>> I think storing the period as an integer type is a good idea.
>>>>
>>>> However if we store the period in nanoseconds, we get at most 1GHz
>>>> frequency.
>>>>
>>>> The attosecond granularity feels overkill.
>>>>
>>>> If we use a 96-bit integer to store picoseconds and use similar SCALE
>>>> macros we get to 1THz.
>>>>
>>>> Regardless the unit chosen, as long it is integer, we can migrate it.
>>>> If can migrate the period, we don't need to migrate the frequency.
>>>> We can then use the float type in with the timer API to pass frequencies
>>>> (which in the modeled hardware are ratios, likely not integers).
>>>>
>>>> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
>>>>
>>>>> Regarding the clock, I don't see any strong obstacle to switch
>>>>> internally to a period based value.
>>>>> The only things we have to choose is how to represent a disabled clock.
>>>>> Since putting a "0" period to a ptimer will disable the timer in
>>>>> ptimer_reload(). We can choose that (and it's a good value because we
>>>>> can multiply or divide it, it stays the same).
>>>>>
>>>>> We could use the same representation as a ptimer. But if we don't keep a
>>>>> C number representation, then computation of frequencies/periods will be
>>>>> complicated at best and error prone.
>>>>>
>>>>>    From that point of view, if we could stick to a 64bits integer (or
>>>>> floating point number) it would be great. Can we use a sub nanosecond
>>>>> unit that fit our needs ?
>>>>>
>>>>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>>>>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>>>>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>>>>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>>>>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>>>>> resolution). We hit 1Hz resolution around 2GHz.
>>>>>
>>>>> So it sounds to me we have largely enough resolution to model clocks in
>>>>> the range of frequencies we will have to handle. What do you think ?
>>>>
>>>> Back to your series, I wonder why you want to store the frequency in
>>>> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
>>>> What matters is ClockOut, and each device exposing ClockOuts has a
>>>> (migrated) state of the output frequencies (rather in fields, or encoded
>>>> in registers). Once migrated, after the state is loaded back into the
>>>> device, we call post_load(). Isn't it a good place to call
>>>> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
>>>> frequency.
>>>>
>>>> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
>>>>
>>>
>>> I agree it is more logical to store the frequency in clock out. But,
>>> regarding migration constraints, we have no choice I think because a
>>> device cannot rely on values that are migrated by another device for
>>> restoring its state. (when I checked, I add the impression that
>>> post_load()s are called on a per device migration basis not all at the
>>> end of migration).
>>
>> Cc'ing David to clear that out.
> 
> 
> That's pretty much right; the 'post_load' is called on a structure at the end
> of loading the data for that structure.
> 
> You can do things at the end of migration; one way is to register a
> vm change state handler (search for qemu_add_vm_change_state_handler)
> and that means you get a kick when the VM starts running or a timer set
> in virtual time (not wall clock time because that becomes sensitive
> to the speed of the host).
> 
> Somewhere ^^^ it says we can't migrate fp values; I'm not sure that's
> true; we've got a VMSTATE_FLOAT64 macro but I don't see it's used
> anywhere.

OK, Cc'ing Alex & Richard now, because I guess remember a discussion 
about "we can not migrate floats because this is a architectural 
implementation, so cross-architecture migration is likely to fail". But 
I can't find trace of a such discussion on the list or IRC logs.
Maybe this was instead about whether we can use the host FPU registers.

I hope I'm wrong and confuse, this is a great news for me to know we
can migrate floats!

> Dave
> 
>>> So we could store the frequency in clock out and migrate things there.
>>> But since we have no way to ensure all clock out states are migrated
>>> before some device fetch a ClockIn: we'll have to say "don't fetch one
>>> of your ClockIn frequency during migration and migrate the value
>>> yourself if you need it", pretty much like gpios.
>>>
>>> So we will probably migrate all ClockOut and almost all ClockIn.
>>>
>>> It would nice if we had a way to ensure clocks are migrated before
>>> devices try to use them. But I don't think this is possible.
>>>
>>> --
>>> Damien
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> On 12/5/19 11:21 AM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> > > On 12/5/19 10:36 AM, Damien Hedde wrote:
> > > > On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
> > > > > On 12/4/19 5:40 PM, Damien Hedde wrote:
> > > > > > On 12/2/19 5:15 PM, Peter Maydell wrote:
> > > > > > > 
> > > > > > > The one topic I think we could do with discussing is whether
> > > > > > > a simple uint64_t giving the frequency of the clock in Hz is
> > > > > > > the right representation. In particular in your patch 9 the
> > > > > > > board has a clock frequency that's not a nice integer number
> > > > > > > of Hz. I think Philippe also mentioned on irc some board where
> > > > > > > the UART clock ends up at a weird frequency. Since the
> > > > > > > representation of the frequency is baked into the migration
> > > > > > > format it's going to be easier to get it right first rather
> > > > > > > than trying to change it later.
> > > > > 
> > > > > Important precision for Damien, IIUC we can not migrate float/double types.
> > > > > 
> > > > > > > So what should the representation be? Some random thoughts:
> > > > > > > 
> > > > > > > 1) ptimer internally uses a 'period plus fraction' representation:
> > > > > > >     int64_t period is the integer part of the period in nanoseconds,
> > > > > > >     uint32_t period_frac is the fractional part of the period
> > > > > > > (if you like you can think of this as "96-bit integer
> > > > > > > period measured in units of one-2^32nd of a nanosecond").
> > > > > > > However its only public interfaces for setting the frequency
> > > > > > > are (a) set the frequency in Hz (uint32_t) or (b) set
> > > > > > > the period in nanoseconds (int64_t); the period_frac part
> > > > > > > is used to handle frequencies which don't work out to
> > > > > > > a nice whole number of nanoseconds per cycle.
> > > > > 
> > > > > This is very clear, thanks Peter!
> > > > > 
> > > > > The period+period_frac split allow us to migrate the 96 bits:
> > > > > 
> > > > >           VMSTATE_UINT32(period_frac, ptimer_state),
> > > > >           VMSTATE_INT64(period, ptimer_state),
> > > > > 
> > > > > > > 2) I hear that SystemC uses "value plus a time unit", with
> > > > > > > the smallest unit being a picosecond. (I think SystemC
> > > > > > > also lets you specify the duty cycle, but we definitely
> > > > > > > don't want to get into that!)
> > > > > > 
> > > > > > The "value" is internally stored in a 64bits unsigned integer.
> > > > > > 
> > > > > > > 
> > > > > > > 3) QEMUTimers are basically just nanosecond timers
> > > > > 
> > > > > Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
> > > > > 
> > > > > #define SCALE_MS 1000000
> > > > > #define SCALE_US 1000
> > > > > #define SCALE_NS 1
> > > > > 
> > > > > > > 
> > > > > > > 4) The MAME emulator seems to work with periods of
> > > > > > > 96-bit attoseconds (represented internally by a
> > > > > > > 32-bit count of seconds plus a 64-bit count of
> > > > > > > attoseconds). One attosecond is 1e-18 seconds.
> > > > > > > 
> > > > > > > Does anybody else have experience with other modelling
> > > > > > > or emulator technology and how it represents clocks ?
> > > > > > 
> > > > > > 5) In linux, a clock rate is an "unsigned long" representing Hz.
> > > > > > 
> > > > > > > 
> > > > > > > I feel we should at least be able to represent clocks
> > > > > > > with the same accuracy that ptimer has.
> > > > > > 
> > > > > > Then is a maybe a good idea to store the period and not the frequency in
> > > > > > clocks so that we don't loose anything when we switch from a clock to a
> > > > > > ptimer ?
> > > > > 
> > > > > I think storing the period as an integer type is a good idea.
> > > > > 
> > > > > However if we store the period in nanoseconds, we get at most 1GHz
> > > > > frequency.
> > > > > 
> > > > > The attosecond granularity feels overkill.
> > > > > 
> > > > > If we use a 96-bit integer to store picoseconds and use similar SCALE
> > > > > macros we get to 1THz.
> > > > > 
> > > > > Regardless the unit chosen, as long it is integer, we can migrate it.
> > > > > If can migrate the period, we don't need to migrate the frequency.
> > > > > We can then use the float type in with the timer API to pass frequencies
> > > > > (which in the modeled hardware are ratios, likely not integers).
> > > > > 
> > > > > So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
> > > > > 
> > > > > > Regarding the clock, I don't see any strong obstacle to switch
> > > > > > internally to a period based value.
> > > > > > The only things we have to choose is how to represent a disabled clock.
> > > > > > Since putting a "0" period to a ptimer will disable the timer in
> > > > > > ptimer_reload(). We can choose that (and it's a good value because we
> > > > > > can multiply or divide it, it stays the same).
> > > > > > 
> > > > > > We could use the same representation as a ptimer. But if we don't keep a
> > > > > > C number representation, then computation of frequencies/periods will be
> > > > > > complicated at best and error prone.
> > > > > > 
> > > > > >    From that point of view, if we could stick to a 64bits integer (or
> > > > > > floating point number) it would be great. Can we use a sub nanosecond
> > > > > > unit that fit our needs ?
> > > > > > 
> > > > > > I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
> > > > > > the unit of the ptimer fractional part ?) and if I'm not mistaken
> > > > > > + we have a frequency range from ~0.2Hz up to 10^18Hz
> > > > > > + the resolution is decreasing with the frequency (but at 100Mhz we have
> > > > > > a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
> > > > > > resolution). We hit 1Hz resolution around 2GHz.
> > > > > > 
> > > > > > So it sounds to me we have largely enough resolution to model clocks in
> > > > > > the range of frequencies we will have to handle. What do you think ?
> > > > > 
> > > > > Back to your series, I wonder why you want to store the frequency in
> > > > > ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
> > > > > What matters is ClockOut, and each device exposing ClockOuts has a
> > > > > (migrated) state of the output frequencies (rather in fields, or encoded
> > > > > in registers). Once migrated, after the state is loaded back into the
> > > > > device, we call post_load(). Isn't it a good place to call
> > > > > clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
> > > > > frequency.
> > > > > 
> > > > > IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
> > > > > 
> > > > 
> > > > I agree it is more logical to store the frequency in clock out. But,
> > > > regarding migration constraints, we have no choice I think because a
> > > > device cannot rely on values that are migrated by another device for
> > > > restoring its state. (when I checked, I add the impression that
> > > > post_load()s are called on a per device migration basis not all at the
> > > > end of migration).
> > > 
> > > Cc'ing David to clear that out.
> > 
> > 
> > That's pretty much right; the 'post_load' is called on a structure at the end
> > of loading the data for that structure.
> > 
> > You can do things at the end of migration; one way is to register a
> > vm change state handler (search for qemu_add_vm_change_state_handler)
> > and that means you get a kick when the VM starts running or a timer set
> > in virtual time (not wall clock time because that becomes sensitive
> > to the speed of the host).
> > 
> > Somewhere ^^^ it says we can't migrate fp values; I'm not sure that's
> > true; we've got a VMSTATE_FLOAT64 macro but I don't see it's used
> > anywhere.
> 
> OK, Cc'ing Alex & Richard now, because I guess remember a discussion about
> "we can not migrate floats because this is a architectural implementation,
> so cross-architecture migration is likely to fail". But I can't find trace
> of a such discussion on the list or IRC logs.
> Maybe this was instead about whether we can use the host FPU registers.

We have to be careful when migrating the FP registers within a CPU,
since they can have crazy values that are not valid/weird corners of
standard FP encodings (e.g. if the guest just uses the FP registers as
a spare 64bit register - which is perfectly valid on some
architectures). However, migrating an actual floating point
real world measurement should be fine.  I'm assuming we can rely on
64 bit IEEE FP format on the wire being portable.

Dave

> I hope I'm wrong and confuse, this is a great news for me to know we
> can migrate floats!
> 
> > Dave
> > 
> > > > So we could store the frequency in clock out and migrate things there.
> > > > But since we have no way to ensure all clock out states are migrated
> > > > before some device fetch a ClockIn: we'll have to say "don't fetch one
> > > > of your ClockIn frequency during migration and migrate the value
> > > > yourself if you need it", pretty much like gpios.
> > > > 
> > > > So we will probably migrate all ClockOut and almost all ClockIn.
> > > > 
> > > > It would nice if we had a way to ensure clocks are migrated before
> > > > devices try to use them. But I don't think this is possible.
> > > > 
> > > > --
> > > > Damien
> > > > 
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 12/5/19 11:56 AM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> On 12/5/19 11:21 AM, Dr. David Alan Gilbert wrote:
>>> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>>>> On 12/5/19 10:36 AM, Damien Hedde wrote:
>>>>> On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
>>>>>> On 12/4/19 5:40 PM, Damien Hedde wrote:
>>>>>>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>>>>>>
>>>>>>>> The one topic I think we could do with discussing is whether
>>>>>>>> a simple uint64_t giving the frequency of the clock in Hz is
>>>>>>>> the right representation. In particular in your patch 9 the
>>>>>>>> board has a clock frequency that's not a nice integer number
>>>>>>>> of Hz. I think Philippe also mentioned on irc some board where
>>>>>>>> the UART clock ends up at a weird frequency. Since the
>>>>>>>> representation of the frequency is baked into the migration
>>>>>>>> format it's going to be easier to get it right first rather
>>>>>>>> than trying to change it later.
>>>>>>
>>>>>> Important precision for Damien, IIUC we can not migrate float/double types.
>>>>>>
>>>>>>>> So what should the representation be? Some random thoughts:
>>>>>>>>
>>>>>>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>>>>>>>      int64_t period is the integer part of the period in nanoseconds,
>>>>>>>>      uint32_t period_frac is the fractional part of the period
>>>>>>>> (if you like you can think of this as "96-bit integer
>>>>>>>> period measured in units of one-2^32nd of a nanosecond").
>>>>>>>> However its only public interfaces for setting the frequency
>>>>>>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>>>>>>> the period in nanoseconds (int64_t); the period_frac part
>>>>>>>> is used to handle frequencies which don't work out to
>>>>>>>> a nice whole number of nanoseconds per cycle.
>>>>>>
>>>>>> This is very clear, thanks Peter!
>>>>>>
>>>>>> The period+period_frac split allow us to migrate the 96 bits:
>>>>>>
>>>>>>            VMSTATE_UINT32(period_frac, ptimer_state),
>>>>>>            VMSTATE_INT64(period, ptimer_state),
>>>>>>
>>>>>>>> 2) I hear that SystemC uses "value plus a time unit", with
>>>>>>>> the smallest unit being a picosecond. (I think SystemC
>>>>>>>> also lets you specify the duty cycle, but we definitely
>>>>>>>> don't want to get into that!)
>>>>>>>
>>>>>>> The "value" is internally stored in a 64bits unsigned integer.
>>>>>>>
>>>>>>>>
>>>>>>>> 3) QEMUTimers are basically just nanosecond timers
>>>>>>
>>>>>> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
>>>>>>
>>>>>> #define SCALE_MS 1000000
>>>>>> #define SCALE_US 1000
>>>>>> #define SCALE_NS 1
>>>>>>
>>>>>>>>
>>>>>>>> 4) The MAME emulator seems to work with periods of
>>>>>>>> 96-bit attoseconds (represented internally by a
>>>>>>>> 32-bit count of seconds plus a 64-bit count of
>>>>>>>> attoseconds). One attosecond is 1e-18 seconds.
>>>>>>>>
>>>>>>>> Does anybody else have experience with other modelling
>>>>>>>> or emulator technology and how it represents clocks ?
>>>>>>>
>>>>>>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>>>>>>
>>>>>>>>
>>>>>>>> I feel we should at least be able to represent clocks
>>>>>>>> with the same accuracy that ptimer has.
>>>>>>>
>>>>>>> Then is a maybe a good idea to store the period and not the frequency in
>>>>>>> clocks so that we don't loose anything when we switch from a clock to a
>>>>>>> ptimer ?
>>>>>>
>>>>>> I think storing the period as an integer type is a good idea.
>>>>>>
>>>>>> However if we store the period in nanoseconds, we get at most 1GHz
>>>>>> frequency.
>>>>>>
>>>>>> The attosecond granularity feels overkill.
>>>>>>
>>>>>> If we use a 96-bit integer to store picoseconds and use similar SCALE
>>>>>> macros we get to 1THz.
>>>>>>
>>>>>> Regardless the unit chosen, as long it is integer, we can migrate it.
>>>>>> If can migrate the period, we don't need to migrate the frequency.
>>>>>> We can then use the float type in with the timer API to pass frequencies
>>>>>> (which in the modeled hardware are ratios, likely not integers).
>>>>>>
>>>>>> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
>>>>>>
>>>>>>> Regarding the clock, I don't see any strong obstacle to switch
>>>>>>> internally to a period based value.
>>>>>>> The only things we have to choose is how to represent a disabled clock.
>>>>>>> Since putting a "0" period to a ptimer will disable the timer in
>>>>>>> ptimer_reload(). We can choose that (and it's a good value because we
>>>>>>> can multiply or divide it, it stays the same).
>>>>>>>
>>>>>>> We could use the same representation as a ptimer. But if we don't keep a
>>>>>>> C number representation, then computation of frequencies/periods will be
>>>>>>> complicated at best and error prone.
>>>>>>>
>>>>>>>     From that point of view, if we could stick to a 64bits integer (or
>>>>>>> floating point number) it would be great. Can we use a sub nanosecond
>>>>>>> unit that fit our needs ?
>>>>>>>
>>>>>>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>>>>>>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>>>>>>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>>>>>>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>>>>>>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>>>>>>> resolution). We hit 1Hz resolution around 2GHz.
>>>>>>>
>>>>>>> So it sounds to me we have largely enough resolution to model clocks in
>>>>>>> the range of frequencies we will have to handle. What do you think ?
>>>>>>
>>>>>> Back to your series, I wonder why you want to store the frequency in
>>>>>> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
>>>>>> What matters is ClockOut, and each device exposing ClockOuts has a
>>>>>> (migrated) state of the output frequencies (rather in fields, or encoded
>>>>>> in registers). Once migrated, after the state is loaded back into the
>>>>>> device, we call post_load(). Isn't it a good place to call
>>>>>> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
>>>>>> frequency.
>>>>>>
>>>>>> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
>>>>>>
>>>>>
>>>>> I agree it is more logical to store the frequency in clock out. But,
>>>>> regarding migration constraints, we have no choice I think because a
>>>>> device cannot rely on values that are migrated by another device for
>>>>> restoring its state. (when I checked, I add the impression that
>>>>> post_load()s are called on a per device migration basis not all at the
>>>>> end of migration).
>>>>
>>>> Cc'ing David to clear that out.
>>>
>>>
>>> That's pretty much right; the 'post_load' is called on a structure at the end
>>> of loading the data for that structure.
>>>
>>> You can do things at the end of migration; one way is to register a
>>> vm change state handler (search for qemu_add_vm_change_state_handler)
>>> and that means you get a kick when the VM starts running or a timer set
>>> in virtual time (not wall clock time because that becomes sensitive
>>> to the speed of the host).
>>>
>>> Somewhere ^^^ it says we can't migrate fp values; I'm not sure that's
>>> true; we've got a VMSTATE_FLOAT64 macro but I don't see it's used
>>> anywhere.
>>
>> OK, Cc'ing Alex & Richard now, because I guess remember a discussion about
>> "we can not migrate floats because this is a architectural implementation,
>> so cross-architecture migration is likely to fail". But I can't find trace
>> of a such discussion on the list or IRC logs.
>> Maybe this was instead about whether we can use the host FPU registers.
> 
> We have to be careful when migrating the FP registers within a CPU,
> since they can have crazy values that are not valid/weird corners of
> standard FP encodings (e.g. if the guest just uses the FP registers as
> a spare 64bit register - which is perfectly valid on some
> architectures). However, migrating an actual floating point
> real world measurement should be fine.  I'm assuming we can rely on
> 64 bit IEEE FP format on the wire being portable.

Understood, thanks for clearing this out!

Side note, we don't do cross-arch migration testing, but we talked about 
having a 'different QEMU version' migration test. When we get a such 
test setup, it shouldn't be too difficult to evolve to some cross-arch 
migration test.

>> I hope I'm wrong and confuse, this is a great news for me to know we
>> can migrate floats!
>>
>>> Dave
>>>
>>>>> So we could store the frequency in clock out and migrate things there.
>>>>> But since we have no way to ensure all clock out states are migrated
>>>>> before some device fetch a ClockIn: we'll have to say "don't fetch one
>>>>> of your ClockIn frequency during migration and migrate the value
>>>>> yourself if you need it", pretty much like gpios.
>>>>>
>>>>> So we will probably migrate all ClockOut and almost all ClockIn.
>>>>>
>>>>> It would nice if we had a way to ensure clocks are migrated before
>>>>> devices try to use them. But I don't think this is possible.
>>>>>
>>>>> --
>>>>> Damien
>>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

On Thu, Dec 05, 2019 at 12:01:56PM +0100, Philippe Mathieu-Daudé wrote:
> 
> Understood, thanks for clearing this out!
> 
> Side note, we don't do cross-arch migration testing, but we talked about
> having a 'different QEMU version' migration test. When we get a such test
> setup, it shouldn't be too difficult to evolve to some cross-arch migration
> test.
>

+Oksana,

Avocado-VT has had this as a core concept as far as I can remember, and
it's exposed even as a command line argument:

   avocado run --help
   ...
   [--vt-qemu-dst-bin VT_DST_QEMU_BIN]
   ...

Oksana is currently working on new migration test cases, and may consider
looking into adding "different QEMU version" support too.

PS: I have to say, though, that I'm trying to get my mind around
cross-arch migration being real.

- Cleber.

> > > I hope I'm wrong and confuse, this is a great news for me to know we
> > > can migrate floats!
> > > 
> > > > Dave
> > > > 
> > > > > > So we could store the frequency in clock out and migrate things there.
> > > > > > But since we have no way to ensure all clock out states are migrated
> > > > > > before some device fetch a ClockIn: we'll have to say "don't fetch one
> > > > > > of your ClockIn frequency during migration and migrate the value
> > > > > > yourself if you need it", pretty much like gpios.
> > > > > > 
> > > > > > So we will probably migrate all ClockOut and almost all ClockIn.
> > > > > > 
> > > > > > It would nice if we had a way to ensure clocks are migrated before
> > > > > > devices try to use them. But I don't think this is possible.
> > > > > > 
> > > > > > --
> > > > > > Damien
> > > > > > 
> > > > > 
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > > 
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 

* Cleber Rosa (crosa@redhat.com) wrote:
> On Thu, Dec 05, 2019 at 12:01:56PM +0100, Philippe Mathieu-Daudé wrote:
> > 
> > Understood, thanks for clearing this out!
> > 
> > Side note, we don't do cross-arch migration testing, but we talked about
> > having a 'different QEMU version' migration test. When we get a such test
> > setup, it shouldn't be too difficult to evolve to some cross-arch migration
> > test.
> >
> 
> +Oksana,
> 
> Avocado-VT has had this as a core concept as far as I can remember, and
> it's exposed even as a command line argument:
> 
>    avocado run --help
>    ...
>    [--vt-qemu-dst-bin VT_DST_QEMU_BIN]
>    ...
> 

Yeh, I've run that in the past - it works OK.

Dave

> Oksana is currently working on new migration test cases, and may consider
> looking into adding "different QEMU version" support too.
> 
> PS: I have to say, though, that I'm trying to get my mind around
> cross-arch migration being real.
> 
> - Cleber.
> 
> > > > I hope I'm wrong and confuse, this is a great news for me to know we
> > > > can migrate floats!
> > > > 
> > > > > Dave
> > > > > 
> > > > > > > So we could store the frequency in clock out and migrate things there.
> > > > > > > But since we have no way to ensure all clock out states are migrated
> > > > > > > before some device fetch a ClockIn: we'll have to say "don't fetch one
> > > > > > > of your ClockIn frequency during migration and migrate the value
> > > > > > > yourself if you need it", pretty much like gpios.
> > > > > > > 
> > > > > > > So we will probably migrate all ClockOut and almost all ClockIn.
> > > > > > > 
> > > > > > > It would nice if we had a way to ensure clocks are migrated before
> > > > > > > devices try to use them. But I don't think this is possible.
> > > > > > > 
> > > > > > > --
> > > > > > > Damien
> > > > > > > 
> > > > > > 
> > > > > --
> > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > > > 
> > > > 
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > 
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK