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

[Bernhard Beschow]

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?

Thanks,
Bernhard

[1] 20221204190553.3274-1-shentey@gmail.com/">https://patchew.org/QEMU/20221204190553.3274-1-shentey@gmail.com/

> 
>     qemu_register_boot_set(pc_boot_set, *rtc_state);
> 
>diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
>index 6aefe9a61b..50fbd57b23 100644
>--- a/hw/mips/jazz.c
>+++ b/hw/mips/jazz.c
>@@ -356,7 +356,7 @@ static void mips_jazz_init(MachineState *machine,
>     fdctrl_init_sysbus(qdev_get_gpio_in(rc4030, 1), 0x80003000, fds);
> 
>     /* Real time clock */
>-    mc146818_rtc_init(isa_bus, 1980, NULL);
>+    mc146818_rtc_init(isa_bus, 1980, NULL, NULL);
>     memory_region_init_io(rtc, NULL, &rtc_ops, NULL, "rtc", 0x1000);
>     memory_region_add_subregion(address_space, 0x80004000, rtc);
> 
>diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>index 3d01e26f84..c5482554b7 100644
>--- a/hw/ppc/pnv.c
>+++ b/hw/ppc/pnv.c
>@@ -992,7 +992,7 @@ static void pnv_init(MachineState *machine)
>     serial_hds_isa_init(pnv->isa_bus, 0, MAX_ISA_SERIAL_PORTS);
> 
>     /* Create an RTC ISA device too */
>-    mc146818_rtc_init(pnv->isa_bus, 2000, NULL);
>+    mc146818_rtc_init(pnv->isa_bus, 2000, NULL, NULL);
> 
>     /*
>      * Create the machine BMC simulator and the IPMI BT device for
>diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>index 1ebb412479..9543ae0279 100644
>--- a/hw/rtc/mc146818rtc.c
>+++ b/hw/rtc/mc146818rtc.c
>@@ -44,11 +44,6 @@
> #include "qapi/visitor.h"
> #include "hw/rtc/mc146818rtc_regs.h"
> 
>-#ifdef TARGET_I386
>-#include "qapi/qapi-commands-misc-target.h"
>-#include "hw/i386/apic.h"
>-#endif
>-
> //#define DEBUG_CMOS
> //#define DEBUG_COALESCED
> 
>@@ -112,7 +107,6 @@ static void rtc_coalesced_timer_update(RTCState *s)
> static QLIST_HEAD(, RTCState) rtc_devices =
>     QLIST_HEAD_INITIALIZER(rtc_devices);
> 
>-#ifdef TARGET_I386
> void qmp_rtc_reset_reinjection(Error **errp)
> {
>     RTCState *s;
>@@ -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);
>+    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);
>-    return false;
>-}
>-#endif
> 
> static uint32_t rtc_periodic_clock_ticks(RTCState *s)
> {
>@@ -922,14 +908,14 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>     rtc_set_date_from_host(isadev);
> 
>     switch (s->lost_tick_policy) {
>-#ifdef TARGET_I386
>-    case LOST_TICK_POLICY_SLEW:
>-        s->coalesced_timer =
>-            timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>-        break;
>-#endif
>     case LOST_TICK_POLICY_DISCARD:
>         break;
>+    case LOST_TICK_POLICY_SLEW:
>+        if (s->policy_slew_deliver_irq) {
>+            s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, 
>s);
>+            break;
>+        }
>+        /* fallthrough */
>     default:
>         error_setg(errp, "Invalid lost tick policy.");
>         return;
>@@ -960,7 +946,8 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>     QLIST_INSERT_HEAD(&rtc_devices, s, link);
> }
> 
>-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))
> {
>     DeviceState *dev;
>     ISADevice *isadev;
>@@ -969,6 +956,7 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, 
>qemu_irq intercept_irq)
>     isadev = isa_new(TYPE_MC146818_RTC);
>     dev = DEVICE(isadev);
>     s = MC146818_RTC(isadev);
>+    s->policy_slew_deliver_irq = policy_slew_deliver_irq;
>     qdev_prop_set_int32(dev, "base_year", base_year);
>     isa_realize_and_unref(isadev, bus, &error_fatal);
>     if (intercept_irq) {
>diff --git a/hw/rtc/meson.build b/hw/rtc/meson.build
>index dc33973384..34a4d316fa 100644
>--- a/hw/rtc/meson.build
>+++ b/hw/rtc/meson.build
>@@ -13,5 +13,4 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: 
>files('aspeed_rtc.c'))
> softmmu_ss.add(when: 'CONFIG_GOLDFISH_RTC', if_true: files('goldfish_rtc.c'))
> softmmu_ss.add(when: 'CONFIG_LS7A_RTC', if_true: files('ls7a_rtc.c'))
> softmmu_ss.add(when: 'CONFIG_ALLWINNER_H3', if_true: files('allwinner-rtc.c'))
>-
>-specific_ss.add(when: 'CONFIG_MC146818RTC', if_true: files('mc146818rtc.c'))
>+softmmu_ss.add(when: 'CONFIG_MC146818RTC', if_true: files('mc146818rtc.c'))