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.
