[Qemu-devel] pl031 time across vm save/reload

[Qemu-devel] pl031 time across vm save/reload

[Peter Maydell]

I've had a report that the way the PL031 model handles time
across a vm save/reload fails to correctly advance the guest
RTC when the host RTC has advanced between the save and reload.
I looked at the code and my correspondent's analysis (which
I quote below, lightly edited) looks correct to me, but I'm not
entirely sure how our RTC stuff is supposed to work. Paolo,
you wrote this (way back in commit b0f26631bc5179006) -- any opinions?

In the pl031 RTC device. the current time is given by:

    int64_t now = qemu_clock_get_ns(rtc_clock);
    return s->tick_offset + now / NANOSECONDS_PER_SECOND;

On save we do:

    /* tick_offset is base_time - rtc_clock base time.  Instead, we want to
     * store the base time relative to the QEMU_CLOCK_VIRTUAL for
backwards-compatibility.  */
    int64_t delta = qemu_clock_get_ns(rtc_clock) -
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
    s->tick_offset_vmstate = s->tick_offset + delta / NANOSECONDS_PER_SECOND;

On restore:

    int64_t delta = qemu_clock_get_ns(rtc_clock) -
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
    s->tick_offset = s->tick_offset_vmstate - delta / NANOSECONDS_PER_SECOND;

So, no matter what is requested, if "qemu_clock_get_ns(rtc_clock)"
increases (eg, because host time increased), then tick_offset
reduces, which makes time follow QEMU_CLOCK_VIRTUAL no matter what
was requested on qemu's command line.

(That is, because we migrate "offset relative to CLOCK_VIRTUAL"
and CLOCK_VIRTUAL does not advance when the VM is stopped,
we don't get the right behaviour of "offset is relative to
the new CLOCK_RTC, which might have advanced".).

thanks
-- PMM

Re: [Qemu-devel] pl031 time across vm save/reload

[Paolo Bonzini]

On 04/07/19 18:02, Peter Maydell wrote:
> I've had a report that the way the PL031 model handles time
> across a vm save/reload fails to correctly advance the guest
> RTC when the host RTC has advanced between the save and reload.
> I looked at the code and my correspondent's analysis (which
> I quote below, lightly edited) looks correct to me, but I'm not
> entirely sure how our RTC stuff is supposed to work. Paolo,
> you wrote this (way back in commit b0f26631bc5179006) -- any opinions?
> 
> In the pl031 RTC device. the current time is given by:
> 
>     int64_t now = qemu_clock_get_ns(rtc_clock);
>     return s->tick_offset + now / NANOSECONDS_PER_SECOND;
> 
> On save we do:
> 
>     /* tick_offset is base_time - rtc_clock base time.  Instead, we want to
>      * store the base time relative to the QEMU_CLOCK_VIRTUAL for
> backwards-compatibility.  */
>     int64_t delta = qemu_clock_get_ns(rtc_clock) -
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>     s->tick_offset_vmstate = s->tick_offset + delta / NANOSECONDS_PER_SECOND;
> 
> On restore:
> 
>     int64_t delta = qemu_clock_get_ns(rtc_clock) -
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>     s->tick_offset = s->tick_offset_vmstate - delta / NANOSECONDS_PER_SECOND;
> 
> So, no matter what is requested, if "qemu_clock_get_ns(rtc_clock)"
> increases (eg, because host time increased), then tick_offset
> reduces, which makes time follow QEMU_CLOCK_VIRTUAL no matter what
> was requested on qemu's command line.
> 
> (That is, because we migrate "offset relative to CLOCK_VIRTUAL"
> and CLOCK_VIRTUAL does not advance when the VM is stopped,
> we don't get the right behaviour of "offset is relative to
> the new CLOCK_RTC, which might have advanced".).

You're right, the compatibility causes wrong behavior for the default
-rtc settings (the RC pauses across migration).  The right thing to do
would be to store the base rather than the offset: that is, you store
the time at which LR was written.  Then the offset is s->lr - s->base
and it's independent of the machine on which the rtc_clock is being read.

By the way, the data sheet says "the counter and match values are
compared in a comparator. When both values are equal, the RTCINTR
interrupt is asserted HIGH"; QEMU compares the RTC value (read from
RTC_DR) and not the counter value, but Linux code seems to expect QEMU's
behavior.

Paolo

Re: [Qemu-devel] pl031 time across vm save/reload

[Peter Maydell]

On Fri, 5 Jul 2019 at 10:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> You're right, the compatibility causes wrong behavior for the default
> -rtc settings (the RC pauses across migration).  The right thing to do
> would be to store the base rather than the offset: that is, you store
> the time at which LR was written.  Then the offset is s->lr - s->base
> and it's independent of the machine on which the rtc_clock is being read.

Right. How do we handle this for back-compat purposes? I guess
we need to have a new migration subsection, so if it's present
it has the 'base' value and we ignore the 'offset' in the
main migration data, and if it's not present we assume an
old->new migration and use the existing offset code. New->old
migration would not be possible as the new subsection is
always-present.

> By the way, the data sheet says "the counter and match values are
> compared in a comparator. When both values are equal, the RTCINTR
> interrupt is asserted HIGH"; QEMU compares the RTC value (read from
> RTC_DR) and not the counter value, but Linux code seems to expect QEMU's
> behavior.

The datasheet appears to be confused on this point -- for
instance in section 2.1 figure 2.3 talks about "interrupt
generation when the current RTC value (RTCDR) equals the
Match Register value"...

thanks
-- PMM

Re: [Qemu-devel] pl031 time across vm save/reload

[Paolo Bonzini]

On 05/07/19 11:58, Peter Maydell wrote:
> On Fri, 5 Jul 2019 at 10:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> You're right, the compatibility causes wrong behavior for the default
>> -rtc settings (the RC pauses across migration).  The right thing to do
>> would be to store the base rather than the offset: that is, you store
>> the time at which LR was written.  Then the offset is s->lr - s->base
>> and it's independent of the machine on which the rtc_clock is being read.
> 
> Right. How do we handle this for back-compat purposes? I guess
> we need to have a new migration subsection, so if it's present
> it has the 'base' value and we ignore the 'offset' in the
> main migration data, and if it's not present we assume an
> old->new migration and use the existing offset code. New->old
> migration would not be possible as the new subsection is
> always-present.

Yes, something like that but I would just bump the version.  Version 1
has the old meaning for the first field, version 2 has the new meaning.

And also, since our brains are fresh on pl031... currently s->lr is
always 0; besides the bug that writing RTC_LR should update it, the
datasheet says the counter counts up from 1 so perhaps at startup s->lr
should be set to a nonzero value?   That would be
qemu_ref_timedate(QEMU_CLOCK_VIRTUAL) - 1.

Paolo

Re: [Qemu-devel] pl031 time across vm save/reload

[Peter Maydell]

On Fri, 5 Jul 2019 at 11:13, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 05/07/19 11:58, Peter Maydell wrote:
> > On Fri, 5 Jul 2019 at 10:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> You're right, the compatibility causes wrong behavior for the default
> >> -rtc settings (the RC pauses across migration).  The right thing to do
> >> would be to store the base rather than the offset: that is, you store
> >> the time at which LR was written.  Then the offset is s->lr - s->base
> >> and it's independent of the machine on which the rtc_clock is being read.
> >
> > Right. How do we handle this for back-compat purposes? I guess
> > we need to have a new migration subsection, so if it's present
> > it has the 'base' value and we ignore the 'offset' in the
> > main migration data, and if it's not present we assume an
> > old->new migration and use the existing offset code. New->old
> > migration would not be possible as the new subsection is
> > always-present.
>
> Yes, something like that but I would just bump the version.  Version 1
> has the old meaning for the first field, version 2 has the new meaning.

Yeah, we could do that. I thought we preferred to avoid using
version-numbers for migration though these days ? (cc'ing DG
in case he has an opinion.)

> And also, since our brains are fresh on pl031... currently s->lr is
> always 0; besides the bug that writing RTC_LR should update it, the
> datasheet says the counter counts up from 1 so perhaps at startup s->lr
> should be set to a nonzero value?   That would be
> qemu_ref_timedate(QEMU_CLOCK_VIRTUAL) - 1.

The 'summary of RTC registers' section in the datasheet says
that RTCLR's reset value is zero...

thanks
-- PMM

Re: [Qemu-devel] pl031 time across vm save/reload

[Peter Maydell]

On Fri, 5 Jul 2019 at 10:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> By the way, the data sheet says "the counter and match values are
> compared in a comparator. When both values are equal, the RTCINTR
> interrupt is asserted HIGH"; QEMU compares the RTC value (read from
> RTC_DR) and not the counter value, but Linux code seems to expect QEMU's
> behavior.

Rereading the datasheet more carefully, the RTCMR register
description is clear about the behaviour:
 "An equivalent match value is derived from this register. The
 derived value is compared with the counter value in the CLK1HZ
 domain to generate an interrupt."
and the section 2.2.4 on the "Update block" confirms this.
So in hardware what happens is that there is a free-running
counter, and an offset which is used to derive the actual RTCDR
value from that. For comparisons, the RTCMR value has the
offset applied in reverse to give a derived value which can
be directly compared against the raw free-running counter,
with the equivalent effect as if the RTCMR value was compared
with the RTCDR.

thanks
-- PMM

Re: [Qemu-devel] pl031 time across vm save/reload

[Paolo Bonzini]

On 05/07/19 12:21, Peter Maydell wrote:
> On Fri, 5 Jul 2019 at 11:13, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 05/07/19 11:58, Peter Maydell wrote:
>>> On Fri, 5 Jul 2019 at 10:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> You're right, the compatibility causes wrong behavior for the default
>>>> -rtc settings (the RC pauses across migration).  The right thing to do
>>>> would be to store the base rather than the offset: that is, you store
>>>> the time at which LR was written.  Then the offset is s->lr - s->base
>>>> and it's independent of the machine on which the rtc_clock is being read.
>>>
>>> Right. How do we handle this for back-compat purposes? I guess
>>> we need to have a new migration subsection, so if it's present
>>> it has the 'base' value and we ignore the 'offset' in the
>>> main migration data, and if it's not present we assume an
>>> old->new migration and use the existing offset code. New->old
>>> migration would not be possible as the new subsection is
>>> always-present.
>>
>> Yes, something like that but I would just bump the version.  Version 1
>> has the old meaning for the first field, version 2 has the new meaning.
> 
> Yeah, we could do that. I thought we preferred to avoid using
> version-numbers for migration though these days ? (cc'ing DG
> in case he has an opinion.)

Yeah I suppose a subsection would make it easier to keep the old broken
behavior for old machine types.  It would be a bit more code.

>> And also, since our brains are fresh on pl031... currently s->lr is
>> always 0; besides the bug that writing RTC_LR should update it, the
>> datasheet says the counter counts up from 1 so perhaps at startup s->lr
>> should be set to a nonzero value?   That would be
>> qemu_ref_timedate(QEMU_CLOCK_VIRTUAL) - 1.
> 
> The 'summary of RTC registers' section in the datasheet says
> that RTCLR's reset value is zero...

Right, but RTCDR doesn't return the current wallclock after power up on
real hardware, doesn't it?  So the choices are 1) RTCLR returns 0 and it
looks like the board was powered on in the seventies; 2) RTCLR is not 0
and it looks like some firmware ran initialized RTCLR.

Paolo

Re: [Qemu-devel] pl031 time across vm save/reload

[Peter Maydell]

On Fri, 5 Jul 2019 at 11:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 05/07/19 12:21, Peter Maydell wrote:
> > On Fri, 5 Jul 2019 at 11:13, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> Yes, something like that but I would just bump the version.  Version 1
> >> has the old meaning for the first field, version 2 has the new meaning.
> >
> > Yeah, we could do that. I thought we preferred to avoid using
> > version-numbers for migration though these days ? (cc'ing DG
> > in case he has an opinion.)
>
> Yeah I suppose a subsection would make it easier to keep the old broken
> behavior for old machine types.  It would be a bit more code.

On which note, which of us is going to write this code ? :-)

> >> And also, since our brains are fresh on pl031... currently s->lr is
> >> always 0; besides the bug that writing RTC_LR should update it, the
> >> datasheet says the counter counts up from 1 so perhaps at startup s->lr
> >> should be set to a nonzero value?   That would be
> >> qemu_ref_timedate(QEMU_CLOCK_VIRTUAL) - 1.
> >
> > The 'summary of RTC registers' section in the datasheet says
> > that RTCLR's reset value is zero...
>
> Right, but RTCDR doesn't return the current wallclock after power up on
> real hardware, doesn't it?  So the choices are 1) RTCLR returns 0 and it
> looks like the board was powered on in the seventies; 2) RTCLR is not 0
> and it looks like some firmware ran initialized RTCLR.

I would want to test the behaviour of real hardware here before
bothering to change QEMU's implementation. And I'm not sure
it really justifies fiddling with it... (It's not impossible
that the hardware implementation really does hold "last value
written to this register" distinct from "whatever the offset
we have between the counter and RTCDR is", such that the value
read from RTCLR immediately on reset has no relation to what
the RTC is actually done and is just the reset value of the
relevant flops (ie 0).)

thanks
-- PMM

Re: [Qemu-devel] pl031 time across vm save/reload

[Peter Maydell]

On Fri, 5 Jul 2019 at 10:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> You're right, the compatibility causes wrong behavior for the default
> -rtc settings (the RC pauses across migration).  The right thing to do
> would be to store the base rather than the offset: that is, you store
> the time at which LR was written.  Then the offset is s->lr - s->base
> and it's independent of the machine on which the rtc_clock is being read.

So I thought I'd try writing a patch for this today, but I'm kind
of confused. If I understand you correctly, you're suggesting
that we should migrate a new field:
  base_time = s->lr - s->tick_offset
and then on the destination we can reconstitute tick_offset with
  s->tick_offset = base_time - s->lr
right?

But isn't this just a confusing way of migrating s->tick_offset
directly ? We'll never end up with a value at the destination
that's different from the offset value we started with on the source.

thanks
-- PMM

Re: [Qemu-devel] pl031 time across vm save/reload

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Fri, 5 Jul 2019 at 11:13, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 05/07/19 11:58, Peter Maydell wrote:
> > > On Fri, 5 Jul 2019 at 10:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >> You're right, the compatibility causes wrong behavior for the default
> > >> -rtc settings (the RC pauses across migration).  The right thing to do
> > >> would be to store the base rather than the offset: that is, you store
> > >> the time at which LR was written.  Then the offset is s->lr - s->base
> > >> and it's independent of the machine on which the rtc_clock is being read.
> > >
> > > Right. How do we handle this for back-compat purposes? I guess
> > > we need to have a new migration subsection, so if it's present
> > > it has the 'base' value and we ignore the 'offset' in the
> > > main migration data, and if it's not present we assume an
> > > old->new migration and use the existing offset code. New->old
> > > migration would not be possible as the new subsection is
> > > always-present.
> >
> > Yes, something like that but I would just bump the version.  Version 1
> > has the old meaning for the first field, version 2 has the new meaning.
> 
> Yeah, we could do that. I thought we preferred to avoid using
> version-numbers for migration though these days ? (cc'ing DG
> in case he has an opinion.)

Right.
Add a subsection, make the subsection only be sent if you're on a new
machine type.

(I'm currently getting my head around our x86 RTC code because of a bug
I've been handed involving RTCs and migration; the expectations and the
behaviours are not obvious at all).

Dave

> > And also, since our brains are fresh on pl031... currently s->lr is
> > always 0; besides the bug that writing RTC_LR should update it, the
> > datasheet says the counter counts up from 1 so perhaps at startup s->lr
> > should be set to a nonzero value?   That would be
> > qemu_ref_timedate(QEMU_CLOCK_VIRTUAL) - 1.
> 
> The 'summary of RTC registers' section in the datasheet says
> that RTCLR's reset value is zero...
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK