[Qemu-devel] Re: [PATCH v3 3/3] piix_pci: optimize set irq path

["Michael S. Tsirkin" <mst@redhat.com>]

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