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

[Thomas Huth]

On 31/12/2022 00.45, Bernhard Beschow wrote:
> Am 29. Dezember 2022 10:58:48 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>
> > ---
> > v4: Check for QEMU_ARCH_I386 instead of looking for an APIC
>
> Can we find a more appropriate name for the helpers than "apic" then? If the 
> slew tick policy is a workaround for (x86-) KVM I propose to do s/apic/kvm/ while still 
> compiling for every target.

Yes, since the IRQ-counting is also used by the old i8259 PIC, it likely 
makes sense to rename the functions.

> > static uint32_t rtc_periodic_clock_ticks(RTCState *s)
> > {
> > @@ -922,14 +911,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 on x86 */
> > +        if (arch_type == QEMU_ARCH_I386) {
>
> This reflects the intention much better than before, which is nice.
>
> How does `arch_type` play together with qemu-system-all? IIUC it should be 
> possible to load all arch backends simultaneously while `arch_type` is an 
> external symbol defined by each arch backend differently. So this seems to 
> conflict.

I assume that there still will be a main arch_type for the current selected 
machine? ... not sure how this will exactly work, though ...

> Can we just add a property such as "slew-tick-policy-available" instead? It 
> should default to false and all x86 machines would need to opt in explicitly.

Sounds like a good idea, it's certainly better than checking arch_type here 
... I'll give it a try, thanks!

 Thomas