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

["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.

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