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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>
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.

Jan

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