On Tue, Jan 3, 2023 at 2:32 PM Bernhard Beschow <shen...@gmail.com> wrote:
>
>
> On Tue, Jan 3, 2023 at 9:48 AM Thomas Huth <th...@redhat.com> 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 <th...@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);
>>
>
> In order to not bake in machine-specific assumptions in the device model
> I'd move this assignment to pc_q35.c (see below).
>
> 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;
>> }
>>
>
> This section will be reused for PIIX4 in my PIIX consolidation series.
> PIIX4 is used in Malta where we want the property to be false. What you
> could do instead is to set the property between creation and realization of
> TYPE_PIIX3_DEVICE in pc_piix.c. There is also a patch in my PIIX
> consolidation series you could take advantage of:
> https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg03792.html
>
Perhaps it'd make sense to upstream part of my PIIX consolidation series up
to and including
https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg03804.html to
avoid merge conflicts? This part should not be blocked by MIPS and could
release Phil from some pressure. Since that series also depends on
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03310.html it
could be upstreamed already as well.
Best regards,
Bernhard
Having applied the patch, you can then move the assignment to rtc_state
> between pci_new_multifunction() and pci_realize_and_unref() and set the
> property like so:
>
> https://github.com/shentok/qemu/commit/2277b0abab6bc514824cd7dd76a1476485d67f50
> .
> There, you could even just set the property to true if kvm_enabled() but
> we may need a deprecation period for this. Does it make sense to add a
> deprecation message now?
>
> Moreover, setting the property in pc_piix would also just work with other
> south bridges such as VT82Cxx which I've also got working with the PC
> machine!
> https://github.com/shentok/qemu/tree/pc-via
>
> Best regards,
> Bernhard
>
> > 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(),
>> };
>>
>> --
>> 2.31.1
>>
>>
