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

[Thomas Huth]

On 07/12/2022 00.38, Bernhard Beschow wrote:
> Am 6. Dezember 2022 20:06:41 UTC schrieb Thomas Huth <thuth@redhat.com>:
> > 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;
> > };
> >
> > #define RTC_ISA_IRQ 8
> >
> > -ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
> > -                             qemu_irq intercept_irq);
> > +ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq 
> > intercept_irq,
> > +                             bool (*policy_slew_deliver_irq)(RTCState *s));
> > void rtc_set_memory(ISADevice *dev, int addr, int val);
> > int rtc_get_memory(ISADevice *dev, int addr);
> > +bool rtc_apic_policy_slew_deliver_irq(RTCState *s);
> > +void qmp_rtc_reset_reinjection(Error **errp);
> >
> > #endif /* HW_RTC_MC146818RTC_H */
> > diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> > index c502c8c62a..8723942b52 100644
> > --- a/hw/alpha/dp264.c
> > +++ b/hw/alpha/dp264.c
> > @@ -118,7 +118,7 @@ static void clipper_init(MachineState *machine)
> >      qdev_connect_gpio_out(i82378_dev, 0, isa_irq);
> >
> >      /* Since we have an SRM-compatible PALcode, use the SRM epoch.  */
> > -    mc146818_rtc_init(isa_bus, 1900, rtc_irq);
> > +    mc146818_rtc_init(isa_bus, 1900, rtc_irq, NULL);
> >
> >      /* VGA setup.  Don't bother loading the bios.  */
> >      pci_vga_init(pci_bus);
> > diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> > index de1cc7ab71..311031714a 100644
> > --- a/hw/hppa/machine.c
> > +++ b/hw/hppa/machine.c
> > @@ -232,7 +232,7 @@ static void machine_hppa_init(MachineState *machine)
> >      assert(isa_bus);
> >
> >      /* Realtime clock, used by firmware for PDC_TOD call. */
> > -    mc146818_rtc_init(isa_bus, 2000, NULL);
> > +    mc146818_rtc_init(isa_bus, 2000, NULL, NULL);
> >
> >      /* Serial ports: Lasi and Dino use a 7.272727 MHz clock. */
> >      serial_mm_init(addr_space, LASI_UART_HPA + 0x800, 0,
> > diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> > index 170a331e3f..d0ed4dca50 100644
> > --- a/hw/i386/microvm.c
> > +++ b/hw/i386/microvm.c
> > @@ -267,7 +267,8 @@ static void microvm_devices_init(MicrovmMachineState *mms)
> >
> >      if (mms->rtc == ON_OFF_AUTO_ON ||
> >          (mms->rtc == ON_OFF_AUTO_AUTO && !kvm_enabled())) {
> > -        rtc_state = mc146818_rtc_init(isa_bus, 2000, NULL);
> > +        rtc_state = mc146818_rtc_init(isa_bus, 2000, NULL,
> > +                                      rtc_apic_policy_slew_deliver_irq);
> >          microvm_set_rtc(mms, rtc_state);
> >      }
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 546b703cb4..650e7bc199 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1244,6 +1244,13 @@ static void pc_superio_init(ISABus *isa_bus, bool 
> > create_fdctrl,
> >      g_free(a20_line);
> > }
> >
> > +bool rtc_apic_policy_slew_deliver_irq(RTCState *s)
> > +{
> > +    apic_reset_irq_delivered();
> > +    qemu_irq_raise(s->irq);
> > +    return apic_get_irq_delivered();
> > +}
> > +
> > void pc_basic_device_init(struct PCMachineState *pcms,
> >                            ISABus *isa_bus, qemu_irq *gsi,
> >                            ISADevice **rtc_state,
> > @@ -1299,7 +1306,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
> >          pit_alt_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_PIT_INT);
> >          rtc_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_RTC_INT);
> >      }
> > -    *rtc_state = mc146818_rtc_init(isa_bus, 2000, rtc_irq);
> > +    *rtc_state = mc146818_rtc_init(isa_bus, 2000, rtc_irq,
> > +                                   rtc_apic_policy_slew_deliver_irq);
>
> In my PIIX consolidation series [1] I'm instantiating the RTC in the south 
> bridges since embedding the struct in the host device is the preferred new way. 
> In the end there is one initialization shared by both PIIX3 and -4. While PIIX3 
> (PC) will require rtc_apic_policy_slew_deliver_irq, PIIX4 (Malta) won't. 
> Furthermore, my goal ist to reuse PIIX4 in the PC machine to eliminate today's 
> Frankenstein PIIX4 ACPI controller. Any idea how to solve this?

I assume that you could ignore this in the shared initialization code and 
just add the pointer in the code that sets up the x86 boards. It's a little 
bit ugly [*], but RTCState is public in
include/hw/rtc/mc146818rtc.h, so it should be doable.

 Thomas


[*] Well, that whole driftfix=slew thing is ugly. I'm also not very familiar 
with it and don't know whether this is still in wide use ... does anybody 
know? Otherwise, we could could maybe also deprecate and finally remove that 
driftfix=slew stuff?