| Message ID | 533593a679f7c243ddce87c9c3a7c31c6f67acd7.1300540833.git.yamahata@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On Sat, Mar 19, 2011 at 10:24:52PM +0900, Isaku Yamahata wrote: > optimize irq routing in piix_pic.c which has been a TODO. > So far piix3 tracks each pirq level and checks whether a given pic pins is > asserted by seeing if each pirq is mapped into the pic pin. > This is independent on irq routing, but data path is on slow path. > > Given that irq routing is rarely changed and asserting pic pins is on > data path, the path that asserts pic pins should be optimized and > chainging irq routing should be on slow path. > The new behavior with this patch series is to use bitmap which is addressed > by pirq and pic pins with a given irq routing. > When pirq is asserted, the bitmap is set and see if the pic pins is > asserted by checking the bitmaps. > When irq routing is changed, rebuild the bitmap and re-assert pic pins. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > Changes v1 -> v2: > - some minor clean ups > - commit log message > --- > hw/piix_pci.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++-------- > 1 files changed, 81 insertions(+), 13 deletions(-) > > 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) > +{ > + 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); > + } > +} > + > +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); > } We wouldn't need this loop if piix3_update_irq_levels called piix3_set_irq_level with propagate enabled. > - 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); Couldn't figure out why would we not want to propagate the interrupts here. Could you explain please? What happens if we do propagate them? Nothing bad, right? > + 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 */ > } > -- > 1.7.1.1
On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote: > > +static int piix3_post_load(void *opaque, int version_id) > > +{ > > + PIIX3State *piix3 = opaque; > > + piix3_update_irq_levels(piix3); > > Couldn't figure out why would we not want to > propagate the interrupts here. > Could you explain please? > What happens if we do propagate them? > Nothing bad, right? I wanted to be just conservative. If you are brave enough to change the behavior, I'm fine with propagating interrupts. If we propagate the interrupts, guest OS may see interrupts unnecessarily/spuriously injected after load. Probably such interrupts doesn't harm OSes, so there is nothing bad in theory as you said. On the other hand, I hesitated to change the existing behavior because it would be very difficult to debug it and to test many OSes.
On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote: > > +/* 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); > > + } > > +} > > + > > +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); > > } > > We wouldn't need this loop if piix3_update_irq_levels > called piix3_set_irq_level with propagate enabled. No. piix3_update_irq_levels loops over PIRQs(= 4). But we need to loop over all pic PINs(= 16) to raise/lower irqs.
On Mon, Mar 21, 2011 at 09:10:32PM +0900, Isaku Yamahata wrote: > On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote: > > > +static int piix3_post_load(void *opaque, int version_id) > > > +{ > > > + PIIX3State *piix3 = opaque; > > > + piix3_update_irq_levels(piix3); > > > > Couldn't figure out why would we not want to > > propagate the interrupts here. > > Could you explain please? > > What happens if we do propagate them? > > Nothing bad, right? > > I wanted to be just conservative. > If you are brave enough to change the behavior, I'm fine with propagating > interrupts. > > If we propagate the interrupts, guest OS may see interrupts > unnecessarily/spuriously injected after load. > Probably such interrupts doesn't harm OSes, so there is nothing > bad in theory as you said. > On the other hand, I hesitated to change the existing behavior because > it would be very difficult to debug it and to test many OSes. I expect it won't change the behaviour because the interrupts are level: at the moment e.g. pci devices already reassert interrupts on load. But I agree it better be a separate patch, and needs a lot of testing. > -- > yamahata
On Sat, Mar 19, 2011 at 10:24:52PM +0900, Isaku Yamahata wrote: > optimize irq routing in piix_pic.c which has been a TODO. > So far piix3 tracks each pirq level and checks whether a given pic pins is > asserted by seeing if each pirq is mapped into the pic pin. > This is independent on irq routing, but data path is on slow path. > > Given that irq routing is rarely changed and asserting pic pins is on > data path, the path that asserts pic pins should be optimized and > chainging irq routing should be on slow path. > The new behavior with this patch series is to use bitmap which is addressed > by pirq and pic pins with a given irq routing. > When pirq is asserted, the bitmap is set and see if the pic pins is > asserted by checking the bitmaps. > When irq routing is changed, rebuild the bitmap and re-assert pic pins. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > Changes v1 -> v2: > - some minor clean ups > - commit log message > --- > hw/piix_pci.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++-------- > 1 files changed, 81 insertions(+), 13 deletions(-) > > 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] */ I've changed this to ((uint64_t)PCI_NUM_PINS) Makes sense? > +#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) > +{ > + 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); > + } > +} > + > +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 */ > } > -- > 1.7.1.1
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. For example, ich9 has 8 pirq (PIRQ[A-H]). Probably same pin name (A-D) causes the confusion, they are irrelevant.
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). > +{ > + 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 */ > } >
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? 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, > > +{ > > + 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 */ > > } > > >
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
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) +{ + 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); + } +} + +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 */ }
optimize irq routing in piix_pic.c which has been a TODO. So far piix3 tracks each pirq level and checks whether a given pic pins is asserted by seeing if each pirq is mapped into the pic pin. This is independent on irq routing, but data path is on slow path. Given that irq routing is rarely changed and asserting pic pins is on data path, the path that asserts pic pins should be optimized and chainging irq routing should be on slow path. The new behavior with this patch series is to use bitmap which is addressed by pirq and pic pins with a given irq routing. When pirq is asserted, the bitmap is set and see if the pic pins is asserted by checking the bitmaps. When irq routing is changed, rebuild the bitmap and re-assert pic pins. Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- Changes v1 -> v2: - some minor clean ups - commit log message --- hw/piix_pci.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 81 insertions(+), 13 deletions(-)