Re: [PATCH 4/6] hw/rtc/mc146818rtc: Add a property for the availability of the slew tick policy

[Thomas Huth]

On 04/01/2023 09.55, Mark Cave-Ayland wrote:
> On 03/01/2023 08:47, Thomas Huth wrote:
>
> > We want to get rid of the "#ifdef TARGET_I386" statements in the mc146818
> > code, so we need a different way to decide whether the slew tick policy
> > is available or not. Introduce a new property "slew-tick-policy-available"
> > which can be set by the machines that support this tick policy.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   include/hw/rtc/mc146818rtc.h |  1 +
> >   hw/i386/pc_piix.c            |  1 +
> >   hw/isa/lpc_ich9.c            |  1 +
> >   hw/isa/piix3.c               |  1 +
> >   hw/rtc/mc146818rtc.c         | 16 ++++++++++------
> >   5 files changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> > index 1db0fcee92..54af63d091 100644
> > --- a/include/hw/rtc/mc146818rtc.h
> > +++ b/include/hw/rtc/mc146818rtc.h
> > @@ -45,6 +45,7 @@ struct RTCState {
> >       QEMUTimer *coalesced_timer;
> >       Notifier clock_reset_notifier;
> >       LostTickPolicy lost_tick_policy;
> > +    bool slew_tick_policy_available;
> >       Notifier suspend_notifier;
> >       QLIST_ENTRY(RTCState) link;
> >   };
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index bc9ea8cdae..382c6add3b 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -233,6 +233,7 @@ static void pc_init1(MachineState *machine,
> >           rtc_state = isa_new(TYPE_MC146818_RTC);
> >           qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
> > +        qdev_prop_set_bit(DEVICE(rtc_state), 
> > "slew-tick-policy-available", true);
> >           isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
> >           i8257_dma_init(isa_bus, 0);
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index 498175c1cc..aeab4d8549 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -733,6 +733,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
> >       /* RTC */
> >       qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
> > +    qdev_prop_set_bit(DEVICE(&lpc->rtc), "slew-tick-policy-available", 
> > true);
> >       if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
> >           return;
> >       }
> > diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> > index c68e51ddad..825b1cbee2 100644
> > --- a/hw/isa/piix3.c
> > +++ b/hw/isa/piix3.c
> > @@ -316,6 +316,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error 
> > **errp)
> >       /* RTC */
> >       qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
> > +    qdev_prop_set_bit(DEVICE(&d->rtc), "slew-tick-policy-available", true);
> >       if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
> >           return;
> >       }
> > diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> > index 947d68c257..86381a74c3 100644
> > --- a/hw/rtc/mc146818rtc.c
> > +++ b/hw/rtc/mc146818rtc.c
> > @@ -922,14 +922,16 @@ 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:
> > +#ifdef TARGET_I386
> > +        if (s->slew_tick_policy_available) {
> > +            s->coalesced_timer = timer_new_ns(rtc_clock, 
> > rtc_coalesced_timer, s);
> > +            break;
> > +        }
> > +#endif
> > +        /* fallthrough */
> >       default:
> >           error_setg(errp, "Invalid lost tick policy.");
> >           return;
> > @@ -989,6 +991,8 @@ static Property mc146818rtc_properties[] = {
> >       DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
> >       DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
> >                                  lost_tick_policy, LOST_TICK_POLICY_DISCARD),
> > +    DEFINE_PROP_BOOL("slew-tick-policy-available", RTCState,
> > +                     slew_tick_policy_available, false),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
>
> My first thought when looking at the new "slew-tick-policy-available" 
> property introduced above was that it seems to overlap with the 
> "lost_tick_policy" property defined just above it using 
> DEFINE_PROP_LOSTTICKPOLICY().

You've got a point here ... it's a little bit ugly that we have two 
user-visible properties for the lost tick policy now...

> This made me wonder if a better approach here would be to move the logic 
> that determines if LOST_TICK_POLICY_SLEW is available into the 
> "lost_tick_policy" property setter defined at 
> https://gitlab.com/qemu-project/qemu/-/blob/master/hw/core/qdev-properties-system.c#L558. 
>
> If you look at the code directly below the link above you can see how 
> set_blocksize() overrides the .set function for qdev_prop_blocksize to 
> provide additional validation, which is similar to what we are trying to do 
> here.
>
> I think it may be possible to come up with a similar solution for 
> qdev_prop_losttickpolicy which makes use of the logic you suggested before i.e.
>
>      MachineState *ms = MACHINE(qdev_get_machine());
>
>      if (!object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
>          ....
>      }
>
> which can then emit a suitable warning or return an error accordingly. A 
> quick glance at hw/core/qdev-properties-system.c suggests there are a number 
> of similar examples showing how this could be done.

Thanks, I like that idea! I'll give it a try!

 Thomas