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

[Thomas Huth]

On 02/01/2023 14.36, Thomas Huth wrote:
> On 31/12/2022 00.45, Bernhard Beschow wrote:
> > Am 29. Dezember 2022 10:58:48 UTC schrieb Thomas Huth <thuth@redhat.com>:
[...]
> > > 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!

I've now had a look at this, and it's also getting ugly: Since the property 
has to be set before realize() is done, the setting of the property has to 
be added to the mc146818_rtc_init() function. Thus this function would need 
a new parameter - and it then needs to be changed all over the place, i.e. 
also for all the non-x86 machines, defeating the idea of a default value...

Maybe it makes more sense to check for a TYPE_X86_MACHINE machine type instead?

 Thomas