The only reason for this code being target dependent is the apic-related
code in rtc_policy_slew_deliver_irq(). Since these apic functions are rather
simple, we can easily move them into a new, separate file (apic_irqcount.c)
which will always be compiled and linked if either APIC or the mc146818 device
are required. This way we can get rid of the #ifdef TARGET_I386 switches in
mc146818rtc.c and declare it in the softmmu_ss instead of specific_ss, so
that the code only gets compiled once for all targets.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it

 include/hw/i386/apic.h          |  2 ++
 include/hw/i386/apic_internal.h |  2 --
 include/hw/rtc/mc146818rtc.h    |  1 +
 hw/intc/apic_common.c           | 27 -----------------
 hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
 hw/rtc/mc146818rtc.c            | 25 +++++-----------
 hw/intc/meson.build             |  6 +++-
 hw/rtc/meson.build              |  3 +-
 8 files changed, 69 insertions(+), 50 deletions(-)
 create mode 100644 hw/intc/apic_irqcount.c

diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index da1d2fe155..24069fb961 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -1,6 +1,7 @@
 #ifndef APIC_H
 #define APIC_H
 
+#define TYPE_APIC_COMMON "apic-common"
 
 /* apic.c */
 void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
@@ -9,6 +10,7 @@ int apic_accept_pic_intr(DeviceState *s);
 void apic_deliver_pic_intr(DeviceState *s, int level);
 void apic_deliver_nmi(DeviceState *d);
 int apic_get_interrupt(DeviceState *s);
+void apic_report_irq_delivered(int delivered);
 void apic_reset_irq_delivered(void);
 int apic_get_irq_delivered(void);
 void cpu_set_apic_base(DeviceState *s, uint64_t val);
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index c175e7e718..ff018f1778 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -125,7 +125,6 @@
 
 typedef struct APICCommonState APICCommonState;
 
-#define TYPE_APIC_COMMON "apic-common"
 typedef struct APICCommonClass APICCommonClass;
 DECLARE_OBJ_CHECKERS(APICCommonState, APICCommonClass,
                      APIC_COMMON, TYPE_APIC_COMMON)
@@ -199,7 +198,6 @@ typedef struct VAPICState {
 
 extern bool apic_report_tpr_access;
 
-void apic_report_irq_delivered(int delivered);
 bool apic_next_timer(APICCommonState *s, int64_t current_time);
 void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
 void apic_enable_vapic(DeviceState *d, hwaddr paddr);
diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
index 1db0fcee92..45bcd6f040 100644
--- a/include/hw/rtc/mc146818rtc.h
+++ b/include/hw/rtc/mc146818rtc.h
@@ -55,5 +55,6 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
                              qemu_irq intercept_irq);
 void rtc_set_memory(ISADevice *dev, int addr, int val);
 int rtc_get_memory(ISADevice *dev, int addr);
+void qmp_rtc_reset_reinjection(Error **errp);
 
 #endif /* HW_RTC_MC146818RTC_H */
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 2a20982066..b0f85f9384 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -33,7 +33,6 @@
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 
-static int apic_irq_delivered;
 bool apic_report_tpr_access;
 
 void cpu_set_apic_base(DeviceState *dev, uint64_t val)
@@ -122,32 +121,6 @@ void apic_handle_tpr_access_report(DeviceState *dev, target_ulong ip,
     vapic_report_tpr_access(s->vapic, CPU(s->cpu), ip, access);
 }
 
-void apic_report_irq_delivered(int delivered)
-{
-    apic_irq_delivered += delivered;
-
-    trace_apic_report_irq_delivered(apic_irq_delivered);
-}
-
-void apic_reset_irq_delivered(void)
-{
-    /* Copy this into a local variable to encourage gcc to emit a plain
-     * register for a sys/sdt.h marker.  For details on this workaround, see:
-     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
-     */
-    volatile int a_i_d = apic_irq_delivered;
-    trace_apic_reset_irq_delivered(a_i_d);
-
-    apic_irq_delivered = 0;
-}
-
-int apic_get_irq_delivered(void)
-{
-    trace_apic_get_irq_delivered(apic_irq_delivered);
-
-    return apic_irq_delivered;
-}
-
 void apic_deliver_nmi(DeviceState *dev)
 {
     APICCommonState *s = APIC_COMMON(dev);
diff --git a/hw/intc/apic_irqcount.c b/hw/intc/apic_irqcount.c
new file mode 100644
index 0000000000..0aadef1cb5
--- /dev/null
+++ b/hw/intc/apic_irqcount.c
@@ -0,0 +1,53 @@
+/*
+ * APIC support - functions for counting the delivered IRQs.
+ * (this code is in a separate file since it is used from the
+ * mc146818rtc code on targets without APIC, too)
+ *
+ *  Copyright (c) 2011      Jan Kiszka, Siemens AG
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "hw/i386/apic.h"
+#include "trace.h"
+
+static int apic_irq_delivered;
+
+void apic_report_irq_delivered(int delivered)
+{
+    apic_irq_delivered += delivered;
+
+    trace_apic_report_irq_delivered(apic_irq_delivered);
+}
+
+void apic_reset_irq_delivered(void)
+{
+    /*
+     * Copy this into a local variable to encourage gcc to emit a plain
+     * register for a sys/sdt.h marker.  For details on this workaround, see:
+     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
+     */
+    volatile int a_i_d = apic_irq_delivered;
+    trace_apic_reset_irq_delivered(a_i_d);
+
+    apic_irq_delivered = 0;
+}
+
+int apic_get_irq_delivered(void)
+{
+    trace_apic_get_irq_delivered(apic_irq_delivered);
+
+    return apic_irq_delivered;
+}
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 1ebb412479..d524dc02c2 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -43,11 +43,7 @@
 #include "qapi/qapi-events-misc.h"
 #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 +108,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;
@@ -145,13 +140,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 +910,15 @@ 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:
+        /* Slew tick policy is only available if the machine has an APIC */
+        if (object_resolve_path_type("", TYPE_APIC_COMMON, NULL) != NULL) {
+            s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
+            break;
+        }
+        /* fallthrough */
     default:
         error_setg(errp, "Invalid lost tick policy.");
         return;
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index bcbf22ff51..036ad1936b 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -25,8 +25,12 @@ softmmu_ss.add(when: 'CONFIG_XILINX', if_true: files('xilinx_intc.c'))
 softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true: files('xlnx-zynqmp-ipi.c'))
 softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_PMU', if_true: files('xlnx-pmu-iomod-intc.c'))
 
-specific_ss.add(when: 'CONFIG_ALLWINNER_A10_PIC', if_true: files('allwinner-a10-pic.c'))
+if config_all_devices.has_key('CONFIG_APIC') or config_all_devices.has_key('CONFIG_MC146818RTC')
+    softmmu_ss.add(files('apic_irqcount.c'))
+endif
 specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 'apic_common.c'))
+
+specific_ss.add(when: 'CONFIG_ALLWINNER_A10_PIC', if_true: files('allwinner-a10-pic.c'))
 specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gicv3_cpuif_common.c'))
 specific_ss.add(when: 'CONFIG_ARM_GICV3_TCG', if_true: files('arm_gicv3_cpuif.c'))
 specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: files('arm_gic_kvm.c'))
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'))
-- 
2.31.1

Am 12. Dezember 2022 07:56:00 UTC schrieb Thomas Huth <thuth@redhat.com>:
>The only reason for this code being target dependent is the apic-related
>code in rtc_policy_slew_deliver_irq(). Since these apic functions are rather
>simple, we can easily move them into a new, separate file (apic_irqcount.c)
>which will always be compiled and linked if either APIC or the mc146818 device
>are required. This way we can get rid of the #ifdef TARGET_I386 switches in
>mc146818rtc.c and declare it in the softmmu_ss instead of specific_ss, so
>that the code only gets compiled once for all targets.
>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>Signed-off-by: Thomas Huth <thuth@redhat.com>
>---
> v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it
>
> include/hw/i386/apic.h          |  2 ++
> include/hw/i386/apic_internal.h |  2 --
> include/hw/rtc/mc146818rtc.h    |  1 +
> hw/intc/apic_common.c           | 27 -----------------
> hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
> hw/rtc/mc146818rtc.c            | 25 +++++-----------
> hw/intc/meson.build             |  6 +++-
> hw/rtc/meson.build              |  3 +-
> 8 files changed, 69 insertions(+), 50 deletions(-)
> create mode 100644 hw/intc/apic_irqcount.c
>
>diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
>index da1d2fe155..24069fb961 100644
>--- a/include/hw/i386/apic.h
>+++ b/include/hw/i386/apic.h
>@@ -1,6 +1,7 @@
> #ifndef APIC_H
> #define APIC_H
> 
>+#define TYPE_APIC_COMMON "apic-common"
> 
> /* apic.c */
> void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
>@@ -9,6 +10,7 @@ int apic_accept_pic_intr(DeviceState *s);
> void apic_deliver_pic_intr(DeviceState *s, int level);
> void apic_deliver_nmi(DeviceState *d);
> int apic_get_interrupt(DeviceState *s);
>+void apic_report_irq_delivered(int delivered);
> void apic_reset_irq_delivered(void);
> int apic_get_irq_delivered(void);
> void cpu_set_apic_base(DeviceState *s, uint64_t val);
>diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
>index c175e7e718..ff018f1778 100644
>--- a/include/hw/i386/apic_internal.h
>+++ b/include/hw/i386/apic_internal.h
>@@ -125,7 +125,6 @@
> 
> typedef struct APICCommonState APICCommonState;
> 
>-#define TYPE_APIC_COMMON "apic-common"
> typedef struct APICCommonClass APICCommonClass;
> DECLARE_OBJ_CHECKERS(APICCommonState, APICCommonClass,
>                      APIC_COMMON, TYPE_APIC_COMMON)
>@@ -199,7 +198,6 @@ typedef struct VAPICState {
> 
> extern bool apic_report_tpr_access;
> 
>-void apic_report_irq_delivered(int delivered);
> bool apic_next_timer(APICCommonState *s, int64_t current_time);
> void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
> void apic_enable_vapic(DeviceState *d, hwaddr paddr);
>diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
>index 1db0fcee92..45bcd6f040 100644
>--- a/include/hw/rtc/mc146818rtc.h
>+++ b/include/hw/rtc/mc146818rtc.h
>@@ -55,5 +55,6 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
>                              qemu_irq intercept_irq);
> void rtc_set_memory(ISADevice *dev, int addr, int val);
> int rtc_get_memory(ISADevice *dev, int addr);
>+void qmp_rtc_reset_reinjection(Error **errp);
> 
> #endif /* HW_RTC_MC146818RTC_H */
>diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>index 2a20982066..b0f85f9384 100644
>--- a/hw/intc/apic_common.c
>+++ b/hw/intc/apic_common.c
>@@ -33,7 +33,6 @@
> #include "hw/sysbus.h"
> #include "migration/vmstate.h"
> 
>-static int apic_irq_delivered;
> bool apic_report_tpr_access;
> 
> void cpu_set_apic_base(DeviceState *dev, uint64_t val)
>@@ -122,32 +121,6 @@ void apic_handle_tpr_access_report(DeviceState *dev, target_ulong ip,
>     vapic_report_tpr_access(s->vapic, CPU(s->cpu), ip, access);
> }
> 
>-void apic_report_irq_delivered(int delivered)
>-{
>-    apic_irq_delivered += delivered;
>-
>-    trace_apic_report_irq_delivered(apic_irq_delivered);
>-}
>-
>-void apic_reset_irq_delivered(void)
>-{
>-    /* Copy this into a local variable to encourage gcc to emit a plain
>-     * register for a sys/sdt.h marker.  For details on this workaround, see:
>-     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
>-     */
>-    volatile int a_i_d = apic_irq_delivered;
>-    trace_apic_reset_irq_delivered(a_i_d);
>-
>-    apic_irq_delivered = 0;
>-}
>-
>-int apic_get_irq_delivered(void)
>-{
>-    trace_apic_get_irq_delivered(apic_irq_delivered);
>-
>-    return apic_irq_delivered;
>-}
>-
> void apic_deliver_nmi(DeviceState *dev)
> {
>     APICCommonState *s = APIC_COMMON(dev);
>diff --git a/hw/intc/apic_irqcount.c b/hw/intc/apic_irqcount.c
>new file mode 100644
>index 0000000000..0aadef1cb5
>--- /dev/null
>+++ b/hw/intc/apic_irqcount.c
>@@ -0,0 +1,53 @@
>+/*
>+ * APIC support - functions for counting the delivered IRQs.
>+ * (this code is in a separate file since it is used from the
>+ * mc146818rtc code on targets without APIC, too)
>+ *
>+ *  Copyright (c) 2011      Jan Kiszka, Siemens AG
>+ *
>+ * This library is free software; you can redistribute it and/or
>+ * modify it under the terms of the GNU Lesser General Public
>+ * License as published by the Free Software Foundation; either
>+ * version 2.1 of the License, or (at your option) any later version.
>+ *
>+ * This library is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ * Lesser General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU Lesser General Public
>+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
>+ */
>+
>+#include "qemu/osdep.h"
>+#include "hw/i386/apic.h"
>+#include "trace.h"
>+
>+static int apic_irq_delivered;
>+
>+void apic_report_irq_delivered(int delivered)
>+{
>+    apic_irq_delivered += delivered;
>+
>+    trace_apic_report_irq_delivered(apic_irq_delivered);
>+}
>+
>+void apic_reset_irq_delivered(void)
>+{
>+    /*
>+     * Copy this into a local variable to encourage gcc to emit a plain
>+     * register for a sys/sdt.h marker.  For details on this workaround, see:
>+     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
>+     */
>+    volatile int a_i_d = apic_irq_delivered;
>+    trace_apic_reset_irq_delivered(a_i_d);
>+
>+    apic_irq_delivered = 0;
>+}
>+
>+int apic_get_irq_delivered(void)
>+{
>+    trace_apic_get_irq_delivered(apic_irq_delivered);
>+
>+    return apic_irq_delivered;
>+}
>diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>index 1ebb412479..d524dc02c2 100644
>--- a/hw/rtc/mc146818rtc.c
>+++ b/hw/rtc/mc146818rtc.c
>@@ -43,11 +43,7 @@
> #include "qapi/qapi-events-misc.h"
> #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 +108,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;
>@@ -145,13 +140,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 +910,15 @@ 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:
>+        /* Slew tick policy is only available if the machine has an APIC */
>+        if (object_resolve_path_type("", TYPE_APIC_COMMON, NULL) != NULL) {

This looks like an attempt to fish out x86 machines to preserve behavior. Does this also work for PIC-only x86 machines such as -M isapc?

Best regards,
Bernhard

>+            s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
>+            break;
>+        }
>+        /* fallthrough */
>     default:
>         error_setg(errp, "Invalid lost tick policy.");
>         return;
>diff --git a/hw/intc/meson.build b/hw/intc/meson.build
>index bcbf22ff51..036ad1936b 100644
>--- a/hw/intc/meson.build
>+++ b/hw/intc/meson.build
>@@ -25,8 +25,12 @@ softmmu_ss.add(when: 'CONFIG_XILINX', if_true: files('xilinx_intc.c'))
> softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true: files('xlnx-zynqmp-ipi.c'))
> softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_PMU', if_true: files('xlnx-pmu-iomod-intc.c'))
> 
>-specific_ss.add(when: 'CONFIG_ALLWINNER_A10_PIC', if_true: files('allwinner-a10-pic.c'))
>+if config_all_devices.has_key('CONFIG_APIC') or config_all_devices.has_key('CONFIG_MC146818RTC')
>+    softmmu_ss.add(files('apic_irqcount.c'))
>+endif
> specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 'apic_common.c'))
>+
>+specific_ss.add(when: 'CONFIG_ALLWINNER_A10_PIC', if_true: files('allwinner-a10-pic.c'))
> specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gicv3_cpuif_common.c'))
> specific_ss.add(when: 'CONFIG_ARM_GICV3_TCG', if_true: files('arm_gicv3_cpuif.c'))
> specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: files('arm_gic_kvm.c'))
>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'))

On 16/12/2022 15.26, Bernhard Beschow wrote:
> 
> 
> Am 12. Dezember 2022 07:56:00 UTC schrieb Thomas Huth <thuth@redhat.com>:
>> The only reason for this code being target dependent is the apic-related
>> code in rtc_policy_slew_deliver_irq(). Since these apic functions are rather
>> simple, we can easily move them into a new, separate file (apic_irqcount.c)
>> which will always be compiled and linked if either APIC or the mc146818 device
>> are required. This way we can get rid of the #ifdef TARGET_I386 switches in
>> mc146818rtc.c and declare it in the softmmu_ss instead of specific_ss, so
>> that the code only gets compiled once for all targets.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it
>>
>> include/hw/i386/apic.h          |  2 ++
>> include/hw/i386/apic_internal.h |  2 --
>> include/hw/rtc/mc146818rtc.h    |  1 +
>> hw/intc/apic_common.c           | 27 -----------------
>> hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
>> hw/rtc/mc146818rtc.c            | 25 +++++-----------
>> hw/intc/meson.build             |  6 +++-
>> hw/rtc/meson.build              |  3 +-
>> 8 files changed, 69 insertions(+), 50 deletions(-)
>> create mode 100644 hw/intc/apic_irqcount.c
[...]
>> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>> index 1ebb412479..d524dc02c2 100644
>> --- a/hw/rtc/mc146818rtc.c
>> +++ b/hw/rtc/mc146818rtc.c
>> @@ -43,11 +43,7 @@
>> #include "qapi/qapi-events-misc.h"
>> #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 +108,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;
>> @@ -145,13 +140,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 +910,15 @@ 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:
>> +        /* Slew tick policy is only available if the machine has an APIC */
>> +        if (object_resolve_path_type("", TYPE_APIC_COMMON, NULL) != NULL) {
> 
> This looks like an attempt to fish out x86 machines to preserve behavior. Does this also work for PIC-only x86 machines such as -M isapc?

Drat, I think you might be right. Looks like the slew code might be usable 
via hw/i386/kvm/i8259.c on isapc, too... I guess I have to replace this with 
a more generic check for x86 instead...

Thanks for pointing this out, I'll try to come up with a v4!

  Thomas

On 12/12/2022 07:56, Thomas Huth wrote:

> The only reason for this code being target dependent is the apic-related
> code in rtc_policy_slew_deliver_irq(). Since these apic functions are rather
> simple, we can easily move them into a new, separate file (apic_irqcount.c)
> which will always be compiled and linked if either APIC or the mc146818 device
> are required. This way we can get rid of the #ifdef TARGET_I386 switches in
> mc146818rtc.c and declare it in the softmmu_ss instead of specific_ss, so
> that the code only gets compiled once for all targets.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it
> 
>   include/hw/i386/apic.h          |  2 ++
>   include/hw/i386/apic_internal.h |  2 --
>   include/hw/rtc/mc146818rtc.h    |  1 +
>   hw/intc/apic_common.c           | 27 -----------------
>   hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
>   hw/rtc/mc146818rtc.c            | 25 +++++-----------
>   hw/intc/meson.build             |  6 +++-
>   hw/rtc/meson.build              |  3 +-
>   8 files changed, 69 insertions(+), 50 deletions(-)
>   create mode 100644 hw/intc/apic_irqcount.c
> 
> diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
> index da1d2fe155..24069fb961 100644
> --- a/include/hw/i386/apic.h
> +++ b/include/hw/i386/apic.h
> @@ -1,6 +1,7 @@
>   #ifndef APIC_H
>   #define APIC_H
>   
> +#define TYPE_APIC_COMMON "apic-common"

Ah sorry, I should have been more specific here: what I was suggesting was to move 
the entire QOM type information into apic.h as per the normal convention, as opposed 
to just the #define. At first glance that would involve lines 128-190 in 
apic_internal.h which would also bring in APICCommonClass and APICCommonState - 
possibly the change may warrant its own commit.

On the plus side if nothing else is using TYPE_APIC_COMMON then it's likely that the 
changes will be minimal, or that's the theory anyway...

>   /* apic.c */
>   void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
> @@ -9,6 +10,7 @@ int apic_accept_pic_intr(DeviceState *s);
>   void apic_deliver_pic_intr(DeviceState *s, int level);
>   void apic_deliver_nmi(DeviceState *d);
>   int apic_get_interrupt(DeviceState *s);
> +void apic_report_irq_delivered(int delivered);
>   void apic_reset_irq_delivered(void);
>   int apic_get_irq_delivered(void);
>   void cpu_set_apic_base(DeviceState *s, uint64_t val);
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index c175e7e718..ff018f1778 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -125,7 +125,6 @@
>   
>   typedef struct APICCommonState APICCommonState;
>   
> -#define TYPE_APIC_COMMON "apic-common"
>   typedef struct APICCommonClass APICCommonClass;
>   DECLARE_OBJ_CHECKERS(APICCommonState, APICCommonClass,
>                        APIC_COMMON, TYPE_APIC_COMMON)
> @@ -199,7 +198,6 @@ typedef struct VAPICState {
>   
>   extern bool apic_report_tpr_access;
>   
> -void apic_report_irq_delivered(int delivered);
>   bool apic_next_timer(APICCommonState *s, int64_t current_time);
>   void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
>   void apic_enable_vapic(DeviceState *d, hwaddr paddr);
> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> index 1db0fcee92..45bcd6f040 100644
> --- a/include/hw/rtc/mc146818rtc.h
> +++ b/include/hw/rtc/mc146818rtc.h
> @@ -55,5 +55,6 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
>                                qemu_irq intercept_irq);
>   void rtc_set_memory(ISADevice *dev, int addr, int val);
>   int rtc_get_memory(ISADevice *dev, int addr);
> +void qmp_rtc_reset_reinjection(Error **errp);
>   
>   #endif /* HW_RTC_MC146818RTC_H */
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 2a20982066..b0f85f9384 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -33,7 +33,6 @@
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
>   
> -static int apic_irq_delivered;
>   bool apic_report_tpr_access;
>   
>   void cpu_set_apic_base(DeviceState *dev, uint64_t val)
> @@ -122,32 +121,6 @@ void apic_handle_tpr_access_report(DeviceState *dev, target_ulong ip,
>       vapic_report_tpr_access(s->vapic, CPU(s->cpu), ip, access);
>   }
>   
> -void apic_report_irq_delivered(int delivered)
> -{
> -    apic_irq_delivered += delivered;
> -
> -    trace_apic_report_irq_delivered(apic_irq_delivered);
> -}
> -
> -void apic_reset_irq_delivered(void)
> -{
> -    /* Copy this into a local variable to encourage gcc to emit a plain
> -     * register for a sys/sdt.h marker.  For details on this workaround, see:
> -     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
> -     */
> -    volatile int a_i_d = apic_irq_delivered;
> -    trace_apic_reset_irq_delivered(a_i_d);
> -
> -    apic_irq_delivered = 0;
> -}
> -
> -int apic_get_irq_delivered(void)
> -{
> -    trace_apic_get_irq_delivered(apic_irq_delivered);
> -
> -    return apic_irq_delivered;
> -}
> -
>   void apic_deliver_nmi(DeviceState *dev)
>   {
>       APICCommonState *s = APIC_COMMON(dev);
> diff --git a/hw/intc/apic_irqcount.c b/hw/intc/apic_irqcount.c
> new file mode 100644
> index 0000000000..0aadef1cb5
> --- /dev/null
> +++ b/hw/intc/apic_irqcount.c
> @@ -0,0 +1,53 @@
> +/*
> + * APIC support - functions for counting the delivered IRQs.
> + * (this code is in a separate file since it is used from the
> + * mc146818rtc code on targets without APIC, too)
> + *
> + *  Copyright (c) 2011      Jan Kiszka, Siemens AG
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/i386/apic.h"
> +#include "trace.h"
> +
> +static int apic_irq_delivered;
> +
> +void apic_report_irq_delivered(int delivered)
> +{
> +    apic_irq_delivered += delivered;
> +
> +    trace_apic_report_irq_delivered(apic_irq_delivered);
> +}
> +
> +void apic_reset_irq_delivered(void)
> +{
> +    /*
> +     * Copy this into a local variable to encourage gcc to emit a plain
> +     * register for a sys/sdt.h marker.  For details on this workaround, see:
> +     * https://sourceware.org/bugzilla/show_bug.cgi?id=13296
> +     */
> +    volatile int a_i_d = apic_irq_delivered;
> +    trace_apic_reset_irq_delivered(a_i_d);
> +
> +    apic_irq_delivered = 0;
> +}
> +
> +int apic_get_irq_delivered(void)
> +{
> +    trace_apic_get_irq_delivered(apic_irq_delivered);
> +
> +    return apic_irq_delivered;
> +}
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 1ebb412479..d524dc02c2 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -43,11 +43,7 @@
>   #include "qapi/qapi-events-misc.h"
>   #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 +108,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;
> @@ -145,13 +140,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 +910,15 @@ 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:
> +        /* Slew tick policy is only available if the machine has an APIC */
> +        if (object_resolve_path_type("", TYPE_APIC_COMMON, NULL) != NULL) {
> +            s->coalesced_timer = timer_new_ns(rtc_clock, rtc_coalesced_timer, s);
> +            break;
> +        }
> +        /* fallthrough */
>       default:
>           error_setg(errp, "Invalid lost tick policy.");
>           return;
> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> index bcbf22ff51..036ad1936b 100644
> --- a/hw/intc/meson.build
> +++ b/hw/intc/meson.build
> @@ -25,8 +25,12 @@ softmmu_ss.add(when: 'CONFIG_XILINX', if_true: files('xilinx_intc.c'))
>   softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true: files('xlnx-zynqmp-ipi.c'))
>   softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP_PMU', if_true: files('xlnx-pmu-iomod-intc.c'))
>   
> -specific_ss.add(when: 'CONFIG_ALLWINNER_A10_PIC', if_true: files('allwinner-a10-pic.c'))
> +if config_all_devices.has_key('CONFIG_APIC') or config_all_devices.has_key('CONFIG_MC146818RTC')
> +    softmmu_ss.add(files('apic_irqcount.c'))
> +endif
>   specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 'apic_common.c'))
> +
> +specific_ss.add(when: 'CONFIG_ALLWINNER_A10_PIC', if_true: files('allwinner-a10-pic.c'))
>   specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gicv3_cpuif_common.c'))
>   specific_ss.add(when: 'CONFIG_ARM_GICV3_TCG', if_true: files('arm_gicv3_cpuif.c'))
>   specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: files('arm_gic_kvm.c'))
> 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'))


ATB,

Mark.

On 12/12/2022 14.39, Mark Cave-Ayland wrote:
> On 12/12/2022 07:56, Thomas Huth wrote:
> 
>> The only reason for this code being target dependent is the apic-related
>> code in rtc_policy_slew_deliver_irq(). Since these apic functions are rather
>> simple, we can easily move them into a new, separate file (apic_irqcount.c)
>> which will always be compiled and linked if either APIC or the mc146818 
>> device
>> are required. This way we can get rid of the #ifdef TARGET_I386 switches in
>> mc146818rtc.c and declare it in the softmmu_ss instead of specific_ss, so
>> that the code only gets compiled once for all targets.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it
>>
>>   include/hw/i386/apic.h          |  2 ++
>>   include/hw/i386/apic_internal.h |  2 --
>>   include/hw/rtc/mc146818rtc.h    |  1 +
>>   hw/intc/apic_common.c           | 27 -----------------
>>   hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
>>   hw/rtc/mc146818rtc.c            | 25 +++++-----------
>>   hw/intc/meson.build             |  6 +++-
>>   hw/rtc/meson.build              |  3 +-
>>   8 files changed, 69 insertions(+), 50 deletions(-)
>>   create mode 100644 hw/intc/apic_irqcount.c
>>
>> diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
>> index da1d2fe155..24069fb961 100644
>> --- a/include/hw/i386/apic.h
>> +++ b/include/hw/i386/apic.h
>> @@ -1,6 +1,7 @@
>>   #ifndef APIC_H
>>   #define APIC_H
>> +#define TYPE_APIC_COMMON "apic-common"
> 
> Ah sorry, I should have been more specific here: what I was suggesting was 
> to move the entire QOM type information into apic.h as per the normal 
> convention, as opposed to just the #define. At first glance that would 
> involve lines 128-190 in apic_internal.h which would also bring in 
> APICCommonClass and APICCommonState - possibly the change may warrant its 
> own commit.

At least APICCommonState is target specific since it uses "X86CPU" ... so 
moving that to apic.h would be very counterproductive here.

Anyway, moving those structs is certainly way more than what is required for 
this patch, so if we decide to move anything else related to the APIC, it 
should be done in a separate patch later.

  Thomas

On 12/12/2022 13:48, Thomas Huth wrote:

> On 12/12/2022 14.39, Mark Cave-Ayland wrote:
>> On 12/12/2022 07:56, Thomas Huth wrote:
>>
>>> The only reason for this code being target dependent is the apic-related
>>> code in rtc_policy_slew_deliver_irq(). Since these apic functions are rather
>>> simple, we can easily move them into a new, separate file (apic_irqcount.c)
>>> which will always be compiled and linked if either APIC or the mc146818 device
>>> are required. This way we can get rid of the #ifdef TARGET_I386 switches in
>>> mc146818rtc.c and declare it in the softmmu_ss instead of specific_ss, so
>>> that the code only gets compiled once for all targets.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it
>>>
>>>   include/hw/i386/apic.h          |  2 ++
>>>   include/hw/i386/apic_internal.h |  2 --
>>>   include/hw/rtc/mc146818rtc.h    |  1 +
>>>   hw/intc/apic_common.c           | 27 -----------------
>>>   hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
>>>   hw/rtc/mc146818rtc.c            | 25 +++++-----------
>>>   hw/intc/meson.build             |  6 +++-
>>>   hw/rtc/meson.build              |  3 +-
>>>   8 files changed, 69 insertions(+), 50 deletions(-)
>>>   create mode 100644 hw/intc/apic_irqcount.c
>>>
>>> diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
>>> index da1d2fe155..24069fb961 100644
>>> --- a/include/hw/i386/apic.h
>>> +++ b/include/hw/i386/apic.h
>>> @@ -1,6 +1,7 @@
>>>   #ifndef APIC_H
>>>   #define APIC_H
>>> +#define TYPE_APIC_COMMON "apic-common"
>>
>> Ah sorry, I should have been more specific here: what I was suggesting was to move 
>> the entire QOM type information into apic.h as per the normal convention, as 
>> opposed to just the #define. At first glance that would involve lines 128-190 in 
>> apic_internal.h which would also bring in APICCommonClass and APICCommonState - 
>> possibly the change may warrant its own commit.
> 
> At least APICCommonState is target specific since it uses "X86CPU" ... so moving that 
> to apic.h would be very counterproductive here.

Ah okay I see now - sorry I didn't spot that earlier.

> Anyway, moving those structs is certainly way more than what is required for this 
> patch, so if we decide to move anything else related to the APIC, it should be done 
> in a separate patch later.

Agreed, it makes sense to fix the build issue first and then sort out the headers 
later given that it seems to be less than straightforward :/


ATB,

Mark.

On 12/12/22 14:48, Thomas Huth wrote:
> On 12/12/2022 14.39, Mark Cave-Ayland wrote:
>> On 12/12/2022 07:56, Thomas Huth wrote:
>>
>>> The only reason for this code being target dependent is the apic-related
>>> code in rtc_policy_slew_deliver_irq(). Since these apic functions are 
>>> rather
>>> simple, we can easily move them into a new, separate file 
>>> (apic_irqcount.c)
>>> which will always be compiled and linked if either APIC or the 
>>> mc146818 device
>>> are required. This way we can get rid of the #ifdef TARGET_I386 
>>> switches in
>>> mc146818rtc.c and declare it in the softmmu_ss instead of 
>>> specific_ss, so
>>> that the code only gets compiled once for all targets.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it
>>>
>>>   include/hw/i386/apic.h          |  2 ++
>>>   include/hw/i386/apic_internal.h |  2 --
>>>   include/hw/rtc/mc146818rtc.h    |  1 +
>>>   hw/intc/apic_common.c           | 27 -----------------
>>>   hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
>>>   hw/rtc/mc146818rtc.c            | 25 +++++-----------
>>>   hw/intc/meson.build             |  6 +++-
>>>   hw/rtc/meson.build              |  3 +-
>>>   8 files changed, 69 insertions(+), 50 deletions(-)
>>>   create mode 100644 hw/intc/apic_irqcount.c
>>>
>>> diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
>>> index da1d2fe155..24069fb961 100644
>>> --- a/include/hw/i386/apic.h
>>> +++ b/include/hw/i386/apic.h
>>> @@ -1,6 +1,7 @@
>>>   #ifndef APIC_H
>>>   #define APIC_H
>>> +#define TYPE_APIC_COMMON "apic-common"
>>
>> Ah sorry, I should have been more specific here: what I was suggesting 
>> was to move the entire QOM type information into apic.h as per the 
>> normal convention, as opposed to just the #define. At first glance 
>> that would involve lines 128-190 in apic_internal.h which would also 
>> bring in APICCommonClass and APICCommonState - possibly the change may 
>> warrant its own commit.
> 
> At least APICCommonState is target specific since it uses "X86CPU" ...

Replace it by ArchCPU ;)
> so moving that to apic.h would be very counterproductive here.
> 
> Anyway, moving those structs is certainly way more than what is required 
> for this patch, so if we decide to move anything else related to the 
> APIC, it should be done in a separate patch later.

I concur.

On 12/12/22 08:56, Thomas Huth wrote:
> The only reason for this code being target dependent is the apic-related
> code in rtc_policy_slew_deliver_irq(). Since these apic functions are rather
> simple, we can easily move them into a new, separate file (apic_irqcount.c)
> which will always be compiled and linked if either APIC or the mc146818 device
> are required. This way we can get rid of the #ifdef TARGET_I386 switches in
> mc146818rtc.c and declare it in the softmmu_ss instead of specific_ss, so
> that the code only gets compiled once for all targets.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v3: Move TYPE_APIC_COMMON from apic_internal.h to apic.h and use it

Ah, clever.

>   include/hw/i386/apic.h          |  2 ++
>   include/hw/i386/apic_internal.h |  2 --
>   include/hw/rtc/mc146818rtc.h    |  1 +
>   hw/intc/apic_common.c           | 27 -----------------
>   hw/intc/apic_irqcount.c         | 53 +++++++++++++++++++++++++++++++++
>   hw/rtc/mc146818rtc.c            | 25 +++++-----------
>   hw/intc/meson.build             |  6 +++-
>   hw/rtc/meson.build              |  3 +-
>   8 files changed, 69 insertions(+), 50 deletions(-)
>   create mode 100644 hw/intc/apic_irqcount.c