Re: [Xen-devel] [PATCH v2 03/20] piix4: Add a i8259 Interrupt Controller as specified in datasheet

[Esteban Bosse <estebanbosse@xxxxxxxxx>]

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@xxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > Acked-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > Signed-off-by: Hervé Poussineau <hpoussin@xxxxxxxxxxx>
> > > Message-Id: <20171216090228.28505-8-hpoussin@xxxxxxxxxxx>
> > > Reviewed-by: Aleksandar Markovic <amarkovic@xxxxxxxxxxxx>
> > > [PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
> > > ---
> > >   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 :).


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel