Re: [PATCH v2] mc146818rtc: Make PF independent of PIE

[Paolo Bonzini]

On 23/06/21 16:24, Jason Thorpe wrote:
> > Second, the firmware could set a nonzero period, and this would cause
> > continuous interruptions of the guest after the firmware stops, due to
> > s->periodic_timer firing.  This is "optimized" by the bug that you are
> > fixing.  To keep the optimization you could:
> >
> > - do the timer_mod in periodic_timer_update only if !PF || (PIE && 
> > lost_tick_policy==SLEW)
> >
> > - in cmos_ioport_read, if !timer_pending(s->periodic_timer) call
> >
> >     periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
> >                           s->period, true);
> >
> > to update s->next_periodic_time for the next tick and ensure PF will be set.
>
> I might be missing some subtlety here, but by my reading of
> periodic_timer_update(), either one of those changes would result in a
> delay of the next latching of PF by however many ns the CPU was late in
> reading PF since the last time it was latched  Please correct me if I’m
> wrong about this!

No, it shouldn't.  I may be wrong, but the process is the following:

- the current rtc_clock value is stored in cur_clock

- because period_change is true, the delay between writing PF and 
reading C is stored in lost_clock

- then the delay is compensated by next_irq_clock = cur_clock + period - 
lost_clock

The best way to confirm this would be by writing a testcase (there's 
already an mc146818 suite in tests/qtest).

Paolo

> There exists software out there in the wild that depends on PF latching at 
> regular intervals regardless if when the CPU reads, it, i.e.:
>
> PF          PF          PF          PF          PF          PF
>      C            C                  C      C                  C
>
> -- thorpej