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

[Thomas Huth]

On 07/12/2022 16.29, Mark Cave-Ayland wrote:
> On 06/12/2022 20:06, 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>
> > ---
> >   include/hw/rtc/mc146818rtc.h |  7 +++++--
> >   hw/alpha/dp264.c             |  2 +-
> >   hw/hppa/machine.c            |  2 +-
> >   hw/i386/microvm.c            |  3 ++-
> >   hw/i386/pc.c                 | 10 +++++++++-
> >   hw/mips/jazz.c               |  2 +-
> >   hw/ppc/pnv.c                 |  2 +-
> >   hw/rtc/mc146818rtc.c         | 34 +++++++++++-----------------------
> >   hw/rtc/meson.build           |  3 +--
> >   9 files changed, 32 insertions(+), 33 deletions(-)
> >
> > diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> > index 1db0fcee92..c687953cc4 100644
> > --- a/include/hw/rtc/mc146818rtc.h
> > +++ b/include/hw/rtc/mc146818rtc.h
> > @@ -46,14 +46,17 @@ struct RTCState {
> >       Notifier clock_reset_notifier;
> >       LostTickPolicy lost_tick_policy;
> >       Notifier suspend_notifier;
> > +    bool (*policy_slew_deliver_irq)(RTCState *s);
> >       QLIST_ENTRY(RTCState) link;
> >   };
[...]
> These days we really should try and avoid setting function pointers outside 
> of _init() and class_init() where possible. If I were looking to model this 
> today I'd probably try an approach like this:
>
> - Move apic_irq_delivered into APICCommonState
>
> - Define apic_reset_irq_delivered() and apic_get_irq_delivered() as function 
> pointers in APICCommonState and assign them to the current implementations 
> in apic_common_initfn() adding an APICCommonState parameter as required
>
> - Grab a reference to APICCommonState in mc146818rtc.c's rtc_realizefn() 
> using either an object property link or object_resolve_path_type("", 
> TYPE_APIC_COMMON, NULL)
>
> - Use "if (s->apic) { .. }" or similar in mc146818rtc.c to guard calling the 
> apic_*() functions

That also doesn't really fly since the apic_* functions are only available 
on x86 (and maybe arm), but not in the other targets, so this won't link 
there. Ok, we could provide stubs for the other targets, but that's IMHO 
almost as ugly as having a function pointer in RTCState ... ? I can give it 
a try with using a stub, but not sure whether it will really look much better...

 Thomas