Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[BB]

Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>> On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> hw/isa/vt82c686.c | 12 +++++++++++-
>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 47f2fd2669..ee745d5d49 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -546,6 +546,7 @@ struct ViaISAState {
>>>>     qemu_irq cpu_intr;
>>>>     qemu_irq *isa_irqs;
>>>>     ViaSuperIOState via_sio;
>>>> +    RTCState rtc;
>>>>     PCIIDEState ide;
>>>>     UHCIState uhci[2];
>>>>     ViaPMState pm;
>>>> @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
>>>> {
>>>>     ViaISAState *s = VIA_ISA(obj);
>>>> 
>>>> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>>     object_initialize_child(obj, "uhci1", &s->uhci[0],
>>> "vt82c686b-usb-uhci");
>>>>     object_initialize_child(obj, "uhci2", &s->uhci[1],
>>> "vt82c686b-usb-uhci");
>>>> @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error
>>> **errp)
>>>>     isa_bus_irqs(isa_bus, s->isa_irqs);
>>>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>>     i8257_dma_init(isa_bus, 0);
>>>> -    mc146818_rtc_init(isa_bus, 2000, NULL);
>>>> +
>>>> +    /* RTC */
>>>> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>>> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>>> +        return;
>>>> +    }
>>>> +    object_property_add_alias(qdev_get_machine(), "rtc-time",
>>> OBJECT(&s->rtc),
>>>> +                              "date");
>>>> +    isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>>>> 
>>>>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>>>>         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>>>> 
>>> 
>>> This actually introduces code duplication as all other places except piix4
>>> seem to still use the init function (probably to ensure that the rtc-rime
>>> alias on the machine is properly set) so I'd keep this the same as
>>> everything else and drop this patch until this init function is removed
>>> from all other places as well.
>>> 
>> 
>> Hi Zoltan,
>> 
>> Thanks for the fast reply! Regarding code homogeneity and duplication I've
>> made a similar argument for mc146818_rtc_init() in the past [1] and I've
>> learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
>> as a candidate for the embed-the-device-struct style which - again
>> incidentally - I've now done.
>
>I've seen patches embedding devices recently but in this case it looked not 
>that simple because of the rtc-time alias.
>
>> The rtc-time alias is actually only used by a couple of PPC machines where
>> Pegasos II is one of them. So the alias actually needs to be created only
>> for these machines, and identifying the cases where it has to be preserved
>> requires a lot of careful investigation. In the Pegasos II case this seems
>> especially complicated since one needs to look through several layers of
>> devices. During my work on the VT82xx south bridges I've gained some
>> knowledge such that I'd like to make this simplifying contribution.
>
>I've used it to implement the get-time-of-day rtas call with VOF in pegasos2 
>because otherwise it would need to access internals of the RTC model and/or 
>duplicate some code. Here's the message discussing this:
>
>https://lists.nongnu.org/archive/html/qemu-ppc/2021-10/msg00170.html
>
>so this alias still seems to be the simplest way.
>
>I think the primary function of this alias is not these ppc machines but some 
>QMP/HMP command or to make the guest time available from the monitor or 
>something like that so it's probably also used from there and therefore all 
>rtc probably should have it but I'm not sure about it.

Indeed, the alias seems to be a convenience for some QMP/HMP commands. AFAICS 
only the mc146818 sets the alias while it is probably not the only RTC modelled 
in QEMU. So I wonder why boards using another RTC don't need it and whether 
removing the alias constitutes a compatibility break. 

>> Our discussion makes me realize that the creation of the alias could now
>> actually be moved to the Pegasos II board. This way, the Pegasos II board
>> would both create and consume that alias, which seems to remove quite some
>> cognitive load. Do you agree? Would moving the alias to the board work for
>> you?
>
>Yes I think that would be better. This way the vt82xx and piix4 would be 
>similar and the alias would also be clear within the pegasos2 code and it also 
>has the machine directly at that point so it's clearer that way.

All in all I wonder if we need to preserve the alias for the fuloong2e board?

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan