[Qemu-devel] Re: [STABLE][PATCH] isa-bus: Remove bogus IRQ sharing check

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 9449 bytes --]

On 2011-02-20 00:25, Aurelien Jarno wrote:
> On Sat, Feb 19, 2011 at 06:56:22PM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Nothing prevented IRQ sharing on the ISA bus in principle. Not all
>> boards supported this, neither each and every card nor driver and OS.
>> Still, there existed valid IRQ sharing scenarios, (at least) two of them
>> can also be found in QEMU: >2 PC UARTs and the PREP IDE buses.
>>
>> So remove this artificial restriction from our ISA model and reenable
>> both PREP IDE buses.
> 
> What about the following patch from Gerd Hoffmann?
> 
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg55314.html
> 
> It only allow sharing ISA IRQ for the same kind of devices, which seems
> to also be the case for real ISA IRQ sharing, at least for the example
> that comes to my mind.

Such setups were probably the most common sharing scenarios on ISA as
then the same driver was involved and could be made aware of the sharing
more easily. Still, this has nothing to do with what the bus was capable
of in general.

I remember very well the fiddling with IRQ jumpers (later they became
software-configurable) on sound cards, I/O extension adapters, etc. The
worst you got from setting a conflict was a non-working device or a
locked-up box. I do not remember ever blowing up some hardware this way.
This also complies with what you find about ISA on the web.

So, I don't see any need for this sharing check, with or without a
restriction to different devices.

Jan

> 
> I haven't found time to try it though.
> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>>  hw/hpet.c          |    1 -
>>  hw/ide/piix.c      |    2 +-
>>  hw/ide/via.c       |    2 +-
>>  hw/isa-bus.c       |   16 +++-------------
>>  hw/isa.h           |    2 +-
>>  hw/mips_fulong2e.c |    2 +-
>>  hw/mips_malta.c    |    4 ++--
>>  hw/pc.c            |    2 +-
>>  hw/pc_piix.c       |    4 ++--
>>  hw/ppc_prep.c      |    2 +-
>>  10 files changed, 13 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/hpet.c b/hw/hpet.c
>> index 82a9a21..91ebb75 100644
>> --- a/hw/hpet.c
>> +++ b/hw/hpet.c
>> @@ -713,7 +713,6 @@ static int hpet_init(SysBusDevice *dev)
>>      s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
>>      s->capability |= ((HPET_CLK_PERIOD) << 32);
>>  
>> -    isa_reserve_irq(RTC_ISA_IRQ);
>>      qdev_init_gpio_in(&dev->qdev, hpet_handle_rtc_irq, 1);
>>  
>>      /* HPET Area */
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index d4289af..c349644 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -122,7 +122,7 @@ static void pci_piix_init_ports(PCIIDEState *d) {
>>      for (i = 0; i < 2; i++) {
>>          ide_bus_new(&d->bus[i], &d->dev.qdev, i);
>>          ide_init_ioport(&d->bus[i], port_info[i].iobase, port_info[i].iobase2);
>> -        ide_init2(&d->bus[i], isa_reserve_irq(port_info[i].isairq));
>> +        ide_init2(&d->bus[i], isa_get_irq(port_info[i].isairq));
>>  
>>          bmdma_init(&d->bus[i], &d->bmdma[i]);
>>          d->bmdma[i].bus = &d->bus[i];
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 0e90679..04f3290 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -145,7 +145,7 @@ static void vt82c686b_init_ports(PCIIDEState *d) {
>>      for (i = 0; i < 2; i++) {
>>          ide_bus_new(&d->bus[i], &d->dev.qdev, i);
>>          ide_init_ioport(&d->bus[i], port_info[i].iobase, port_info[i].iobase2);
>> -        ide_init2(&d->bus[i], isa_reserve_irq(port_info[i].isairq));
>> +        ide_init2(&d->bus[i], isa_get_irq(port_info[i].isairq));
>>  
>>          bmdma_init(&d->bus[i], &d->bmdma[i]);
>>          d->bmdma[i].bus = &d->bus[i];
>> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
>> index 6f349a5..d07aa41 100644
>> --- a/hw/isa-bus.c
>> +++ b/hw/isa-bus.c
>> @@ -25,7 +25,6 @@
>>  struct ISABus {
>>      BusState qbus;
>>      qemu_irq *irqs;
>> -    uint32_t assigned;
>>  };
>>  static ISABus *isabus;
>>  target_phys_addr_t isa_mem_base = 0;
>> @@ -61,33 +60,24 @@ void isa_bus_irqs(qemu_irq *irqs)
>>  }
>>  
>>  /*
>> - * isa_reserve_irq() reserves the ISA irq and returns the corresponding
>> - * qemu_irq entry for the i8259.
>> + * isa_get_irq() returns the corresponding qemu_irq entry for the i8259.
>>   *
>>   * This function is only for special cases such as the 'ferr', and
>>   * temporary use for normal devices until they are converted to qdev.
>>   */
>> -qemu_irq isa_reserve_irq(int isairq)
>> +qemu_irq isa_get_irq(int isairq)
>>  {
>>      if (isairq < 0 || isairq > 15) {
>>          hw_error("isa irq %d invalid", isairq);
>>      }
>> -    if (isabus->assigned & (1 << isairq)) {
>> -        hw_error("isa irq %d already assigned", isairq);
>> -    }
>> -    isabus->assigned |= (1 << isairq);
>>      return isabus->irqs[isairq];
>>  }
>>  
>>  void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq)
>>  {
>>      assert(dev->nirqs < ARRAY_SIZE(dev->isairq));
>> -    if (isabus->assigned & (1 << isairq)) {
>> -        hw_error("isa irq %d already assigned", isairq);
>> -    }
>> -    isabus->assigned |= (1 << isairq);
>>      dev->isairq[dev->nirqs] = isairq;
>> -    *p = isabus->irqs[isairq];
>> +    *p = isa_get_irq(isairq);
>>      dev->nirqs++;
>>  }
>>  
>> diff --git a/hw/isa.h b/hw/isa.h
>> index e26abfa..d2b6126 100644
>> --- a/hw/isa.h
>> +++ b/hw/isa.h
>> @@ -26,7 +26,7 @@ struct ISADeviceInfo {
>>  
>>  ISABus *isa_bus_new(DeviceState *dev);
>>  void isa_bus_irqs(qemu_irq *irqs);
>> -qemu_irq isa_reserve_irq(int isairq);
>> +qemu_irq isa_get_irq(int isairq);
>>  void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
>>  void isa_init_ioport(ISADevice *dev, uint16_t ioport);
>>  void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length);
>> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
>> index 2783ed5..3b0abdb 100644
>> --- a/hw/mips_fulong2e.c
>> +++ b/hw/mips_fulong2e.c
>> @@ -369,7 +369,7 @@ static void mips_fulong2e_init(ram_addr_t ram_size, const char *boot_device,
>>      qdev_init_nofail(eeprom);
>>  
>>      /* init other devices */
>> -    pit = pit_init(0x40, isa_reserve_irq(0));
>> +    pit = pit_init(0x40, isa_get_irq(0));
>>      cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
>>      DMA_init(0, cpu_exit_irq);
>>  
>> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
>> index 930c51c..f86bcff 100644
>> --- a/hw/mips_malta.c
>> +++ b/hw/mips_malta.c
>> @@ -919,7 +919,7 @@ void mips_malta_init (ram_addr_t ram_size,
>>      isa_bus_irqs(i8259);
>>      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
>>      usb_uhci_piix4_init(pci_bus, piix4_devfn + 2);
>> -    smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100, isa_reserve_irq(9),
>> +    smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100, isa_get_irq(9),
>>                            NULL, NULL, 0);
>>      eeprom_buf = qemu_mallocz(8 * 256); /* XXX: make this persistent */
>>      for (i = 0; i < 8; i++) {
>> @@ -930,7 +930,7 @@ void mips_malta_init (ram_addr_t ram_size,
>>          qdev_prop_set_ptr(eeprom, "data", eeprom_buf + (i * 256));
>>          qdev_init_nofail(eeprom);
>>      }
>> -    pit = pit_init(0x40, isa_reserve_irq(0));
>> +    pit = pit_init(0x40, isa_get_irq(0));
>>      cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
>>      DMA_init(0, cpu_exit_irq);
>>  
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 56bf1d6..81d7a87 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -1118,7 +1118,7 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>>  
>>      qemu_register_boot_set(pc_boot_set, *rtc_state);
>>  
>> -    pit = pit_init(0x40, isa_reserve_irq(0));
>> +    pit = pit_init(0x40, isa_get_irq(0));
>>      pcspk_init(pit);
>>  
>>      for(i = 0; i < MAX_SERIAL_PORTS; i++) {
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index 2918454..97c5754 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -114,7 +114,7 @@ static void pc_init1(ram_addr_t ram_size,
>>      }
>>      isa_bus_irqs(isa_irq);
>>  
>> -    pc_register_ferr_irq(isa_reserve_irq(13));
>> +    pc_register_ferr_irq(isa_get_irq(13));
>>  
>>      pc_vga_init(pci_enabled? pci_bus: NULL);
>>  
>> @@ -170,7 +170,7 @@ static void pc_init1(ram_addr_t ram_size,
>>          smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
>>          /* TODO: Populate SPD eeprom data.  */
>>          smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
>> -                              isa_reserve_irq(9), *cmos_s3, *smi_irq,
>> +                              isa_get_irq(9), *cmos_s3, *smi_irq,
>>                                kvm_enabled());
>>          for (i = 0; i < 8; i++) {
>>              DeviceState *eeprom;
>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>> index 6c1499a..6b22122 100644
>> --- a/hw/ppc_prep.c
>> +++ b/hw/ppc_prep.c
>> @@ -690,7 +690,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>          hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
>>      }
>>  
>> -    for(i = 0; i < 1/*MAX_IDE_BUS*/; i++) {
>> +    for(i = 0; i < MAX_IDE_BUS; i++) {
>>          isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
>>                       hd[2 * i],
>>  		     hd[2 * i + 1]);
>> -- 
>> 1.7.1
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]