Re: [PATCH v6 28/33] hw/isa/piix3: Merge hw/isa/piix4.c

[Bernhard Beschow]

Am 12. Januar 2023 16:31:23 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>On 12/1/23 16:04, Philippe Mathieu-Daudé wrote:
>> On 9/1/23 18:23, Bernhard Beschow wrote:
>>> Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
>>> their implementations can be merged into one file for further
>>> consolidation.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Message-Id: <20221022150508.26830-37-shentey@gmail.com>
>>> ---
>>>   hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
>>>   hw/isa/piix4.c             | 285 -------------------------------------
>>>   MAINTAINERS                |   6 +-
>>>   hw/i386/Kconfig            |   2 +-
>>>   hw/isa/Kconfig             |  12 +-
>>>   hw/isa/meson.build         |   3 +-
>>>   hw/mips/Kconfig            |   2 +-
>>>   7 files changed, 165 insertions(+), 303 deletions(-)
>>>   rename hw/isa/{piix3.c => piix.c} (75%)
>>>   delete mode 100644 hw/isa/piix4.c
>> 
>>> +static void piix4_realize(PCIDevice *dev, Error **errp)
>>> +{
>>> +    PIIXState *s = PIIX_PCI_DEVICE(dev);
>>> +    PCIBus *pci_bus = pci_get_bus(dev);
>>> +    ISABus *isa_bus;
>>> +
>>> +    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
>>> +                          pci_address_space_io(dev), errp);
>>> +    if (!isa_bus) {
>>> +        return;
>>> +    }
>>> +
>>> +    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
>>> +                          "reset-control", 1);
>>> +    memory_region_add_subregion_overlap(pci_address_space_io(dev),
>>> +                                        PIIX_RCR_IOPORT, &s->rcr_mem, 1);
>>> +
>>> +    /* initialize i8259 pic */
>>> +    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
>>> +        return;
>>> +    }
>>> +
>>> +    /* initialize ISA irqs */
>>> +    isa_bus_irqs(isa_bus, s->pic.in_irqs);
>>> +
>>> +    /* initialize pit */
>>> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>> +
>>> +    /* DMA */
>>> +    i8257_dma_init(isa_bus, 0);
>>> +
>>> +    /* RTC */
>>> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>> +        return;
>>> +    }
>>> +    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
>> 
>> Pre-existing; it seems this difference with PIIX3 can be removed
>> because already taken care by calling isa_connect_gpio_out() in 
>> mc146818_rtc_init()?
>> 
>> ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq 
>> intercept_irq)
>> {
>>      DeviceState *dev;
>>      ISADevice *isadev;
>>      RTCState *s;
>> 
>>      isadev = isa_new(TYPE_MC146818_RTC);
>>      dev = DEVICE(isadev);
>>      s = MC146818_RTC(isadev);
>>      qdev_prop_set_int32(dev, "base_year", base_year);
>>      isa_realize_and_unref(isadev, bus, &error_fatal);
>>      if (intercept_irq) {
>>          qdev_connect_gpio_out(dev, 0, intercept_irq);
>>      } else {
>>          isa_connect_gpio_out(isadev, 0, s->isairq);
>> 
>
>I meant to paste:
>
>static void rtc_realizefn(DeviceState *dev, Error **errp)
>{
>    ...
>    qdev_init_gpio_out(dev, &s->irq, 1);
>
>
>> Having:
>> 
>> void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
>> {
>>      qemu_irq irq = isa_get_irq(isadev, isairq);
>>      qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
>> }
>> 
>> What do you think?
>

In "[PATCH v6 11/33] hw/i386/pc: Create RTC controllers in south bridges" 
mc146818_rtc_init() got inlined, taking into account the intercept_irq, which 
required the rtc interrupt to be wired up in board code. Since we don't have to 
deal with intercept_irq in the Malta code, wiring up of the rtc interrupt could 
be moved into PIIX4.

I would prefer to wire up the rtc interrupt in PIIX3 as well, and to re-wire it 
in board code in case of intercept_irq != NULL. That's still an open question 
which needs to be solved for PIIX4 to become a drop-in replacement for PIIX3. 
Any ideas?

Best regards,
Bernhard