Re: [PATCH v3] hw/rtc/mc146818rtc: Make this rtc device target independent

[Thomas Huth]

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