| Message ID | 20191018134754.16362-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge | expand |
Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年10月18日周五 下午9:52写道： > 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); > +} > + > +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. > 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. 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. 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. Thanks, Li Qiang Author: Li Qiang <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 > > >
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); > +} > + > +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); My question is not about this patch: The function name is "qdev_init_gpio_out_named" but support more than 1 gpio, right? in this case, the name shouldn't be something like "qdev_init_gpios_out_named"? > + > 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); Why do you use the priority 1 in this case? > > + /* 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);
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); > + > +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);
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); >
On 10/22/19 10:44 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); >> +} >> + >> +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); > My question is not about this patch: > > The function name is "qdev_init_gpio_out_named" but support more than 1 > gpio, right? in this case, the name shouldn't be something like > "qdev_init_gpios_out_named"? Indeed devices can have various IRQ output lines. Note, QEMU does not intend to model full devices, but only the part required to run a guest. If a guest doesn't use some part of a device, QEMU will likely not model it. For example, sometimes a device can have N output IRQ to signal various error conditions, which are usually used by specific firmwares in embedded devices. QEMU might not model embedded boards using this device but we can find it in a generic machine which runs a full operating system. So far these OS don't care about handling these errors, so QEMU will only model the IRQ line required to run the OS, no more. This is on purpose. Now about the naming, I have no preference which form is better. >> + >> 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); > Why do you use the priority 1 in this case? >> >> + /* 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]; 16 -> ISA_NUM_IRQS >> } 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); >
On Tue, 22 Oct 2019 at 09:52, Esteban Bosse <estebanbosse@gmail.com> wrote: > > El vie, 18-10-2019 a las 15:47 +0200, Philippe Mathieu-Daudé escribió: > > +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); The simple answer to "why don't we need a cast" is "because the type of 'opaque' is 'void *', and in C there is no need to explicitly cast a 'void *' as it will be implicitly converted to the pointer type of the destination". (This is different from C++, which does require an explicit cast for void*.) For QOM types, QEMU conventionally uses the QOM casting macro to convert a pointer-to-instance to pointer-to-instance-of-parent-class and vice versa. In some places, like this one, what we have is just a void* representing opaque data having been passed around. You could use the QOM cast macro here, which would add a bit of extra type-safety, but the project doesn't have a strong convention here on whether to do so or not, so you'll often see the just-assignment code. thanks -- PMM
On Friday, October 18, 2019, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > 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(-) > > A detail: In the title: Add a i8259 -> Add an i8259 A. > 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); > + > 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); > -- > 2.21.0 > > >
On 10/22/19 11:53 AM, Aleksandar Markovic wrote: > > > On Friday, October 18, 2019, Philippe Mathieu-Daudé <philmd@redhat.com > <mailto:philmd@redhat.com>> wrote: > > 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(-) > > > > A detail: In the title: > Add a i8259 -> Add an i8259 OK.
El mar, 22-10-2019 a las 10:42 +0100, Peter Maydell escribió: > On Tue, 22 Oct 2019 at 09:52, Esteban Bosse <estebanbosse@gmail.com> > wrote: > > El vie, 18-10-2019 a las 15:47 +0200, Philippe Mathieu-Daudé > > escribió: > > > +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); > > The simple answer to "why don't we need a cast" is > "because the type of 'opaque' is 'void *', and in C there is > no need to explicitly cast a 'void *' as it will be implicitly > converted to the pointer type of the destination". (This is > different from C++, which does require an explicit cast for void*.) > > For QOM types, QEMU conventionally uses the QOM casting > macro to convert a pointer-to-instance to > pointer-to-instance-of-parent-class and vice versa. > In some places, like this one, what we have is just a > void* representing opaque data having been passed around. > You could use the QOM cast macro here, which would add > a bit of extra type-safety, but the project doesn't have > a strong convention here on whether to do so or not, so > you'll often see the just-assignment code. > > thanks > -- PMM Thank you very much for your detailed explanation :).
El mar, 22-10-2019 a las 11:35 +0200, Philippe Mathieu-Daudé escribió: > On 10/22/19 10:44 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); > > > +} > > > + > > > +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); > > My question is not about this patch: > > > > The function name is "qdev_init_gpio_out_named" but support more > > than 1 > > gpio, right? in this case, the name shouldn't be something like > > "qdev_init_gpios_out_named"? > > Indeed devices can have various IRQ output lines. > > Note, QEMU does not intend to model full devices, but only the > part required to run a guest. If a guest doesn't use some part > of a device, QEMU will likely not model it. > > For example, sometimes a device can have N output IRQ to signal > various error conditions, which are usually used by specific > firmwares in embedded devices. QEMU might not model embedded > boards using this device but we can find it in a generic machine > which runs a full operating system. So far these OS don't care > about handling these errors, so QEMU will only model the IRQ > line required to run the OS, no more. This is on purpose. > > Now about the naming, I have no preference which form is better. > > > > + > > > 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(de > > > v), > > > 0xcf9, > > > &s->rcr_mem, 1); > > Why do you use the priority 1 in this case? > > > > > > + /* 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]; > > 16 -> ISA_NUM_IRQS > > > > } 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); Thank you very much for your explanation :).
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 > >
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); + 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);