Re: [PATCH v4 01/12] x86/rtc: drop code related to strict mode

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 29, 2021 at 04:53:07PM +0200, Jan Beulich wrote:
> On 20.04.2021 16:07, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/rtc.c
> > +++ b/xen/arch/x86/hvm/rtc.c
> > @@ -46,15 +46,6 @@
> >  #define epoch_year     1900
> >  #define get_year(x)    (x + epoch_year)
> >  
> > -enum rtc_mode {
> > -   rtc_mode_no_ack,
> > -   rtc_mode_strict
> > -};
> > -
> > -/* This must be in sync with how hvmloader sets the ACPI WAET flags. */
> > -#define mode_is(d, m) ((void)(d), rtc_mode_##m == rtc_mode_no_ack)
> > -#define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m)
> 
> Leaving aside my concerns about this removal, I think some form of
> reference to hvmloader and its respective behavior should remain
> here, presumably in form of a (replacement) comment.

What about adding a comment in rtc_pf_callback:

/*
 * The current RTC implementation will inject an interrupt regardless
 * of whether REG_C has been read since the last interrupt was
 * injected. This is why the ACPI WAET 'RTC good' flag must be
 * unconditionally set by hvmloader.
 */

> > @@ -337,8 +336,7 @@ int pt_update_irq(struct vcpu *v)
> >      {
> >          if ( pt->pending_intr_nr )
> >          {
> > -            /* RTC code takes care of disabling the timer itself. */
> > -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> > +            if ( pt_irq_masked(pt) &&
> >                   /* Level interrupts should be asserted even if masked. */
> >                   !pt->level )
> >              {
> 
> I'm struggling to relate this to any other part of the patch. In
> particular I can't find the case where a periodic timer would be
> registered with RTC_IRQ and a NULL private pointer. The only use
> I can find is with a non-NULL pointer, which would mean the "else"
> path is always taken at present for the RTC case (which you now
> change).

Right, the else case was always taken because as the comment noted RTC
would take care of disabling itself (by calling destroy_periodic_time
from the callback when using strict_mode). When no_ack mode was
implemented this wasn't taken into account AFAICT, and thus the RTC
was never removed from the list even when masked.

I think with no_ack mode the RTC shouldn't have this specific handling
in pt_update_irq, as it should behave like any other virtual timer.
I could try to split this as a separate bugfix, but then I would have
to teach pt_update_irq to differentiate between strict_mode and no_ack
mode.

Would you be fine if the following is added to the commit message
instead:

"Note that the special handling of the RTC timer done in pt_update_irq
is wrong for the no_ack mode, as the RTC timer callback won't disable
the timer anymore when it detects the guest is not reading REG_C. As
such remove the code as part of the removal of strict_mode, and don't
special case the RTC timer anymore in pt_update_irq."

Thanks, Roger.