Re: [Xen-devel] [PATCH] x86/hvm/rtc: preserved guest RTC offset during suspend/resume/migrate

["Durrant, Paul" <pdurrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 19 December 2019 11:30
> To: Durrant, Paul <pdurrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxxxxx>; Ian Jackson
> <ian.jackson@xxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné
> <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH] x86/hvm/rtc: preserved guest RTC offset during
> suspend/resume/migrate
> 
> On 18.12.2019 15:41, Paul Durrant wrote:
> > The emulated RTC is synchronized with the PV wallclock; any write to the
> > RTC will update struct domain's 'time_offset_seconds' field and call
> > update_domain_wallclock().
> >
> > However, the value of 'time_offset_seconds' is not preserved in any save
> > record and indeed, when the RTC save record is loaded, the CMOS values
> > will be updated based on an offset value which may or may not have been
> > set by the toolstack [1]. This may result in making bogus values
> available
> > to the guest and messing up any calculations done in the call to
> > alarm_timer_update() at the end of rtc_load().
> >
> > This patch extends the RTC save record to contain an offset value, which
> > will be zero filled on load of an older record. The
> 'time_offset_secoonds'
> > field in struct domain is also modified into a 'time_offset' struct,
> > containing a 'seconds' field and a boolean 'set' field.
> >
> > The code in rtc_load() then uses the new value in the save record to
> > update the value of struct domain's 'time_offset.seconds' unless
> > 'time_offset.set' is true, which will only be the case if the toolstack
> has
> > already performed a XEN_DOMCTL_settimeoffset.
> >
> > [1] There is currently no way for a toolstack to read the value of
> >     'time_offset_seconds' from struct domain. In the past, any hope of
> >     preservation of the value across a guest life-cycle operation was
> based
> >     on relying on qemu-dm to write a value into xenstore whenever the
> RTC
> >     was updated, in response to an IOREQ with type IOREQ_TYPE_TIMEOFFSET
> >     being sent by Xen; see:
> >
> >     https://xenbits.xen.org/gitweb/?p=qemu-xen-
> traditional.git;a=blob;f=i386-dm/helper2.c#l457
> >
> >     but this behaviour was never forward-ported into upstream QEMU,
> which
> >     completely ignores that IOREQ type.
> >     In either case, nothing in xl or libxl ever samples the value of
> >     RTC offset from xenstore so any offset adjustment to a non-zero
> value
> >     performed by the guest (which in the case of Windows is highly
> likely
> >     as it normally writes RTC in local time, whereas Xen maintains time
> in
> >     UTC) is completely lost with the de-facto toolstack, and always has
> >     been. Instead, PV drivers are relied upon to paper over this gaping
> >     hole.
> >
> > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> with one remark:
> 
> > @@ -771,6 +773,12 @@ static int rtc_load(struct domain *d,
> hvm_domain_context_t *h)
> >
> >      /* Reset the wall-clock time.  In normal running, this runs with
> host
> >       * time, so let's keep doing that. */
> > +    if ( !d->time_offset.set )
> > +    {
> > +        d->time_offset.seconds = s->hw.rtc_offset;
> > +        update_domain_wallclock_time(d);
> > +    }
> 
> It's not really clear to me which of the possible behaviors is the
> better one - overriding a tool stack set value with what the save
> record says, or (like you do) the other way around.

It's not clear to me either, which is why I erred on the side of caution. I 
didn't want to break any out-of-tree toolstacks that might be overriding the 
offset early in the restore phase from a value acquired via QEMU hackery. I 
guess the boolean could be dispensed with if we retire the IOREQ and silently 
ignore the DOMCTL for HVM guests (so overrides from the aforementioned 
toolstacks simply have no effect).

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel