Am 19. Februar 2024 08:51:07 UTC schrieb "Philippe Mathieu-Daudé"
<phi...@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é <phi...@linaro.org>
>> Signed-off-by: Bernhard Beschow <shen...@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"));
>>
>
