Re: [PATCH v2 03/20] piix4: Add a i8259 Interrupt Controller as specified in datasheet - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: "Li Qiang" <liq3ea@gmail.com>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"Aleksandar Markovic" <amarkovic@wavecomp.com>,
	"Aurelien Jarno" <aurelien@aurel32.net>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org, Paul Durrant <paul@xen.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Qemu Developers <qemu-devel@nongnu.org>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Igor Mammedov <imammedo@redhat.com>,
	Anthony Perard <anthony.perard@citrix.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v2 03/20] piix4: Add a i8259 Interrupt Controller as specified in datasheet
Date: Sat, 26 Oct 2019 16:29:25 +0200	[thread overview]
Message-ID: <f2a36567-195e-f1c2-12f6-b2f10c37d933@redhat.com> (raw)
In-Reply-To: <CAKXe6SJB5cqR7yq-_TDcMzWCq7UoGvq=A9kCh11tNoqYjQgp3g@mail.gmail.com>

Hi Li,

On 10/21/19 4:59 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于 
> 2019年10月18日周五 下午9:52写道：
> 
>     From: Hervé Poussineau <hpoussin@reactos.org
>     <mailto:hpoussin@reactos.org>>
> 
>     Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4
>     gpio out.
>     Remove i8259 instanciated in malta board, to not have it twice.
> 
>     We can also remove the now unused piix4_init() function.
> 
>     Acked-by: Michael S. Tsirkin <mst@redhat.com <mailto:mst@redhat.com>>
>     Acked-by: Paolo Bonzini <pbonzini@redhat.com
>     <mailto:pbonzini@redhat.com>>
>     Signed-off-by: Hervé Poussineau <hpoussin@reactos.org
>     <mailto:hpoussin@reactos.org>>
>     Message-Id: <20171216090228.28505-8-hpoussin@reactos.org
>     <mailto:20171216090228.28505-8-hpoussin@reactos.org>>
>     Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com
>     <mailto:amarkovic@wavecomp.com>>
>     [PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>       hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++-----------
>       hw/mips/mips_malta.c | 32 +++++++++++++-------------------
>       include/hw/i386/pc.h |  1 -
>       3 files changed, 45 insertions(+), 31 deletions(-)
> 
>     diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>     index d0b18e0586..9c37c85ae2 100644
>     --- a/hw/isa/piix4.c
>     +++ b/hw/isa/piix4.c
>     @@ -24,6 +24,7 @@
>        */
> 
>       #include "qemu/osdep.h"
>     +#include "hw/irq.h"
>       #include "hw/i386/pc.h"
>       #include "hw/pci/pci.h"
>       #include "hw/isa/isa.h"
>     @@ -36,6 +37,8 @@ PCIDevice *piix4_dev;
> 
>       typedef struct PIIX4State {
>           PCIDevice dev;
>     +    qemu_irq cpu_intr;
>     +    qemu_irq *isa;
> 
>           /* Reset Control Register */
>           MemoryRegion rcr_mem;
>     @@ -94,6 +97,18 @@ static const VMStateDescription vmstate_piix4 = {
>           }
>       };
> 
>     +static void piix4_request_i8259_irq(void *opaque, int irq, int level)
>     +{
>     +    PIIX4State *s = opaque;
>     +    qemu_set_irq(s->cpu_intr, level);
>     +}
>     +
>     +static void piix4_set_i8259_irq(void *opaque, int irq, int level)
>     +{
>     +    PIIX4State *s = opaque;
>     +    qemu_set_irq(s->isa[irq], level);
>     +}
>     +
>       static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
>                                   unsigned int len)
>       {
>     @@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops = {
>       static void piix4_realize(PCIDevice *dev, Error **errp)
>       {
>           PIIX4State *s = PIIX4_PCI_DEVICE(dev);
>     +    ISABus *isa_bus;
>     +    qemu_irq *i8259_out_irq;
> 
>     -    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
>     -                     pci_address_space_io(dev), errp)) {
>     +    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
>     +                          pci_address_space_io(dev), errp);
>     +    if (!isa_bus) {
>               return;
>           }
> 
>     +    qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
>     +                            "isa", ISA_NUM_IRQS);
>     +    qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
>     +                             "intr", 1);
>     +
> 
> 
> 
> Does the piix4 hardware has the GPIO for interrupt? Seems not.

Yes it does, you can check in the datasheet:
https://www.intel.com/Assets/PDF/datasheet/290562.pdf

Page 3 is the 'Simplified Block Diagram' you see the INTR pin.

Page 24 table "2.1.5. INTERRUPT CONTROLLER/APIC SIGNALS"

   INTR: INTERRUPT. See CPU Interface Signals.

Page 26 "2.1.6. CPU INTERFACE SIGNALS"

   CPU INTERRUPT. INTR is driven by PIIX4 to signal the CPU that
   an interrupt request is pending and needs to be serviced. It
   is asynchronous with respect to SYSCLK or PCICLK and is always
   an output. The interrupt controller must be programmed following
   PCIRST# to ensure that INTR is at a known state.

> 
>           memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops, s,
>                                 "reset-control", 1);
>           memory_region_add_subregion_overlap(pci_address_space_io(dev),
>     0xcf9,
>                                               &s->rcr_mem, 1);
> 
>     +    /* initialize i8259 pic */
>     +    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s, 1);
>     +    s->isa = i8259_init(isa_bus, *i8259_out_irq);
> 
> 
> In i8259_init, we also allocate 16 input line and 1 output line.
> Seems it is duplicated with the GPIO allocation in previous.

No, this is different, here we don't allocate the 16 i8259
INPUT lines, we allocate 1 input IRQ and pass it to the
i8259_init function which uses it as its OUTPUT line.

IOW:

* i8259
   - 16 INPUT (from ISA devices)
   - 1 OUTPUT (to PIIX)

* PIIX
   - 1 INPUT (from i8259)
   - 1 OUTPUT (to CPU, on the Malta board: CPU IRQ2)

> Also
> Maybe here can uses
> i8259(isa_bus, qemu_allocate_irq(piix4_request_i8259_irq, s, 0));
> 
>     +
>     +    /* initialize ISA irqs */
>     +    isa_bus_irqs(isa_bus, s->isa);
>     +
>           piix4_dev = dev;
>       }
> 
>     -int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
>     -{
>     -    PCIDevice *d;
>     -
>     -    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
>     -    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
>     -    return d->devfn;
>     -}
>     -
>       static void piix4_class_init(ObjectClass *klass, void *data)
>       {
>           DeviceClass *dc = DEVICE_CLASS(klass);
>     diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>     index 4d9c64b36a..7d25ab6c23 100644
>     --- a/hw/mips/mips_malta.c
>     +++ b/hw/mips/mips_malta.c
>     @@ -97,7 +97,7 @@ typedef struct {
>           SysBusDevice parent_obj;
> 
>           MIPSCPSState cps;
>     -    qemu_irq *i8259;
>     +    qemu_irq i8259[16];
>       } MaltaState;
> 
>       static ISADevice *pit;
>     @@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
>           int64_t kernel_entry, bootloader_run_addr;
>           PCIBus *pci_bus;
>           ISABus *isa_bus;
>     -    qemu_irq *isa_irq;
>           qemu_irq cbus_irq, i8259_irq;
>     +    PCIDevice *pci;
>           int piix4_devfn;
>           I2CBus *smbus;
>           DriveInfo *dinfo;
>     @@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState *machine)
>           /* Board ID = 0x420 (Malta Board with CoreLV) */
>           stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
> 
>     -    /*
>     -     * We have a circular dependency problem: pci_bus depends on
>     isa_irq,
>     -     * isa_irq is provided by i8259, i8259 depends on ISA, ISA depends
>     -     * on piix4, and piix4 depends on pci_bus.  To stop the cycle
>     we have
>     -     * qemu_irq_proxy() adds an extra bit of indirection, allowing us
>     -     * to resolve the isa_irq -> i8259 dependency after i8259 is
>     initialized.
>     -     */
>     -    isa_irq = qemu_irq_proxy(&s->i8259, 16);
>     -
>           /* Northbridge */
>     -    pci_bus = gt64120_register(isa_irq);
>     +    pci_bus = gt64120_register(s->i8259);
> 
>           /* Southbridge */
>           ide_drive_get(hd, ARRAY_SIZE(hd));
> 
>     -    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
>     +    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
>     +                                          true, "PIIX4");
>     +    dev = DEVICE(pci);
>     +    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>     +    piix4_devfn = pci->devfn;
> 
>     -    /*
>     -     * Interrupt controller
>     -     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
>     -     */
>     -    s->i8259 = i8259_init(isa_bus, i8259_irq);
>     +    /* Interrupt controller */
>     +    qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
>     +    for (int i = 0; i < ISA_NUM_IRQS; i++) {
>     +        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
>     +    }
> 
> 
> Also here s->i8259 and the piix4 isa point to the same input line. Seems 
> duplicated.

'i8259_irq' might be misnamed, it is initialized as the CPU INPUT IRQ:

   mips_create_cpu(machine, s, &cbus_irq, &i8259_irq);

Then we connect the ISA PIC OUTPUT IRQ to the CPU INPUT IRQ:

   qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);

Earlier, we created the GT64120 with an array of OUTPUT IRQs:

   pci_bus = gt64120_register(s->i8259);

Here we connect the GT64120 OUTPUT IRQs from the PCI bus to
be INPUT of the ISA PIC:

   for (int i = 0; i < ISA_NUM_IRQS; i++) {
       s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
   }

There were the previously used 'proxy-irq'. Now the circular
problem is resolved:

/*
  * We have a circular dependency problem: pci_bus depends on isa_irq,
  * isa_irq is provided by i8259, i8259 depends on ISA, ISA depends
  * on piix4, and piix4 depends on pci_bus.  To stop the cycle we have
  * qemu_irq_proxy() adds an extra bit of indirection, allowing us
  * to resolve the isa_irq -> i8259 dependency after i8259 is
  * initialized.
  */

Future plan is to remove the X86 dependency on the i8259 PIC,
then we'll be able to improve the GT64120 model and simplify
how IRQs are connected.

> I have come up with a more cleaner patch as following:
> 
> Though 'i8259_init' is called in the mips_malta_init. But is uses the 
> isa bus from piix4 device.
> And seems it's more clean.
> You can test it with more tests.

You can test with:

$ make mips{,64}{,el}-softmmu/all
$ make check-venv
$ AVOCADO_TIMEOUT_EXPECTED=1 \
   tests/venv/bin/avocado \
     --show=app,ssh,console \
     run \
       -t arch:mips \
       -t arch:mipsel \
       -t arch:mips64 \
       -t arch:mips64el \
     tests/acceptance/

Regards,

Phil.

> Author: Li Qiang <liq3ea@163.com <mailto:liq3ea@163.com>>
> Date:   Mon Oct 21 22:41:17 2019 +0800
> 
>      piix4
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index d0b18e0586..66a041040a 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -24,6 +24,7 @@
>    */
> 
>   #include "qemu/osdep.h"
> +#include "hw/irq.h"
>   #include "hw/i386/pc.h"
>   #include "hw/pci/pci.h"
>   #include "hw/isa/isa.h"
> @@ -46,6 +47,7 @@ typedef struct PIIX4State {
>   #define PIIX4_PCI_DEVICE(obj) \
>       OBJECT_CHECK(PIIX4State, (obj), TYPE_PIIX4_PCI_DEVICE)
> 
> +
>   static void piix4_isa_reset(DeviceState *dev)
>   {
>       PIIX4State *d = PIIX4_PCI_DEVICE(dev);
> @@ -141,14 +143,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>       piix4_dev = dev;
>   }
> 
> -int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
> -{
> -    PCIDevice *d;
> -
> -    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
> -    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
> -    return d->devfn;
> -}
> 
>   static void piix4_class_init(ObjectClass *klass, void *data)
>   {
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 4d9c64b36a..420e0e9e80 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -28,6 +28,7 @@
>   #include "cpu.h"
>   #include "hw/i386/pc.h"
>   #include "hw/isa/superio.h"
> +//#include "hw/isa/piix4.h"
>   #include "hw/dma/i8257.h"
>   #include "hw/char/serial.h"
>   #include "net/net.h"
> @@ -97,7 +98,7 @@ typedef struct {
>       SysBusDevice parent_obj;
> 
>       MIPSCPSState cps;
> -    qemu_irq *i8259;
> +    qemu_irq i8259[ISA_NUM_IRQS];
>   } MaltaState;
> 
>   static ISADevice *pit;
> @@ -1235,8 +1236,9 @@ void mips_malta_init(MachineState *machine)
>       int64_t kernel_entry, bootloader_run_addr;
>       PCIBus *pci_bus;
>       ISABus *isa_bus;
> -    qemu_irq *isa_irq;
>       qemu_irq cbus_irq, i8259_irq;
> +    qemu_irq *i8259;
> +    PCIDevice *pci;
>       int piix4_devfn;
>       I2CBus *smbus;
>       DriveInfo *dinfo;
> @@ -1407,29 +1409,24 @@ void mips_malta_init(MachineState *machine)
>       /* Board ID = 0x420 (Malta Board with CoreLV) */
>       stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
> 
> -    /*
> -     * We have a circular dependency problem: pci_bus depends on isa_irq,
> -     * isa_irq is provided by i8259, i8259 depends on ISA, ISA depends
> -     * on piix4, and piix4 depends on pci_bus.  To stop the cycle we have
> -     * qemu_irq_proxy() adds an extra bit of indirection, allowing us
> -     * to resolve the isa_irq -> i8259 dependency after i8259 is 
> initialized.
> -     */
> -    isa_irq = qemu_irq_proxy(&s->i8259, 16);
> -
>       /* Northbridge */
> -    pci_bus = gt64120_register(isa_irq);
> +    pci_bus = gt64120_register(s->i8259);
> 
>       /* Southbridge */
>       ide_drive_get(hd, ARRAY_SIZE(hd));
> 
> -    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
> +    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
> +                                          true, "PIIX4");
> +    dev = DEVICE(pci);
> +    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> +    piix4_devfn = pci->devfn;
> 
> -    /*
> -     * Interrupt controller
> -     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
> -     */
> -    s->i8259 = i8259_init(isa_bus, i8259_irq);
> 
> +    i8259 = i8259_init(isa_bus, i8259_irq);
> +    for (int i = 0; i < ISA_NUM_IRQS; i++) {
> +        s->i8259[i] = i8259[i];
> +    }
> +    g_free(i8259);
>       isa_bus_irqs(isa_bus, s->i8259);
>       pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
>       pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
> 
>     -    isa_bus_irqs(isa_bus, s->i8259);
>           pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
>           pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
>           smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
>     diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>     index 37bfd95113..374f3e8835 100644
>     --- a/include/hw/i386/pc.h
>     +++ b/include/hw/i386/pc.h
>     @@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type, const
>     char *pci_type,
>       PCIBus *find_i440fx(void);
>       /* piix4.c */
>       extern PCIDevice *piix4_dev;
>     -int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> 
>       /* pc_sysfw.c */
>       void pc_system_flash_create(PCMachineState *pcms);
>     -- 
>     2.21.0
> 
> 