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

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

On Mon, May 03, 2021 at 04:58:07PM +0200, Jan Beulich wrote:
> On 03.05.2021 16:47, Roger Pau Monné wrote:
> > On Mon, May 03, 2021 at 02:26:51PM +0200, Jan Beulich wrote:
> >> On 03.05.2021 11:28, Roger Pau Monné wrote:
> >>> On Thu, Apr 29, 2021 at 04:53:07PM +0200, Jan Beulich wrote:
> >>>> On 20.04.2021 16:07, Roger Pau Monne wrote:
> >> (I've also not seen the
> >> flag named "RTC good" - the ACPI constant is ACPI_WAET_RTC_NO_ACK, for
> >> example.)
> > 
> > I'm reading the WAET spec as published my Microsoft:
> > 
> > http://msdn.microsoft.com/en-us/windows/hardware/gg487524.aspx
> > 
> > Where the flag is listed as 'RTC good'. Maybe that's outdated now?
> > Seems to be the official source for the specification from
> > https://uefi.org/acpi.
> 
> Well, I guess the wording wasn't used for the constant's name because
> the RTC isn't "bad" otherwise?

I guess so, that's the name given my Microsoft anyway. The
descriptions speaks of an 'enhanced RTC', so you could differentiate
between plain RTC and enhanced one :).

> >>>>> @@ -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.
> >>
> >> A fair part of my confusion was about "&& !pt->priv".
> > 
> > I think you meant "|| !pt->priv"?
> 
> Oops, indeed.
> 
> >> I've looked back
> >> at 9607327abbd3 ("x86/HVM: properly handle RTC periodic timer even when
> >> !RTC_PIE"), where this was added. It was, afaict, to cover for
> >> hpet_set_timer() passing NULL with RTC_IRQ.
> > 
> > That's tricky, as hpet_set_timer hardcodes 8 instead of using RTC_IRQ
> > which makes it really easy to miss.
> > 
> >> Which makes me suspect that
> >> be07023be115 ("x86/vhpet: add support for level triggered interrupts")
> >> may have subtly broken things.
> > 
> > Right - as that would have made the RTC irq when generated from the
> > HPET no longer be suspended when masked (as pt->priv would no longer
> > be NULL). Could be fixed with:
> > 
> > diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
> > index ca94e8b4538..f2cbd12f400 100644
> > --- a/xen/arch/x86/hvm/hpet.c
> > +++ b/xen/arch/x86/hvm/hpet.c
> > @@ -318,7 +318,8 @@ static void hpet_set_timer(HPETState *h, unsigned int 
> > tn,
> >                           hpet_tick_to_ns(h, diff),
> >                           oneshot ? 0 : hpet_tick_to_ns(h, 
> > h->hpet.period[tn]),
> >                           irq, timer_level(h, tn) ? hpet_timer_fired : NULL,
> > -                         (void *)(unsigned long)tn, timer_level(h, tn));
> > +                         timer_level(h, tn) ? (void *)(unsigned long)tn : 
> > NULL,
> > +                         timer_level(h, tn));
> >  }
> >  
> >  static inline uint64_t hpet_fixup_reg(
> > 
> > Passing again NULL as the callback private data for edge triggered
> > interrupts.
> 
> Right, plus perhaps at the same time replacing the hardcoded 8.

Right, but if you agree to take this patch and remove strict_mode then
the emulated RTC won't disable itself anymore, and hence needs to be
handled as any other virtual timer?

I will submit the HPET patch so it can be backported to stable
releases anyway, just wanted to check whether you would agree to
remove the strict_mode, and whether you then agree that the special
handling of the RTC done in pt_update_irq is no longer needed.

> >>> 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."
> >>
> >> Not sure yet - as per above I'm still not convinced this part of the
> >> change is correct.
> > 
> > I believe part of this handling is kind of bogus - for example I'm
> > unsure Xen should account masked interrupt injections as missed ticks.
> > A guest might decide to mask it's interrupt source for whatever
> > reason, and then it shouldn't receive a flurry of interrupts when
> > unmasked. Ie: missed ticks should only be accounted for interrupts
> > that should have been delivered but the guest wasn't scheduled. I
> > think such model would also simplify some of the logic that we
> > currently have.
> > 
> > In fact I have a patch on top of this current series which I haven't
> > posted yet that does implement this new mode of not accounting masked
> > interrupts as missed ticks to the delivered later.
> 
> This may be problematic: Iirc one of the goals of this mode is to cover
> for the case where a guest simply doesn't get around to unmasking the
> IRQ until the next one occurs. Yes, it feels bogus, but I'm not sure it
> can be done away with.

Well, an OS shouldn't really mask the interrupt source without being
capable of handling missed interrupts. As even when running native a
SMM could steal time from the OS and thus miss timer ticks?

> I also can't seem to be able to think of a
> heuristic by which the two scenarios could be told apart halfway
> reliably.

I've tested with Windows 7 limited to 2% capacity and seems to be able
to keep track of time correctly when masked timer interrupts are not
accounted as missed ticks. Note than doing the same with
no_missed_ticks_pending leads to time skews (so limiting the CPU
capacity to 2% does indeed force timer ticks to accumulate).

Anyway, we can discuss later, once this initial batch of patches is
in.

Thanks, Roger.