Re: [PATCH for-8.0] hw/rtc/mc146818rtc: Make this rtc device target independent

[Thomas Huth]

On 07/12/2022 00.12, BALATON Zoltan wrote:
> On Tue, 6 Dec 2022, Thomas Huth wrote:
> > The only code that is really, really target dependent is the apic-related
> > code in rtc_policy_slew_deliver_irq(). By moving this code into the hw/i386/
> > folder (renamed to rtc_apic_policy_slew_deliver_irq()) and passing this
> > function as parameter to mc146818_rtc_init(), we can make the RTC completely
> > target-independent.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
...
> > @@ -124,9 +118,8 @@ void qmp_rtc_reset_reinjection(Error **errp)
> >
> > static bool rtc_policy_slew_deliver_irq(RTCState *s)
> > {
> > -    apic_reset_irq_delivered();
> > -    qemu_irq_raise(s->irq);
> > -    return apic_get_irq_delivered();
> > +    assert(s->policy_slew_deliver_irq);
>
> Is this assert necessary here? Since it seems that creating the timer that 
> would call this is testing for s->policy_slew_deliver_irq being non-NULL 
> there should be no way to call this without policy_slew_deliver_irq set.

There was an assert(0) in the original code on non-x86 targets, too, see 
below. I would like to keep that logic here.

> If 
> you drop the assert then this function also become redundant and 
> s->policy_slew_deliver_irq() can be used directly instead simplifying this a 
> bit more.

I'd agree, but I really would like to keep the assert(). (additionally, the 
patch stays smaller this way)

> > +    return s->policy_slew_deliver_irq(s);
> > }
> >
> > static void rtc_coalesced_timer(void *opaque)
> > @@ -145,13 +138,6 @@ static void rtc_coalesced_timer(void *opaque)
> >
> >     rtc_coalesced_timer_update(s);
> > }
> > -#else
> > -static bool rtc_policy_slew_deliver_irq(RTCState *s)
> > -{
> > -    assert(0);

This ----^ is the assert() I was talking about.

> > -    return false;
> > -}
> > -#endif

 Thomas