Re: [PATCH v6 0/9] Clock framework API

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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