On Tue, Mar 22, 2011 at 11:07:03PM +0900, Isaku Yamahata wrote:
> On Tue, Mar 22, 2011 at 03:40:16PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 22, 2011 at 09:50:37AM +0900, Isaku Yamahata wrote:
> > > On Mon, Mar 21, 2011 at 04:10:22PM +0200, Michael S. Tsirkin wrote:
> > > > > @@ -37,8 +37,27 @@
> > > > >
> > > > > typedef PCIHostState I440FXState;
> > > > >
> > > > > +#define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> > > > > +#define PIIX_NUM_PIRQS 4ULL /* PIRQ[A-D] */
> > > >
> > > > I've changed this to
> > > > ((uint64_t)PCI_NUM_PINS)
> > > >
> > > > Makes sense?
> > >
> > > No. The number of pirq is independent of PCI_NUM_PINS.
> >
> > OK, here's what confuses me:
> >
> > > diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> > > index e88df43..f07e19d 100644
> > > --- a/hw/piix_pci.c
> > > +++ b/hw/piix_pci.c
> > > @@ -37,8 +37,27 @@
> > >
> > > typedef PCIHostState I440FXState;
> > >
> > > +#define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> > > +#define PIIX_NUM_PIRQS 4ULL /* PIRQ[A-D] */
> > > +#define PIIX_PIRQC 0x60
> > > +
> > > typedef struct PIIX3State {
> > > PCIDevice dev;
> > > +
> > > + /*
> > > + * bitmap to track pic levels.
> > > + * The pic level is the logical OR of all the PCI irqs mapped to it
> > > + * So one PIC level is tracked by PIIX_NUM_PIRQS bits.
> > > + *
> > > + * PIRQ is mapped to PIC pins, we track it by
> > > + * PIIX_NUM_PIRQS * PIIX_NUM_PIC_IRQS = 64 bits with
> > > + * pic_irq * PIIX_NUM_PIRQS + pirq
> > > + */
> > > +#if PIIX_NUM_PIC_IRQS * PIIX_NUM_PIRQS > 64
> > > +#error "unable to encode pic state in 64bit in pic_levels."
> > > +#endif
> > > + uint64_t pic_levels;
> > > +
> > > qemu_irq *pic;
> > >
> > > /* This member isn't used. Just for save/load compatibility */
> > > @@ -254,25 +273,63 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > int *piix3_devfn, qemu_irq *
> > > }
> > >
> > > /* PIIX3 PCI to ISA bridge */
> > > +static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
> > > +{
> > > + qemu_set_irq(piix3->pic[pic_irq],
> > > + !!(piix3->pic_levels &
> > > + ((PIIX_NUM_PIRQS - 1) << (pic_irq *
> > > PIIX_NUM_PIRQS))));
> > > +}
> > > +
> > > +static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int
> > > level,
> > > + bool propagate)
> >
> > This is called from piix3_set_irq_pic and gets INTX# (0 to PCI_NUM_PINS).
>
> I used irq_num some places because of the existing code and pci.c.
> Here irq_num = PIRQ number. But irq_num = intx in pci_lost_get_pirq()
> Hmm should I avoid irq_num totally, and use intx, pirq, pic_irq?
Right, this might make it clearer.
> I hope the following clarifies the conversion.
>
> A pci device asserts a interrupt pin of intx
> |
> V
> pci_set_irq()
> |
> V
> pci_slot_get_pirq() = bus->map_irq() gets (PCIDevice, intx)
> returns pirq
> This corresponds to how pci intx lines are connected to piix3 pirq pins
> |
> V
> piix3_set_irq() = bus->set_irq() gets pirq
> |
> V
> piix3_set_irq_levels() gets pirq
> converts pirq# into pic irq
> calls piix3_set_irq_pic() with pic_irq
> |
> V
> piix3_set_irq_pic() gets pic_irq
> |
> V
> pic code
>
>
> thanks,
I think I get it.
> > > +{
> > > + int pic_irq;
> > > + uint64_t mask;
> > > +
> > > + pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
> > > + if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> > > + return;
> > > + }
> > > +
> > > + mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + irq_num);
> > > + piix3->pic_levels &= ~mask;
> > > + piix3->pic_levels |= mask * !!level;
> > > +
> > > + if (propagate) {
> > > + piix3_set_irq_pic(piix3, pic_irq);
> > > + }
> > > +}
> > >
> > > static void piix3_set_irq(void *opaque, int irq_num, int level)
> > > {
> > > - int i, pic_irq, pic_level;
> > > PIIX3State *piix3 = opaque;
> > > + piix3_set_irq_level(piix3, irq_num, level, true);
> > > +}
> > >
> > > - /* now we change the pic irq level according to the piix irq
> > > mappings */
> > > - /* XXX: optimize */
> > > - pic_irq = piix3->dev.config[0x60 + irq_num];
> > > - if (pic_irq < 16) {
> > > - /* The pic level is the logical OR of all the PCI irqs mapped
> > > - to it */
> > > - pic_level = 0;
> > > - for (i = 0; i < 4; i++) {
> > > - if (pic_irq == piix3->dev.config[0x60 + i]) {
> > > - pic_level |= pci_bus_get_irq_level(piix3->dev.bus, i);
> > > - }
> > > +/* irq routing is changed. so rebuild bitmap */
> > > +static void piix3_update_irq_levels(PIIX3State *piix3)
> > > +{
> > > + int pirq;
> > > +
> > > + piix3->pic_levels = 0;
> > > + for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
> > > + piix3_set_irq_level(piix3, pirq,
> > > + pci_bus_get_irq_level(piix3->dev.bus, pirq),
> > > + false);
> >
> > Here it is called with PIRQ # 0 to PIIX_NUM_PIRQS.
> >
> > > + }
> > > +}
> > > +
> > > +static void piix3_write_config(PCIDevice *dev,
> > > + uint32_t address, uint32_t val, int len)
> > > +{
> > > + pci_default_write_config(dev, address, val, len);
> > > + if (ranges_overlap(address, len, PIIX_PIRQC, 4)) {
> > > + PIIX3State *piix3 = DO_UPCAST(PIIX3State, dev, dev);
> > > + int pic_irq;
> > > + piix3_update_irq_levels(piix3);
> > > + for (pic_irq = 0; pic_irq < PIIX_NUM_PIC_IRQS; pic_irq++) {
> > > + piix3_set_irq_pic(piix3, pic_irq);
> > > }
> > > - qemu_set_irq(piix3->pic[pic_irq], pic_level);
> > > }
> > > }
> > >
> > > @@ -312,6 +369,15 @@ static void piix3_reset(void *opaque)
> > > pci_conf[0xab] = 0x00;
> > > pci_conf[0xac] = 0x00;
> > > pci_conf[0xae] = 0x00;
> > > +
> > > + d->pic_levels = 0;
> > > +}
> > > +
> > > +static int piix3_post_load(void *opaque, int version_id)
> > > +{
> > > + PIIX3State *piix3 = opaque;
> > > + piix3_update_irq_levels(piix3);
> > > + return 0;
> > > }
> > >
> > > static void piix3_pre_save(void *opaque)
> > > @@ -330,6 +396,7 @@ static const VMStateDescription vmstate_piix3 = {
> > > .version_id = 3,
> > > .minimum_version_id = 2,
> > > .minimum_version_id_old = 2,
> > > + .post_load = piix3_post_load,
> > > .pre_save = piix3_pre_save,
> > > .fields = (VMStateField []) {
> > > VMSTATE_PCI_DEVICE(dev, PIIX3State),
> > > @@ -372,6 +439,7 @@ static PCIDeviceInfo i440fx_info[] = {
> > > .qdev.no_user = 1,
> > > .no_hotplug = 1,
> > > .init = piix3_initfn,
> > > + .config_write = piix3_write_config,
> > > },{
> > > /* end of list */
> > > }
> > >
> >
>
> --
> yamahata
