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

[Mark Cave-Ayland]

On 09/12/2022 11:15, 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.

> Signed-off-by: Thomas Huth <thuth@redhat.com>

Looks much better!

> ---
>   v2: Move the apic functions into a separate file instead of using
>       an ugly function pointer
>
>   include/hw/i386/apic.h          |  1 +
>   include/hw/i386/apic_internal.h |  1 -
>   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, 68 insertions(+), 49 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..96b9939425 100644
> --- a/include/hw/i386/apic.h
> +++ b/include/hw/i386/apic.h
> @@ -9,6 +9,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..e61ad04769 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -199,7 +199,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;
> -}
> -

Just a comment whilst reviewing this patch: apic_irq_delivered is also used by 
hw/i386/kvm/i8259.c which seems incorrect since it is a PIC rather than an APIC, but 
that's why it isn't immediately possible to move apic_irq_delivered into APICCommonState.

I suspect what should happen is that PICCommonState should have its own KVM irq 
delivery count rather than relying on the APIC one, which might actually be the 
reason that this dependency between mc146818rtc and APIC exists in the first place. 
Any x86 people care to comment?

>   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..afb1f385a3 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("", "apic-common", NULL) != NULL) {

Hmmm. These days we should be using TYPE_APIC_COMMON, however it seems that 
APICCommonState is defined in apic_internal.h rather than apic.h which seems wrong 
(it prevents you from passing an APICCommonState as an object link property as well 
as from using TYPE_APIC_COMMON).

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

Fixing up the headers to allow TYPE_APIC_COMMON rather than hard-coding "apic-common" 
would be my preference, however there is a lot of legacy code here and the fear would 
be that once you pull on that string then more will keep unravelling... so if that 
isn't something you can easily get to work I'm inclined to give a:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

regardless since it nudges things in the right direction until we can get some 
guidance from more knowledgeable x86 people.


ATB,

Mark.