On Thu, Mar 17, 2011 at 10:49:53PM +0900, Isaku Yamahata wrote:
> optimize irq routing in piix_pic.c which has been a TODO.
>
> Cc: Michael S. Tsirkin <m...@redhat.com>
> Signed-off-by: Isaku Yamahata <yamah...@valinux.co.jp>
Some minor comments, and looks like load has a minor bug -
probably an old one as we didn't use to have a post load.
> ---
> hw/piix_pci.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 files changed, 90 insertions(+), 13 deletions(-)
>
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 2d0ad9b..80ce205 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -29,6 +29,7 @@
> #include "isa.h"
> #include "sysbus.h"
> #include "range.h"
> +#include "bitops.h"
still needed?
>
> /*
> * I440FX chipset data sheet.
> @@ -37,8 +38,32 @@
>
> typedef PCIHostState I440FXState;
>
> +#define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> +#define PIIX_NUM_PIRQS 4 /* 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
> + */
> +#define PIIX_PIC_LEVEL_MASK(pic_irq) \
> + (((uint64_t)PIIX_NUM_PIRQS - 1) << ((pic_irq) * PIIX_NUM_PIRQS))
> +#define PIIX_PIC_LEVEL_BIT(pic_irq, pirq) \
> + (1ULL << (((pic_irq) * PIIX_NUM_PIRQS) + (pirq)))
I'll be happier with these open-coded: will be clearer
without all the () that macros need.
And we can make PIIX_NUM_PIRQS ULL so won't need a cast.
But not critical.
> +
> +#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;
> +
> int32_t dummy_for_save_load_compat[4];
> qemu_irq *pic;
> } PIIX3State;
> @@ -252,25 +277,66 @@ 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_PIC_LEVEL_MASK(pic_irq)));
> +}
> +
> +static int piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level)
> +{
> + int pic_irq;
> + uint64_t mask;
> +
> + pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
> + if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> + return -1;
> + }
> +
> + mask = PIIX_PIC_LEVEL_BIT(pic_irq, irq_num);
> + piix3->pic_levels &= ~mask;
> + piix3->pic_levels |= mask * !!level;
> + return pic_irq;
> +}
>
> static void piix3_set_irq(void *opaque, int irq_num, int level)
I find the split between piix3_set_irq_level
and piix3_set_irq_level unintuitive.
It seems that we can just always call piix3_set_irq
and then piix3_set_irq_level can be inlined:
return instead of pic_irq >= 0 below.
> {
> - int i, pic_irq, pic_level;
> PIIX3State *piix3 = opaque;
> + int pic_irq;
> + pic_irq = piix3_set_irq_level(piix3, irq_num, level);
> + if (pic_irq >= 0) {
> + piix3_set_irq_pic(piix3, pic_irq);
> + }
> +}
>
> - /* 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);
> - }
> +static void piix3_reset_irq_levels(PIIX3State *piix3)
> +{
> + piix3->pic_levels = 0;
> +}
> +
Going overboard on the abstraction front IMO.
> +/* irq routing is changed. so rebuild bitmap */
> +static void piix3_rebuild_irq_levels(PIIX3State *piix3)
I iked _update_ better than rebuild, but not critical.
> +{
> + int pirq;
> +
> + piix3_reset_irq_levels(piix3);
> + for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
> + piix3_set_irq_level(piix3, pirq,
> + pci_bus_get_irq_level(piix3->dev.bus, pirq));
> + }
> +}
> +
> +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_rebuild_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);
> }
> }
>
> @@ -310,6 +376,15 @@ static void piix3_reset(void *opaque)
> pci_conf[0xab] = 0x00;
> pci_conf[0xac] = 0x00;
> pci_conf[0xae] = 0x00;
> +
> + piix3_reset_irq_levels(d);
> +}
> +
> +static int piix3_post_load(void *opaque, int version_id)
> +{
> + PIIX3State *piix3 = opaque;
> + piix3_rebuild_irq_levels(piix3);
Don't we need to set_irq_pic here as well?
And in that case, just make the for loop
part of piix3_rebuild_irq_levels.
> + return 0;
> }
>
> static void piix3_pre_save(void *opaque)
> @@ -328,6 +403,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),
> @@ -370,6 +446,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
