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

[Thomas Huth <thuth@redhat.com>]

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