Re: [Qemu-devel] [PATCH 4/7] RTC: Update the RTC clock only when reading it

[Paolo Bonzini]

Il 23/07/2012 07:17, Juan Quintela ha scritto:
> Paolo Bonzini <address@hidden> wrote:
>> From: "Zhang, Yang Z" <address@hidden>
>>
>> Calculate guest RTC based on the time of the last update, instead of
>> using timers.  The formula is
>>
>>     (base_rtc + guest_time_now - guest_time_last_update + offset)
>>
>> Base_rtc is the RTC value when the RTC was last updated.
>> Guest_time_now is the guest time when the access happens.
>> Guest_time_last_update was the guest time when the RTC was last updated.
>> Offset is used when divider reset happens or the set bit is toggled.
>>
>> The timer is kept in order to signal interrupts, but it only needs to
>> run when either UF or AF is cleared.  When the bits are both set, the
>> timer does not run.
>>
>> UIP is now synthesized when reading register A.  If the timer is not set,
>> or if there is more than one second before it (as is the case at the
>> end of this series), the leading edge of UIP is computed and the rising
>> edge occurs 220us later.  If the update timer occurs within one second,
>> however, the rising edge of the AF and UF bits should coincide withe
>> the falling edge of UIP.  We do not know exactly when this will happen
>> because there could be delays in the servicing of the timer.  Hence, in
>> this case reading register A only computes for the rising edge of UIP,
>> and latches the bit until the timer is fired and clears it.
>>
>> Signed-off-by: Yang Zhang <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> @@ -540,11 +593,12 @@ static const VMStateDescription vmstate_rtc = {
>>          VMSTATE_INT32(current_tm.tm_mday, RTCState),
>>          VMSTATE_INT32(current_tm.tm_mon, RTCState),
>>          VMSTATE_INT32(current_tm.tm_year, RTCState),
>> +        VMSTATE_UINT64(base_rtc, RTCState),
>> +        VMSTATE_UINT64(last_update, RTCState),
>> +        VMSTATE_INT64(offset, RTCState),
>            VMSTATE_UINT64_V(base_rtc, RTCState, 3)
> same ofr the others.
> 
> Normally, new fields are added at the end of the structure.

Doesn't really matter if you bump the minimum version but you're right
that it is more correct.

> 
>>          VMSTATE_TIMER(periodic_timer, RTCState),
>>          VMSTATE_INT64(next_periodic_time, RTCState),
>> -        VMSTATE_INT64(next_second_time, RTCState),
>> -        VMSTATE_TIMER(second_timer, RTCState),
>> -        VMSTATE_TIMER(second_timer2, RTCState),
>> +        VMSTATE_TIMER(update_timer, RTCState),
> 
> I have to read the rest of the patch to know what is the relation of
> this 4 fields, to see if there is any way to create this in any sane
> way that is compatible.

next_second_time is computed like this (see check_update_timer):

    guest_nsec = get_guest_rtc_ns(s) % NSEC_PER_SEC;
    next_update_time = qemu_get_clock_ns(rtc_clock)
        + NSEC_PER_SEC - guest_nsec;

One of second_timer and second_timer2 is unset, depending on whether
UIP=1 or UIP=0 respectively.

second_timer if set is next_second_time.

second_timer2 if set is next_second_time + get_ticks_per_sec/100.

> The new fields can go in a different subsection.

But then you would transmit the subsection always, and old QEMU doesn't
know how to skip it.  So it doesn't really help.

This is because base_rtc, last_update and offset need to be transmitted
always.  next_alarm_time only if no alarm fired already, but the common
case is yes.  Only for update_timer we can hack and transmit it in the
slot that was used for next_second_time.  Unless you want to transmit
the subsection only for the new machine version, which is never done in
the QEMU tree.

Paolo