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;

This lost_tick_policy attribute along with its enum is now redundant and can be 
removed. Removing it avoids an error condition (see below).

>     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);
> 
>     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;
>+        }

With lost_tick_policy removed, the whole switch statement can be collapsed into 
this if statement and it avoids below error case in the first place.

Best regards,
Bernhard

>+        /* 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'))