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

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

[Bernhard Beschow]

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"

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"));
 
-- 
2.43.2

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

[Philippe Mathieu-Daudé]

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.

Why no CI test caught that?

> Fixes: fc11ca08bc29 "hw/i386/q35: Realize LPC PCI function before accessing it"
> 
> 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"));
>   

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

[Bernhard Beschow]

Am 18. Februar 2024 10:47:26 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.
>
>Why no CI test caught that?

Good question. I think the missing IRQ connection would even be hardly noticeable when running a full and modern graphical Linux distro as guest. There is rtc-test.c but I don't know if wiring is in its scope.

Best regards,
Bernhard

>
>> Fixes: fc11ca08bc29 "hw/i386/q35: Realize LPC PCI function before accessing it"
>> 
>> 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"));
>>   
>

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

[Philippe Mathieu-Daudé]

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.

> 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"));
>   

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

[Bernhard Beschow]

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"));
>>   
>