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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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
>