Re: [PATCH] hw/i386/pc_q35: Populate interrupt handlers before realizing LPC PCI function

[Bernhard Beschow <shentey@gmail.com>]

Am 19. Februar 2024 08:51:07 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 17/2/24 11:46, Bernhard Beschow wrote:
>> The interrupt handlers need to be populated before the device is realized since
>> internal devices such as the RTC are wired during realize(). If the interrupt
>> handlers aren't populated, devices such as the RTC will be wired with a NULL
>> interrupt handler, i.e. MC146818RtcState::irq is NULL.
>> 
>> Fixes: fc11ca08bc29 "hw/i386/q35: Realize LPC PCI function before accessing it"
>
>I think this commit is correct, but exposes a pre-existing bug.
>
>I noticed it for the PC equivalent, so didn't posted the
>pci_realize_and_unref() change there, but missed the Q35 is
>similarly affected.
>
>IMO the problem is how the GSI lines are allocated. The ISA
>ones are allocated twice!
>
>Before this patch, the 1st alloc is just overwritten and
>ignored, ISA RTC IRQ is assigned to the 2nd alloc.
>
>After this patch, ISA RTC IRQ is assigned to the 1st alloc,
>then the 2nd alloc wipe it, and an empty IRQ is eventually
>wired later.
>
>The proper fix is to alloc ISA IRQs just once. Either filling
>GSI with them, or having GSI take care of that.
>
>Since GSI is not a piece of HW but a concept to simplify
>developers writing x86 HW drivers, I currently think we shouldn't
>model it as a QOM container.

The qdev_connect_gpio_out_named() call below populates an internal array of IOAPIC_NUM_PINS callbacks inside the LPC device. These callbacks trigger IRQs. The RTC inside the LPC device relies on this array to be populated with valid handlers during LPC's realize, else the RTC gets wired with no/invalid callbacks. This patch fixes this array to be populated before realize. Before this patch, the array was populated after LPC's realize, causing NULL callbacks to be assigned to the RTC there.

Thus, IRQ allocations don't seem like the underlying problem to me.

The general pattern I see here is that qdev_connect_gpio_out_*() should be performed *before* realizing the device passed as the first argument. The reason is that this device could contain an arbitrarily deep nesting of internal devices which may want to be assigned valid IRQ callbacks during its realize. AFAICS this pattern would work recursively, so internal devices which have themselves internal devices would be wired correctly. This pattern may not be immediately evident since most of the time we're wiring "leaf" devices which can be wired either way.

Furthermore, it seems that qdev_get_gpio_in_*() may need to be called *after* a device's realize because the device may need to prepare its IRQs before exposing them. So it looks like qdev_get_gpio_in_*() and qdev_get_gpio_out_*() should be treated in dual manner.

Note that "IRQ forwarders" like piix_request_i8259_irq() may allow qdev_connect_gpio_out_*() to be called after a device has been realized. This pattern comes with a small performance penalty and might add some cognitive load when trying to understand code. So the above pattern seems like the preferable solution.

Best regards,
Bernhard

>
>> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/i386/pc_q35.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index d346fa3b1d..43675bf597 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -240,10 +240,10 @@ static void pc_q35_init(MachineState *machine)
>>       lpc_dev = DEVICE(lpc);
>>       qdev_prop_set_bit(lpc_dev, "smm-enabled",
>>                         x86_machine_is_smm_enabled(x86ms));
>> -    pci_realize_and_unref(lpc, host_bus, &error_fatal);
>>       for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>>           qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
>>       }
>> +    pci_realize_and_unref(lpc, host_bus, &error_fatal);
>>         rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
>>   
>