Re: [Qemu-devel] [PATCHv2 4/8] Store IDE bus id in IDEBus structure for easy access.

[Gleb Natapov]

On Wed, Nov 03, 2010 at 06:22:11PM +0100, Markus Armbruster wrote:
> Gleb Natapov <address@hidden> writes:
> 
> > On Wed, Nov 03, 2010 at 04:18:18PM +0100, Markus Armbruster wrote:
> >> Gleb Natapov <address@hidden> 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:
> >
> > /address@hidden/address@hidden,1/address@hidden/address@hidden
> > /address@hidden/address@hidden,1/address@hidden/address@hidden
> > /address@hidden/address@hidden,1/address@hidden/address@hidden
> > /address@hidden/address@hidden,1/address@hidden/address@hidden
> > /address@hidden/address@hidden/address@hidden
> > /address@hidden/address@hidden/address@hidden/address@hidden/address@hidden
> >
> > 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/address@hidden/address@hidden/address@hidden
> > /isa/address@hidden/address@hidden/address@hidden
> > /isa/address@hidden/address@hidden/address@hidden
> > /isa/address@hidden/address@hidden/address@hidden
> >
> > 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 address@hidden
for first isa IDE controller and address@hidden 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.