Re: [PATCH] hw/rtc/pl031: Send RTC_CHANGE QMP event

[Eric Auger]

Hi Peter,

On 9/16/21 8:24 PM, Peter Maydell wrote:
> On Thu, 16 Sept 2021 at 18:19, Eric Auger <eric.auger@redhat.com> wrote:
>> Hi Peter,
>> On 9/16/21 3:32 PM, Peter Maydell wrote:
>>> None of the other users of qapi_event_send_rtc_change()
>>> seem to have to track the baseline time like this. Shouldn't
>>> this be doing something involving using qemu_ref_timedate()
>>> as the baseline that it uses to figure out the change value ?
>>> (The other users do this via qemu_timedate_diff() but since we
>>> start with a value-in-seconds we might want to expose some other
>>> API in softmmu/rtc.c.)
>> I struggled understanding the various kinds of clocks modeled in qemu
>> and the PL031 implementation. Both devices calling
>> qapi_event_send_rtc_change() seem to store the base rtc in their state
>> (mc146818rtc.c cmos data or spapr_rtc tas_ld(args, )) and then
>> effectivelly call qemu_timedate_diff() on this base rtc value. I did not
>> figure to do the equivalent with the pl031. I will further investigate
>> how I can mimic their implementation though.
> mc146818rtc.c calls qapi_event_send_rtc_change() in rtc_set_time().
> It looks to me like that is just "when the guest writes to an
> RTC register such that the guest RTC time has changed, use
> qemu_timedate_diff() to find out the delta between that and what
> the softmmu/rtc.c clock has as its baseline time, and then pass
> that delta in seconds to qapi_event_send_rtc_change()".
> This RTC has a lot of separate day/month/year registers, so the
> implementation's idea of "current guest RTC setting" is a
> complicated thing that it fills in into a struct tm, and which
> qemu_timedate_diff() then parses back out into a "seconds" value.
>
> spapr_rtc() is a hypercall implementation, so the guest passes it
> a complete set of year/month/day/etc values all at once to set the time.
>
> pl031 is a lot simpler as it is essentially just a count of
> time in seconds, which we set up as "seconds since the Unix epoch".
> But the basic principle is the same: the device contains the state
> that tells you what it thinks the guest RTC value is now, and the
> value we want to pass qapi_event_send_rtc_change() is the
> difference between that and the reference time kept in
> softmmu/rtc.c.
>
> I think what you want is probably:
>
>     struct tm tm;
>
>     qemu_get_timedate(&tm, s->tick_offset);
>     qapi_event_send_rtc_change(qemu_timedate_diff(&tm));

Thank you for the explanation. It works on my end and indeed looks
similar to what other rtc do.

I will respin accordingly adding your Sob.

Thanks

Eric
>
> But I'm not 100% sure. I also feel like this is doing a
> roundtrip from seconds to struct tm to seconds again, which
> seems a bit silly (but it might matter if the rtc is configured
> to 'localtime'? At any rate it's not completely clear that
> it's always a no-op roundtrip).
>
> I've cc'd Paolo who might be a bit more familiar with the rtc code
> than I am.
>
> -- PMM
>