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

[Mark Cave-Ayland]

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().

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.


ATB,

Mark.