| Message ID | 1287928933-11423-2-git-send-email-gleb@redhat.com |
|---|---|
| State | New |
| Headers | show |
Gleb Natapov <gleb@redhat.com> writes:
> Prevent two devices from claiming the same io port.
Really?
Your new check for double-claim is in the new isa_init_ioport(), which
is for ISADevice only. Thus, only qdevified ISA devices can opt for
this protection, by calling isa_init_ioport(). It doesn't protect from
devices who don't or can't opt in, such as PCI devices.
Anyway, we already check for double-claim in
register_ioport_{read,write}(). The check has issues --- hw_error() is
wrong there for hot plug. But it's where the check should be, isn't it?
On Mon, Oct 25, 2010 at 05:06:51PM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > Prevent two devices from claiming the same io port. > > Really? > > Your new check for double-claim is in the new isa_init_ioport(), which > is for ISADevice only. Thus, only qdevified ISA devices can opt for > this protection, by calling isa_init_ioport(). It doesn't protect from > devices who don't or can't opt in, such as PCI devices. > I didn't claim different. This obviously works only for ISA qdev devices. > Anyway, we already check for double-claim in > register_ioport_{read,write}(). The check has issues --- hw_error() is > wrong there for hot plug. But it's where the check should be, isn't it? You don't like double-claim checking? Forget about it (all 3 lines of code). The real point of the patch is to have ISA resources used by devices to be stored in common place (ISADevice) which allows get_dev_path() to be implemented. -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Mon, Oct 25, 2010 at 05:06:51PM +0200, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > Prevent two devices from claiming the same io port. >> >> Really? >> >> Your new check for double-claim is in the new isa_init_ioport(), which >> is for ISADevice only. Thus, only qdevified ISA devices can opt for >> this protection, by calling isa_init_ioport(). It doesn't protect from >> devices who don't or can't opt in, such as PCI devices. >> > I didn't claim different. This obviously works only for ISA qdev > devices. I read "Prevent two devices from claiming the same io port" as such. >> Anyway, we already check for double-claim in >> register_ioport_{read,write}(). The check has issues --- hw_error() is >> wrong there for hot plug. But it's where the check should be, isn't it? > You don't like double-claim checking? Forget about it (all 3 lines > of code). The real point of the patch is to have ISA resources used > by devices to be stored in common place (ISADevice) which allows > get_dev_path() to be implemented. We already track I/O port use, in ioport.c. I don't like that duplicated. Even less so if the dupe catches fewer double-claims than the original. Perhaps your new function should wrap around register_ioport_*() instead of supplementing it.
On Mon, Oct 25, 2010 at 08:01:13PM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Mon, Oct 25, 2010 at 05:06:51PM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > Prevent two devices from claiming the same io port. > >> > >> Really? > >> > >> Your new check for double-claim is in the new isa_init_ioport(), which > >> is for ISADevice only. Thus, only qdevified ISA devices can opt for > >> this protection, by calling isa_init_ioport(). It doesn't protect from > >> devices who don't or can't opt in, such as PCI devices. > >> > > I didn't claim different. This obviously works only for ISA qdev > > devices. > > I read "Prevent two devices from claiming the same io port" as such. > And have you read subject with that? > >> Anyway, we already check for double-claim in > >> register_ioport_{read,write}(). The check has issues --- hw_error() is > >> wrong there for hot plug. But it's where the check should be, isn't it? > > You don't like double-claim checking? Forget about it (all 3 lines > > of code). The real point of the patch is to have ISA resources used > > by devices to be stored in common place (ISADevice) which allows > > get_dev_path() to be implemented. > > We already track I/O port use, in ioport.c. I don't like that > duplicated. Even less so if the dupe catches fewer double-claims than > the original. Consider it removed although we do keep track of irqs there and this tracking is also incomplete. Why is it there? > > Perhaps your new function should wrap around register_ioport_*() instead > of supplementing it. register_ioport_*() is disconnected from qdev in general and from ISADEvice in particular, so how "wrap around register_ioport_*()" will help me to have get_dev_path() for ISABus is beyond my understanding. -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Mon, Oct 25, 2010 at 08:01:13PM +0200, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Mon, Oct 25, 2010 at 05:06:51PM +0200, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > Prevent two devices from claiming the same io port. >> >> >> >> Really? >> >> >> >> Your new check for double-claim is in the new isa_init_ioport(), which >> >> is for ISADevice only. Thus, only qdevified ISA devices can opt for >> >> this protection, by calling isa_init_ioport(). It doesn't protect from >> >> devices who don't or can't opt in, such as PCI devices. >> >> >> > I didn't claim different. This obviously works only for ISA qdev >> > devices. >> >> I read "Prevent two devices from claiming the same io port" as such. >> > And have you read subject with that? Yes. Nevertheless, I found it a bit misleading, so I commented. >> >> Anyway, we already check for double-claim in >> >> register_ioport_{read,write}(). The check has issues --- hw_error() is >> >> wrong there for hot plug. But it's where the check should be, isn't it? >> > You don't like double-claim checking? Forget about it (all 3 lines >> > of code). The real point of the patch is to have ISA resources used >> > by devices to be stored in common place (ISADevice) which allows >> > get_dev_path() to be implemented. >> >> We already track I/O port use, in ioport.c. I don't like that >> duplicated. Even less so if the dupe catches fewer double-claims than >> the original. > Consider it removed although we do keep track of irqs there and this > tracking is also incomplete. Why is it there? Where's "there"? Let's not add to the code duplication if we can help it. It's bad enough as it is. >> Perhaps your new function should wrap around register_ioport_*() instead >> of supplementing it. > register_ioport_*() is disconnected from qdev in general and from ISADEvice > in particular, so how "wrap around register_ioport_*()" will help me to > have get_dev_path() for ISABus is beyond my understanding. I was too terse, sorry. Maybe we should have functions for ISADevices to register ISA resources. Functions that are *not* disconnected from qdev. Instead of code like register_ioport_write(pm_io_base, 64, 2, pm_ioport_writew, s); register_ioport_read(pm_io_base, 64, 2, pm_ioport_readw, s); register_ioport_write(pm_io_base, 64, 4, pm_ioport_writel, s); register_ioport_read(pm_io_base, 64, 4, pm_ioport_readl, s); isa_init_ioport_range(dev, pm_ioport_base, 64); or, with Avi's proposed interface ioport_register(&s->ioport, pm_io_base, 64); isa_init_ioport_range(dev, pm_ioport_base, 64); we'd get something like isa_ioport_register(dev, &s->ioport, pm_io_base, 64); Isn't that neater? Since some PCI devices also register ISA resources, we'd have to offer functions for them as well, properly integrated. Now, I didn't mean to ask you to do all that as a precondition for getting your patch in. I was just observing.
On Tue, Oct 26, 2010 at 11:49:26AM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Mon, Oct 25, 2010 at 08:01:13PM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > On Mon, Oct 25, 2010 at 05:06:51PM +0200, Markus Armbruster wrote: > >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> > >> >> > Prevent two devices from claiming the same io port. > >> >> > >> >> Really? > >> >> > >> >> Your new check for double-claim is in the new isa_init_ioport(), which > >> >> is for ISADevice only. Thus, only qdevified ISA devices can opt for > >> >> this protection, by calling isa_init_ioport(). It doesn't protect from > >> >> devices who don't or can't opt in, such as PCI devices. > >> >> > >> > I didn't claim different. This obviously works only for ISA qdev > >> > devices. > >> > >> I read "Prevent two devices from claiming the same io port" as such. > >> > > And have you read subject with that? > > Yes. Nevertheless, I found it a bit misleading, so I commented. > Well, I hope it is clear now. > >> >> Anyway, we already check for double-claim in > >> >> register_ioport_{read,write}(). The check has issues --- hw_error() is > >> >> wrong there for hot plug. But it's where the check should be, isn't it? > >> > You don't like double-claim checking? Forget about it (all 3 lines > >> > of code). The real point of the patch is to have ISA resources used > >> > by devices to be stored in common place (ISADevice) which allows > >> > get_dev_path() to be implemented. > >> > >> We already track I/O port use, in ioport.c. I don't like that > >> duplicated. Even less so if the dupe catches fewer double-claims than > >> the original. > > Consider it removed although we do keep track of irqs there and this > > tracking is also incomplete. Why is it there? > > Where's "there"? > In ISADevice. Look for "isabus->assigned" in isa-bus.c. > Let's not add to the code duplication if we can help it. It's bad > enough as it is. > > >> Perhaps your new function should wrap around register_ioport_*() instead > >> of supplementing it. > > register_ioport_*() is disconnected from qdev in general and from ISADEvice > > in particular, so how "wrap around register_ioport_*()" will help me to > > have get_dev_path() for ISABus is beyond my understanding. > > I was too terse, sorry. > > Maybe we should have functions for ISADevices to register ISA resources. > Functions that are *not* disconnected from qdev. Instead of code like > > register_ioport_write(pm_io_base, 64, 2, pm_ioport_writew, s); > register_ioport_read(pm_io_base, 64, 2, pm_ioport_readw, s); > register_ioport_write(pm_io_base, 64, 4, pm_ioport_writel, s); > register_ioport_read(pm_io_base, 64, 4, pm_ioport_readl, s); > isa_init_ioport_range(dev, pm_ioport_base, 64); > > or, with Avi's proposed interface > > ioport_register(&s->ioport, pm_io_base, 64); > isa_init_ioport_range(dev, pm_ioport_base, 64); > > we'd get something like > > isa_ioport_register(dev, &s->ioport, pm_io_base, 64); > > Isn't that neater? > Neater, but out of scope for my work. Obviously all resource management should go via qdev at the end. > Since some PCI devices also register ISA resources, we'd have to > offer functions for them as well, properly integrated. > > Now, I didn't mean to ask you to do all that as a precondition for > getting your patch in. I was just observing. OK. -- Gleb.
diff --git a/hw/cs4231a.c b/hw/cs4231a.c index 4d5ce5c..598f032 100644 --- a/hw/cs4231a.c +++ b/hw/cs4231a.c @@ -645,6 +645,7 @@ static int cs4231a_initfn (ISADevice *dev) isa_init_irq (dev, &s->pic, s->irq); for (i = 0; i < 4; i++) { + isa_init_ioport(dev, i); register_ioport_write (s->port + i, 1, 1, cs_write, s); register_ioport_read (s->port + i, 1, 1, cs_read, s); } diff --git a/hw/fdc.c b/hw/fdc.c index c159dcb..1f38d0d 100644 --- a/hw/fdc.c +++ b/hw/fdc.c @@ -1983,6 +1983,9 @@ static int isabus_fdc_init1(ISADevice *dev) &fdctrl_write_port, fdctrl); register_ioport_write(iobase + 0x07, 1, 1, &fdctrl_write_port, fdctrl); + isa_init_ioport_range(dev, iobase + 1, 5); + isa_init_ioport(dev, iobase + 7); + isa_init_irq(&isa->busdev, &fdctrl->irq, isairq); fdctrl->dma_chann = dma_chann; diff --git a/hw/gus.c b/hw/gus.c index e9016d8..ff9e7c7 100644 --- a/hw/gus.c +++ b/hw/gus.c @@ -264,20 +264,24 @@ static int gus_initfn (ISADevice *dev) register_ioport_write (s->port, 1, 1, gus_writeb, s); register_ioport_write (s->port, 1, 2, gus_writew, s); + isa_init_ioport_range(dev, s->port, 2); register_ioport_read ((s->port + 0x100) & 0xf00, 1, 1, gus_readb, s); register_ioport_read ((s->port + 0x100) & 0xf00, 1, 2, gus_readw, s); + isa_init_ioport_range(dev, (s->port + 0x100) & 0xf00, 2); register_ioport_write (s->port + 6, 10, 1, gus_writeb, s); register_ioport_write (s->port + 6, 10, 2, gus_writew, s); register_ioport_read (s->port + 6, 10, 1, gus_readb, s); register_ioport_read (s->port + 6, 10, 2, gus_readw, s); + isa_init_ioport_range(dev, s->port + 6, 10); register_ioport_write (s->port + 0x100, 8, 1, gus_writeb, s); register_ioport_write (s->port + 0x100, 8, 2, gus_writew, s); register_ioport_read (s->port + 0x100, 8, 1, gus_readb, s); register_ioport_read (s->port + 0x100, 8, 2, gus_readw, s); + isa_init_ioport_range(dev, s->port + 0x100, 8); DMA_register_channel (s->emu.gusdma, GUS_read_DMA, s); s->emu.himemaddr = s->himem; diff --git a/hw/ide/isa.c b/hw/ide/isa.c index 6b57e0d..163ffba 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -70,6 +70,8 @@ static int isa_ide_initfn(ISADevice *dev) ide_bus_new(&s->bus, &s->dev.qdev); ide_init_ioport(&s->bus, s->iobase, s->iobase2); isa_init_irq(dev, &s->irq, s->isairq); + isa_init_ioport_range(dev, s->iobase, 8); + isa_init_ioport(dev, s->iobase2); ide_init2(&s->bus, s->irq); vmstate_register(&dev->qdev, 0, &vmstate_ide_isa, s); return 0; diff --git a/hw/isa-bus.c b/hw/isa-bus.c index 4e306de..c509d56 100644 --- a/hw/isa-bus.c +++ b/hw/isa-bus.c @@ -26,6 +26,7 @@ struct ISABus { BusState qbus; qemu_irq *irqs; uint32_t assigned; + uint32_t assigned_iop[65536/32]; }; static ISABus *isabus; target_phys_addr_t isa_mem_base = 0; @@ -92,6 +93,36 @@ void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq) dev->nirqs++; } +static void isa_init_ioport_one(ISADevice *dev, uint16_t ioport) +{ + assert(dev->nioports < ARRAY_SIZE(dev->ioports)); + if (isabus->assigned_iop[ioport >> 5] & (1 << (ioport & 0x1f))) { + fprintf(stderr, "isa ioport 0x%x already assigned\n", ioport); + exit(1); + } + isabus->assigned_iop[ioport >> 5] |= (1 << (ioport & 0x1f)); + dev->ioports[dev->nioports++] = ioport; +} + +static int isa_cmp_ports(const void *p1, const void *p2) +{ + return *(uint16_t*)p1 - *(uint16_t*)p2; +} + +void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length) +{ + int i; + for (i = start; i < start + length; i++) { + isa_init_ioport_one(dev, i); + } + qsort(dev->ioports, dev->nioports, sizeof(dev->ioports[0]), isa_cmp_ports); +} + +void isa_init_ioport(ISADevice *dev, uint16_t ioport) +{ + isa_init_ioport_range(dev, ioport, 1); +} + static int isa_qdev_init(DeviceState *qdev, DeviceInfo *base) { ISADevice *dev = DO_UPCAST(ISADevice, qdev, qdev); diff --git a/hw/isa.h b/hw/isa.h index aaf0272..4794b76 100644 --- a/hw/isa.h +++ b/hw/isa.h @@ -14,6 +14,8 @@ struct ISADevice { DeviceState qdev; uint32_t isairq[2]; int nirqs; + uint16_t ioports[32]; + int nioports; }; typedef int (*isa_qdev_initfn)(ISADevice *dev); @@ -26,6 +28,8 @@ ISABus *isa_bus_new(DeviceState *dev); void isa_bus_irqs(qemu_irq *irqs); qemu_irq isa_reserve_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); void isa_qdev_register(ISADeviceInfo *info); ISADevice *isa_create(const char *name); ISADevice *isa_create_simple(const char *name); diff --git a/hw/m48t59.c b/hw/m48t59.c index c7492a6..75a94e1 100644 --- a/hw/m48t59.c +++ b/hw/m48t59.c @@ -680,6 +680,7 @@ M48t59State *m48t59_init_isa(uint32_t io_base, uint16_t size, int type) if (io_base != 0) { register_ioport_read(io_base, 0x04, 1, NVRAM_readb, s); register_ioport_write(io_base, 0x04, 1, NVRAM_writeb, s); + isa_init_ioport_range(dev, io_base, 4); } return s; diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c index 2b91fa8..6466aff 100644 --- a/hw/mc146818rtc.c +++ b/hw/mc146818rtc.c @@ -605,6 +605,7 @@ static int rtc_initfn(ISADevice *dev) register_ioport_write(base, 2, 1, cmos_ioport_write, s); register_ioport_read(base, 2, 1, cmos_ioport_read, s); + isa_init_ioport_range(dev, base, 2); qdev_set_legacy_instance_id(&dev->qdev, base, 2); qemu_register_reset(rtc_reset, s); diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c index 03a5a1f..3ff0d89 100644 --- a/hw/ne2000-isa.c +++ b/hw/ne2000-isa.c @@ -68,14 +68,17 @@ static int isa_ne2000_initfn(ISADevice *dev) register_ioport_write(isa->iobase, 16, 1, ne2000_ioport_write, s); register_ioport_read(isa->iobase, 16, 1, ne2000_ioport_read, s); + isa_init_ioport_range(dev, isa->iobase, 16); register_ioport_write(isa->iobase + 0x10, 1, 1, ne2000_asic_ioport_write, s); register_ioport_read(isa->iobase + 0x10, 1, 1, ne2000_asic_ioport_read, s); register_ioport_write(isa->iobase + 0x10, 2, 2, ne2000_asic_ioport_write, s); register_ioport_read(isa->iobase + 0x10, 2, 2, ne2000_asic_ioport_read, s); + isa_init_ioport_range(dev, isa->iobase + 0x10, 2); register_ioport_write(isa->iobase + 0x1f, 1, 1, ne2000_reset_ioport_write, s); register_ioport_read(isa->iobase + 0x1f, 1, 1, ne2000_reset_ioport_read, s); + isa_init_ioport(dev, isa->iobase + 0x1f); isa_init_irq(dev, &s->irq, isa->isairq); diff --git a/hw/parallel.c b/hw/parallel.c index 6b11672..6270b53 100644 --- a/hw/parallel.c +++ b/hw/parallel.c @@ -481,16 +481,21 @@ static int parallel_isa_initfn(ISADevice *dev) if (s->hw_driver) { register_ioport_write(base, 8, 1, parallel_ioport_write_hw, s); register_ioport_read(base, 8, 1, parallel_ioport_read_hw, s); + isa_init_ioport_range(dev, base, 8); + register_ioport_write(base+4, 1, 2, parallel_ioport_eppdata_write_hw2, s); register_ioport_read(base+4, 1, 2, parallel_ioport_eppdata_read_hw2, s); register_ioport_write(base+4, 1, 4, parallel_ioport_eppdata_write_hw4, s); register_ioport_read(base+4, 1, 4, parallel_ioport_eppdata_read_hw4, s); + isa_init_ioport(dev, base+4); register_ioport_write(base+0x400, 8, 1, parallel_ioport_ecp_write, s); register_ioport_read(base+0x400, 8, 1, parallel_ioport_ecp_read, s); + isa_init_ioport_range(dev, base+0x400, 8); } else { register_ioport_write(base, 8, 1, parallel_ioport_write_sw, s); register_ioport_read(base, 8, 1, parallel_ioport_read_sw, s); + isa_init_ioport_range(dev, base, 8); } return 0; } diff --git a/hw/pckbd.c b/hw/pckbd.c index 6e4e406..e736505 100644 --- a/hw/pckbd.c +++ b/hw/pckbd.c @@ -484,10 +484,13 @@ static int i8042_initfn(ISADevice *dev) register_ioport_read(0x60, 1, 1, kbd_read_data, s); register_ioport_write(0x60, 1, 1, kbd_write_data, s); + isa_init_ioport(dev, 0x60); register_ioport_read(0x64, 1, 1, kbd_read_status, s); register_ioport_write(0x64, 1, 1, kbd_write_command, s); + isa_init_ioport(dev, 0x64); register_ioport_read(0x92, 1, 1, ioport92_read, s); register_ioport_write(0x92, 1, 1, ioport92_write, s); + isa_init_ioport(dev, 0x92); s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s); s->mouse = ps2_mouse_init(kbd_update_aux_irq, s); diff --git a/hw/sb16.c b/hw/sb16.c index 78590a7..c9d37ad 100644 --- a/hw/sb16.c +++ b/hw/sb16.c @@ -1368,16 +1368,20 @@ static int sb16_initfn (ISADevice *dev) for (i = 0; i < ARRAY_SIZE (dsp_write_ports); i++) { register_ioport_write (s->port + dsp_write_ports[i], 1, 1, dsp_write, s); + isa_init_ioport(dev, s->port + dsp_write_ports[i]); } for (i = 0; i < ARRAY_SIZE (dsp_read_ports); i++) { register_ioport_read (s->port + dsp_read_ports[i], 1, 1, dsp_read, s); + isa_init_ioport(dev, s->port + dsp_read_ports[i]); } register_ioport_write (s->port + 0x4, 1, 1, mixer_write_indexb, s); register_ioport_write (s->port + 0x4, 1, 2, mixer_write_indexw, s); + isa_init_ioport(dev, s->port + 0x4); register_ioport_read (s->port + 0x5, 1, 1, mixer_read, s); register_ioport_write (s->port + 0x5, 1, 1, mixer_write_datab, s); + isa_init_ioport(dev, s->port + 0x5); DMA_register_channel (s->hdma, SB_read_DMA, s); DMA_register_channel (s->dma, SB_read_DMA, s); diff --git a/hw/serial.c b/hw/serial.c index 9ebc452..20902ae 100644 --- a/hw/serial.c +++ b/hw/serial.c @@ -778,6 +778,7 @@ static int serial_isa_initfn(ISADevice *dev) register_ioport_write(isa->iobase, 8, 1, serial_ioport_write, s); register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, s); + isa_init_ioport_range(dev, isa->iobase, 8); return 0; }
Prevent two devices from claiming the same io port. Signed-off-by: Gleb Natapov <gleb@redhat.com> --- hw/cs4231a.c | 1 + hw/fdc.c | 3 +++ hw/gus.c | 4 ++++ hw/ide/isa.c | 2 ++ hw/isa-bus.c | 31 +++++++++++++++++++++++++++++++ hw/isa.h | 4 ++++ hw/m48t59.c | 1 + hw/mc146818rtc.c | 1 + hw/ne2000-isa.c | 3 +++ hw/parallel.c | 5 +++++ hw/pckbd.c | 3 +++ hw/sb16.c | 4 ++++ hw/serial.c | 1 + 13 files changed, 63 insertions(+), 0 deletions(-)