Re: [PATCH v8 4/6] hw/ppc/pegasos2: Fix PCI interrupt routing

[Mark Cave-Ayland]

On 06/03/2023 12:33, BALATON Zoltan wrote:

> According to the PegasosII schematics the PCI interrupt lines are
> connected to both the gpp pins of the Mv64361 north bridge and the
> PINT pins of the VT8231 south bridge so guests can get interrupts from
> either of these. So far we only had the MV64361 connections which
> worked for on board devices but for additional PCI devices (such as
> network or sound card added with -device) guest OSes expect interrupt
> from the ISA IRQ 9 where the firmware routes these PCI interrupts in
> VT8231 ISA bridge. After the previous patches we can now model this
> and also remove the board specific connection from mv64361. Also
> configure routing of these lines when using Virtual Open Firmware to
> match board firmware for guests that expect this.
>
> This fixes PCI interrupts on pegasos2 under Linux, MorphOS and AmigaOS.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Tested-by: Rene Engel <ReneEngel80@emailn.de>
> ---
>   hw/pci-host/mv64361.c |  4 ----
>   hw/ppc/pegasos2.c     | 26 +++++++++++++++++++++++++-
>   2 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/hw/pci-host/mv64361.c b/hw/pci-host/mv64361.c
> index 298564f1f5..19e8031a3f 100644
> --- a/hw/pci-host/mv64361.c
> +++ b/hw/pci-host/mv64361.c
> @@ -873,10 +873,6 @@ static void mv64361_realize(DeviceState *dev, Error **errp)
>       }
>       sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq);
>       qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32);
> -    /* FIXME: PCI IRQ connections may be board specific */
> -    for (i = 0; i < PCI_NUM_PINS; i++) {
> -        s->pci[1].irq[i] = qdev_get_gpio_in_named(dev, "gpp", 12 + i);
> -    }
>   }
>   
>   static void mv64361_reset(DeviceState *dev)
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index b0ada9c963..ded5dc2dc9 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -73,6 +73,8 @@ struct Pegasos2MachineState {
>       MachineState parent_obj;
>       PowerPCCPU *cpu;
>       DeviceState *mv;
> +    qemu_irq mv_pirq[PCI_NUM_PINS];
> +    qemu_irq via_pirq[PCI_NUM_PINS];
>       Vof *vof;
>       void *fdt_blob;
>       uint64_t kernel_addr;
> @@ -95,6 +97,15 @@ static void pegasos2_cpu_reset(void *opaque)
>       }
>   }
>   
> +static void pegasos2_pci_irq(void *opaque, int n, int level)
> +{
> +    Pegasos2MachineState *pm = opaque;
> +
> +    /* PCI interrupt lines are connected to both MV64361 and VT8231 */
> +    qemu_set_irq(pm->mv_pirq[n], level);
> +    qemu_set_irq(pm->via_pirq[n], level);
> +}
> +

Can you explain a bit more about how the PCI interrupt lines are connected to both 
the MV64361 and VT8231? The reason for asking is that I see a similar pattern in the 
bonito board, but there I can't see how those lines would be used because they can 
also raise a CPU interrupt, yet it is a different one compared to the 8259.

Given that we know from Bernhard's tests that the fuloong2e board works with 
pci_bus_irqs() included in via_isa_realize() which overwrites the bonito equivalent, 
I'm wondering if the mv_pirq array is actually needed at all and whether it may just 
be a debugging aid? Certainly it makes things simpler to just route everything to the 
VIA device.

>   static void pegasos2_init(MachineState *machine)
>   {
>       Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
> @@ -106,7 +117,7 @@ static void pegasos2_init(MachineState *machine)
>       I2CBus *i2c_bus;
>       const char *fwname = machine->firmware ?: PROM_FILENAME;
>       char *filename;
> -    int sz;
> +    int i, sz;
>       uint8_t *spd_data;
>   
>       /* init CPU */
> @@ -156,7 +167,11 @@ static void pegasos2_init(MachineState *machine)
>       /* Marvell Discovery II system controller */
>       pm->mv = DEVICE(sysbus_create_simple(TYPE_MV64361, -1,
>                             qdev_get_gpio_in(DEVICE(pm->cpu), 
> PPC6xx_INPUT_INT)));
> +    for (i = 0; i < PCI_NUM_PINS; i++) {
> +        pm->mv_pirq[i] = qdev_get_gpio_in_named(pm->mv, "gpp", 12 + i);
> +    }
>       pci_bus = mv64361_get_pci_bus(pm->mv, 1);
> +    pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS);

This doesn't make sense to me either, since the PCI bus IRQs should be owned by the 
device that contains the PCI bus and not the board.

>       /* VIA VT8231 South Bridge (multifunction PCI device) */
>       via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), true,
> @@ -164,6 +179,9 @@ static void pegasos2_init(MachineState *machine)
>       qdev_connect_gpio_out(DEVICE(via), 0,
>                             qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>       pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_fatal);
> +    for (i = 0; i < PCI_NUM_PINS; i++) {
> +        pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i);
> +    }
>   
>       object_property_add_alias(OBJECT(machine), "rtc-time",
>                                 object_resolve_path_component(via, "rtc"),
> @@ -269,6 +287,12 @@ static void pegasos2_machine_reset(MachineState *machine, 
> ShutdownCause reason)
>                                 PCI_INTERRUPT_LINE, 2, 0x9);
>       pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>                                 0x50, 1, 0x2);
> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
> +                              0x55, 1, 0x90);
> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
> +                              0x56, 1, 0x99);
> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
> +                              0x57, 1, 0x90);
>   
>       pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 1) << 8) |
>                                 PCI_INTERRUPT_LINE, 2, 0x109);

This should be a separate commit because it's not part of the PCI interrupt routing 
as per my comment at https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg00193.html.


ATB,

Mark.