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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.04.2024 18:38, Ross Lagerwall wrote:
> On Thu, Apr 18, 2024 at 2:24 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 08.04.2024 18:26, Ross Lagerwall wrote:
>>> --- 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.
> 
> Are you suggesting something like this?
> 
> diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> index 4bb1c7505534..eb4901bf129a 100644
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -240,7 +240,8 @@ static void cf_check rtc_update_timer2(void *opaque)
>          s->hw.cmos_data[RTC_REG_C] |= RTC_UF;
>          s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
>          rtc_update_irq(s);
> -        check_update_timer(s);
> +        stop_timer(&s->update_timer);
> +        s->use_timer = 0;
>      }
>      spin_unlock(&s->lock);
>  }
> 
> That may indeed be an improvement but I'm not sure it is really related
> to this patch?

Well, the connection is the initial question of this part of my earlier
reply. That's not to say the further change needs (or even wants) doing
right here. However, upon looking again I get the impression that I was
mixing up timer and timer2. The code path you alter is one where timer
is set, not timer2.

Overall:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan