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

[BB]

Am 29. August 2022 19:50:10 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 29 Aug 2022, BB wrote:
>> 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?
>
>I don't know. A quick investigation shows that it seems to be added by commit 
>654a36d857ff94 which suggests something may use it (or was intended to use it 
>back then, but not sure if things have changed in the meantime). I don't think 
>any management app cares about fuloong2e but if this should be a generic thing 
>then all machine may need it. Then it's also mentioned in commit 29551fdcf4d99 
>that suggests one ought to be careful moving this around as it may cause 
>unexpected problems. But doing it from the machine init should be OK.

Then I'll create the alias in fuloong2e, too.

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan