Re: [PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[Bernhard Beschow]

Am 27. April 2023 13:04:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Thu, 27 Apr 2023, Mark Cave-Ayland wrote:
>> On 27/04/2023 08:58, Bernhard Beschow wrote:
>>> Am 26. April 2023 12:50:08 UTC schrieb Mark Cave-Ayland 
>>> <mark.cave-ayland@ilande.co.uk>:
>>>> On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:
>>>> 
>>>>> Since pc_init1() has access to the ISABus*, retrieve the
>>>>> ISA IRQs with isa_bus_get_irq().
>>>>> 
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>>    hw/i386/pc_piix.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>> index 126b6c11df..1e90b9ff0d 100644
>>>>> --- a/hw/i386/pc_piix.c
>>>>> +++ b/hw/i386/pc_piix.c
>>>>> @@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
>>>>>        if (pcmc->pci_enabled) {
>>>>>            PCIDevice *dev;
>>>>>    -        dev = pci_create_simple(pci_bus, piix3_devfn + 1, 
>>>>> TYPE_PIIX3_IDE);
>>>>> +        dev = pci_new_multifunction(piix3_devfn + 1, false, 
>>>>> TYPE_PIIX3_IDE);
>>>>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
>>>>> +                                    isa_bus_get_irq(isa_bus, 14));
>>>>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
>>>>> +                                    isa_bus_get_irq(isa_bus, 15));
>>>>> +        pci_realize_and_unref(dev, pci_bus, &error_fatal);
>>>>> +
>>>>>            pci_ide_create_devs(dev);
>>>>>            idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
>>>>>            idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
>>>> 
>>>> Another reason this probably isn't a good idea: you're having to call 
>>>> qdev_connect_gpio_*() before realizing the device :(
>>> 
>>> Just curious: Resources like memory regions are assigned before 
>>> realization, see e.g. i440fx or q35. Interrupts are also resources. What 
>>> makes them special?
>> 
>> I think I've covered the .instance_init() vs. realize() part in my reply to 
>> Zoltan, but in terms of qdev_connect_gpio_out_named() the reasoning here is
>
>Well, not quite I'm afaid as I still have questions as it's not clear what 
>should be in init and in realize.
>
>I've looked at i440fx and it has no init just realize which does what init 
>method shoulc do anf the i440fx_init there is a legacy init function which 
>should probably be realize so this does not look to be a good example. The q35 
>model is more complex and I proably don't understand it fully but still has a 
>realize where most of the memory regions are created and an init method where 
>some tweaks are done that are mentioned in a comment as needed but not the 
>norm so it may also not be the best example so I'm not even getting why 
>Bernhard's cited these in the first place.

Let's not confuse the topics ".instance_init() vs. .realize()" and "resources". 
I440fx seems to be very old code -- so old that it still uses legacy init 
methods (not to be confused with .instance_init()" methods). I've chosen the 
examples in context of the "resources" topic.

Best regards,
Bernhard

>
>Maybe some QOM/qdev experts could give some advice here.
>
>Regards,
>BALATON Zoltan
>
>> that a device shouldn't change it's internal behaviour depending upon how it 
>> is wired. In other words a standalone device will behave exactly the same as 
>> a device connected into a machine.
>> 
>>> I'm asking since this issue seems to be the main blocker for the piix 
>>> consolidation to be merged.
>> 
>> I did have a brief catch-up with Phil at the start of the week, and he's 
>> tagged your series for review but he is really busy at the moment. As before 
>> I repeat my offer to help out if needed as I think it's a good series, but 
>> for now we're waiting for Phil to let us know what the next steps are...
>> 
>> 
>> ATB,
>> 
>> Mark.
>> 
>>