Re: [PATCH] x86/rtc: Avoid UIP flag being set for longer than expected

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2024 18:26, Ross Lagerwall wrote:
> In a test, OVMF reported an error initializing the RTC without
> indicating the precise nature of the error. The only plausible
> explanation I can find is as follows:
> 
> As part of the initialization, OVMF reads register C and then reads
> register A repatedly until the UIP flag is not set. If this takes longer
> than 100 ms, OVMF fails and reports an error. This may happen with the
> following sequence of events:
> 
> At guest time=0s, rtc_init() calls check_update_timer() which schedules
> update_timer for t=(1 - 244us).
> 
> At t=1s, the update_timer function happens to have been called >= 244us
> late.

Any theory on why this timer runs so late? (It needs dealing with anyway,
but I'm still curious.)

> In the timer callback, it sets the UIP flag and schedules
> update_timer2 for t=1s.
> 
> Before update_timer2 runs, the guest reads register C which calls
> check_update_timer(). check_update_timer() stops the scheduled
> update_timer2 and since the guest time is now outside of the update
> cycle, it schedules update_timer for t=(2 - 244us).
> 
> The UIP flag will therefore be set for a whole second from t=1 to t=2
> while the guest repeatedly reads register A waiting for the UIP flag to
> clear. Fix it by clearing the UIP flag when scheduling update_timer.

I can accept this as a workaround (perhaps with a tweak, see below), but
I wonder whether we couldn't do this in a less ad hoc way. If stop_timer()
returned whether the timer was actually stopped, couldn't the clearing of
UIP be made conditional upon that?

> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -202,6 +202,7 @@ static void check_update_timer(RTCState *s)
>          }
>          else
>          {
> +            s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
>              next_update_time = (USEC_PER_SEC - guest_usec - 244) * 
> NS_PER_USEC;
>              expire_time = NOW() + next_update_time;
>              s->next_update_time = expire_time;

Is rtc_update_timer2() clearing UIP then still necessary, ahead of calling
here? Hmm, yes, it is; the question is rather why the function calls
check_update_timer() when all that'll do (due to UF now being set) is stop
the other timer (in case it's also running) and clear ->use_timer.

Jan