[Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Gleb Natapov]

PCI bus already has one. For ISA bus this patch series uses device's
ioports to uniquely describe it. For isa-ide, for example, get_dev_path
method returns:
01f0-01f7,03f6 for first IDE controller
0170-0177,0376 for second one

Gleb Natapov (2):
  Keep track of ISA ports ISA device is using in qdev.
  Add get_dev_path callback to ISA bus in qdev.

 hw/cs4231a.c     |    1 +
 hw/fdc.c         |    3 ++
 hw/gus.c         |    4 +++
 hw/ide/isa.c     |    2 +
 hw/isa-bus.c     |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 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, 102 insertions(+), 0 deletions(-)

[Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev.

[Gleb Natapov]

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(-)

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;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] Add get_dev_path callback to ISA bus in qdev.

[Gleb Natapov]

Use device ioports to create unique device path.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 hw/isa-bus.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index c509d56..ed0b665 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -32,11 +32,13 @@ static ISABus *isabus;
 target_phys_addr_t isa_mem_base = 0;
 
 static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent);
+static char *isabus_get_dev_path(DeviceState *dev);
 
 static struct BusInfo isa_bus_info = {
     .name      = "ISA",
     .size      = sizeof(ISABus),
     .print_dev = isabus_dev_print,
+    .get_dev_path = isabus_get_dev_path,
 };
 
 ISABus *isa_bus_new(DeviceState *dev)
@@ -193,4 +195,41 @@ static void isabus_register_devices(void)
     sysbus_register_withprop(&isabus_bridge_info);
 }
 
+static int find_range_end(ISADevice *d, int i)
+{
+    uint16_t p = d->ioports[i++];
+
+    while (i < d->nioports) {
+        if (d->ioports[i] - p != 1) {
+            break;
+        }
+        p = d->ioports[i++];
+    }
+    return i - 1;
+}
+
+static char *isabus_get_dev_path(DeviceState *dev)
+{
+    ISADevice *d = (ISADevice*)dev;
+    char path[100];
+    int i = 0, off = 0;
+
+    while (i < d->nioports) {
+        int e = find_range_end(d, i);
+        if (off)
+            path[off++] = ',';
+        if (i == e) {
+            off += snprintf(path + off, sizeof(path) - off, "%04x",
+                            d->ioports[i]);
+            i++;
+        } else {
+            off += snprintf(path + off, sizeof(path) - off, "%04x-%04x",
+                            d->ioports[i], d->ioports[e]);
+            i = e + 1;
+        }
+    }
+
+    return strdup(path);
+}
+
 device_init(isabus_register_devices)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev.

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Markus Armbruster]

Gleb Natapov <gleb@redhat.com> writes:

> PCI bus already has one. For ISA bus this patch series uses device's
> ioports to uniquely describe it. For isa-ide, for example, get_dev_path
> method returns:
> 01f0-01f7,03f6 for first IDE controller
> 0170-0177,0376 for second one

Any I/O port used by the device identifies it.  I'd say a common
identifier is the "I/O base", the lowest I/O port used.

Re: [Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Gleb Natapov]

On Mon, Oct 25, 2010 at 05:10:15PM +0200, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > PCI bus already has one. For ISA bus this patch series uses device's
> > ioports to uniquely describe it. For isa-ide, for example, get_dev_path
> > method returns:
> > 01f0-01f7,03f6 for first IDE controller
> > 0170-0177,0376 for second one
> 
> Any I/O port used by the device identifies it.  I'd say a common
> identifier is the "I/O base", the lowest I/O port used.
So use only first port from the string. More information is better then
less information. You can always drop information you do not need.

--
			Gleb.

Re: [Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Markus Armbruster]

Gleb Natapov <gleb@redhat.com> writes:

> On Mon, Oct 25, 2010 at 05:10:15PM +0200, Markus Armbruster wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > PCI bus already has one. For ISA bus this patch series uses device's
>> > ioports to uniquely describe it. For isa-ide, for example, get_dev_path
>> > method returns:
>> > 01f0-01f7,03f6 for first IDE controller
>> > 0170-0177,0376 for second one
>> 
>> Any I/O port used by the device identifies it.  I'd say a common
>> identifier is the "I/O base", the lowest I/O port used.
> So use only first port from the string. More information is better then
> less information. You can always drop information you do not need.

I'd prefer canonical bus addresses to be terse.  It's not the place to
give additional information.

Re: [Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev.

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Gleb Natapov]

On Mon, Oct 25, 2010 at 07:53:41PM +0200, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > On Mon, Oct 25, 2010 at 05:10:15PM +0200, Markus Armbruster wrote:
> >> Gleb Natapov <gleb@redhat.com> writes:
> >> 
> >> > PCI bus already has one. For ISA bus this patch series uses device's
> >> > ioports to uniquely describe it. For isa-ide, for example, get_dev_path
> >> > method returns:
> >> > 01f0-01f7,03f6 for first IDE controller
> >> > 0170-0177,0376 for second one
> >> 
> >> Any I/O port used by the device identifies it.  I'd say a common
> >> identifier is the "I/O base", the lowest I/O port used.
> > So use only first port from the string. More information is better then
> > less information. You can always drop information you do not need.
> 
> I'd prefer canonical bus addresses to be terse.  It's not the place to
> give additional information.

I'd prefer them to give full info but not more. I don't see why you
point is more valid then mine.

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev.

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Markus Armbruster]

Gleb Natapov <gleb@redhat.com> writes:

> On Mon, Oct 25, 2010 at 07:53:41PM +0200, Markus Armbruster wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > On Mon, Oct 25, 2010 at 05:10:15PM +0200, Markus Armbruster wrote:
>> >> Gleb Natapov <gleb@redhat.com> writes:
>> >> 
>> >> > PCI bus already has one. For ISA bus this patch series uses device's
>> >> > ioports to uniquely describe it. For isa-ide, for example, get_dev_path
>> >> > method returns:
>> >> > 01f0-01f7,03f6 for first IDE controller
>> >> > 0170-0177,0376 for second one
>> >> 
>> >> Any I/O port used by the device identifies it.  I'd say a common
>> >> identifier is the "I/O base", the lowest I/O port used.
>> > So use only first port from the string. More information is better then
>> > less information. You can always drop information you do not need.
>> 
>> I'd prefer canonical bus addresses to be terse.  It's not the place to
>> give additional information.
>
> I'd prefer them to give full info but not more. I don't see why you
> point is more valid then mine.

Full information about ISA resources is more than I/O ports, it also
includes IRQs and DMA channels.

An address is not the place to give full information.  The purpose of an
address is to name a thing, not to give full information about that
thing.


By the way, get_dev_path() really needs a written contract.  Actually,
all the qdev and qbus callbacks do.

Re: [Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Gleb Natapov]

On Tue, Oct 26, 2010 at 11:57:53AM +0200, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > On Mon, Oct 25, 2010 at 07:53:41PM +0200, Markus Armbruster wrote:
> >> Gleb Natapov <gleb@redhat.com> writes:
> >> 
> >> > On Mon, Oct 25, 2010 at 05:10:15PM +0200, Markus Armbruster wrote:
> >> >> Gleb Natapov <gleb@redhat.com> writes:
> >> >> 
> >> >> > PCI bus already has one. For ISA bus this patch series uses device's
> >> >> > ioports to uniquely describe it. For isa-ide, for example, get_dev_path
> >> >> > method returns:
> >> >> > 01f0-01f7,03f6 for first IDE controller
> >> >> > 0170-0177,0376 for second one
> >> >> 
> >> >> Any I/O port used by the device identifies it.  I'd say a common
> >> >> identifier is the "I/O base", the lowest I/O port used.
> >> > So use only first port from the string. More information is better then
> >> > less information. You can always drop information you do not need.
> >> 
> >> I'd prefer canonical bus addresses to be terse.  It's not the place to
> >> give additional information.
> >
> > I'd prefer them to give full info but not more. I don't see why you
> > point is more valid then mine.
> 
> Full information about ISA resources is more than I/O ports, it also
> includes IRQs and DMA channels.
So you are arguing to add more info there? :) Theoretically there is
nothing that prevents ISA devices from sharing IRQ line (existing ISA
devices just not built to do that and nobody produces new once) and
DMA is separate device on ISA bus AFAIK.

> 
> An address is not the place to give full information.  The purpose of an
> address is to name a thing, not to give full information about that
> thing.
> 
Address should contain info that allows to find device easily. If
whoever looking for a device knows only one port device is using he
will not find the device if we'll use different port to name it. May be
not the problem in practice.

> 
> By the way, get_dev_path() really needs a written contract.  Actually,
> all the qdev and qbus callbacks do.
What do you mean by that? It should be stable ABI, is this what you
mean?

--
			Gleb.

Re: [Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Markus Armbruster]

Gleb Natapov <gleb@redhat.com> writes:

> On Tue, Oct 26, 2010 at 11:57:53AM +0200, Markus Armbruster wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > On Mon, Oct 25, 2010 at 07:53:41PM +0200, Markus Armbruster wrote:
>> >> Gleb Natapov <gleb@redhat.com> writes:
>> >> 
>> >> > On Mon, Oct 25, 2010 at 05:10:15PM +0200, Markus Armbruster wrote:
>> >> >> Gleb Natapov <gleb@redhat.com> writes:
>> >> >> 
>> >> >> > PCI bus already has one. For ISA bus this patch series uses device's
>> >> >> > ioports to uniquely describe it. For isa-ide, for example, get_dev_path
>> >> >> > method returns:
>> >> >> > 01f0-01f7,03f6 for first IDE controller
>> >> >> > 0170-0177,0376 for second one
>> >> >> 
>> >> >> Any I/O port used by the device identifies it.  I'd say a common
>> >> >> identifier is the "I/O base", the lowest I/O port used.
>> >> > So use only first port from the string. More information is better then
>> >> > less information. You can always drop information you do not need.
>> >> 
>> >> I'd prefer canonical bus addresses to be terse.  It's not the place to
>> >> give additional information.
>> >
>> > I'd prefer them to give full info but not more. I don't see why you
>> > point is more valid then mine.
>> 
>> Full information about ISA resources is more than I/O ports, it also
>> includes IRQs and DMA channels.
> So you are arguing to add more info there? :) Theoretically there is
> nothing that prevents ISA devices from sharing IRQ line (existing ISA
> devices just not built to do that and nobody produces new once) and
> DMA is separate device on ISA bus AFAIK.
>
>> 
>> An address is not the place to give full information.  The purpose of an
>> address is to name a thing, not to give full information about that
>> thing.
>> 
> Address should contain info that allows to find device easily. If
> whoever looking for a device knows only one port device is using he
> will not find the device if we'll use different port to name it. May be
> not the problem in practice.

If he knows only one port, he can't form the address proposed by you.
He needs to know *all* ports.  So what he does is search all the devices
for the one that uses the port in question.  Same for the address I
propose.  The proposal differ for the case where the user remembers just
the base address.  I believe that's the common case.

>> By the way, get_dev_path() really needs a written contract.  Actually,
>> all the qdev and qbus callbacks do.
> What do you mean by that? It should be stable ABI, is this what you
> mean?

A contract between qdev core and the device models, so that device model
authors know what exactly their callback functions are supposed to do.

Stability is a separate matter.  Since it's a purely internal interface,
we don't want it.

Re: [Qemu-devel] [PATCH 0/2] Add get_dev_path callback to ISA bus.

[Gleb Natapov]

On Tue, Oct 26, 2010 at 01:04:18PM +0200, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > On Tue, Oct 26, 2010 at 11:57:53AM +0200, Markus Armbruster wrote:
> >> Gleb Natapov <gleb@redhat.com> writes:
> >> 
> >> > On Mon, Oct 25, 2010 at 07:53:41PM +0200, Markus Armbruster wrote:
> >> >> Gleb Natapov <gleb@redhat.com> writes:
> >> >> 
> >> >> > On Mon, Oct 25, 2010 at 05:10:15PM +0200, Markus Armbruster wrote:
> >> >> >> Gleb Natapov <gleb@redhat.com> writes:
> >> >> >> 
> >> >> >> > PCI bus already has one. For ISA bus this patch series uses device's
> >> >> >> > ioports to uniquely describe it. For isa-ide, for example, get_dev_path
> >> >> >> > method returns:
> >> >> >> > 01f0-01f7,03f6 for first IDE controller
> >> >> >> > 0170-0177,0376 for second one
> >> >> >> 
> >> >> >> Any I/O port used by the device identifies it.  I'd say a common
> >> >> >> identifier is the "I/O base", the lowest I/O port used.
> >> >> > So use only first port from the string. More information is better then
> >> >> > less information. You can always drop information you do not need.
> >> >> 
> >> >> I'd prefer canonical bus addresses to be terse.  It's not the place to
> >> >> give additional information.
> >> >
> >> > I'd prefer them to give full info but not more. I don't see why you
> >> > point is more valid then mine.
> >> 
> >> Full information about ISA resources is more than I/O ports, it also
> >> includes IRQs and DMA channels.
> > So you are arguing to add more info there? :) Theoretically there is
> > nothing that prevents ISA devices from sharing IRQ line (existing ISA
> > devices just not built to do that and nobody produces new once) and
> > DMA is separate device on ISA bus AFAIK.
> >
> >> 
> >> An address is not the place to give full information.  The purpose of an
> >> address is to name a thing, not to give full information about that
> >> thing.
> >> 
> > Address should contain info that allows to find device easily. If
> > whoever looking for a device knows only one port device is using he
> > will not find the device if we'll use different port to name it. May be
> > not the problem in practice.
> 
> If he knows only one port, he can't form the address proposed by you.
He doesn't need to. He scans all devices and search for one that
list this ioport in its bus name. 

> He needs to know *all* ports.  
Why? If there is device ISA@0000-0005,0008 and I want to know what
devices uses port 0003 I can easily find it.

>                                So what he does is search all the devices
> for the one that uses the port in question.  Same for the address I
> propose.  The proposal differ for the case where the user remembers just
> the base address.  I believe that's the common case.
> 
> >> By the way, get_dev_path() really needs a written contract.  Actually,
> >> all the qdev and qbus callbacks do.
> > What do you mean by that? It should be stable ABI, is this what you
> > mean?
> 
> A contract between qdev core and the device models, so that device model
> authors know what exactly their callback functions are supposed to do.
I thought that was worked out when get_dev_path() callback was
introduced.

> 
> Stability is a separate matter.  Since it's a purely internal interface,
> we don't want it.
I think that where your misunderstanding of most of my patches are
coming from :( The way I am going to use it it will not be internal
interface anymore. It will be ABI between qemu and a guest.

--
			Gleb.