[PATCH v2] mc146818rtc: Make PF independent of PIE

[PATCH v2] mc146818rtc: Make PF independent of PIE

[Jason Thorpe]

Make the PF flag behave like real hardware by always running the
periodic timer without regard to the setting of the PIE bit, so
that the PF will be set when the period expires even if an interrupt
will not be raised.  This behavior is documented on page 16 of the
MC146818A advance information datasheet.

Signed-off-by: Jason Thorpe <thorpej@me.com>
---
 hw/rtc/mc146818rtc.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 4fbafddb22..85abdfd9d1 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -155,9 +155,24 @@ static uint32_t rtc_periodic_clock_ticks(RTCState *s)
 {
     int period_code;
 
-    if (!(s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
-        return 0;
-     }
+    /*
+     * Quoting the data sheet "MC146818A Advance Information", 1984,
+     * page 16:
+     *
+     * <quote>
+     * PF - The periodic interrupt flag (PF) is a read-only bit which is
+     * set to a "1" when a particular edge is detected on the selected tap
+     * of the divider chain.  The RS3 to RS0 bits establish the periodic
+     * rate.  PF is set to "1" independent of the state of the PIE bit.
+     * PF initiates an ~IRQ signal and sets the IRQF bit when PIE is also
+     * a "1".  The PF bit is cleared by a ~RESET or a software read of
+     * Register C.
+     * </quote>
+     *
+     * As such, we always run the timer irrespective if the state of
+     * either bit so as to always set PF at regular intervals regardless
+     * of when software reads it.
+     */
 
     period_code = s->cmos_data[RTC_REG_A] & 0x0f;
 
-- 
2.30.2

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

[Paolo Bonzini]

On 19/06/21 21:38, Jason Thorpe wrote:
> Make the PF flag behave like real hardware by always running the
> periodic timer without regard to the setting of the PIE bit, so
> that the PF will be set when the period expires even if an interrupt
> will not be raised.  This behavior is documented on page 16 of the
> MC146818A advance information datasheet.
> 
> Signed-off-by: Jason Thorpe <thorpej@me.com>

I agree that there's obviously a bug in QEMU.  However, I'm worried of 
two things with this patch.

First, the RTC device model has a complicated mechanism to deliver 
missed ticks of the periodic timer.  This is used with old versions of 
Windows which lose track of time if periodic timer interrupts are not 
delivered.  This mechanism (implemented by rtc_coalesced_timer) right 
now always triggers an interrupt.  You need to change 
periodic_timer_update to not activate this mechanism if PIE=0, by 
checking for PIE=1 at the

     if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW)

conditional.

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.

Thanks,

Paolo

> ---
>   hw/rtc/mc146818rtc.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 4fbafddb22..85abdfd9d1 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -155,9 +155,24 @@ static uint32_t rtc_periodic_clock_ticks(RTCState *s)
>   {
>       int period_code;
>   
> -    if (!(s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
> -        return 0;
> -     }
> +    /*
> +     * Quoting the data sheet "MC146818A Advance Information", 1984,
> +     * page 16:
> +     *
> +     * <quote>
> +     * PF - The periodic interrupt flag (PF) is a read-only bit which is
> +     * set to a "1" when a particular edge is detected on the selected tap
> +     * of the divider chain.  The RS3 to RS0 bits establish the periodic
> +     * rate.  PF is set to "1" independent of the state of the PIE bit.
> +     * PF initiates an ~IRQ signal and sets the IRQF bit when PIE is also
> +     * a "1".  The PF bit is cleared by a ~RESET or a software read of
> +     * Register C.
> +     * </quote>
> +     *
> +     * As such, we always run the timer irrespective if the state of
> +     * either bit so as to always set PF at regular intervals regardless
> +     * of when software reads it.
> +     */
>   
>       period_code = s->cmos_data[RTC_REG_A] & 0x0f;
>   
> 

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

[Jason Thorpe]

> On Jun 21, 2021, at 7:46 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> I agree that there's obviously a bug in QEMU.  However, I'm worried of two things with this patch.
> 
> First, the RTC device model has a complicated mechanism to deliver missed ticks of the periodic timer.  This is used with old versions of Windows which lose track of time if periodic timer interrupts are not delivered.  This mechanism (implemented by rtc_coalesced_timer) right now always triggers an interrupt.  You need to change periodic_timer_update to not activate this mechanism if PIE=0, by checking for PIE=1 at the
> 
>    if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW)
> 
> conditional.

Makes sense.

> 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!

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

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
> 
>