Patchwork [PATCHv2,4/8] Store IDE bus id in IDEBus structure for easy access.

login
register
mail settings
Submitter Gleb Natapov
Date Nov. 3, 2010, 1:47 p.m.
Message ID <20101103134726.GI7881@redhat.com>
Download mbox | patch
Permalink /patch/70006/
State New
Headers show

Comments

Gleb Natapov - Nov. 3, 2010, 1:47 p.m.
On Wed, Nov 03, 2010 at 02:39:52PM +0100, Markus Armbruster wrote:
> Here's a generic answer to the question "which of the device's buses is
> this?"
> 
> int qbus_index(BusState *bus)
> {
>     BusState *b;
>     int i, index;
> 
>     index = -1;
>     i = 0;
>     QLIST_FOREACH(b, &bus->parent->child_bus, sibling) {
>         if (b == bus) {
>             index = i;
>         }
>         i++;
>     }
>     assert(0 <= index && index < i);
>     return i - 1 - index;
> }
> 
> The bus created first has index 0.
> 
> Note that the child_bus holds the children in reverse creation order,
> and we can't traverse it backwards.  Same problem also visible with
> makes info qtree:
> 
>       dev: piix3-ide, id ""
> [...]
>         bus: ide.1
>           type IDE
>         bus: ide.0
>           type IDE
Isn't this too implementation dependant? Are you against adding bus_id
to IDEBus? And will it work with ISA? I do not think IDEBus is
added to bus->parent->child_bus in case of ISA otherwise why both
IDEBuses have same name in isapc. Currently I have following patch
queued. It should fix isapc case too.

    Fix isapc IDE bus creation
    
    Currently we create two ide buses with the same name, so it is impossible
    to use -device ide-drive,bus= to address all possible disks in the isapc
    machine. Fix that by giving different names to different ide buses. Also
    store IDE bus id in IDEBus structure for easy access.

--
			Gleb.
Markus Armbruster - Nov. 3, 2010, 3:18 p.m.
Gleb Natapov <gleb@redhat.com> writes:

> On Wed, Nov 03, 2010 at 02:39:52PM +0100, Markus Armbruster wrote:
>> Here's a generic answer to the question "which of the device's buses is
>> this?"
>> 
>> int qbus_index(BusState *bus)
>> {
>>     BusState *b;
>>     int i, index;
>> 
>>     index = -1;
>>     i = 0;
>>     QLIST_FOREACH(b, &bus->parent->child_bus, sibling) {
>>         if (b == bus) {
>>             index = i;
>>         }
>>         i++;
>>     }
>>     assert(0 <= index && index < i);
>>     return i - 1 - index;
>> }
>> 
>> The bus created first has index 0.
>> 
>> Note that the child_bus holds the children in reverse creation order,
>> and we can't traverse it backwards.  Same problem also visible with
>> makes info qtree:
>> 
>>       dev: piix3-ide, id ""
>> [...]
>>         bus: ide.1
>>           type IDE
>>         bus: ide.0
>>           type IDE
> Isn't this too implementation dependant?

Well, it's the implementation depending on itself.

>                                          Are you against adding bus_id
> to IDEBus?

"Against" is too hard a word.  If it's a general question, I'd prefer a
general answer.

>            And will it work with ISA? I do not think IDEBus is
> added to bus->parent->child_bus in case of ISA otherwise why both
> IDEBuses have same name in isapc. Currently I have following patch
> queued. It should fix isapc case too.

You're right, it's not helpful there: two separate devices, both
providing just one bus.

>     Fix isapc IDE bus creation
>     
>     Currently we create two ide buses with the same name, so it is impossible
>     to use -device ide-drive,bus= to address all possible disks in the isapc
>     machine. Fix that by giving different names to different ide buses. Also
>     store IDE bus id in IDEBus structure for easy access.

Changing the name of the second default isa-ide device's bus to "ide.1"
is fine with me.

But with that change in place, why do we need to store the bus number?
Why can't we just use the name?

Moreover, I think the meaning of "bus number" is unclear.  See comments
inline.

> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index ff80dd5..b2cbdbc 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -257,8 +257,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
>      pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
>  
>      irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
> -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>      ide_init2(&d->bus[0], irq[0]);
>      ide_init2(&d->bus[1], irq[1]);
>  
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 4165543..bde2664 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -448,6 +448,7 @@ struct IDEBus {
>      IDEDevice *slave;
>      BMDMAState *bmdma;
>      IDEState ifs[2];
> +    uint8_t bus_id;

Why is this uint8_t?

>      uint8_t unit;
>      uint8_t cmd;
>      qemu_irq irq;
> @@ -564,7 +565,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
>  void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
>  
>  /* hw/ide/qdev.c */
> -void ide_bus_new(IDEBus *idebus, DeviceState *dev);
> +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id);
>  IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
>  
>  #endif /* HW_IDE_INTERNAL_H */
> diff --git a/hw/ide/isa.c b/hw/ide/isa.c
> index 9b94495..b000ab8 100644
> --- a/hw/ide/isa.c
> +++ b/hw/ide/isa.c
> @@ -66,8 +66,9 @@ static const VMStateDescription vmstate_ide_isa = {
>  static int isa_ide_initfn(ISADevice *dev)
>  {
>      ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
> +    static uint8_t bus_id = 0;
>  
> -    ide_bus_new(&s->bus, &s->dev.qdev);
> +    ide_bus_new(&s->bus, &s->dev.qdev, bus_id++);
>      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);

Works for any number of isa-ide devices.  Each one provides one IDE bus,
and the buses are numbered 0, ... in the order the isa-ide devices are
created.

> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 6206201..e56777f 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -129,8 +129,8 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
>  
>      vmstate_register(&d->dev.qdev, 0, &vmstate_ide_pci, d);
>  
> -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
>      ide_init_ioport(&d->bus[1], 0x170, 0x376);
>  

Each piix3-ide device provides two IDE buses numbered 0 and 1.

If you create multiple IDE controller devices, then the IDE bus numbers
and names clash, unless they're all isa-ide devices.

What is the IDE bus number supposed to mean?

Example: default piix3-ide plus a tertiary IDE channel
-M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11

> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 336ffe1..220729e 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -29,9 +29,13 @@ static struct BusInfo ide_bus_info = {
>      .size  = sizeof(IDEBus),
>  };
>  
> -void ide_bus_new(IDEBus *idebus, DeviceState *dev)
> +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id)
>  {
> -    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
> +    char name[10];
> +
> +    snprintf(name, sizeof(name), "ide.%d", bus_id); 
> +    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, name);
> +    idebus->bus_id = bus_id;
>  }

Incompatible change.

Before: -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11,id=foo
provides an IDE bus called "foo.0":

          dev: isa-ide, id "foo"
            dev-prop: iobase = 0x1e8
            dev-prop: iobase2 = 0x3ee
            dev-prop: irq = 11
            isa irq 11
            bus: foo.0
              type IDE

After: it's called "ide.0", I think.

Likely to break non-default IDE controller use.  Could be rare enough
for us not to care, I don't know.

If we need to avoid this change, then ide_bus_new() must not pass a bus
name to qbus_create_inplace() for devices added with -device/device_add.

>  
>  static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index b2c7cad..cc48b2b 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -158,8 +158,8 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
>  
>      vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
>  
> -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>      ide_init2(&d->bus[0], isa_reserve_irq(14));
>      ide_init2(&d->bus[1], isa_reserve_irq(15));
>      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 3d07ce5..cec6c9b 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -163,7 +163,7 @@ static void pc_init1(ram_addr_t ram_size,
>              ISADevice *dev;
>              dev = isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
>                                 hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
> -            idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
> +            idebus[i] = qdev_get_child_bus(&dev->qdev, i ? "ide.1" : "ide.0");
>          }
>      }
Gleb Natapov - Nov. 3, 2010, 4:43 p.m.
On Wed, Nov 03, 2010 at 04:18:18PM +0100, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > On Wed, Nov 03, 2010 at 02:39:52PM +0100, Markus Armbruster wrote:
> >> Here's a generic answer to the question "which of the device's buses is
> >> this?"
> >> 
> >> int qbus_index(BusState *bus)
> >> {
> >>     BusState *b;
> >>     int i, index;
> >> 
> >>     index = -1;
> >>     i = 0;
> >>     QLIST_FOREACH(b, &bus->parent->child_bus, sibling) {
> >>         if (b == bus) {
> >>             index = i;
> >>         }
> >>         i++;
> >>     }
> >>     assert(0 <= index && index < i);
> >>     return i - 1 - index;
> >> }
> >> 
> >> The bus created first has index 0.
> >> 
> >> Note that the child_bus holds the children in reverse creation order,
> >> and we can't traverse it backwards.  Same problem also visible with
> >> makes info qtree:
> >> 
> >>       dev: piix3-ide, id ""
> >> [...]
> >>         bus: ide.1
> >>           type IDE
> >>         bus: ide.0
> >>           type IDE
> > Isn't this too implementation dependant?
> 
> Well, it's the implementation depending on itself.
> 
In a sense yes, but we expose internal qdev state in upper layer of the
stack. Look like leaking abstraction problem to me. What if we will
change qdev to store child buses in hash instead of list?

> >                                          Are you against adding bus_id
> > to IDEBus?
> 
> "Against" is too hard a word.  If it's a general question, I'd prefer a
> general answer.
It is as general as "what pci slot/func of a pci device". We store those
in PCIDevice.

> 
> >            And will it work with ISA? I do not think IDEBus is
> > added to bus->parent->child_bus in case of ISA otherwise why both
> > IDEBuses have same name in isapc. Currently I have following patch
> > queued. It should fix isapc case too.
> 
> You're right, it's not helpful there: two separate devices, both
> providing just one bus.
> 
> >     Fix isapc IDE bus creation
> >     
> >     Currently we create two ide buses with the same name, so it is impossible
> >     to use -device ide-drive,bus= to address all possible disks in the isapc
> >     machine. Fix that by giving different names to different ide buses. Also
> >     store IDE bus id in IDEBus structure for easy access.
> 
> Changing the name of the second default isa-ide device's bus to "ide.1"
> is fine with me.
> 
> But with that change in place, why do we need to store the bus number?
> Why can't we just use the name?
This name has no meaning outside of qemu internals. I can derive bus
number from bus name by parsing the string, but I don't see why is it
better than storing bus_id (that was used to create this bus name in the
first place) for easy access.

> 
> Moreover, I think the meaning of "bus number" is unclear.  See comments
> inline.
> 
> > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> > index ff80dd5..b2cbdbc 100644
> > --- a/hw/ide/cmd646.c
> > +++ b/hw/ide/cmd646.c
> > @@ -257,8 +257,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
> >      pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
> >  
> >      irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
> >      ide_init2(&d->bus[0], irq[0]);
> >      ide_init2(&d->bus[1], irq[1]);
> >  
> > diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> > index 4165543..bde2664 100644
> > --- a/hw/ide/internal.h
> > +++ b/hw/ide/internal.h
> > @@ -448,6 +448,7 @@ struct IDEBus {
> >      IDEDevice *slave;
> >      BMDMAState *bmdma;
> >      IDEState ifs[2];
> > +    uint8_t bus_id;
> 
> Why is this uint8_t?
> 
I do not expect to have more then 255 ide buses provided by one device :)

> >      uint8_t unit;
> >      uint8_t cmd;
> >      qemu_irq irq;
> > @@ -564,7 +565,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
> >  void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
> >  
> >  /* hw/ide/qdev.c */
> > -void ide_bus_new(IDEBus *idebus, DeviceState *dev);
> > +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id);
> >  IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
> >  
> >  #endif /* HW_IDE_INTERNAL_H */
> > diff --git a/hw/ide/isa.c b/hw/ide/isa.c
> > index 9b94495..b000ab8 100644
> > --- a/hw/ide/isa.c
> > +++ b/hw/ide/isa.c
> > @@ -66,8 +66,9 @@ static const VMStateDescription vmstate_ide_isa = {
> >  static int isa_ide_initfn(ISADevice *dev)
> >  {
> >      ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
> > +    static uint8_t bus_id = 0;
> >  
> > -    ide_bus_new(&s->bus, &s->dev.qdev);
> > +    ide_bus_new(&s->bus, &s->dev.qdev, bus_id++);
> >      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);
> 
> Works for any number of isa-ide devices.  Each one provides one IDE bus,
> and the buses are numbered 0, ... in the order the isa-ide devices are
> created.
> 
Except that we use those names to address device on command line. And 
this is very unfortunate since now we can't address all ide devices in
isapc case and in the future we can have same problem with other kind of
devices. Actually adding such way may be better solution then what I did
in this patch.

> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index 6206201..e56777f 100644
> > --- a/hw/ide/piix.c
> > +++ b/hw/ide/piix.c
> > @@ -129,8 +129,8 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
> >  
> >      vmstate_register(&d->dev.qdev, 0, &vmstate_ide_pci, d);
> >  
> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
> >      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> >      ide_init_ioport(&d->bus[1], 0x170, 0x376);
> >  
> 
> Each piix3-ide device provides two IDE buses numbered 0 and 1.
> 
> If you create multiple IDE controller devices, then the IDE bus numbers
> and names clash, unless they're all isa-ide devices.
> 
> What is the IDE bus number supposed to mean?
Bus number on parent bus. I don't care if name clashes since device
that provide those IDE buses will have different names.

> 
> Example: default piix3-ide plus a tertiary IDE channel
> -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11
Here are the device names my patch produces (for pci ide disks one
virtio and one isa-ide:

/pci@i0cf8/ata@1,1/ide@0/disk@0
/pci@i0cf8/ata@1,1/ide@0/disk@1
/pci@i0cf8/ata@1,1/ide@1/disk@0
/pci@i0cf8/ata@1,1/ide@1/disk@1
/pci@i0cf8/virtio-blk@3/disk@0
/pci@i0cf8/pci-isa-bridge@1/ata@01e8/ide@0/disk@0

As you can see there is no name clashes at all. Each device has device
name that can be used to identify it in firmware.

> 
> > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> > index 336ffe1..220729e 100644
> > --- a/hw/ide/qdev.c
> > +++ b/hw/ide/qdev.c
> > @@ -29,9 +29,13 @@ static struct BusInfo ide_bus_info = {
> >      .size  = sizeof(IDEBus),
> >  };
> >  
> > -void ide_bus_new(IDEBus *idebus, DeviceState *dev)
> > +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id)
> >  {
> > -    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
> > +    char name[10];
> > +
> > +    snprintf(name, sizeof(name), "ide.%d", bus_id); 
> > +    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, name);
> > +    idebus->bus_id = bus_id;
> >  }
> 
> Incompatible change.
> 
> Before: -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11,id=foo
> provides an IDE bus called "foo.0":
> 
>           dev: isa-ide, id "foo"
>             dev-prop: iobase = 0x1e8
>             dev-prop: iobase2 = 0x3ee
>             dev-prop: irq = 11
>             isa irq 11
>             bus: foo.0
>               type IDE
> 
> After: it's called "ide.0", I think.
> 
snprintf(name, sizeof(name), "%s.%d", dev->id ? : "ide", bus_id);
will take care of it. But I am starting to think that this is indeed not
needed. Without the change my patch produce name like that in isapc case:
/isa/ata@01f0/ide@0/disk@0
/isa/ata@01f0/ide@0/disk@1
/isa/ata@0170/ide@0/disk@0
/isa/ata@0170/ide@0/disk@1

so the paths are unique and usable by firmware. The only problem is that
not all disks are addressable from -device option, so I can't set
bootindex on some of them.


> Likely to break non-default IDE controller use.  Could be rare enough
> for us not to care, I don't know.
> 
> If we need to avoid this change, then ide_bus_new() must not pass a bus
> name to qbus_create_inplace() for devices added with -device/device_add.
> 
> >  
> >  static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
> > diff --git a/hw/ide/via.c b/hw/ide/via.c
> > index b2c7cad..cc48b2b 100644
> > --- a/hw/ide/via.c
> > +++ b/hw/ide/via.c
> > @@ -158,8 +158,8 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
> >  
> >      vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
> >  
> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
> >      ide_init2(&d->bus[0], isa_reserve_irq(14));
> >      ide_init2(&d->bus[1], isa_reserve_irq(15));
> >      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 3d07ce5..cec6c9b 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -163,7 +163,7 @@ static void pc_init1(ram_addr_t ram_size,
> >              ISADevice *dev;
> >              dev = isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
> >                                 hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
> > -            idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
> > +            idebus[i] = qdev_get_child_bus(&dev->qdev, i ? "ide.1" : "ide.0");
> >          }
> >      }

--
			Gleb.
Markus Armbruster - Nov. 3, 2010, 5:22 p.m.
Gleb Natapov <gleb@redhat.com> writes:

> On Wed, Nov 03, 2010 at 04:18:18PM +0100, Markus Armbruster wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > On Wed, Nov 03, 2010 at 02:39:52PM +0100, Markus Armbruster wrote:
>> >> Here's a generic answer to the question "which of the device's buses is
>> >> this?"
>> >> 
>> >> int qbus_index(BusState *bus)
>> >> {
>> >>     BusState *b;
>> >>     int i, index;
>> >> 
>> >>     index = -1;
>> >>     i = 0;
>> >>     QLIST_FOREACH(b, &bus->parent->child_bus, sibling) {
>> >>         if (b == bus) {
>> >>             index = i;
>> >>         }
>> >>         i++;
>> >>     }
>> >>     assert(0 <= index && index < i);
>> >>     return i - 1 - index;
>> >> }
>> >> 
>> >> The bus created first has index 0.
>> >> 
>> >> Note that the child_bus holds the children in reverse creation order,
>> >> and we can't traverse it backwards.  Same problem also visible with
>> >> makes info qtree:
>> >> 
>> >>       dev: piix3-ide, id ""
>> >> [...]
>> >>         bus: ide.1
>> >>           type IDE
>> >>         bus: ide.0
>> >>           type IDE
>> > Isn't this too implementation dependant?
>> 
>> Well, it's the implementation depending on itself.
>> 
> In a sense yes, but we expose internal qdev state in upper layer of the
> stack. Look like leaking abstraction problem to me. What if we will
> change qdev to store child buses in hash instead of list?

The function is defined in terms of the qdev interface: it depends only
on the order in which the parent device's buses are created.  The fact
that my implementation happens to depend on how we store child buses
doesn't change that.

>> >                                          Are you against adding bus_id
>> > to IDEBus?
>> 
>> "Against" is too hard a word.  If it's a general question, I'd prefer a
>> general answer.
> It is as general as "what pci slot/func of a pci device". We store those
> in PCIDevice.

It's actually more general than that :)

PCI slot.function is the address of a PCI device on its parent bus.
It's specific to PCI buses.

The bus number is the "address" of a bus on its parent device.  It's the
same regardless of the device.

>> >            And will it work with ISA? I do not think IDEBus is
>> > added to bus->parent->child_bus in case of ISA otherwise why both
>> > IDEBuses have same name in isapc. Currently I have following patch
>> > queued. It should fix isapc case too.
>> 
>> You're right, it's not helpful there: two separate devices, both
>> providing just one bus.
>> 
>> >     Fix isapc IDE bus creation
>> >     
>> >     Currently we create two ide buses with the same name, so it is impossible
>> >     to use -device ide-drive,bus= to address all possible disks in the isapc
>> >     machine. Fix that by giving different names to different ide buses. Also
>> >     store IDE bus id in IDEBus structure for easy access.
>> 
>> Changing the name of the second default isa-ide device's bus to "ide.1"
>> is fine with me.
>> 
>> But with that change in place, why do we need to store the bus number?
>> Why can't we just use the name?
> This name has no meaning outside of qemu internals. I can derive bus
> number from bus name by parsing the string, but I don't see why is it
> better than storing bus_id (that was used to create this bus name in the
> first place) for easy access.
>
>> 
>> Moreover, I think the meaning of "bus number" is unclear.  See comments
>> inline.
>> 
>> > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> > index ff80dd5..b2cbdbc 100644
>> > --- a/hw/ide/cmd646.c
>> > +++ b/hw/ide/cmd646.c
>> > @@ -257,8 +257,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
>> >      pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
>> >  
>> >      irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
>> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
>> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
>> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
>> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>> >      ide_init2(&d->bus[0], irq[0]);
>> >      ide_init2(&d->bus[1], irq[1]);
>> >  
>> > diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>> > index 4165543..bde2664 100644
>> > --- a/hw/ide/internal.h
>> > +++ b/hw/ide/internal.h
>> > @@ -448,6 +448,7 @@ struct IDEBus {
>> >      IDEDevice *slave;
>> >      BMDMAState *bmdma;
>> >      IDEState ifs[2];
>> > +    uint8_t bus_id;
>> 
>> Why is this uint8_t?
>> 
> I do not expect to have more then 255 ide buses provided by one device :)

Bah, let's make it an int.

>> >      uint8_t unit;
>> >      uint8_t cmd;
>> >      qemu_irq irq;
>> > @@ -564,7 +565,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
>> >  void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
>> >  
>> >  /* hw/ide/qdev.c */
>> > -void ide_bus_new(IDEBus *idebus, DeviceState *dev);
>> > +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id);
>> >  IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
>> >  
>> >  #endif /* HW_IDE_INTERNAL_H */
>> > diff --git a/hw/ide/isa.c b/hw/ide/isa.c
>> > index 9b94495..b000ab8 100644
>> > --- a/hw/ide/isa.c
>> > +++ b/hw/ide/isa.c
>> > @@ -66,8 +66,9 @@ static const VMStateDescription vmstate_ide_isa = {
>> >  static int isa_ide_initfn(ISADevice *dev)
>> >  {
>> >      ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
>> > +    static uint8_t bus_id = 0;
>> >  
>> > -    ide_bus_new(&s->bus, &s->dev.qdev);
>> > +    ide_bus_new(&s->bus, &s->dev.qdev, bus_id++);
>> >      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);
>> 
>> Works for any number of isa-ide devices.  Each one provides one IDE bus,
>> and the buses are numbered 0, ... in the order the isa-ide devices are
>> created.
>> 
> Except that we use those names to address device on command line. And 
> this is very unfortunate since now we can't address all ide devices in
> isapc case and in the future we can have same problem with other kind of
> devices. Actually adding such way may be better solution then what I did
> in this patch.
>
>> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> > index 6206201..e56777f 100644
>> > --- a/hw/ide/piix.c
>> > +++ b/hw/ide/piix.c
>> > @@ -129,8 +129,8 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
>> >  
>> >      vmstate_register(&d->dev.qdev, 0, &vmstate_ide_pci, d);
>> >  
>> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
>> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
>> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
>> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>> >      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
>> >      ide_init_ioport(&d->bus[1], 0x170, 0x376);
>> >  
>> 
>> Each piix3-ide device provides two IDE buses numbered 0 and 1.
>> 
>> If you create multiple IDE controller devices, then the IDE bus numbers
>> and names clash, unless they're all isa-ide devices.
>> 
>> What is the IDE bus number supposed to mean?
> Bus number on parent bus. I don't care if name clashes since device
> that provide those IDE buses will have different names.

Bus number on parent *bus*?  Not parent *device*?

If you mean bus number on parent device, then your code for isa-ide is
wrong.  Each isa-ide device provides one bus.  By your definition, its
bus number must be 0.

If you mean bus number on parent bus, and by parent bus you mean the
parent device's parent bus, then your code for PCI IDE controllers such
as piix3-ide is wrong.

Please define bus number clearly.  Be verbose if you have to.  We've
been running in circles all this time in part because we miscommunicate/
misunderstand basic definitions such as bus number.

>> Example: default piix3-ide plus a tertiary IDE channel
>> -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11
> Here are the device names my patch produces (for pci ide disks one
> virtio and one isa-ide:
>
> /pci@i0cf8/ata@1,1/ide@0/disk@0
> /pci@i0cf8/ata@1,1/ide@0/disk@1
> /pci@i0cf8/ata@1,1/ide@1/disk@0
> /pci@i0cf8/ata@1,1/ide@1/disk@1
> /pci@i0cf8/virtio-blk@3/disk@0
> /pci@i0cf8/pci-isa-bridge@1/ata@01e8/ide@0/disk@0
>
> As you can see there is no name clashes at all. Each device has device
> name that can be used to identify it in firmware.
>
>> 
>> > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> > index 336ffe1..220729e 100644
>> > --- a/hw/ide/qdev.c
>> > +++ b/hw/ide/qdev.c
>> > @@ -29,9 +29,13 @@ static struct BusInfo ide_bus_info = {
>> >      .size  = sizeof(IDEBus),
>> >  };
>> >  
>> > -void ide_bus_new(IDEBus *idebus, DeviceState *dev)
>> > +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id)
>> >  {
>> > -    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
>> > +    char name[10];
>> > +
>> > +    snprintf(name, sizeof(name), "ide.%d", bus_id); 
>> > +    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, name);
>> > +    idebus->bus_id = bus_id;
>> >  }
>> 
>> Incompatible change.
>> 
>> Before: -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11,id=foo
>> provides an IDE bus called "foo.0":
>> 
>>           dev: isa-ide, id "foo"
>>             dev-prop: iobase = 0x1e8
>>             dev-prop: iobase2 = 0x3ee
>>             dev-prop: irq = 11
>>             isa irq 11
>>             bus: foo.0
>>               type IDE
>> 
>> After: it's called "ide.0", I think.
>> 
> snprintf(name, sizeof(name), "%s.%d", dev->id ? : "ide", bus_id);
> will take care of it.

Not fully.

-M pc
-device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11,id=foo
-device isa-ide,iobase=0x168,iobase2=0x36e,irq=10,id=bar

Before: foo.0 and bar.0
After: foo.0 and bar.1

>                       But I am starting to think that this is indeed not
> needed. Without the change my patch produce name like that in isapc case:
> /isa/ata@01f0/ide@0/disk@0
> /isa/ata@01f0/ide@0/disk@1
> /isa/ata@0170/ide@0/disk@0
> /isa/ata@0170/ide@0/disk@1
>
> so the paths are unique and usable by firmware. The only problem is that
> not all disks are addressable from -device option, so I can't set
> bootindex on some of them.

Perhaps we can treat the non-addressability of -M isapc's second IDE bus
as a separate problem.

>> Likely to break non-default IDE controller use.  Could be rare enough
>> for us not to care, I don't know.
>> 
>> If we need to avoid this change, then ide_bus_new() must not pass a bus
>> name to qbus_create_inplace() for devices added with -device/device_add.
>> 
>> >  
>> >  static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
>> > diff --git a/hw/ide/via.c b/hw/ide/via.c
>> > index b2c7cad..cc48b2b 100644
>> > --- a/hw/ide/via.c
>> > +++ b/hw/ide/via.c
>> > @@ -158,8 +158,8 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
>> >  
>> >      vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
>> >  
>> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
>> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
>> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
>> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>> >      ide_init2(&d->bus[0], isa_reserve_irq(14));
>> >      ide_init2(&d->bus[1], isa_reserve_irq(15));
>> >      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
>> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> > index 3d07ce5..cec6c9b 100644
>> > --- a/hw/pc_piix.c
>> > +++ b/hw/pc_piix.c
>> > @@ -163,7 +163,7 @@ static void pc_init1(ram_addr_t ram_size,
>> >              ISADevice *dev;
>> >              dev = isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
>> >                                 hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
>> > -            idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
>> > +            idebus[i] = qdev_get_child_bus(&dev->qdev, i ? "ide.1" : "ide.0");
>> >          }
>> >      }
Gleb Natapov - Nov. 4, 2010, 8:07 a.m.
On Wed, Nov 03, 2010 at 06:22:11PM +0100, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > On Wed, Nov 03, 2010 at 04:18:18PM +0100, Markus Armbruster wrote:
> >> Gleb Natapov <gleb@redhat.com> writes:
> >> 
> >> > On Wed, Nov 03, 2010 at 02:39:52PM +0100, Markus Armbruster wrote:
> >> >> Here's a generic answer to the question "which of the device's buses is
> >> >> this?"
> >> >> 
> >> >> int qbus_index(BusState *bus)
> >> >> {
> >> >>     BusState *b;
> >> >>     int i, index;
> >> >> 
> >> >>     index = -1;
> >> >>     i = 0;
> >> >>     QLIST_FOREACH(b, &bus->parent->child_bus, sibling) {
> >> >>         if (b == bus) {
> >> >>             index = i;
> >> >>         }
> >> >>         i++;
> >> >>     }
> >> >>     assert(0 <= index && index < i);
> >> >>     return i - 1 - index;
> >> >> }
> >> >> 
> >> >> The bus created first has index 0.
> >> >> 
> >> >> Note that the child_bus holds the children in reverse creation order,
> >> >> and we can't traverse it backwards.  Same problem also visible with
> >> >> makes info qtree:
> >> >> 
> >> >>       dev: piix3-ide, id ""
> >> >> [...]
> >> >>         bus: ide.1
> >> >>           type IDE
> >> >>         bus: ide.0
> >> >>           type IDE
> >> > Isn't this too implementation dependant?
> >> 
> >> Well, it's the implementation depending on itself.
> >> 
> > In a sense yes, but we expose internal qdev state in upper layer of the
> > stack. Look like leaking abstraction problem to me. What if we will
> > change qdev to store child buses in hash instead of list?
> 
> The function is defined in terms of the qdev interface: it depends only
> on the order in which the parent device's buses are created.  The fact
> that my implementation happens to depend on how we store child buses
> doesn't change that.
> 
But why order of device creation is important? It shouldn't be if we
want to move HW description into config file. We even may allow creating
piix3-ide with only second IDE bus, but not first.


> >> >                                          Are you against adding bus_id
> >> > to IDEBus?
> >> 
> >> "Against" is too hard a word.  If it's a general question, I'd prefer a
> >> general answer.
> > It is as general as "what pci slot/func of a pci device". We store those
> > in PCIDevice.
> 
> It's actually more general than that :)
> 
> PCI slot.function is the address of a PCI device on its parent bus.
> It's specific to PCI buses.
> 
> The bus number is the "address" of a bus on its parent device.  It's the
> same regardless of the device.
> 
In case of IDE bus siting on piix3-ide it is not just arbitrary number
like you suggest here. It actually tells how to talk to a device. IDE
bus 0 is reachable through BAR0,1 of piix3-ide and IDE bus 1 is reachable
through BAR2,3. So this number is part of the device address as much as
pci slot/func is.


> >> >            And will it work with ISA? I do not think IDEBus is
> >> > added to bus->parent->child_bus in case of ISA otherwise why both
> >> > IDEBuses have same name in isapc. Currently I have following patch
> >> > queued. It should fix isapc case too.
> >> 
> >> You're right, it's not helpful there: two separate devices, both
> >> providing just one bus.
> >> 
> >> >     Fix isapc IDE bus creation
> >> >     
> >> >     Currently we create two ide buses with the same name, so it is impossible
> >> >     to use -device ide-drive,bus= to address all possible disks in the isapc
> >> >     machine. Fix that by giving different names to different ide buses. Also
> >> >     store IDE bus id in IDEBus structure for easy access.
> >> 
> >> Changing the name of the second default isa-ide device's bus to "ide.1"
> >> is fine with me.
> >> 
> >> But with that change in place, why do we need to store the bus number?
> >> Why can't we just use the name?
> > This name has no meaning outside of qemu internals. I can derive bus
> > number from bus name by parsing the string, but I don't see why is it
> > better than storing bus_id (that was used to create this bus name in the
> > first place) for easy access.
> >
> >> 
> >> Moreover, I think the meaning of "bus number" is unclear.  See comments
> >> inline.
> >> 
> >> > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> >> > index ff80dd5..b2cbdbc 100644
> >> > --- a/hw/ide/cmd646.c
> >> > +++ b/hw/ide/cmd646.c
> >> > @@ -257,8 +257,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
> >> >      pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
> >> >  
> >> >      irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
> >> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> >> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> >> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> >> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
> >> >      ide_init2(&d->bus[0], irq[0]);
> >> >      ide_init2(&d->bus[1], irq[1]);
> >> >  
> >> > diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> >> > index 4165543..bde2664 100644
> >> > --- a/hw/ide/internal.h
> >> > +++ b/hw/ide/internal.h
> >> > @@ -448,6 +448,7 @@ struct IDEBus {
> >> >      IDEDevice *slave;
> >> >      BMDMAState *bmdma;
> >> >      IDEState ifs[2];
> >> > +    uint8_t bus_id;
> >> 
> >> Why is this uint8_t?
> >> 
> > I do not expect to have more then 255 ide buses provided by one device :)
> 
> Bah, let's make it an int.
OK.

> 
> >> >      uint8_t unit;
> >> >      uint8_t cmd;
> >> >      qemu_irq irq;
> >> > @@ -564,7 +565,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
> >> >  void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
> >> >  
> >> >  /* hw/ide/qdev.c */
> >> > -void ide_bus_new(IDEBus *idebus, DeviceState *dev);
> >> > +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id);
> >> >  IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
> >> >  
> >> >  #endif /* HW_IDE_INTERNAL_H */
> >> > diff --git a/hw/ide/isa.c b/hw/ide/isa.c
> >> > index 9b94495..b000ab8 100644
> >> > --- a/hw/ide/isa.c
> >> > +++ b/hw/ide/isa.c
> >> > @@ -66,8 +66,9 @@ static const VMStateDescription vmstate_ide_isa = {
> >> >  static int isa_ide_initfn(ISADevice *dev)
> >> >  {
> >> >      ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
> >> > +    static uint8_t bus_id = 0;
> >> >  
> >> > -    ide_bus_new(&s->bus, &s->dev.qdev);
> >> > +    ide_bus_new(&s->bus, &s->dev.qdev, bus_id++);
> >> >      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);
> >> 
> >> Works for any number of isa-ide devices.  Each one provides one IDE bus,
> >> and the buses are numbered 0, ... in the order the isa-ide devices are
> >> created.
> >> 
> > Except that we use those names to address device on command line. And 
> > this is very unfortunate since now we can't address all ide devices in
> > isapc case and in the future we can have same problem with other kind of
> > devices. Actually adding such way may be better solution then what I did
> > in this patch.
> >
> >> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> >> > index 6206201..e56777f 100644
> >> > --- a/hw/ide/piix.c
> >> > +++ b/hw/ide/piix.c
> >> > @@ -129,8 +129,8 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
> >> >  
> >> >      vmstate_register(&d->dev.qdev, 0, &vmstate_ide_pci, d);
> >> >  
> >> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> >> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> >> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> >> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
> >> >      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> >> >      ide_init_ioport(&d->bus[1], 0x170, 0x376);
> >> >  
> >> 
> >> Each piix3-ide device provides two IDE buses numbered 0 and 1.
> >> 
> >> If you create multiple IDE controller devices, then the IDE bus numbers
> >> and names clash, unless they're all isa-ide devices.
> >> 
> >> What is the IDE bus number supposed to mean?
> > Bus number on parent bus. I don't care if name clashes since device
> > that provide those IDE buses will have different names.
> 
> Bus number on parent *bus*?  Not parent *device*?
> 
On parent device.

> If you mean bus number on parent device, then your code for isa-ide is
> wrong.  Each isa-ide device provides one bus.  By your definition, its
> bus number must be 0.
> 
> If you mean bus number on parent bus, and by parent bus you mean the
> parent device's parent bus, then your code for PCI IDE controllers such
> as piix3-ide is wrong.
> 
> Please define bus number clearly.  Be verbose if you have to.  We've
> been running in circles all this time in part because we miscommunicate/
> misunderstand basic definitions such as bus number.
> 
I agree it is conceptually wrong. It is not even needed for unique device
path generation. It is only needed to make both IDE buses configurable
from command line in ISA machine. I'll drop it in favor of other solution,
but qdev has to rethink how devices should it addressable from command
line. Current way is broken.

> >> Example: default piix3-ide plus a tertiary IDE channel
> >> -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11
> > Here are the device names my patch produces (for pci ide disks one
> > virtio and one isa-ide:
> >
> > /pci@i0cf8/ata@1,1/ide@0/disk@0
> > /pci@i0cf8/ata@1,1/ide@0/disk@1
> > /pci@i0cf8/ata@1,1/ide@1/disk@0
> > /pci@i0cf8/ata@1,1/ide@1/disk@1
> > /pci@i0cf8/virtio-blk@3/disk@0
> > /pci@i0cf8/pci-isa-bridge@1/ata@01e8/ide@0/disk@0
> >
> > As you can see there is no name clashes at all. Each device has device
> > name that can be used to identify it in firmware.
> >
> >> 
> >> > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> >> > index 336ffe1..220729e 100644
> >> > --- a/hw/ide/qdev.c
> >> > +++ b/hw/ide/qdev.c
> >> > @@ -29,9 +29,13 @@ static struct BusInfo ide_bus_info = {
> >> >      .size  = sizeof(IDEBus),
> >> >  };
> >> >  
> >> > -void ide_bus_new(IDEBus *idebus, DeviceState *dev)
> >> > +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id)
> >> >  {
> >> > -    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
> >> > +    char name[10];
> >> > +
> >> > +    snprintf(name, sizeof(name), "ide.%d", bus_id); 
> >> > +    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, name);
> >> > +    idebus->bus_id = bus_id;
> >> >  }
> >> 
> >> Incompatible change.
> >> 
> >> Before: -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11,id=foo
> >> provides an IDE bus called "foo.0":
> >> 
> >>           dev: isa-ide, id "foo"
> >>             dev-prop: iobase = 0x1e8
> >>             dev-prop: iobase2 = 0x3ee
> >>             dev-prop: irq = 11
> >>             isa irq 11
> >>             bus: foo.0
> >>               type IDE
> >> 
> >> After: it's called "ide.0", I think.
> >> 
> > snprintf(name, sizeof(name), "%s.%d", dev->id ? : "ide", bus_id);
> > will take care of it.
> 
> Not fully.
> 
> -M pc
> -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11,id=foo
> -device isa-ide,iobase=0x168,iobase2=0x36e,irq=10,id=bar
> 
> Before: foo.0 and bar.0
> After: foo.0 and bar.1
> 
> >                       But I am starting to think that this is indeed not
> > needed. Without the change my patch produce name like that in isapc case:
> > /isa/ata@01f0/ide@0/disk@0
> > /isa/ata@01f0/ide@0/disk@1
> > /isa/ata@0170/ide@0/disk@0
> > /isa/ata@0170/ide@0/disk@1
> >
> > so the paths are unique and usable by firmware. The only problem is that
> > not all disks are addressable from -device option, so I can't set
> > bootindex on some of them.
> 
> Perhaps we can treat the non-addressability of -M isapc's second IDE bus
> as a separate problem.
> 
Agree, hence I will not use this patch and will get back to just
recording bus_id. But -M isapc is just a symptom of much more serious
problem in qdev. The way devices addressable there is not well defined.
Two devices may have the same device path. Ultimately I think qdev
should use device addresses similar to what I am trying to achieve here.
For ISA machine it should automatically generate ids like isa-ide@0x170
for first isa IDE controller and isa-ide@0x1f0 for second one. And get
rid of user assigned ids. They are good for nothing and exist only
because qdev developer didn't agree on how to name devices.

> >> Likely to break non-default IDE controller use.  Could be rare enough
> >> for us not to care, I don't know.
> >> 
> >> If we need to avoid this change, then ide_bus_new() must not pass a bus
> >> name to qbus_create_inplace() for devices added with -device/device_add.
> >> 
> >> >  
> >> >  static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
> >> > diff --git a/hw/ide/via.c b/hw/ide/via.c
> >> > index b2c7cad..cc48b2b 100644
> >> > --- a/hw/ide/via.c
> >> > +++ b/hw/ide/via.c
> >> > @@ -158,8 +158,8 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
> >> >  
> >> >      vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
> >> >  
> >> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
> >> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
> >> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
> >> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
> >> >      ide_init2(&d->bus[0], isa_reserve_irq(14));
> >> >      ide_init2(&d->bus[1], isa_reserve_irq(15));
> >> >      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> >> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> >> > index 3d07ce5..cec6c9b 100644
> >> > --- a/hw/pc_piix.c
> >> > +++ b/hw/pc_piix.c
> >> > @@ -163,7 +163,7 @@ static void pc_init1(ram_addr_t ram_size,
> >> >              ISADevice *dev;
> >> >              dev = isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
> >> >                                 hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
> >> > -            idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
> >> > +            idebus[i] = qdev_get_child_bus(&dev->qdev, i ? "ide.1" : "ide.0");
> >> >          }
> >> >      }

--
			Gleb.
Markus Armbruster - Nov. 4, 2010, 8:46 a.m.
Gleb Natapov <gleb@redhat.com> writes:

> On Wed, Nov 03, 2010 at 06:22:11PM +0100, Markus Armbruster wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > On Wed, Nov 03, 2010 at 04:18:18PM +0100, Markus Armbruster wrote:
>> >> Gleb Natapov <gleb@redhat.com> writes:
>> >> 
>> >> > On Wed, Nov 03, 2010 at 02:39:52PM +0100, Markus Armbruster wrote:
>> >> >> Here's a generic answer to the question "which of the device's buses is
>> >> >> this?"
>> >> >> 
>> >> >> int qbus_index(BusState *bus)
>> >> >> {
>> >> >>     BusState *b;
>> >> >>     int i, index;
>> >> >> 
>> >> >>     index = -1;
>> >> >>     i = 0;
>> >> >>     QLIST_FOREACH(b, &bus->parent->child_bus, sibling) {
>> >> >>         if (b == bus) {
>> >> >>             index = i;
>> >> >>         }
>> >> >>         i++;
>> >> >>     }
>> >> >>     assert(0 <= index && index < i);
>> >> >>     return i - 1 - index;
>> >> >> }
>> >> >> 
>> >> >> The bus created first has index 0.
>> >> >> 
>> >> >> Note that the child_bus holds the children in reverse creation order,
>> >> >> and we can't traverse it backwards.  Same problem also visible with
>> >> >> makes info qtree:
>> >> >> 
>> >> >>       dev: piix3-ide, id ""
>> >> >> [...]
>> >> >>         bus: ide.1
>> >> >>           type IDE
>> >> >>         bus: ide.0
>> >> >>           type IDE
>> >> > Isn't this too implementation dependant?
>> >> 
>> >> Well, it's the implementation depending on itself.
>> >> 
>> > In a sense yes, but we expose internal qdev state in upper layer of the
>> > stack. Look like leaking abstraction problem to me. What if we will
>> > change qdev to store child buses in hash instead of list?
>> 
>> The function is defined in terms of the qdev interface: it depends only
>> on the order in which the parent device's buses are created.  The fact
>> that my implementation happens to depend on how we store child buses
>> doesn't change that.
>> 
> But why order of device creation is important? It shouldn't be if we
> want to move HW description into config file. We even may allow creating
> piix3-ide with only second IDE bus, but not first.

That's not how buses work in qdev.

Configuration file, command line and monitor configure *devices*.
Devices need to be created after the device providing their parent bus,
obviously.  Other than that, order should not matter.

Buses are always created by device model code.  Thus, creation order is
fixed in code.  Thus defining bus number in terms of creation order is
fine.

If we create piix3-ide with only the second bus (for the sake of
argument), then its bus number is zero by definition, as clarified by
you below.  Unless you want to amend your definition :)

>> >> >                                          Are you against adding bus_id
>> >> > to IDEBus?
>> >> 
>> >> "Against" is too hard a word.  If it's a general question, I'd prefer a
>> >> general answer.
>> > It is as general as "what pci slot/func of a pci device". We store those
>> > in PCIDevice.
>> 
>> It's actually more general than that :)
>> 
>> PCI slot.function is the address of a PCI device on its parent bus.
>> It's specific to PCI buses.
>> 
>> The bus number is the "address" of a bus on its parent device.  It's the
>> same regardless of the device.
>> 
> In case of IDE bus siting on piix3-ide it is not just arbitrary number
> like you suggest here. It actually tells how to talk to a device. IDE
> bus 0 is reachable through BAR0,1 of piix3-ide and IDE bus 1 is reachable
> through BAR2,3. So this number is part of the device address as much as
> pci slot/func is.

What I mean to say: regardless of what the device is, or what kind of
buses it provides, the buses can *always* be identified the same way:
define an order, identify bus by ordinal number.

Because of that, I believe the concept "bus number" should be a generic
qdev concept, not special to IDE buses.

>> >> >            And will it work with ISA? I do not think IDEBus is
>> >> > added to bus->parent->child_bus in case of ISA otherwise why both
>> >> > IDEBuses have same name in isapc. Currently I have following patch
>> >> > queued. It should fix isapc case too.
>> >> 
>> >> You're right, it's not helpful there: two separate devices, both
>> >> providing just one bus.
>> >> 
>> >> >     Fix isapc IDE bus creation
>> >> >     
>> >> >     Currently we create two ide buses with the same name, so it is impossible
>> >> >     to use -device ide-drive,bus= to address all possible disks in the isapc
>> >> >     machine. Fix that by giving different names to different ide buses. Also
>> >> >     store IDE bus id in IDEBus structure for easy access.
>> >> 
>> >> Changing the name of the second default isa-ide device's bus to "ide.1"
>> >> is fine with me.
>> >> 
>> >> But with that change in place, why do we need to store the bus number?
>> >> Why can't we just use the name?
>> > This name has no meaning outside of qemu internals. I can derive bus
>> > number from bus name by parsing the string, but I don't see why is it
>> > better than storing bus_id (that was used to create this bus name in the
>> > first place) for easy access.
>> >
>> >> 
>> >> Moreover, I think the meaning of "bus number" is unclear.  See comments
>> >> inline.
>> >> 
>> >> > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> >> > index ff80dd5..b2cbdbc 100644
>> >> > --- a/hw/ide/cmd646.c
>> >> > +++ b/hw/ide/cmd646.c
>> >> > @@ -257,8 +257,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
>> >> >      pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
>> >> >  
>> >> >      irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
>> >> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
>> >> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
>> >> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
>> >> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>> >> >      ide_init2(&d->bus[0], irq[0]);
>> >> >      ide_init2(&d->bus[1], irq[1]);
>> >> >  
>> >> > diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>> >> > index 4165543..bde2664 100644
>> >> > --- a/hw/ide/internal.h
>> >> > +++ b/hw/ide/internal.h
>> >> > @@ -448,6 +448,7 @@ struct IDEBus {
>> >> >      IDEDevice *slave;
>> >> >      BMDMAState *bmdma;
>> >> >      IDEState ifs[2];
>> >> > +    uint8_t bus_id;
>> >> 
>> >> Why is this uint8_t?
>> >> 
>> > I do not expect to have more then 255 ide buses provided by one device :)
>> 
>> Bah, let's make it an int.
> OK.
>
>> 
>> >> >      uint8_t unit;
>> >> >      uint8_t cmd;
>> >> >      qemu_irq irq;
>> >> > @@ -564,7 +565,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
>> >> >  void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
>> >> >  
>> >> >  /* hw/ide/qdev.c */
>> >> > -void ide_bus_new(IDEBus *idebus, DeviceState *dev);
>> >> > +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id);
>> >> >  IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
>> >> >  
>> >> >  #endif /* HW_IDE_INTERNAL_H */
>> >> > diff --git a/hw/ide/isa.c b/hw/ide/isa.c
>> >> > index 9b94495..b000ab8 100644
>> >> > --- a/hw/ide/isa.c
>> >> > +++ b/hw/ide/isa.c
>> >> > @@ -66,8 +66,9 @@ static const VMStateDescription vmstate_ide_isa = {
>> >> >  static int isa_ide_initfn(ISADevice *dev)
>> >> >  {
>> >> >      ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
>> >> > +    static uint8_t bus_id = 0;
>> >> >  
>> >> > -    ide_bus_new(&s->bus, &s->dev.qdev);
>> >> > +    ide_bus_new(&s->bus, &s->dev.qdev, bus_id++);
>> >> >      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);
>> >> 
>> >> Works for any number of isa-ide devices.  Each one provides one IDE bus,
>> >> and the buses are numbered 0, ... in the order the isa-ide devices are
>> >> created.
>> >> 
>> > Except that we use those names to address device on command line. And 
>> > this is very unfortunate since now we can't address all ide devices in
>> > isapc case and in the future we can have same problem with other kind of
>> > devices. Actually adding such way may be better solution then what I did
>> > in this patch.
>> >
>> >> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> >> > index 6206201..e56777f 100644
>> >> > --- a/hw/ide/piix.c
>> >> > +++ b/hw/ide/piix.c
>> >> > @@ -129,8 +129,8 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
>> >> >  
>> >> >      vmstate_register(&d->dev.qdev, 0, &vmstate_ide_pci, d);
>> >> >  
>> >> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
>> >> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
>> >> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
>> >> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>> >> >      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
>> >> >      ide_init_ioport(&d->bus[1], 0x170, 0x376);
>> >> >  
>> >> 
>> >> Each piix3-ide device provides two IDE buses numbered 0 and 1.
>> >> 
>> >> If you create multiple IDE controller devices, then the IDE bus numbers
>> >> and names clash, unless they're all isa-ide devices.
>> >> 
>> >> What is the IDE bus number supposed to mean?
>> > Bus number on parent bus. I don't care if name clashes since device
>> > that provide those IDE buses will have different names.
>> 
>> Bus number on parent *bus*?  Not parent *device*?
>> 
> On parent device.

Makes sense to me.

>> If you mean bus number on parent device, then your code for isa-ide is
>> wrong.  Each isa-ide device provides one bus.  By your definition, its
>> bus number must be 0.
>> 
>> If you mean bus number on parent bus, and by parent bus you mean the
>> parent device's parent bus, then your code for PCI IDE controllers such
>> as piix3-ide is wrong.
>> 
>> Please define bus number clearly.  Be verbose if you have to.  We've
>> been running in circles all this time in part because we miscommunicate/
>> misunderstand basic definitions such as bus number.
>> 
> I agree it is conceptually wrong. It is not even needed for unique device
> path generation. It is only needed to make both IDE buses configurable
> from command line in ISA machine. I'll drop it in favor of other solution,
> but qdev has to rethink how devices should it addressable from command
> line. Current way is broken.

I agree it's broken and needs fixing.  I appreciate you trying to fix
it, but it indeed looks like it's better to fix it separately.

>> >> Example: default piix3-ide plus a tertiary IDE channel
>> >> -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11
>> > Here are the device names my patch produces (for pci ide disks one
>> > virtio and one isa-ide:
>> >
>> > /pci@i0cf8/ata@1,1/ide@0/disk@0
>> > /pci@i0cf8/ata@1,1/ide@0/disk@1
>> > /pci@i0cf8/ata@1,1/ide@1/disk@0
>> > /pci@i0cf8/ata@1,1/ide@1/disk@1
>> > /pci@i0cf8/virtio-blk@3/disk@0
>> > /pci@i0cf8/pci-isa-bridge@1/ata@01e8/ide@0/disk@0
>> >
>> > As you can see there is no name clashes at all. Each device has device
>> > name that can be used to identify it in firmware.
>> >
>> >> 
>> >> > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> >> > index 336ffe1..220729e 100644
>> >> > --- a/hw/ide/qdev.c
>> >> > +++ b/hw/ide/qdev.c
>> >> > @@ -29,9 +29,13 @@ static struct BusInfo ide_bus_info = {
>> >> >      .size  = sizeof(IDEBus),
>> >> >  };
>> >> >  
>> >> > -void ide_bus_new(IDEBus *idebus, DeviceState *dev)
>> >> > +void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id)
>> >> >  {
>> >> > -    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
>> >> > +    char name[10];
>> >> > +
>> >> > +    snprintf(name, sizeof(name), "ide.%d", bus_id); 
>> >> > +    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, name);
>> >> > +    idebus->bus_id = bus_id;
>> >> >  }
>> >> 
>> >> Incompatible change.
>> >> 
>> >> Before: -M pc -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11,id=foo
>> >> provides an IDE bus called "foo.0":
>> >> 
>> >>           dev: isa-ide, id "foo"
>> >>             dev-prop: iobase = 0x1e8
>> >>             dev-prop: iobase2 = 0x3ee
>> >>             dev-prop: irq = 11
>> >>             isa irq 11
>> >>             bus: foo.0
>> >>               type IDE
>> >> 
>> >> After: it's called "ide.0", I think.
>> >> 
>> > snprintf(name, sizeof(name), "%s.%d", dev->id ? : "ide", bus_id);
>> > will take care of it.
>> 
>> Not fully.
>> 
>> -M pc
>> -device isa-ide,iobase=0x1e8,iobase2=0x3ee,irq=11,id=foo
>> -device isa-ide,iobase=0x168,iobase2=0x36e,irq=10,id=bar
>> 
>> Before: foo.0 and bar.0
>> After: foo.0 and bar.1
>> 
>> >                       But I am starting to think that this is indeed not
>> > needed. Without the change my patch produce name like that in isapc case:
>> > /isa/ata@01f0/ide@0/disk@0
>> > /isa/ata@01f0/ide@0/disk@1
>> > /isa/ata@0170/ide@0/disk@0
>> > /isa/ata@0170/ide@0/disk@1
>> >
>> > so the paths are unique and usable by firmware. The only problem is that
>> > not all disks are addressable from -device option, so I can't set
>> > bootindex on some of them.
>> 
>> Perhaps we can treat the non-addressability of -M isapc's second IDE bus
>> as a separate problem.
>> 
> Agree, hence I will not use this patch and will get back to just
> recording bus_id. But -M isapc is just a symptom of much more serious
> problem in qdev. The way devices addressable there is not well defined.
> Two devices may have the same device path. Ultimately I think qdev
> should use device addresses similar to what I am trying to achieve here.
> For ISA machine it should automatically generate ids like isa-ide@0x170
> for first isa IDE controller and isa-ide@0x1f0 for second one. And get
> rid of user assigned ids. They are good for nothing and exist only
> because qdev developer didn't agree on how to name devices.

Yes, ambigous paths are a well-known problem.  For user-defined devices,
users can define the (unique) ID, which provides an unambigous path.
But default devices don't have one.  When we have two of identical
devices without ID on the same bus, their paths become ambigous.

There has been quite some discussion on "canonical path" on the list,
but no consensus.  Ironically, one of the places where we got stuck was
ISA.  You cut right through that, so that's progress.  Maybe people
aren't looking ;)

>> >> Likely to break non-default IDE controller use.  Could be rare enough
>> >> for us not to care, I don't know.
>> >> 
>> >> If we need to avoid this change, then ide_bus_new() must not pass a bus
>> >> name to qbus_create_inplace() for devices added with -device/device_add.
>> >> 
>> >> >  
>> >> >  static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
>> >> > diff --git a/hw/ide/via.c b/hw/ide/via.c
>> >> > index b2c7cad..cc48b2b 100644
>> >> > --- a/hw/ide/via.c
>> >> > +++ b/hw/ide/via.c
>> >> > @@ -158,8 +158,8 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
>> >> >  
>> >> >      vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
>> >> >  
>> >> > -    ide_bus_new(&d->bus[0], &d->dev.qdev);
>> >> > -    ide_bus_new(&d->bus[1], &d->dev.qdev);
>> >> > +    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
>> >> > +    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
>> >> >      ide_init2(&d->bus[0], isa_reserve_irq(14));
>> >> >      ide_init2(&d->bus[1], isa_reserve_irq(15));
>> >> >      ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
>> >> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> >> > index 3d07ce5..cec6c9b 100644
>> >> > --- a/hw/pc_piix.c
>> >> > +++ b/hw/pc_piix.c
>> >> > @@ -163,7 +163,7 @@ static void pc_init1(ram_addr_t ram_size,
>> >> >              ISADevice *dev;
>> >> >              dev = isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
>> >> >                                 hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
>> >> > -            idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
>> >> > +            idebus[i] = qdev_get_child_bus(&dev->qdev, i ? "ide.1" : "ide.0");
>> >> >          }
>> >> >      }
Gleb Natapov - Nov. 4, 2010, 9:23 a.m.
On Thu, Nov 04, 2010 at 09:46:57AM +0100, Markus Armbruster wrote:
> > But why order of device creation is important? It shouldn't be if we
> > want to move HW description into config file. We even may allow creating
> > piix3-ide with only second IDE bus, but not first.
> 
> That's not how buses work in qdev.
> 
> Configuration file, command line and monitor configure *devices*.
> Devices need to be created after the device providing their parent bus,
> obviously.  Other than that, order should not matter.
> 
Correct, but it will if we use your function for looking for IDEBus id.
See below.

> Buses are always created by device model code.  Thus, creation order is
> fixed in code.  Thus defining bus number in terms of creation order is
> fine.
So I can't create piix3-ide with only one bus. Looks like arbitrary
limitation for me.

> 
> If we create piix3-ide with only the second bus (for the sake of
> argument), then its bus number is zero by definition, as clarified by
> you below.  Unless you want to amend your definition :)
> 
As clarified by me below in case of piix3-ide bus number is actually
used to figure out how to talk to device on that bus. So if (for
the sake of argument of course) we will create piix3-ide device with
only secondary bus (the one accessible through BAR2,3 of piix3-ide),
device sitting on it will be named ide@0 since there will be only one
bus on piix3-ide. Given name ide@0 guest will try to use BAR0,1 and
fail. I understand that such config is not possible today (at least
with piix3-ide) but it is important to understand that this is not
"just a number showing in which order bus was created"


> >> >> >                                          Are you against adding bus_id
> >> >> > to IDEBus?
> >> >> 
> >> >> "Against" is too hard a word.  If it's a general question, I'd prefer a
> >> >> general answer.
> >> > It is as general as "what pci slot/func of a pci device". We store those
> >> > in PCIDevice.
> >> 
> >> It's actually more general than that :)
> >> 
> >> PCI slot.function is the address of a PCI device on its parent bus.
> >> It's specific to PCI buses.
> >> 
> >> The bus number is the "address" of a bus on its parent device.  It's the
> >> same regardless of the device.
> >> 
> > In case of IDE bus siting on piix3-ide it is not just arbitrary number
> > like you suggest here. It actually tells how to talk to a device. IDE
> > bus 0 is reachable through BAR0,1 of piix3-ide and IDE bus 1 is reachable
> > through BAR2,3. So this number is part of the device address as much as
> > pci slot/func is.
> 
> What I mean to say: regardless of what the device is, or what kind of
> buses it provides, the buses can *always* be identified the same way:
> define an order, identify bus by ordinal number.
Only if you always create them all and in the correct order. Then, just by
coincidence (not by design), your assertion above will be true.

> 
> Because of that, I believe the concept "bus number" should be a generic
> qdev concept, not special to IDE buses.
If you think that other devices may benefit from "bus number" I can
move it into BusState. Important thing is that "bus number" should
be determined by bus creator and should be independent from order of
creation. The thing is other devices may use other means to address
different buses. For instance system bus may have two PCI domains
one is addressable via 0x0cf8 IO port another is addressable through
MMIO address 0xf8000000. "bus number" is meaningless for those two
buses. Instead one of them will be named pci@i0cf8 and another one is
pci@f8000000.


> > I agree it is conceptually wrong. It is not even needed for unique device
> > path generation. It is only needed to make both IDE buses configurable
> > from command line in ISA machine. I'll drop it in favor of other solution,
> > but qdev has to rethink how devices should it addressable from command
> > line. Current way is broken.
> 
> I agree it's broken and needs fixing.  I appreciate you trying to fix
> it, but it indeed looks like it's better to fix it separately.
> 
OK.

> >> 
> >> Perhaps we can treat the non-addressability of -M isapc's second IDE bus
> >> as a separate problem.
> >> 
> > Agree, hence I will not use this patch and will get back to just
> > recording bus_id. But -M isapc is just a symptom of much more serious
> > problem in qdev. The way devices addressable there is not well defined.
> > Two devices may have the same device path. Ultimately I think qdev
> > should use device addresses similar to what I am trying to achieve here.
> > For ISA machine it should automatically generate ids like isa-ide@0x170
> > for first isa IDE controller and isa-ide@0x1f0 for second one. And get
> > rid of user assigned ids. They are good for nothing and exist only
> > because qdev developer didn't agree on how to name devices.
> 
> Yes, ambigous paths are a well-known problem.  For user-defined devices,
> users can define the (unique) ID, which provides an unambigous path.
This is just dropping problem onto a user. Qemu is capable to
create unique ids by itself. Advantage is that it will work for
internally create devices and user-defined devices.

> But default devices don't have one.  When we have two of identical
> devices without ID on the same bus, their paths become ambigous.
> 
> There has been quite some discussion on "canonical path" on the list,
> but no consensus.  Ironically, one of the places where we got stuck was
> ISA.  You cut right through that, so that's progress.  Maybe people
> aren't looking ;)
That is funny since the problem was already solved looong time ago. Just
look at Open Firmware device path. They are capable of addressing all
devices just fine, ISA devices included. What specific problem you had
with ISA bus? 

--
			Gleb.
Markus Armbruster - Nov. 4, 2010, 2:22 p.m.
Gleb Natapov <gleb@redhat.com> writes:

> On Thu, Nov 04, 2010 at 09:46:57AM +0100, Markus Armbruster wrote:
>> > But why order of device creation is important? It shouldn't be if we
>> > want to move HW description into config file. We even may allow creating
>> > piix3-ide with only second IDE bus, but not first.
>> 
>> That's not how buses work in qdev.
>> 
>> Configuration file, command line and monitor configure *devices*.
>> Devices need to be created after the device providing their parent bus,
>> obviously.  Other than that, order should not matter.
>> 
> Correct, but it will if we use your function for looking for IDEBus id.
> See below.

Where I'll explain why device order creation can't affect bus numbers.

>> Buses are always created by device model code.  Thus, creation order is
>> fixed in code.  Thus defining bus number in terms of creation order is
>> fine.
> So I can't create piix3-ide with only one bus. Looks like arbitrary
> limitation for me.

If it has just one bus, it's simply not piix3-ide anymore.  The real
PIIX3 PCI device has an IDE function that provides two channels, no
more, no less.

>> If we create piix3-ide with only the second bus (for the sake of
>> argument), then its bus number is zero by definition, as clarified by
>> you below.  Unless you want to amend your definition :)
>> 
> As clarified by me below in case of piix3-ide bus number is actually
> used to figure out how to talk to device on that bus. So if (for
> the sake of argument of course) we will create piix3-ide device with
> only secondary bus (the one accessible through BAR2,3 of piix3-ide),
> device sitting on it will be named ide@0 since there will be only one
> bus on piix3-ide. Given name ide@0 guest will try to use BAR0,1 and
> fail. I understand that such config is not possible today (at least
> with piix3-ide) but it is important to understand that this is not
> "just a number showing in which order bus was created"

Why would a hypothetical piix3-ide with just one IDE channel
nevertheless expose BARs for *two* IDE channels?

To me it looks like the need for "holes" in the bus number sequence is
purely theoretical.

>> >> >> >                                          Are you against adding bus_id
>> >> >> > to IDEBus?
>> >> >> 
>> >> >> "Against" is too hard a word.  If it's a general question, I'd prefer a
>> >> >> general answer.
>> >> > It is as general as "what pci slot/func of a pci device". We store those
>> >> > in PCIDevice.
>> >> 
>> >> It's actually more general than that :)
>> >> 
>> >> PCI slot.function is the address of a PCI device on its parent bus.
>> >> It's specific to PCI buses.
>> >> 
>> >> The bus number is the "address" of a bus on its parent device.  It's the
>> >> same regardless of the device.
>> >> 
>> > In case of IDE bus siting on piix3-ide it is not just arbitrary number
>> > like you suggest here. It actually tells how to talk to a device. IDE
>> > bus 0 is reachable through BAR0,1 of piix3-ide and IDE bus 1 is reachable
>> > through BAR2,3. So this number is part of the device address as much as
>> > pci slot/func is.
>> 
>> What I mean to say: regardless of what the device is, or what kind of
>> buses it provides, the buses can *always* be identified the same way:
>> define an order, identify bus by ordinal number.
> Only if you always create them all and in the correct order. Then, just by
> coincidence (not by design), your assertion above will be true.

By definition of bus number, not by coincidence.

It's trivial to create buses in order.  It also ensures the numbers are
sane: start with 0, no holes.

Passing bus numbers explicitly isn't hard either.  Programmer is free to
pass nonsensical numbers.  For the most common case of one bus, the bus
number parameter is just noise.

There are two disputed issues here:

1. Whether to pass bus numbers explicitly or not.  I'd prefer not.  But
   I won't insist on it.

2. Whether bus numbers are IDE specific or general.  In my opinion, they
   are general.

>> Because of that, I believe the concept "bus number" should be a generic
>> qdev concept, not special to IDE buses.
> If you think that other devices may benefit from "bus number" I can
> move it into BusState. Important thing is that "bus number" should
> be determined by bus creator and should be independent from order of
> creation. The thing is other devices may use other means to address
> different buses. For instance system bus may have two PCI domains
> one is addressable via 0x0cf8 IO port another is addressable through
> MMIO address 0xf8000000. "bus number" is meaningless for those two
> buses. Instead one of them will be named pci@i0cf8 and another one is
> pci@f8000000.

The system bus doesn't "have two PCI domains".  There are *devices*
providing PCI buses on the system bus.

In your example, there are two such devices on the system bus.  One with
configuration I/O port 0x0cf8, and one with memory-mapped configuration
at 0xf8000000.  Bus number is indeed meaningless, because a bus number
numbers a device's buses, not the system bus's devices!

Devices are identified by their address on the parent bus.  The
addressing scheme is specific to the parent bus type.

Buses are identified by their bus number within their parent device.
The addressing scheme is always the same: ordinal number.

>> > I agree it is conceptually wrong. It is not even needed for unique device
>> > path generation. It is only needed to make both IDE buses configurable
>> > from command line in ISA machine. I'll drop it in favor of other solution,
>> > but qdev has to rethink how devices should it addressable from command
>> > line. Current way is broken.
>> 
>> I agree it's broken and needs fixing.  I appreciate you trying to fix
>> it, but it indeed looks like it's better to fix it separately.
>> 
> OK.
>
>> >> 
>> >> Perhaps we can treat the non-addressability of -M isapc's second IDE bus
>> >> as a separate problem.
>> >> 
>> > Agree, hence I will not use this patch and will get back to just
>> > recording bus_id. But -M isapc is just a symptom of much more serious
>> > problem in qdev. The way devices addressable there is not well defined.
>> > Two devices may have the same device path. Ultimately I think qdev
>> > should use device addresses similar to what I am trying to achieve here.
>> > For ISA machine it should automatically generate ids like isa-ide@0x170
>> > for first isa IDE controller and isa-ide@0x1f0 for second one. And get
>> > rid of user assigned ids. They are good for nothing and exist only
>> > because qdev developer didn't agree on how to name devices.
>> 
>> Yes, ambigous paths are a well-known problem.  For user-defined devices,
>> users can define the (unique) ID, which provides an unambigous path.
> This is just dropping problem onto a user. Qemu is capable to
> create unique ids by itself. Advantage is that it will work for
> internally create devices and user-defined devices.

IDs are a user feature.  The user has full control over them.  If we
created IDs automatically, they could clash with the user's.

The problem is that our rules what to do when the user didn't assign ID
are broken.  Yes, we should make sure every device has an unambigous
name even then.  More so for devices created automatically.

>> But default devices don't have one.  When we have two of identical
>> devices without ID on the same bus, their paths become ambigous.
>> 
>> There has been quite some discussion on "canonical path" on the list,
>> but no consensus.  Ironically, one of the places where we got stuck was
>> ISA.  You cut right through that, so that's progress.  Maybe people
>> aren't looking ;)
> That is funny since the problem was already solved looong time ago. Just
> look at Open Firmware device path. They are capable of addressing all
> devices just fine, ISA devices included. What specific problem you had
> with ISA bus? 

Lack of consensus.  I was in favour of using I/O base, just like you do.
There were worries about ISA devices not using any I/O ports.
Gleb Natapov - Nov. 4, 2010, 3:26 p.m.
On Thu, Nov 04, 2010 at 03:22:50PM +0100, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > On Thu, Nov 04, 2010 at 09:46:57AM +0100, Markus Armbruster wrote:
> >> > But why order of device creation is important? It shouldn't be if we
> >> > want to move HW description into config file. We even may allow creating
> >> > piix3-ide with only second IDE bus, but not first.
> >> 
> >> That's not how buses work in qdev.
> >> 
> >> Configuration file, command line and monitor configure *devices*.
> >> Devices need to be created after the device providing their parent bus,
> >> obviously.  Other than that, order should not matter.
> >> 
> > Correct, but it will if we use your function for looking for IDEBus id.
> > See below.
> 
> Where I'll explain why device order creation can't affect bus numbers.
> 
Bus order creation will affect bus number thought.

> >> Buses are always created by device model code.  Thus, creation order is
> >> fixed in code.  Thus defining bus number in terms of creation order is
> >> fine.
> > So I can't create piix3-ide with only one bus. Looks like arbitrary
> > limitation for me.
> 
> If it has just one bus, it's simply not piix3-ide anymore.  The real
> PIIX3 PCI device has an IDE function that provides two channels, no
> more, no less.
> 
We are doing mental exercise to see if our design is flexible enough

> >> If we create piix3-ide with only the second bus (for the sake of
> >> argument), then its bus number is zero by definition, as clarified by
> >> you below.  Unless you want to amend your definition :)
> >> 
> > As clarified by me below in case of piix3-ide bus number is actually
> > used to figure out how to talk to device on that bus. So if (for
> > the sake of argument of course) we will create piix3-ide device with
> > only secondary bus (the one accessible through BAR2,3 of piix3-ide),
> > device sitting on it will be named ide@0 since there will be only one
> > bus on piix3-ide. Given name ide@0 guest will try to use BAR0,1 and
> > fail. I understand that such config is not possible today (at least
> > with piix3-ide) but it is important to understand that this is not
> > "just a number showing in which order bus was created"
> 
> Why would a hypothetical piix3-ide with just one IDE channel
> nevertheless expose BARs for *two* IDE channels?
> 
> To me it looks like the need for "holes" in the bus number sequence is
> purely theoretical.
> 
And to me it looks like relying on implicit bus ordering may limit us in
the situation too.

> >> >> >> >                                          Are you against adding bus_id
> >> >> >> > to IDEBus?
> >> >> >> 
> >> >> >> "Against" is too hard a word.  If it's a general question, I'd prefer a
> >> >> >> general answer.
> >> >> > It is as general as "what pci slot/func of a pci device". We store those
> >> >> > in PCIDevice.
> >> >> 
> >> >> It's actually more general than that :)
> >> >> 
> >> >> PCI slot.function is the address of a PCI device on its parent bus.
> >> >> It's specific to PCI buses.
> >> >> 
> >> >> The bus number is the "address" of a bus on its parent device.  It's the
> >> >> same regardless of the device.
> >> >> 
> >> > In case of IDE bus siting on piix3-ide it is not just arbitrary number
> >> > like you suggest here. It actually tells how to talk to a device. IDE
> >> > bus 0 is reachable through BAR0,1 of piix3-ide and IDE bus 1 is reachable
> >> > through BAR2,3. So this number is part of the device address as much as
> >> > pci slot/func is.
> >> 
> >> What I mean to say: regardless of what the device is, or what kind of
> >> buses it provides, the buses can *always* be identified the same way:
> >> define an order, identify bus by ordinal number.
> > Only if you always create them all and in the correct order. Then, just by
> > coincidence (not by design), your assertion above will be true.
> 
> By definition of bus number, not by coincidence.
> 
> It's trivial to create buses in order.  It also ensures the numbers are
> sane: start with 0, no holes.
In case of piix3-ide I agree it is indeed trivial to create them in
order, but are you sure about all other, not yet implemented, cases?
> 
> Passing bus numbers explicitly isn't hard either.  Programmer is free to
> pass nonsensical numbers.  For the most common case of one bus, the bus
> number parameter is just noise.
If programmer makes an error this is a bug that should be fixed.

> 
> There are two disputed issues here:
> 
> 1. Whether to pass bus numbers explicitly or not.  I'd prefer not.  But
>    I won't insist on it.
> 
> 2. Whether bus numbers are IDE specific or general.  In my opinion, they
>    are general.
> 
Do we have other cases of device providing two buses in qemu tree now?
I prefer to do it only for piix3-ide for now and change it later if
this is a common pattern.

> >> Because of that, I believe the concept "bus number" should be a generic
> >> qdev concept, not special to IDE buses.
> > If you think that other devices may benefit from "bus number" I can
> > move it into BusState. Important thing is that "bus number" should
> > be determined by bus creator and should be independent from order of
> > creation. The thing is other devices may use other means to address
> > different buses. For instance system bus may have two PCI domains
> > one is addressable via 0x0cf8 IO port another is addressable through
> > MMIO address 0xf8000000. "bus number" is meaningless for those two
> > buses. Instead one of them will be named pci@i0cf8 and another one is
> > pci@f8000000.
> 
> The system bus doesn't "have two PCI domains".  There are *devices*
> providing PCI buses on the system bus.
> 
Correct. Not a good example indeed. I can't think of other device with
two buses except piix3_ide.

> In your example, there are two such devices on the system bus.  One with
> configuration I/O port 0x0cf8, and one with memory-mapped configuration
> at 0xf8000000.  Bus number is indeed meaningless, because a bus number
> numbers a device's buses, not the system bus's devices!
> 
> Devices are identified by their address on the parent bus.  The
> addressing scheme is specific to the parent bus type.
> 
> Buses are identified by their bus number within their parent device.
> The addressing scheme is always the same: ordinal number.
> 
> >> > I agree it is conceptually wrong. It is not even needed for unique device
> >> > path generation. It is only needed to make both IDE buses configurable
> >> > from command line in ISA machine. I'll drop it in favor of other solution,
> >> > but qdev has to rethink how devices should it addressable from command
> >> > line. Current way is broken.
> >> 
> >> I agree it's broken and needs fixing.  I appreciate you trying to fix
> >> it, but it indeed looks like it's better to fix it separately.
> >> 
> > OK.
> >
> >> >> 
> >> >> Perhaps we can treat the non-addressability of -M isapc's second IDE bus
> >> >> as a separate problem.
> >> >> 
> >> > Agree, hence I will not use this patch and will get back to just
> >> > recording bus_id. But -M isapc is just a symptom of much more serious
> >> > problem in qdev. The way devices addressable there is not well defined.
> >> > Two devices may have the same device path. Ultimately I think qdev
> >> > should use device addresses similar to what I am trying to achieve here.
> >> > For ISA machine it should automatically generate ids like isa-ide@0x170
> >> > for first isa IDE controller and isa-ide@0x1f0 for second one. And get
> >> > rid of user assigned ids. They are good for nothing and exist only
> >> > because qdev developer didn't agree on how to name devices.
> >> 
> >> Yes, ambigous paths are a well-known problem.  For user-defined devices,
> >> users can define the (unique) ID, which provides an unambigous path.
> > This is just dropping problem onto a user. Qemu is capable to
> > create unique ids by itself. Advantage is that it will work for
> > internally create devices and user-defined devices.
> 
> IDs are a user feature.  The user has full control over them.  If we
> created IDs automatically, they could clash with the user's.
> 
That is why I am saying that it is misfeature. It should have been
automatically created for all devices.

> The problem is that our rules what to do when the user didn't assign ID
> are broken.  Yes, we should make sure every device has an unambigous
> name even then.  More so for devices created automatically.
> 
> >> But default devices don't have one.  When we have two of identical
> >> devices without ID on the same bus, their paths become ambigous.
> >> 
> >> There has been quite some discussion on "canonical path" on the list,
> >> but no consensus.  Ironically, one of the places where we got stuck was
> >> ISA.  You cut right through that, so that's progress.  Maybe people
> >> aren't looking ;)
> > That is funny since the problem was already solved looong time ago. Just
> > look at Open Firmware device path. They are capable of addressing all
> > devices just fine, ISA devices included. What specific problem you had
> > with ISA bus? 
> 
> Lack of consensus.  I was in favour of using I/O base, just like you do.
> There were worries about ISA devices not using any I/O ports.
There is a solution for that problem for almost 15 years and we are
still looking for consensus on qemu list?! Here is ISA device binding
spec for Open Firmware: http://playground.sun.com/1275/bindings/isa/isa0_4d.ps 
If ISA device have no IO ports MMIO is used.

--
			Gleb.
Markus Armbruster - Nov. 5, 2010, 2:04 p.m.
Gleb Natapov <gleb@redhat.com> writes:

> On Thu, Nov 04, 2010 at 03:22:50PM +0100, Markus Armbruster wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > On Thu, Nov 04, 2010 at 09:46:57AM +0100, Markus Armbruster wrote:
>> >> > But why order of device creation is important? It shouldn't be if we
>> >> > want to move HW description into config file. We even may allow creating
>> >> > piix3-ide with only second IDE bus, but not first.
>> >> 
>> >> That's not how buses work in qdev.
>> >> 
>> >> Configuration file, command line and monitor configure *devices*.
>> >> Devices need to be created after the device providing their parent bus,
>> >> obviously.  Other than that, order should not matter.
>> >> 
>> > Correct, but it will if we use your function for looking for IDEBus id.
>> > See below.
>> 
>> Where I'll explain why device order creation can't affect bus numbers.
>> 
> Bus order creation will affect bus number thought.

Yes, but that order is completely arbitrary in the code, so you can just
as well create the buses in the natural order.

>> >> Buses are always created by device model code.  Thus, creation order is
>> >> fixed in code.  Thus defining bus number in terms of creation order is
>> >> fine.
>> > So I can't create piix3-ide with only one bus. Looks like arbitrary
>> > limitation for me.
>> 
>> If it has just one bus, it's simply not piix3-ide anymore.  The real
>> PIIX3 PCI device has an IDE function that provides two channels, no
>> more, no less.
>> 
> We are doing mental exercise to see if our design is flexible enough

Fair enough.  What I mean to say is that a device's buses are an
integral part of it, not something that's pluggable.

>> >> If we create piix3-ide with only the second bus (for the sake of
>> >> argument), then its bus number is zero by definition, as clarified by
>> >> you below.  Unless you want to amend your definition :)
>> >> 
>> > As clarified by me below in case of piix3-ide bus number is actually
>> > used to figure out how to talk to device on that bus. So if (for
>> > the sake of argument of course) we will create piix3-ide device with
>> > only secondary bus (the one accessible through BAR2,3 of piix3-ide),
>> > device sitting on it will be named ide@0 since there will be only one
>> > bus on piix3-ide. Given name ide@0 guest will try to use BAR0,1 and
>> > fail. I understand that such config is not possible today (at least
>> > with piix3-ide) but it is important to understand that this is not
>> > "just a number showing in which order bus was created"
>> 
>> Why would a hypothetical piix3-ide with just one IDE channel
>> nevertheless expose BARs for *two* IDE channels?
>> 
>> To me it looks like the need for "holes" in the bus number sequence is
>> purely theoretical.
>> 
> And to me it looks like relying on implicit bus ordering may limit us in
> the situation too.
>
>> >> >> >> >                                          Are you against adding bus_id
>> >> >> >> > to IDEBus?
>> >> >> >> 
>> >> >> >> "Against" is too hard a word.  If it's a general question, I'd prefer a
>> >> >> >> general answer.
>> >> >> > It is as general as "what pci slot/func of a pci device". We store those
>> >> >> > in PCIDevice.
>> >> >> 
>> >> >> It's actually more general than that :)
>> >> >> 
>> >> >> PCI slot.function is the address of a PCI device on its parent bus.
>> >> >> It's specific to PCI buses.
>> >> >> 
>> >> >> The bus number is the "address" of a bus on its parent device.  It's the
>> >> >> same regardless of the device.
>> >> >> 
>> >> > In case of IDE bus siting on piix3-ide it is not just arbitrary number
>> >> > like you suggest here. It actually tells how to talk to a device. IDE
>> >> > bus 0 is reachable through BAR0,1 of piix3-ide and IDE bus 1 is reachable
>> >> > through BAR2,3. So this number is part of the device address as much as
>> >> > pci slot/func is.
>> >> 
>> >> What I mean to say: regardless of what the device is, or what kind of
>> >> buses it provides, the buses can *always* be identified the same way:
>> >> define an order, identify bus by ordinal number.
>> > Only if you always create them all and in the correct order. Then, just by
>> > coincidence (not by design), your assertion above will be true.
>> 
>> By definition of bus number, not by coincidence.
>> 
>> It's trivial to create buses in order.  It also ensures the numbers are
>> sane: start with 0, no holes.
> In case of piix3-ide I agree it is indeed trivial to create them in
> order, but are you sure about all other, not yet implemented, cases?

Bus creation is basically "malloc the bus state, initialize it, add to
the parent's list of children".  Can't see how we could get dependencies
in there.

>> Passing bus numbers explicitly isn't hard either.  Programmer is free to
>> pass nonsensical numbers.  For the most common case of one bus, the bus
>> number parameter is just noise.
> If programmer makes an error this is a bug that should be fixed.
>
>> 
>> There are two disputed issues here:
>> 
>> 1. Whether to pass bus numbers explicitly or not.  I'd prefer not.  But
>>    I won't insist on it.
>> 
>> 2. Whether bus numbers are IDE specific or general.  In my opinion, they
>>    are general.
>> 
> Do we have other cases of device providing two buses in qemu tree now?
> I prefer to do it only for piix3-ide for now and change it later if
> this is a common pattern.

A quick, superficial search finds

device          buses           function creating them
corgi-ssp       3 x SSI         corgi_ssp_init()
evb6965-ssi     2 x SSI         stellaris_ssi_bus_init()
cmd646-ide      2 x IDE         pci_cmd646_ide_initfn()
piix[34]-ide    2 x IDE         pci_piix_ide_initfn()
via-ide         2 x IDE         vt82c686b_ide_initfn()

It's even conceivable that one device provides two different kinds of
buses.  I'd expect sane PCI devices to use separate functions for that,
but not everything's PCI, and not everything's sane.

>> >> Because of that, I believe the concept "bus number" should be a generic
>> >> qdev concept, not special to IDE buses.
>> > If you think that other devices may benefit from "bus number" I can
>> > move it into BusState. Important thing is that "bus number" should
>> > be determined by bus creator and should be independent from order of
>> > creation. The thing is other devices may use other means to address
>> > different buses. For instance system bus may have two PCI domains
>> > one is addressable via 0x0cf8 IO port another is addressable through
>> > MMIO address 0xf8000000. "bus number" is meaningless for those two
>> > buses. Instead one of them will be named pci@i0cf8 and another one is
>> > pci@f8000000.
>> 
>> The system bus doesn't "have two PCI domains".  There are *devices*
>> providing PCI buses on the system bus.
>> 
> Correct. Not a good example indeed. I can't think of other device with
> two buses except piix3_ide.
>
>> In your example, there are two such devices on the system bus.  One with
>> configuration I/O port 0x0cf8, and one with memory-mapped configuration
>> at 0xf8000000.  Bus number is indeed meaningless, because a bus number
>> numbers a device's buses, not the system bus's devices!
>> 
>> Devices are identified by their address on the parent bus.  The
>> addressing scheme is specific to the parent bus type.
>> 
>> Buses are identified by their bus number within their parent device.
>> The addressing scheme is always the same: ordinal number.
>> 
>> >> > I agree it is conceptually wrong. It is not even needed for unique device
>> >> > path generation. It is only needed to make both IDE buses configurable
>> >> > from command line in ISA machine. I'll drop it in favor of other solution,
>> >> > but qdev has to rethink how devices should it addressable from command
>> >> > line. Current way is broken.
>> >> 
>> >> I agree it's broken and needs fixing.  I appreciate you trying to fix
>> >> it, but it indeed looks like it's better to fix it separately.
>> >> 
>> > OK.
>> >
>> >> >> 
>> >> >> Perhaps we can treat the non-addressability of -M isapc's second IDE bus
>> >> >> as a separate problem.
>> >> >> 
>> >> > Agree, hence I will not use this patch and will get back to just
>> >> > recording bus_id. But -M isapc is just a symptom of much more serious
>> >> > problem in qdev. The way devices addressable there is not well defined.
>> >> > Two devices may have the same device path. Ultimately I think qdev
>> >> > should use device addresses similar to what I am trying to achieve here.
>> >> > For ISA machine it should automatically generate ids like isa-ide@0x170
>> >> > for first isa IDE controller and isa-ide@0x1f0 for second one. And get
>> >> > rid of user assigned ids. They are good for nothing and exist only
>> >> > because qdev developer didn't agree on how to name devices.
>> >> 
>> >> Yes, ambigous paths are a well-known problem.  For user-defined devices,
>> >> users can define the (unique) ID, which provides an unambigous path.
>> > This is just dropping problem onto a user. Qemu is capable to
>> > create unique ids by itself. Advantage is that it will work for
>> > internally create devices and user-defined devices.
>> 
>> IDs are a user feature.  The user has full control over them.  If we
>> created IDs automatically, they could clash with the user's.
>> 
> That is why I am saying that it is misfeature. It should have been
> automatically created for all devices.

Automatically created IDs tend to suck for human users.

Yes, the problem could have been avoided with a bit more thought.  Too
bad.  We'll fix it.

>> The problem is that our rules what to do when the user didn't assign ID
>> are broken.  Yes, we should make sure every device has an unambigous
>> name even then.  More so for devices created automatically.
>> 
>> >> But default devices don't have one.  When we have two of identical
>> >> devices without ID on the same bus, their paths become ambigous.
>> >> 
>> >> There has been quite some discussion on "canonical path" on the list,
>> >> but no consensus.  Ironically, one of the places where we got stuck was
>> >> ISA.  You cut right through that, so that's progress.  Maybe people
>> >> aren't looking ;)
>> > That is funny since the problem was already solved looong time ago. Just
>> > look at Open Firmware device path. They are capable of addressing all
>> > devices just fine, ISA devices included. What specific problem you had
>> > with ISA bus? 
>> 
>> Lack of consensus.  I was in favour of using I/O base, just like you do.
>> There were worries about ISA devices not using any I/O ports.
> There is a solution for that problem for almost 15 years and we are
> still looking for consensus on qemu list?! Here is ISA device binding
> spec for Open Firmware: http://playground.sun.com/1275/bindings/isa/isa0_4d.ps 
> If ISA device have no IO ports MMIO is used.

Precedence should promote consensus, but it can't replace it.  If you
can push the list to consensus, more power to you.
Gleb Natapov - Nov. 5, 2010, 3:54 p.m.
On Fri, Nov 05, 2010 at 03:04:05PM +0100, Markus Armbruster wrote:
[skip]
> >> Passing bus numbers explicitly isn't hard either.  Programmer is free to
> >> pass nonsensical numbers.  For the most common case of one bus, the bus
> >> number parameter is just noise.
> > If programmer makes an error this is a bug that should be fixed.
> >
> >> 
> >> There are two disputed issues here:
> >> 
> >> 1. Whether to pass bus numbers explicitly or not.  I'd prefer not.  But
> >>    I won't insist on it.
> >> 
> >> 2. Whether bus numbers are IDE specific or general.  In my opinion, they
> >>    are general.
> >> 
> > Do we have other cases of device providing two buses in qemu tree now?
> > I prefer to do it only for piix3-ide for now and change it later if
> > this is a common pattern.
> 
> A quick, superficial search finds
> 
> device          buses           function creating them
> corgi-ssp       3 x SSI         corgi_ssp_init()
> evb6965-ssi     2 x SSI         stellaris_ssi_bus_init()
I'll look at those.

> cmd646-ide      2 x IDE         pci_cmd646_ide_initfn()
> piix[34]-ide    2 x IDE         pci_piix_ide_initfn()
> via-ide         2 x IDE         vt82c686b_ide_initfn()
And those are the same IDEBus we already discussing.

> 
> It's even conceivable that one device provides two different kinds of
> buses.  I'd expect sane PCI devices to use separate functions for that,
> but not everything's PCI, and not everything's sane.
> 
Exactly, so we should be as flexible as possible to accommodate insane
HW.

> >> >> Because of that, I believe the concept "bus number" should be a generic
> >> >> qdev concept, not special to IDE buses.
> >> > If you think that other devices may benefit from "bus number" I can
> >> > move it into BusState. Important thing is that "bus number" should
> >> > be determined by bus creator and should be independent from order of
> >> > creation. The thing is other devices may use other means to address
> >> > different buses. For instance system bus may have two PCI domains
> >> > one is addressable via 0x0cf8 IO port another is addressable through
> >> > MMIO address 0xf8000000. "bus number" is meaningless for those two
> >> > buses. Instead one of them will be named pci@i0cf8 and another one is
> >> > pci@f8000000.
> >> 
> >> The system bus doesn't "have two PCI domains".  There are *devices*
> >> providing PCI buses on the system bus.
> >> 
> > Correct. Not a good example indeed. I can't think of other device with
> > two buses except piix3_ide.
> >
> >> In your example, there are two such devices on the system bus.  One with
> >> configuration I/O port 0x0cf8, and one with memory-mapped configuration
> >> at 0xf8000000.  Bus number is indeed meaningless, because a bus number
> >> numbers a device's buses, not the system bus's devices!
> >> 
> >> Devices are identified by their address on the parent bus.  The
> >> addressing scheme is specific to the parent bus type.
> >> 
> >> Buses are identified by their bus number within their parent device.
> >> The addressing scheme is always the same: ordinal number.
> >> 
> >> >> > I agree it is conceptually wrong. It is not even needed for unique device
> >> >> > path generation. It is only needed to make both IDE buses configurable
> >> >> > from command line in ISA machine. I'll drop it in favor of other solution,
> >> >> > but qdev has to rethink how devices should it addressable from command
> >> >> > line. Current way is broken.
> >> >> 
> >> >> I agree it's broken and needs fixing.  I appreciate you trying to fix
> >> >> it, but it indeed looks like it's better to fix it separately.
> >> >> 
> >> > OK.
> >> >
> >> >> >> 
> >> >> >> Perhaps we can treat the non-addressability of -M isapc's second IDE bus
> >> >> >> as a separate problem.
> >> >> >> 
> >> >> > Agree, hence I will not use this patch and will get back to just
> >> >> > recording bus_id. But -M isapc is just a symptom of much more serious
> >> >> > problem in qdev. The way devices addressable there is not well defined.
> >> >> > Two devices may have the same device path. Ultimately I think qdev
> >> >> > should use device addresses similar to what I am trying to achieve here.
> >> >> > For ISA machine it should automatically generate ids like isa-ide@0x170
> >> >> > for first isa IDE controller and isa-ide@0x1f0 for second one. And get
> >> >> > rid of user assigned ids. They are good for nothing and exist only
> >> >> > because qdev developer didn't agree on how to name devices.
> >> >> 
> >> >> Yes, ambigous paths are a well-known problem.  For user-defined devices,
> >> >> users can define the (unique) ID, which provides an unambigous path.
> >> > This is just dropping problem onto a user. Qemu is capable to
> >> > create unique ids by itself. Advantage is that it will work for
> >> > internally create devices and user-defined devices.
> >> 
> >> IDs are a user feature.  The user has full control over them.  If we
> >> created IDs automatically, they could clash with the user's.
> >> 
> > That is why I am saying that it is misfeature. It should have been
> > automatically created for all devices.
> 
> Automatically created IDs tend to suck for human users.
> 
You can mitigate it by allowing user to query existing device paths and
making matching more flexible. For instance if you have only one pci
domain with name ACME,pci it will be enough for user to specify only pci
as path element.

> Yes, the problem could have been avoided with a bit more thought.  Too
> bad.  We'll fix it.
> 
> >> The problem is that our rules what to do when the user didn't assign ID
> >> are broken.  Yes, we should make sure every device has an unambigous
> >> name even then.  More so for devices created automatically.
> >> 
> >> >> But default devices don't have one.  When we have two of identical
> >> >> devices without ID on the same bus, their paths become ambigous.
> >> >> 
> >> >> There has been quite some discussion on "canonical path" on the list,
> >> >> but no consensus.  Ironically, one of the places where we got stuck was
> >> >> ISA.  You cut right through that, so that's progress.  Maybe people
> >> >> aren't looking ;)
> >> > That is funny since the problem was already solved looong time ago. Just
> >> > look at Open Firmware device path. They are capable of addressing all
> >> > devices just fine, ISA devices included. What specific problem you had
> >> > with ISA bus? 
> >> 
> >> Lack of consensus.  I was in favour of using I/O base, just like you do.
> >> There were worries about ISA devices not using any I/O ports.
> > There is a solution for that problem for almost 15 years and we are
> > still looking for consensus on qemu list?! Here is ISA device binding
> > spec for Open Firmware: http://playground.sun.com/1275/bindings/isa/isa0_4d.ps 
> > If ISA device have no IO ports MMIO is used.
> 
> Precedence should promote consensus, but it can't replace it.  If you
> can push the list to consensus, more power to you.
I do not see disagreement right now :) You are saying you agree. Blue
Swirl asked me to use Open Firmware so I assume he agrees to. So who is
against and what are his arguments?

--
			Gleb.
Markus Armbruster - Nov. 5, 2010, 4:31 p.m.
Gleb Natapov <gleb@redhat.com> writes:

> On Fri, Nov 05, 2010 at 03:04:05PM +0100, Markus Armbruster wrote:
[...]
>> >> >> There has been quite some discussion on "canonical path" on the list,
>> >> >> but no consensus.  Ironically, one of the places where we got stuck was
>> >> >> ISA.  You cut right through that, so that's progress.  Maybe people
>> >> >> aren't looking ;)
>> >> > That is funny since the problem was already solved looong time ago. Just
>> >> > look at Open Firmware device path. They are capable of addressing all
>> >> > devices just fine, ISA devices included. What specific problem you had
>> >> > with ISA bus? 
>> >> 
>> >> Lack of consensus.  I was in favour of using I/O base, just like you do.
>> >> There were worries about ISA devices not using any I/O ports.
>> > There is a solution for that problem for almost 15 years and we are
>> > still looking for consensus on qemu list?! Here is ISA device binding
>> > spec for Open Firmware: http://playground.sun.com/1275/bindings/isa/isa0_4d.ps 
>> > If ISA device have no IO ports MMIO is used.
>> 
>> Precedence should promote consensus, but it can't replace it.  If you
>> can push the list to consensus, more power to you.
> I do not see disagreement right now :) You are saying you agree. Blue
> Swirl asked me to use Open Firmware so I assume he agrees to. So who is
> against and what are his arguments?

Start here:

http://lists.nongnu.org/archive/html/qemu-devel/2010-06/msg01618.html
Gleb Natapov - Nov. 5, 2010, 6:44 p.m.
On Fri, Nov 05, 2010 at 05:31:38PM +0100, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > On Fri, Nov 05, 2010 at 03:04:05PM +0100, Markus Armbruster wrote:
> [...]
> >> >> >> There has been quite some discussion on "canonical path" on the list,
> >> >> >> but no consensus.  Ironically, one of the places where we got stuck was
> >> >> >> ISA.  You cut right through that, so that's progress.  Maybe people
> >> >> >> aren't looking ;)
> >> >> > That is funny since the problem was already solved looong time ago. Just
> >> >> > look at Open Firmware device path. They are capable of addressing all
> >> >> > devices just fine, ISA devices included. What specific problem you had
> >> >> > with ISA bus? 
> >> >> 
> >> >> Lack of consensus.  I was in favour of using I/O base, just like you do.
> >> >> There were worries about ISA devices not using any I/O ports.
> >> > There is a solution for that problem for almost 15 years and we are
> >> > still looking for consensus on qemu list?! Here is ISA device binding
> >> > spec for Open Firmware: http://playground.sun.com/1275/bindings/isa/isa0_4d.ps 
> >> > If ISA device have no IO ports MMIO is used.
> >> 
> >> Precedence should promote consensus, but it can't replace it.  If you
> >> can push the list to consensus, more power to you.
> > I do not see disagreement right now :) You are saying you agree. Blue
> > Swirl asked me to use Open Firmware so I assume he agrees to. So who is
> > against and what are his arguments?
> 
> Start here:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2010-06/msg01618.html

I saw this in fact. The wouldn't agree with this device path proposal
too. It mixes qemu internal names (which is a big no-no for my purpose)
and bus addresses. Paul made sensible points there and if you look
closely what he proposes is what I implemented here. Regarding ISA
("busses that don't have a consistent addressing scheme" he called it)
he himself proposed to use address of the first IO port/memory region
as an ID. This is what is already implemented by my patch.

--
			Gleb.
Markus Armbruster - Nov. 6, 2010, 9:25 a.m.
Gleb Natapov <gleb@redhat.com> writes:

> On Fri, Nov 05, 2010 at 05:31:38PM +0100, Markus Armbruster wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > On Fri, Nov 05, 2010 at 03:04:05PM +0100, Markus Armbruster wrote:
>> [...]
>> >> >> >> There has been quite some discussion on "canonical path" on the list,
>> >> >> >> but no consensus.  Ironically, one of the places where we got stuck was
>> >> >> >> ISA.  You cut right through that, so that's progress.  Maybe people
>> >> >> >> aren't looking ;)
>> >> >> > That is funny since the problem was already solved looong time ago. Just
>> >> >> > look at Open Firmware device path. They are capable of addressing all
>> >> >> > devices just fine, ISA devices included. What specific problem you had
>> >> >> > with ISA bus? 
>> >> >> 
>> >> >> Lack of consensus.  I was in favour of using I/O base, just like you do.
>> >> >> There were worries about ISA devices not using any I/O ports.
>> >> > There is a solution for that problem for almost 15 years and we are
>> >> > still looking for consensus on qemu list?! Here is ISA device binding
>> >> > spec for Open Firmware: http://playground.sun.com/1275/bindings/isa/isa0_4d.ps 
>> >> > If ISA device have no IO ports MMIO is used.
>> >> 
>> >> Precedence should promote consensus, but it can't replace it.  If you
>> >> can push the list to consensus, more power to you.
>> > I do not see disagreement right now :) You are saying you agree. Blue
>> > Swirl asked me to use Open Firmware so I assume he agrees to. So who is
>> > against and what are his arguments?
>> 
>> Start here:
>> 
>> http://lists.nongnu.org/archive/html/qemu-devel/2010-06/msg01618.html
>
> I saw this in fact. The wouldn't agree with this device path proposal
> too. It mixes qemu internal names (which is a big no-no for my purpose)
> and bus addresses. Paul made sensible points there and if you look
> closely what he proposes is what I implemented here. Regarding ISA
> ("busses that don't have a consistent addressing scheme" he called it)
> he himself proposed to use address of the first IO port/memory region
> as an ID. This is what is already implemented by my patch.

You don't have to convince me; I was with Paul in that thread.

Regarding DeviceInfo member name values being QEMU internal: hardly.
They're ABI.  They're what we use to identify device types on external
interfaces including command line, human monitor and QMP.
Gleb Natapov - Nov. 6, 2010, 11:37 a.m.
On Sat, Nov 06, 2010 at 10:25:31AM +0100, Markus Armbruster wrote:
> Gleb Natapov <gleb@redhat.com> writes:
> 
> > On Fri, Nov 05, 2010 at 05:31:38PM +0100, Markus Armbruster wrote:
> >> Gleb Natapov <gleb@redhat.com> writes:
> >> 
> >> > On Fri, Nov 05, 2010 at 03:04:05PM +0100, Markus Armbruster wrote:
> >> [...]
> >> >> >> >> There has been quite some discussion on "canonical path" on the list,
> >> >> >> >> but no consensus.  Ironically, one of the places where we got stuck was
> >> >> >> >> ISA.  You cut right through that, so that's progress.  Maybe people
> >> >> >> >> aren't looking ;)
> >> >> >> > That is funny since the problem was already solved looong time ago. Just
> >> >> >> > look at Open Firmware device path. They are capable of addressing all
> >> >> >> > devices just fine, ISA devices included. What specific problem you had
> >> >> >> > with ISA bus? 
> >> >> >> 
> >> >> >> Lack of consensus.  I was in favour of using I/O base, just like you do.
> >> >> >> There were worries about ISA devices not using any I/O ports.
> >> >> > There is a solution for that problem for almost 15 years and we are
> >> >> > still looking for consensus on qemu list?! Here is ISA device binding
> >> >> > spec for Open Firmware: http://playground.sun.com/1275/bindings/isa/isa0_4d.ps 
> >> >> > If ISA device have no IO ports MMIO is used.
> >> >> 
> >> >> Precedence should promote consensus, but it can't replace it.  If you
> >> >> can push the list to consensus, more power to you.
> >> > I do not see disagreement right now :) You are saying you agree. Blue
> >> > Swirl asked me to use Open Firmware so I assume he agrees to. So who is
> >> > against and what are his arguments?
> >> 
> >> Start here:
> >> 
> >> http://lists.nongnu.org/archive/html/qemu-devel/2010-06/msg01618.html
> >
> > I saw this in fact. The wouldn't agree with this device path proposal
> > too. It mixes qemu internal names (which is a big no-no for my purpose)
> > and bus addresses. Paul made sensible points there and if you look
> > closely what he proposes is what I implemented here. Regarding ISA
> > ("busses that don't have a consistent addressing scheme" he called it)
> > he himself proposed to use address of the first IO port/memory region
> > as an ID. This is what is already implemented by my patch.
> 
> You don't have to convince me; I was with Paul in that thread.
> 
So who should I convince :)? Alex? He is CCed. Jan? I do not see him
complaining here.

> Regarding DeviceInfo member name values being QEMU internal: hardly.
> They're ABI.  They're what we use to identify device types on external
> interfaces including command line, human monitor and QMP.
Correct. They are ABI since they are user visible. But they describe
device type where I prefer to have a field that specifies device
functionality for cases when one driver handles multiple devices in a
guest. So to clarify: "name" will work (that is why I use it when
driver_name is not set), but I want something more optimal.

--
			Gleb.
Markus Armbruster - Nov. 6, 2010, 12:46 p.m.
Gleb Natapov <gleb@redhat.com> writes:

> On Sat, Nov 06, 2010 at 10:25:31AM +0100, Markus Armbruster wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>> 
>> > On Fri, Nov 05, 2010 at 05:31:38PM +0100, Markus Armbruster wrote:
>> >> Gleb Natapov <gleb@redhat.com> writes:
>> >> 
>> >> > On Fri, Nov 05, 2010 at 03:04:05PM +0100, Markus Armbruster wrote:
>> >> [...]
>> >> >> >> >> There has been quite some discussion on "canonical path" on the list,
>> >> >> >> >> but no consensus.  Ironically, one of the places where we got stuck was
>> >> >> >> >> ISA.  You cut right through that, so that's progress.  Maybe people
>> >> >> >> >> aren't looking ;)
>> >> >> >> > That is funny since the problem was already solved looong time ago. Just
>> >> >> >> > look at Open Firmware device path. They are capable of addressing all
>> >> >> >> > devices just fine, ISA devices included. What specific problem you had
>> >> >> >> > with ISA bus? 
>> >> >> >> 
>> >> >> >> Lack of consensus.  I was in favour of using I/O base, just like you do.
>> >> >> >> There were worries about ISA devices not using any I/O ports.
>> >> >> > There is a solution for that problem for almost 15 years and we are
>> >> >> > still looking for consensus on qemu list?! Here is ISA device binding
>> >> >> > spec for Open Firmware: http://playground.sun.com/1275/bindings/isa/isa0_4d.ps 
>> >> >> > If ISA device have no IO ports MMIO is used.
>> >> >> 
>> >> >> Precedence should promote consensus, but it can't replace it.  If you
>> >> >> can push the list to consensus, more power to you.
>> >> > I do not see disagreement right now :) You are saying you agree. Blue
>> >> > Swirl asked me to use Open Firmware so I assume he agrees to. So who is
>> >> > against and what are his arguments?
>> >> 
>> >> Start here:
>> >> 
>> >> http://lists.nongnu.org/archive/html/qemu-devel/2010-06/msg01618.html
>> >
>> > I saw this in fact. The wouldn't agree with this device path proposal
>> > too. It mixes qemu internal names (which is a big no-no for my purpose)
>> > and bus addresses. Paul made sensible points there and if you look
>> > closely what he proposes is what I implemented here. Regarding ISA
>> > ("busses that don't have a consistent addressing scheme" he called it)
>> > he himself proposed to use address of the first IO port/memory region
>> > as an ID. This is what is already implemented by my patch.
>> 
>> You don't have to convince me; I was with Paul in that thread.
>> 
> So who should I convince :)? Alex? He is CCed. Jan? I do not see him
> complaining here.

No more complaints, no more convincing.

[...]

Patch

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index ff80dd5..b2cbdbc 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -257,8 +257,8 @@  static int pci_cmd646_ide_initfn(PCIDevice *dev)
     pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
 
     irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
-    ide_bus_new(&d->bus[0], &d->dev.qdev);
-    ide_bus_new(&d->bus[1], &d->dev.qdev);
+    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
+    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
     ide_init2(&d->bus[0], irq[0]);
     ide_init2(&d->bus[1], irq[1]);
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 4165543..bde2664 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -448,6 +448,7 @@  struct IDEBus {
     IDEDevice *slave;
     BMDMAState *bmdma;
     IDEState ifs[2];
+    uint8_t bus_id;
     uint8_t unit;
     uint8_t cmd;
     qemu_irq irq;
@@ -564,7 +565,7 @@  void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
 
 /* hw/ide/qdev.c */
-void ide_bus_new(IDEBus *idebus, DeviceState *dev);
+void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id);
 IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
 
 #endif /* HW_IDE_INTERNAL_H */
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 9b94495..b000ab8 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -66,8 +66,9 @@  static const VMStateDescription vmstate_ide_isa = {
 static int isa_ide_initfn(ISADevice *dev)
 {
     ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
+    static uint8_t bus_id = 0;
 
-    ide_bus_new(&s->bus, &s->dev.qdev);
+    ide_bus_new(&s->bus, &s->dev.qdev, bus_id++);
     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);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 6206201..e56777f 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -129,8 +129,8 @@  static int pci_piix_ide_initfn(PCIIDEState *d)
 
     vmstate_register(&d->dev.qdev, 0, &vmstate_ide_pci, d);
 
-    ide_bus_new(&d->bus[0], &d->dev.qdev);
-    ide_bus_new(&d->bus[1], &d->dev.qdev);
+    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
+    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
     ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
     ide_init_ioport(&d->bus[1], 0x170, 0x376);
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 336ffe1..220729e 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -29,9 +29,13 @@  static struct BusInfo ide_bus_info = {
     .size  = sizeof(IDEBus),
 };
 
-void ide_bus_new(IDEBus *idebus, DeviceState *dev)
+void ide_bus_new(IDEBus *idebus, DeviceState *dev, uint8_t bus_id)
 {
-    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
+    char name[10];
+
+    snprintf(name, sizeof(name), "ide.%d", bus_id); 
+    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, name);
+    idebus->bus_id = bus_id;
 }
 
 static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index b2c7cad..cc48b2b 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -158,8 +158,8 @@  static int vt82c686b_ide_initfn(PCIDevice *dev)
 
     vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
 
-    ide_bus_new(&d->bus[0], &d->dev.qdev);
-    ide_bus_new(&d->bus[1], &d->dev.qdev);
+    ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
+    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
     ide_init2(&d->bus[0], isa_reserve_irq(14));
     ide_init2(&d->bus[1], isa_reserve_irq(15));
     ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 3d07ce5..cec6c9b 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -163,7 +163,7 @@  static void pc_init1(ram_addr_t ram_size,
             ISADevice *dev;
             dev = isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i],
                                hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
-            idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0");
+            idebus[i] = qdev_get_child_bus(&dev->qdev, i ? "ide.1" : "ide.0");
         }
     }