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: Esteban Bosse <estebanbosse@gmail.com>, qemu-devel@nongnu.org
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org, "Paul Durrant" <paul@xen.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"Aleksandar Markovic" <amarkovic@wavecomp.com>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Anthony Perard" <anthony.perard@citrix.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Aleksandar Rikalo" <aleksandar.rikalo@rt-rk.com>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Richard Henderson" <rth@twiddle.net>
Subject: Re: [PATCH v2 03/20] piix4: Add a i8259 Interrupt Controller as specified in datasheet
Date: Tue, 22 Oct 2019 11:24:01 +0200	[thread overview]
Message-ID: <7861a0c5-fcc5-2e39-a403-0165d5809158@redhat.com> (raw)
In-Reply-To: <b1255a22fe4409244f7a68887137fbe43b71a08d.camel@gmail.com>

On 10/22/19 10:48 AM, Esteban Bosse wrote:
> El vie, 18-10-2019 a las 15:47 +0200, Philippe Mathieu-Daudé escribió:
>> From: Hervé Poussineau <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>
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> Message-Id: <20171216090228.28505-8-hpoussin@reactos.org>
>> Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
>> [PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]
>> Signed-off-by: Philippe Mathieu-Daudé <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);
>> +}
> I would like to understand why in `PIIX4State *s = opaque;` its not
> necessary a cast or a object macro magic.
> Something like:
> PIIX4State *s = (PIIX4State*)opaque;
> PIIX4State *s = PIIX4STATE(opaque);

I guess you mean:

#define PIIX4_PCI_DEVICE(obj) \
     OBJECT_CHECK(PIIX4State, (obj), TYPE_PIIX4_PCI_DEVICE)

IIUC the OBJECT_CHECK(STATE, OBJ, TYPE) macro verify the QEMU Object
OBJ is of the correct type TYPE, then cast it as a pointer to STATE.
This has some runtime cost.

This is useful when you deal with some child Object which is not TYPE
but inheritate TYPE from a parent, or if the object is an abstract
parent and you want to use its children TYPE implementations.

In piix4_realize(), the function piix4_request_i8259_irq() is registered 
by qemu_allocate_irqs() as a handler with 's' as opaque pointer, and we
already know 's' is of type PIIX4State, so using OBJECT_CHECK() is not
necessary.

>> +
>> +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);
>> +
>>       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);
>> +
>> +    /* 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);
>> +    }
>>   
>> -    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);
> 