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

[B]

Am 9. Januar 2023 20:12:29 UTC schrieb 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...

Indeed!

>> 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!

Does the microvm need consideration as well?

Best regards,
Bernhard
>
> Thomas
>