On Thu, Mar 17, 2011 at 03:05:00PM +0900, Isaku Yamahata wrote:
> On Thu, Mar 17, 2011 at 07:29:09AM +0200, Michael S. Tsirkin wrote:
> > On Wed, Mar 16, 2011 at 06:29:15PM +0900, Isaku Yamahata wrote:
> > > Introduce accessor function to know INTx levels.
> > > It will be used later by q35.
> > > Although piix_pci tracks the intx line levels, it can be eliminated
> > > by this helper function.
> >
> > At least for piix, the right thing to IMO is to have bit per
> > IRQ, then the for loop can be replaced with a single !!. There's a TODO
> > there which this will fix. I think we can reuse pci device irq_state
> > for this: need to check. Haven't looked at q35 yet - applies there as
> > well?
>
> Yes, such bitmap optimization is possible.
> But this accessor function is still necessary,
OK, I'm convinced. It makes sense off data path,
much easier than try to unswizzle and swizzle back
to the new values.
> please see the following. (I didn't do any test yet. Just to show the idea)
> If you like it, I'll post it as separate patch.
Yes. BTW as long as we touch it, we might want some symbolic
name for constants 0x60, 16, and use PCI_NUM_PINS instead of 4.
Some more suggestions below.
Also, save/restore needs to be updated.
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 151353c..82b7daf 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -40,6 +40,7 @@ typedef PCIHostState I440FXState;
>
> typedef struct PIIX3State {
> PCIDevice dev;
> + unsigned long irq_level[16];
That's 1024 bits. We really only need 4*16 = 64 bits.
Also pic_levels might be a better name.
So just
uint64_t pic_levels;
Maybe stick a check there:
#if PCI_NUM_PINS * PIIX_NUM_PIC_IRQS > 64
#error unable to encode pic state in 64 bit in pic_levels
#endif
Also, need to clear on init?
> int32_t dummy_for_save_load_compat[4];
> qemu_irq *pic;
> } PIIX3State;
> @@ -200,25 +201,51 @@ PCIBus *i440fx_init(int *piix3_devfn, qemu_irq *pic,
> ram_addr_t ram_size)
> }
>
> /* PIIX3 PCI to ISA bridge */
> -
> static void piix3_set_irq(void *opaque, int irq_num, int level)
> {
> int i, pic_irq, pic_level;
> PIIX3State *piix3 = opaque;
>
> - /* 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);
> - }
> + if (pic_irq >= 16) {
> + return;
> + }
> +
> + /* The pic level is the logical OR of all the PCI irqs mapped to it */
> + if (level) {
> + set_bit(&piix3->irq_level[pic_irq], irq_num);
> + } else {
> + clear_bit(&piix3->irq_level[pic_irq], irq_num);
> + }
We can do this without a branch too I think (assuming uint64_t suggested
above):
mask = 0x1ull << (pic_irq * 16 + irq_num);
piix3->pic_levels &= ~mask;
piix3->pic_levels |= mask;
> + qemu_set_irq(piix3->pic[pic_irq], !!piix3->irq_level[pic_irq]);
> +}
> +
> +/* irq routing is changed. so rebuild bitmap */
> +static void piix3_update_irq_levels(PIIX3State *piix3)
> +{
> + int i;
> + for (i = 0; i < 16; i++) {
> + piix3->irq_level[i] = 0;
> + }
memset(piix3->irq_level, 0, sizeof piix3->irq_level);
> + for (i = 0; i < 4; i++) {
> + int pic_irq = piix3->dev.config[0x60 + irq_num];
> + if (pic_irq >= 16) {
> + continue;
> + }
> + if (pci_bus_get_irq_level(piix3->dev.bus, i)) {
> + set_bit(&piix3->irq_level[pic_irq], i);
> }
> - qemu_set_irq(piix3->pic[pic_irq], pic_level);
Hmm, don't we need to set the levels in guest appropriately?
There's also some duplication here.
Can't we just do
for (i = 0; i < 4; i++) {
piix3_set_irq(piix3, i, pci_bus_get_irq_level(piix3->dev.bus,
i));
}
?
> + }
> +}
> +
> +static void piix3_write_config(PCIDevice *dev,
> + uint32_t address, uint32_t val, int len)
> +{
> + PIIX3State *piix3 = DO_UPCAST(PIIX3State, dev, dev);
> +
> + pci_default_write_config(dev, address, val, len);
> + if (ranges_overlap(address, len, 0x60, 4)) {
> + piix3_update_irq_levels(piix3);
> }
> }
>
> @@ -318,6 +345,7 @@ static PCIDeviceInfo i440fx_info[] = {
> .qdev.no_user = 1,
> .no_hotplug = 1,
> .init = piix3_initfn,
> + .config_write = piix3_write_config,
> },{
> /* end of list */
> }
>
> --
> yamahata
