| Message ID | 20101024162315.GG2343@redhat.com |
|---|---|
| State | New |
| Headers | show |
Gleb Natapov <gleb@redhat.com> writes: > Without this patch both buses on PIIX3_IDE device have the same unit id. Are you sure that's wrong? As far as I can see, IDEBus member unit is the currently active unit, set by write to port 6 (in ide_ioport_write()) and cleared on reset (in ide_bus_reset()). > Signed-off-by: Gleb Natapov <gleb@redhat.com> > 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..3da93df 100644 > --- a/hw/ide/internal.h > +++ b/hw/ide/internal.h > @@ -564,7 +564,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 unit); > 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 163ffba..907d862 100644 > --- a/hw/ide/isa.c > +++ b/hw/ide/isa.c > @@ -67,7 +67,7 @@ static int isa_ide_initfn(ISADevice *dev) > { > ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev); > > - ide_bus_new(&s->bus, &s->dev.qdev); > + ide_bus_new(&s->bus, &s->dev.qdev, 0); > 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 07483e8..d0b04a3 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 0808760..1705a12 100644 > --- a/hw/ide/qdev.c > +++ b/hw/ide/qdev.c > @@ -29,9 +29,10 @@ 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 unit) > { > qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL); > + idebus->unit = unit; > } > > 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); > -- > Gleb.
On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > Without this patch both buses on PIIX3_IDE device have the same unit id. > > Are you sure that's wrong? > So how do I know which bus is it on PIIX3_IDE? > As far as I can see, IDEBus member unit is the currently active unit, > set by write to port 6 (in ide_ioport_write()) and cleared on reset (in > ide_bus_reset()). > > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > > 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..3da93df 100644 > > --- a/hw/ide/internal.h > > +++ b/hw/ide/internal.h > > @@ -564,7 +564,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 unit); > > 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 163ffba..907d862 100644 > > --- a/hw/ide/isa.c > > +++ b/hw/ide/isa.c > > @@ -67,7 +67,7 @@ static int isa_ide_initfn(ISADevice *dev) > > { > > ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev); > > > > - ide_bus_new(&s->bus, &s->dev.qdev); > > + ide_bus_new(&s->bus, &s->dev.qdev, 0); > > 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 07483e8..d0b04a3 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 0808760..1705a12 100644 > > --- a/hw/ide/qdev.c > > +++ b/hw/ide/qdev.c > > @@ -29,9 +29,10 @@ 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 unit) > > { > > qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL); > > + idebus->unit = unit; > > } > > > > 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); > > -- > > Gleb. -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. >> >> Are you sure that's wrong? >> > So how do I know which bus is it on PIIX3_IDE? [...] Let me try to explain the IDE pointer thicket. piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev stores child buses in dev.qdev.child_bus. IDEBus points back: qbus.parent. Up to two IDE devices can sit on each IDE bus. The first one uses IDEBus members master and ifs[0], the second one uses slave and ifs[1]. ifs[i].bus points back to the IDE bus. {master,slave}.qdev.parent_bus point back to the IDE bus. Say you got an IDEDevice and want to know which to which of the two buses it's connected. Let's call it d. d->qdev.parent_bus is the BusState. Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). b->qbus.parent is the IDE controller. Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); If c->bus[0] == b, it's on the first bus. Else it must be on the second bus, i.e. c->bus[1] == b. Hope I didn't screw this up too badly.
On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > Without this patch both buses on PIIX3_IDE device have the same unit id. > >> > >> Are you sure that's wrong? > >> > > So how do I know which bus is it on PIIX3_IDE? > [...] > > Let me try to explain the IDE pointer thicket. > > piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in > PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev > stores child buses in dev.qdev.child_bus. > > IDEBus points back: qbus.parent. > > Up to two IDE devices can sit on each IDE bus. The first one uses > IDEBus members master and ifs[0], the second one uses slave and ifs[1]. > > ifs[i].bus points back to the IDE bus. > > {master,slave}.qdev.parent_bus point back to the IDE bus. > > Say you got an IDEDevice and want to know which to which of the two > buses it's connected. Let's call it d. > > d->qdev.parent_bus is the BusState. > > Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). > > b->qbus.parent is the IDE controller. > > Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); > > If c->bus[0] == b, it's on the first bus. > > Else it must be on the second bus, i.e. c->bus[1] == b. > > Hope I didn't screw this up too badly. Will check tomorrow if this works, but why not have simple property on IDEBus that says which one is it? BTW what -device magic should I use to create secondary disk on second IDE bus? -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. >> >> >> >> Are you sure that's wrong? >> >> >> > So how do I know which bus is it on PIIX3_IDE? >> [...] >> >> Let me try to explain the IDE pointer thicket. >> >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev >> stores child buses in dev.qdev.child_bus. >> >> IDEBus points back: qbus.parent. >> >> Up to two IDE devices can sit on each IDE bus. The first one uses >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. >> >> ifs[i].bus points back to the IDE bus. >> >> {master,slave}.qdev.parent_bus point back to the IDE bus. >> >> Say you got an IDEDevice and want to know which to which of the two >> buses it's connected. Let's call it d. >> >> d->qdev.parent_bus is the BusState. >> >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). >> >> b->qbus.parent is the IDE controller. >> >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); >> >> If c->bus[0] == b, it's on the first bus. >> >> Else it must be on the second bus, i.e. c->bus[1] == b. >> >> Hope I didn't screw this up too badly. > Will check tomorrow if this works, but why not have simple property on > IDEBus that says which one is it? Because nobody has needed it so far? Does the bus care whether it's first or second? > BTW what -device magic should I use to > create secondary disk on second IDE bus? Try -device ide-drive,bus=ide.1,unit=1,drive=...
On Mon, Oct 25, 2010 at 08:06:13PM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: > >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> > >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. > >> >> > >> >> Are you sure that's wrong? > >> >> > >> > So how do I know which bus is it on PIIX3_IDE? > >> [...] > >> > >> Let me try to explain the IDE pointer thicket. > >> > >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in > >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev > >> stores child buses in dev.qdev.child_bus. > >> > >> IDEBus points back: qbus.parent. > >> > >> Up to two IDE devices can sit on each IDE bus. The first one uses > >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. > >> > >> ifs[i].bus points back to the IDE bus. > >> > >> {master,slave}.qdev.parent_bus point back to the IDE bus. > >> > >> Say you got an IDEDevice and want to know which to which of the two > >> buses it's connected. Let's call it d. > >> > >> d->qdev.parent_bus is the BusState. > >> > >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). > >> > >> b->qbus.parent is the IDE controller. > >> > >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); > >> > >> If c->bus[0] == b, it's on the first bus. > >> > >> Else it must be on the second bus, i.e. c->bus[1] == b. > >> > >> Hope I didn't screw this up too badly. > > Will check tomorrow if this works, but why not have simple property on > > IDEBus that says which one is it? > > Because nobody has needed it so far? > > Does the bus care whether it's first or second? > User that wants to address particular disk cares. > > BTW what -device magic should I use to > > create secondary disk on second IDE bus? > > Try -device ide-drive,bus=ide.1,unit=1,drive=... Where ide.1 comes from? Why do I need to write ide.1 not just 1. Can I instantiate ide-device on USB bus by doing -device ide-drive,bus=usb? -- Gleb.
On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > Without this patch both buses on PIIX3_IDE device have the same unit id. > >> > >> Are you sure that's wrong? > >> > > So how do I know which bus is it on PIIX3_IDE? > [...] > > Let me try to explain the IDE pointer thicket. > > piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in > PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev > stores child buses in dev.qdev.child_bus. > > IDEBus points back: qbus.parent. > > Up to two IDE devices can sit on each IDE bus. The first one uses > IDEBus members master and ifs[0], the second one uses slave and ifs[1]. > > ifs[i].bus points back to the IDE bus. > > {master,slave}.qdev.parent_bus point back to the IDE bus. > > Say you got an IDEDevice and want to know which to which of the two > buses it's connected. Let's call it d. > > d->qdev.parent_bus is the BusState. > > Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). > > b->qbus.parent is the IDE controller. > > Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); > This will not work if IDEBus sits on ISA bus. Any other ideas? > If c->bus[0] == b, it's on the first bus. > > Else it must be on the second bus, i.e. c->bus[1] == b. > > Hope I didn't screw this up too badly. -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. >> >> >> >> Are you sure that's wrong? >> >> >> > So how do I know which bus is it on PIIX3_IDE? >> [...] >> >> Let me try to explain the IDE pointer thicket. >> >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev >> stores child buses in dev.qdev.child_bus. >> >> IDEBus points back: qbus.parent. >> >> Up to two IDE devices can sit on each IDE bus. The first one uses >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. >> >> ifs[i].bus points back to the IDE bus. >> >> {master,slave}.qdev.parent_bus point back to the IDE bus. >> >> Say you got an IDEDevice and want to know which to which of the two >> buses it's connected. Let's call it d. >> >> d->qdev.parent_bus is the BusState. >> >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). >> >> b->qbus.parent is the IDE controller. >> >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); >> > This will not work if IDEBus sits on ISA bus. Any other ideas? > >> If c->bus[0] == b, it's on the first bus. >> >> Else it must be on the second bus, i.e. c->bus[1] == b. Well, you asked for piix3-ide specifically :) isa-ide provides just one IDE bus. Thus we have two isa-ide devices. To find them, you need to walk the ISA devices. Our PCish machines have just one ISA bus, and it's called "isa.0". You could try bus = qbus_find("isa.0"); QLIST_FOREACH(dev, &bus->children, sibling) { // examine dev // it's safe to upcast dev to ISADevice } Perhaps best to have some means in qdev.h to iterate over a bus like that. If dev->info->name is "isa-ide", you found one of the controllers. How to tell whether it's primary or secondary? Primary uses I/O ports 0x1f0..0x1ff,0x3f0..0x3ff and IRQ 14. IRQ could be easier to check, because it should be right in ISADevice member isairq[0]. Alternatively, upcast to ISAIDEState and check members iobase or isairq. Hmm, there's a way that doesn't require special-casing the IDE controller devices: find the IDEBus as above. Then check the IRQ# b->irq->n: 14 is primary, 15 is secondary.
Gleb Natapov <gleb@redhat.com> writes: > On Mon, Oct 25, 2010 at 08:06:13PM +0200, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: >> >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. >> >> >> >> >> >> Are you sure that's wrong? >> >> >> >> >> > So how do I know which bus is it on PIIX3_IDE? >> >> [...] >> >> >> >> Let me try to explain the IDE pointer thicket. >> >> >> >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in >> >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev >> >> stores child buses in dev.qdev.child_bus. >> >> >> >> IDEBus points back: qbus.parent. >> >> >> >> Up to two IDE devices can sit on each IDE bus. The first one uses >> >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. >> >> >> >> ifs[i].bus points back to the IDE bus. >> >> >> >> {master,slave}.qdev.parent_bus point back to the IDE bus. >> >> >> >> Say you got an IDEDevice and want to know which to which of the two >> >> buses it's connected. Let's call it d. >> >> >> >> d->qdev.parent_bus is the BusState. >> >> >> >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). >> >> >> >> b->qbus.parent is the IDE controller. >> >> >> >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); >> >> >> >> If c->bus[0] == b, it's on the first bus. >> >> >> >> Else it must be on the second bus, i.e. c->bus[1] == b. >> >> >> >> Hope I didn't screw this up too badly. >> > Will check tomorrow if this works, but why not have simple property on >> > IDEBus that says which one is it? >> >> Because nobody has needed it so far? >> >> Does the bus care whether it's first or second? >> > User that wants to address particular disk cares. Define "user". For users of qemu, the way to address a particular disk (or any qdev device) is by user-specified ID. For users within qemu, there are other ways. I'm asking whether the bus cares, because IDEBus holds the state of the bus. If the bus itself doesn't care whether it's primary or secondary, then this state should not carry that information. >> > BTW what -device magic should I use to >> > create secondary disk on second IDE bus? >> >> Try -device ide-drive,bus=ide.1,unit=1,drive=... > Can I In qdev, we address buses by ID, just like devices. Unlike device IDs, which are specified by the user, the bus IDs are auto-generated. The secondary bus of piix3-ide gets the bus ID "ide.1". We do have usability problems there. There is no way for the user to specify an ID for a default device, as far as I know. The bus IDs are defined by the device providing the bus. If it provides none, qdev core makes one up. This breaks down in corner cases. For instance, with -M isapc, we get two isa-ide. Each provides an IDE bus without specifying its ID. qdev core assigns the same name "ide.0" to both. bus=ide.0 picks the first one (in not-really-specified qtree traversal order). As far as I can tell, there's no way to address the second one. > Can I > instantiate ide-device on USB bus by doing -device ide-drive,bus=usb? -usb -drive if=none,file=tmp.qcow2,id=dr0 \ -device ide-drive,bus=usb.0,unit=1,drive=dr0 qemu-system-x86_64: -device ide-drive,bus=usb.0,unit=1,drive=dr0: Device 'ide-drive' can't go on a USB bus
On Tue, Oct 26, 2010 at 11:14:20AM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: > >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> > >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. > >> >> > >> >> Are you sure that's wrong? > >> >> > >> > So how do I know which bus is it on PIIX3_IDE? > >> [...] > >> > >> Let me try to explain the IDE pointer thicket. > >> > >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in > >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev > >> stores child buses in dev.qdev.child_bus. > >> > >> IDEBus points back: qbus.parent. > >> > >> Up to two IDE devices can sit on each IDE bus. The first one uses > >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. > >> > >> ifs[i].bus points back to the IDE bus. > >> > >> {master,slave}.qdev.parent_bus point back to the IDE bus. > >> > >> Say you got an IDEDevice and want to know which to which of the two > >> buses it's connected. Let's call it d. > >> > >> d->qdev.parent_bus is the BusState. > >> > >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). > >> > >> b->qbus.parent is the IDE controller. > >> > >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); > >> > > This will not work if IDEBus sits on ISA bus. Any other ideas? > > > >> If c->bus[0] == b, it's on the first bus. > >> > >> Else it must be on the second bus, i.e. c->bus[1] == b. > > Well, you asked for piix3-ide specifically :) > No I didn't. > isa-ide provides just one IDE bus. Thus we have two isa-ide devices. > To find them, you need to walk the ISA devices. Our PCish machines have > just one ISA bus, and it's called "isa.0". You could try > > bus = qbus_find("isa.0"); > QLIST_FOREACH(dev, &bus->children, sibling) { > // examine dev > // it's safe to upcast dev to ISADevice > } > > Perhaps best to have some means in qdev.h to iterate over a bus like > that. > > If dev->info->name is "isa-ide", you found one of the controllers. > When I am on IDEBus level I shouldn't care what bus it resides on to figure out its properties. The things you describe here just show qdev failure to provide reasonable device abstraction for IDEBus. > How to tell whether it's primary or secondary? Primary uses I/O ports > 0x1f0..0x1ff,0x3f0..0x3ff and IRQ 14. IRQ could be easier to check, > because it should be right in ISADevice member isairq[0]. > Alternatively, upcast to ISAIDEState and check members iobase or isairq. Again idebus_dev_path() should not care about what device IDEBus resides on. > > > Hmm, there's a way that doesn't require special-casing the IDE > controller devices: find the IDEBus as above. Then check the IRQ# > b->irq->n: 14 is primary, 15 is secondary. Instead of this horrible hack (which may work, but only for PC), why not add bus_id to IDEBus and be done with it. I already have the patch. -- Gleb.
On Tue, Oct 26, 2010 at 11:29:49AM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Mon, Oct 25, 2010 at 08:06:13PM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: > >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> > >> >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: > >> >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> >> > >> >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. > >> >> >> > >> >> >> Are you sure that's wrong? > >> >> >> > >> >> > So how do I know which bus is it on PIIX3_IDE? > >> >> [...] > >> >> > >> >> Let me try to explain the IDE pointer thicket. > >> >> > >> >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in > >> >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev > >> >> stores child buses in dev.qdev.child_bus. > >> >> > >> >> IDEBus points back: qbus.parent. > >> >> > >> >> Up to two IDE devices can sit on each IDE bus. The first one uses > >> >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. > >> >> > >> >> ifs[i].bus points back to the IDE bus. > >> >> > >> >> {master,slave}.qdev.parent_bus point back to the IDE bus. > >> >> > >> >> Say you got an IDEDevice and want to know which to which of the two > >> >> buses it's connected. Let's call it d. > >> >> > >> >> d->qdev.parent_bus is the BusState. > >> >> > >> >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). > >> >> > >> >> b->qbus.parent is the IDE controller. > >> >> > >> >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); > >> >> > >> >> If c->bus[0] == b, it's on the first bus. > >> >> > >> >> Else it must be on the second bus, i.e. c->bus[1] == b. > >> >> > >> >> Hope I didn't screw this up too badly. > >> > Will check tomorrow if this works, but why not have simple property on > >> > IDEBus that says which one is it? > >> > >> Because nobody has needed it so far? > >> > >> Does the bus care whether it's first or second? > >> > > User that wants to address particular disk cares. > > Define "user". > User is a guest OS. > For users of qemu, the way to address a particular disk (or any qdev > device) is by user-specified ID. > > For users within qemu, there are other ways. > Which ways? > I'm asking whether the bus cares, because IDEBus holds the state of the > bus. If the bus itself doesn't care whether it's primary or secondary, > then this state should not carry that information. > Bus cares because devices on the bus are addressed differently depending on which bus it resides on. > >> > BTW what -device magic should I use to > >> > create secondary disk on second IDE bus? > >> > >> Try -device ide-drive,bus=ide.1,unit=1,drive=... > > Can I > > In qdev, we address buses by ID, just like devices. Unlike device IDs, > which are specified by the user, the bus IDs are auto-generated. The > secondary bus of piix3-ide gets the bus ID "ide.1". Unfortunately those ids are meaningless from outside of qemu. > > We do have usability problems there. There is no way for the user to > specify an ID for a default device, as far as I know. The bus IDs are > defined by the device providing the bus. If it provides none, qdev core > makes one up. This breaks down in corner cases. For instance, with -M > isapc, we get two isa-ide. Each provides an IDE bus without specifying > its ID. qdev core assigns the same name "ide.0" to both. bus=ide.0 > picks the first one (in not-really-specified qtree traversal order). > As far as I can tell, there's no way to address the second one. > This looks like fundamental problem in qdev design. In my case bus names assigned by qdev are useless even when done right though :( > > Can I > > instantiate ide-device on USB bus by doing -device ide-drive,bus=usb? > > -usb -drive if=none,file=tmp.qcow2,id=dr0 \ > -device ide-drive,bus=usb.0,unit=1,drive=dr0 > qemu-system-x86_64: -device ide-drive,bus=usb.0,unit=1,drive=dr0: Device 'ide-drive' can't go on a USB bus -- Gleb.
[Note cc: Alex] Gleb Natapov <gleb@redhat.com> writes: > On Tue, Oct 26, 2010 at 11:14:20AM +0200, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: >> >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. >> >> >> >> >> >> Are you sure that's wrong? >> >> >> >> >> > So how do I know which bus is it on PIIX3_IDE? Here's where you ask about piix3-ide specifically. >> >> [...] >> >> >> >> Let me try to explain the IDE pointer thicket. >> >> >> >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in >> >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev >> >> stores child buses in dev.qdev.child_bus. >> >> >> >> IDEBus points back: qbus.parent. >> >> >> >> Up to two IDE devices can sit on each IDE bus. The first one uses >> >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. >> >> >> >> ifs[i].bus points back to the IDE bus. >> >> >> >> {master,slave}.qdev.parent_bus point back to the IDE bus. >> >> >> >> Say you got an IDEDevice and want to know which to which of the two >> >> buses it's connected. Let's call it d. >> >> >> >> d->qdev.parent_bus is the BusState. >> >> >> >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). >> >> >> >> b->qbus.parent is the IDE controller. >> >> >> >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); >> >> >> > This will not work if IDEBus sits on ISA bus. Any other ideas? >> > >> >> If c->bus[0] == b, it's on the first bus. >> >> >> >> Else it must be on the second bus, i.e. c->bus[1] == b. >> >> Well, you asked for piix3-ide specifically :) >> > No I didn't. See above. >> isa-ide provides just one IDE bus. Thus we have two isa-ide devices. >> To find them, you need to walk the ISA devices. Our PCish machines have >> just one ISA bus, and it's called "isa.0". You could try >> >> bus = qbus_find("isa.0"); >> QLIST_FOREACH(dev, &bus->children, sibling) { >> // examine dev >> // it's safe to upcast dev to ISADevice >> } >> >> Perhaps best to have some means in qdev.h to iterate over a bus like >> that. >> >> If dev->info->name is "isa-ide", you found one of the controllers. >> > When I am on IDEBus level I shouldn't care what bus it resides on to > figure out its properties. The things you describe here just show qdev > failure to provide reasonable device abstraction for IDEBus. > >> How to tell whether it's primary or secondary? Primary uses I/O ports >> 0x1f0..0x1ff,0x3f0..0x3ff and IRQ 14. IRQ could be easier to check, >> because it should be right in ISADevice member isairq[0]. >> Alternatively, upcast to ISAIDEState and check members iobase or isairq. > Again idebus_dev_path() should not care about what device IDEBus resides on. We really need to define what qbus's get_dev_path() callback is supposed to do before we continue. Alex, you created it. What's your take on its semantics? >> Hmm, there's a way that doesn't require special-casing the IDE >> controller devices: find the IDEBus as above. Then check the IRQ# >> b->irq->n: 14 is primary, 15 is secondary. > Instead of this horrible hack (which may work, but only for PC), why not > add bus_id to IDEBus and be done with it. I already have the patch. How's that bus_id defined? A disk on an IDE bus has no idea whether its the first or the 77th IDE bus in the system. It only knows whether it's master or slave on this bus. Likewise, the IDE controller doesn't know about other IDE buses in the system. It only knows that it provides N IDE buses. It doesn't know whether they are #1..#N globally. A particular system architecture may order its IDE buses and thus assign global numbers by convention. For instance, the PC architecture has a primary and a secondary IDE controller, using well-known ISA resources.
Gleb Natapov <gleb@redhat.com> writes: > On Tue, Oct 26, 2010 at 11:29:49AM +0200, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Mon, Oct 25, 2010 at 08:06:13PM +0200, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: >> >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> >> >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: >> >> >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> >> >> >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. >> >> >> >> >> >> >> >> Are you sure that's wrong? >> >> >> >> >> >> >> > So how do I know which bus is it on PIIX3_IDE? >> >> >> [...] >> >> >> >> >> >> Let me try to explain the IDE pointer thicket. >> >> >> >> >> >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in >> >> >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev >> >> >> stores child buses in dev.qdev.child_bus. >> >> >> >> >> >> IDEBus points back: qbus.parent. >> >> >> >> >> >> Up to two IDE devices can sit on each IDE bus. The first one uses >> >> >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. >> >> >> >> >> >> ifs[i].bus points back to the IDE bus. >> >> >> >> >> >> {master,slave}.qdev.parent_bus point back to the IDE bus. >> >> >> >> >> >> Say you got an IDEDevice and want to know which to which of the two >> >> >> buses it's connected. Let's call it d. >> >> >> >> >> >> d->qdev.parent_bus is the BusState. >> >> >> >> >> >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). >> >> >> >> >> >> b->qbus.parent is the IDE controller. >> >> >> >> >> >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); >> >> >> >> >> >> If c->bus[0] == b, it's on the first bus. >> >> >> >> >> >> Else it must be on the second bus, i.e. c->bus[1] == b. >> >> >> >> >> >> Hope I didn't screw this up too badly. >> >> > Will check tomorrow if this works, but why not have simple property on >> >> > IDEBus that says which one is it? >> >> >> >> Because nobody has needed it so far? >> >> >> >> Does the bus care whether it's first or second? >> >> >> > User that wants to address particular disk cares. >> >> Define "user". >> > User is a guest OS. How do you know how the guest OS numbers its IDE buses? More below. >> For users of qemu, the way to address a particular disk (or any qdev >> device) is by user-specified ID. >> >> For users within qemu, there are other ways. >> > Which ways? Walk the device tree. >> I'm asking whether the bus cares, because IDEBus holds the state of the >> bus. If the bus itself doesn't care whether it's primary or secondary, >> then this state should not carry that information. >> > Bus cares because devices on the bus are addressed differently depending > on which bus it resides on. I'm afraid we're talking past each other. The bus is a part of the controller device. The IDE controller device has no idea which other IDE controller devices are in the system. It can't care for a global bus number, because it doesn't know it. The guest OS enumerates the IDE controller devices, and names or numbers them and their IDE buses how it sees fit. It's well advised to follow the system architecture's conventions, where such conventions exist. They need not exist. Consider some non-PC architecture that knows nothing about IDE, but still has a PCI bus. A user plugs in two PCI cards, each one has two IDE buses. How do you number these four buses globally? The sane way to address such a bus is by (device-address-of-controller, bus#-on-this-controller). And the general way to do a device address is (bus, address-on-bus). Recurses up the device tree. > >> > BTW what -device magic should I use to >> >> > create secondary disk on second IDE bus? >> >> >> >> Try -device ide-drive,bus=ide.1,unit=1,drive=... >> > Can I >> >> In qdev, we address buses by ID, just like devices. Unlike device IDs, >> which are specified by the user, the bus IDs are auto-generated. The >> secondary bus of piix3-ide gets the bus ID "ide.1". > Unfortunately those ids are meaningless from outside of qemu. > >> >> We do have usability problems there. There is no way for the user to >> specify an ID for a default device, as far as I know. The bus IDs are >> defined by the device providing the bus. If it provides none, qdev core >> makes one up. This breaks down in corner cases. For instance, with -M >> isapc, we get two isa-ide. Each provides an IDE bus without specifying >> its ID. qdev core assigns the same name "ide.0" to both. bus=ide.0 >> picks the first one (in not-really-specified qtree traversal order). >> As far as I can tell, there's no way to address the second one. >> > This looks like fundamental problem in qdev design. It's not fundamental, it's just screwed up. > In my case bus names > assigned by qdev are useless even when done right though :( IDs are a qdev artifact, yes. [...]
On Tue, Oct 26, 2010 at 01:20:47PM +0200, Markus Armbruster wrote: > [Note cc: Alex] > > Gleb Natapov <gleb@redhat.com> writes: > > > On Tue, Oct 26, 2010 at 11:14:20AM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: > >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> > >> >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: > >> >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> >> > >> >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. > >> >> >> > >> >> >> Are you sure that's wrong? > >> >> >> > >> >> > So how do I know which bus is it on PIIX3_IDE? > > Here's where you ask about piix3-ide specifically. > Because with ISA there is only one, so default value of zero is good enough. > >> >> [...] > >> >> > >> >> Let me try to explain the IDE pointer thicket. > >> >> > >> >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in > >> >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev > >> >> stores child buses in dev.qdev.child_bus. > >> >> > >> >> IDEBus points back: qbus.parent. > >> >> > >> >> Up to two IDE devices can sit on each IDE bus. The first one uses > >> >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. > >> >> > >> >> ifs[i].bus points back to the IDE bus. > >> >> > >> >> {master,slave}.qdev.parent_bus point back to the IDE bus. > >> >> > >> >> Say you got an IDEDevice and want to know which to which of the two > >> >> buses it's connected. Let's call it d. > >> >> > >> >> d->qdev.parent_bus is the BusState. > >> >> > >> >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). > >> >> > >> >> b->qbus.parent is the IDE controller. > >> >> > >> >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); > >> >> > >> > This will not work if IDEBus sits on ISA bus. Any other ideas? > >> > > >> >> If c->bus[0] == b, it's on the first bus. > >> >> > >> >> Else it must be on the second bus, i.e. c->bus[1] == b. > >> > >> Well, you asked for piix3-ide specifically :) > >> > > No I didn't. > > See above. > You take things out of context in individual mails and in a context of patch series. Are you doing it for the sake of arguing? > >> isa-ide provides just one IDE bus. Thus we have two isa-ide devices. > >> To find them, you need to walk the ISA devices. Our PCish machines have > >> just one ISA bus, and it's called "isa.0". You could try > >> > >> bus = qbus_find("isa.0"); > >> QLIST_FOREACH(dev, &bus->children, sibling) { > >> // examine dev > >> // it's safe to upcast dev to ISADevice > >> } > >> > >> Perhaps best to have some means in qdev.h to iterate over a bus like > >> that. > >> > >> If dev->info->name is "isa-ide", you found one of the controllers. > >> > > When I am on IDEBus level I shouldn't care what bus it resides on to > > figure out its properties. The things you describe here just show qdev > > failure to provide reasonable device abstraction for IDEBus. > > > >> How to tell whether it's primary or secondary? Primary uses I/O ports > >> 0x1f0..0x1ff,0x3f0..0x3ff and IRQ 14. IRQ could be easier to check, > >> because it should be right in ISADevice member isairq[0]. > >> Alternatively, upcast to ISAIDEState and check members iobase or isairq. > > Again idebus_dev_path() should not care about what device IDEBus resides on. > > We really need to define what qbus's get_dev_path() callback is supposed > to do before we continue. Alex, you created it. What's your take on > its semantics? It exist only for PCI device and there it gives device address on a PCI bus in a way that can be mapped back to individual device give only a string returned by the callback. Combined with the callback name I would guess that this is its semantics. > > >> Hmm, there's a way that doesn't require special-casing the IDE > >> controller devices: find the IDEBus as above. Then check the IRQ# > >> b->irq->n: 14 is primary, 15 is secondary. > > Instead of this horrible hack (which may work, but only for PC), why not > > add bus_id to IDEBus and be done with it. I already have the patch. > > How's that bus_id defined? How is PCI domain number defined? > > A disk on an IDE bus has no idea whether its the first or the 77th IDE > bus in the system. It only knows whether it's master or slave on this > bus. Likewise, the IDE controller doesn't know about other IDE buses in > the system. It only knows that it provides N IDE buses. It doesn't > know whether they are #1..#N globally. That doesn't mean you do not enumerate them. Each one of them use different resources. So they are not the same. If they are not the same you need to distinguish them. On ISA it is done by ISA bus address IDEBus resides on. On PCI they reside on the same PCI address. > > A particular system architecture may order its IDE buses and thus assign > global numbers by convention. For instance, the PC architecture has a > primary and a secondary IDE controller, using well-known ISA resources. And other architecture may have PCI card with to IDE controllers that do not use well-know ISA resources, but use whatever resources guest OS configured via PCI bars, but you still what to distinguish them. SCSI has LUN for instance. -- Gleb.
On Tue, Oct 26, 2010 at 01:59:45PM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Tue, Oct 26, 2010 at 11:29:49AM +0200, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > On Mon, Oct 25, 2010 at 08:06:13PM +0200, Markus Armbruster wrote: > >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> > >> >> > On Mon, Oct 25, 2010 at 06:22:12PM +0200, Markus Armbruster wrote: > >> >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> >> > >> >> >> > On Mon, Oct 25, 2010 at 04:29:35PM +0200, Markus Armbruster wrote: > >> >> >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> >> >> > >> >> >> >> > Without this patch both buses on PIIX3_IDE device have the same unit id. > >> >> >> >> > >> >> >> >> Are you sure that's wrong? > >> >> >> >> > >> >> >> > So how do I know which bus is it on PIIX3_IDE? > >> >> >> [...] > >> >> >> > >> >> >> Let me try to explain the IDE pointer thicket. > >> >> >> > >> >> >> piix3-ide provides two IDE buses. pci_piix_ide_initfn() stores them in > >> >> >> PCIIDEState member IDEBus bus[2]. Technically redundant, because qdev > >> >> >> stores child buses in dev.qdev.child_bus. > >> >> >> > >> >> >> IDEBus points back: qbus.parent. > >> >> >> > >> >> >> Up to two IDE devices can sit on each IDE bus. The first one uses > >> >> >> IDEBus members master and ifs[0], the second one uses slave and ifs[1]. > >> >> >> > >> >> >> ifs[i].bus points back to the IDE bus. > >> >> >> > >> >> >> {master,slave}.qdev.parent_bus point back to the IDE bus. > >> >> >> > >> >> >> Say you got an IDEDevice and want to know which to which of the two > >> >> >> buses it's connected. Let's call it d. > >> >> >> > >> >> >> d->qdev.parent_bus is the BusState. > >> >> >> > >> >> >> Upcast to IDEBus: b = DO_UPCAST(IDEBus, qbus, d->qdev.parent_bus). > >> >> >> > >> >> >> b->qbus.parent is the IDE controller. > >> >> >> > >> >> >> Upcast to PCIIDEState: c = DO_UPCAST(PCIIDEState, dev, n->qbus.parent); > >> >> >> > >> >> >> If c->bus[0] == b, it's on the first bus. > >> >> >> > >> >> >> Else it must be on the second bus, i.e. c->bus[1] == b. > >> >> >> > >> >> >> Hope I didn't screw this up too badly. > >> >> > Will check tomorrow if this works, but why not have simple property on > >> >> > IDEBus that says which one is it? > >> >> > >> >> Because nobody has needed it so far? > >> >> > >> >> Does the bus care whether it's first or second? > >> >> > >> > User that wants to address particular disk cares. > >> > >> Define "user". > >> > > User is a guest OS. > > How do you know how the guest OS numbers its IDE buses? More below. > It enumerates all IDE controllers using PCI bus enumeration. And each controller has two IDE buses 0 and 1. 0 uses bar 0,1 1 uses bar 2,3, no need to enumerate any further. > >> For users of qemu, the way to address a particular disk (or any qdev > >> device) is by user-specified ID. > >> > >> For users within qemu, there are other ways. > >> > > Which ways? > > Walk the device tree. What device tree? And how qemu tells to a guest what device to search for? > > >> I'm asking whether the bus cares, because IDEBus holds the state of the > >> bus. If the bus itself doesn't care whether it's primary or secondary, > >> then this state should not carry that information. > >> > > Bus cares because devices on the bus are addressed differently depending > > on which bus it resides on. > > I'm afraid we're talking past each other. > My impression too. > The bus is a part of the controller device. The IDE controller device > has no idea which other IDE controller devices are in the system. It > can't care for a global bus number, because it doesn't know it. But qdev creases it anyway "ide.1" "ide.0" (It only does it in a guest unusable way). And it has to do it since you want to give user ability to attach disk image to specific device, so user have to be able to specify exact device path you claim does not exit internally in qemu! > > The guest OS enumerates the IDE controller devices, and names or numbers > them and their IDE buses how it sees fit. It's well advised to follow > the system architecture's conventions, where such conventions exist. > They need not exist. Now we have a situation where qemu needs to communicate whereabouts of some device it created to a guest OS. How do you propose to do that? > > Consider some non-PC architecture that knows nothing about IDE, but > still has a PCI bus. A user plugs in two PCI cards, each one has two > IDE buses. How do you number these four buses globally? > You don't. You number only IDE buses under one PCI device, so you'll have paths like: PCI@0000:00:05.0/IDE@1:0 PCI@0000:00:06.0/IDE@1:0 And they point to two different disks in a way perfectly understandable by any guest. > The sane way to address such a bus is by (device-address-of-controller, > bus#-on-this-controller). And the general way to do a device address is > (bus, address-on-bus). Recurses up the device tree. That is what I did above. The question is "what is bus# of IDE device on IDE bus"? In my case it is 0/1, > > > >> > BTW what -device magic should I use to > >> >> > create secondary disk on second IDE bus? > >> >> > >> >> Try -device ide-drive,bus=ide.1,unit=1,drive=... > >> > Can I > >> > >> In qdev, we address buses by ID, just like devices. Unlike device IDs, > >> which are specified by the user, the bus IDs are auto-generated. The > >> secondary bus of piix3-ide gets the bus ID "ide.1". > > Unfortunately those ids are meaningless from outside of qemu. > > > >> > >> We do have usability problems there. There is no way for the user to > >> specify an ID for a default device, as far as I know. The bus IDs are > >> defined by the device providing the bus. If it provides none, qdev core > >> makes one up. This breaks down in corner cases. For instance, with -M > >> isapc, we get two isa-ide. Each provides an IDE bus without specifying > >> its ID. qdev core assigns the same name "ide.0" to both. bus=ide.0 > >> picks the first one (in not-really-specified qtree traversal order). > >> As far as I can tell, there's no way to address the second one. > >> > > This looks like fundamental problem in qdev design. > > It's not fundamental, it's just screwed up. > > > In my case bus names > > assigned by qdev are useless even when done right though :( > > IDs are a qdev artifact, yes. > > [...] -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: [...] > You take things out of context in individual mails and in a context of > patch series. Are you doing it for the sake of arguing? I'm trying to help. If you don't need or want my help, I'll find something more productive to do. I'm prescribing myself a little cool-off time before I come back to this thread. See you later :) [...]
On Tue, Oct 26, 2010 at 02:39:28PM +0200, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > [...] > > You take things out of context in individual mails and in a context of > > patch series. Are you doing it for the sake of arguing? > > I'm trying to help. If you don't need or want my help, I'll find > something more productive to do. > I need constrictive help. But I do not like when words are taken out of context and instead of productive discussion argument is developed around them. > I'm prescribing myself a little cool-off time before I come back to this > thread. See you later :) Feel good. -- Gleb.
On Tue, 2010-10-26 at 13:20 +0200, Markus Armbruster wrote: > >> How to tell whether it's primary or secondary? Primary uses I/O ports > >> 0x1f0..0x1ff,0x3f0..0x3ff and IRQ 14. IRQ could be easier to check, > >> because it should be right in ISADevice member isairq[0]. > >> Alternatively, upcast to ISAIDEState and check members iobase or isairq. > > Again idebus_dev_path() should not care about what device IDEBus resides on. > > We really need to define what qbus's get_dev_path() callback is supposed > to do before we continue. Alex, you created it. What's your take on > its semantics? Perhaps going into too much detail, but the idea of get_dev_path() is that it's a recursive call up the device try, where each level adds a string to come up with a unique description. We currently only have this of PCI because it's device path is obvious, while others are not, particularly ISA. To start, I'd think IDE could easily implement a get_dev_path(), where the IDE qbus calls up to it's parent bus, then appends IDE channel and slot. So for a PCI IDE controller, we get something like PCI:0000:00:03.0/piix3-IDE0.master (change this to match qdev info strings, I'm just making it up here). An ISA IDE controller is harder since ISA doesn't have a bus enumeration mechanism. IIRC, there we some suggestions to use ioports and/or irqs to define a path on ISA, but not all ISA qdev devices expose these. So perhaps we'd want something like isa.irq14/piix3-IDE0.master. Doing ISA correctly probably means standardizing some portion of the qdev info for all the ISA devices. Thanks, Alex
On Tue, Oct 26, 2010 at 08:15:52AM -0600, Alex Williamson wrote: > On Tue, 2010-10-26 at 13:20 +0200, Markus Armbruster wrote: > > >> How to tell whether it's primary or secondary? Primary uses I/O ports > > >> 0x1f0..0x1ff,0x3f0..0x3ff and IRQ 14. IRQ could be easier to check, > > >> because it should be right in ISADevice member isairq[0]. > > >> Alternatively, upcast to ISAIDEState and check members iobase or isairq. > > > Again idebus_dev_path() should not care about what device IDEBus resides on. > > > > We really need to define what qbus's get_dev_path() callback is supposed > > to do before we continue. Alex, you created it. What's your take on > > its semantics? > > Perhaps going into too much detail, but the idea of get_dev_path() is > that it's a recursive call up the device try, where each level adds a > string to come up with a unique description. We currently only have > this of PCI because it's device path is obvious, while others are not, > particularly ISA. > > To start, I'd think IDE could easily implement a get_dev_path(), where > the IDE qbus calls up to it's parent bus, then appends IDE channel and > slot. So for a PCI IDE controller, we get something like > PCI:0000:00:03.0/piix3-IDE0.master (change this to match qdev info > strings, I'm just making it up here). > If we will do it like you suggest device path will be meaningless outside of qemu internals. I would like get_dev_path() to build device path that can be used in a guest to locate the device. In your example above piix3-IDE0 does not provide any additional information anyway since PCI:0000:00:03.0 already point at a device that can be easily identified as piix3-ede0 by looking into PCI config space. So for IDE I would like it to be like that: PCI@0000:00:03.0/IDE@0:1 > An ISA IDE controller is harder since ISA doesn't have a bus enumeration > mechanism. IIRC, there we some suggestions to use ioports and/or irqs > to define a path on ISA, but not all ISA qdev devices expose these. So > perhaps we'd want something like isa.irq14/piix3-IDE0.master. Doing ISA > correctly probably means standardizing some portion of the qdev info for > all the ISA devices. Thanks, Have you looked at my isabus_get_dev_path() patch? It keeps track of ioports used by ISA device at ISADevice level. -- Gleb.
On Tue, 2010-10-26 at 16:26 +0200, Gleb Natapov wrote: > On Tue, Oct 26, 2010 at 08:15:52AM -0600, Alex Williamson wrote: > > On Tue, 2010-10-26 at 13:20 +0200, Markus Armbruster wrote: > > > >> How to tell whether it's primary or secondary? Primary uses I/O ports > > > >> 0x1f0..0x1ff,0x3f0..0x3ff and IRQ 14. IRQ could be easier to check, > > > >> because it should be right in ISADevice member isairq[0]. > > > >> Alternatively, upcast to ISAIDEState and check members iobase or isairq. > > > > Again idebus_dev_path() should not care about what device IDEBus resides on. > > > > > > We really need to define what qbus's get_dev_path() callback is supposed > > > to do before we continue. Alex, you created it. What's your take on > > > its semantics? > > > > Perhaps going into too much detail, but the idea of get_dev_path() is > > that it's a recursive call up the device try, where each level adds a > > string to come up with a unique description. We currently only have > > this of PCI because it's device path is obvious, while others are not, > > particularly ISA. > > > > To start, I'd think IDE could easily implement a get_dev_path(), where > > the IDE qbus calls up to it's parent bus, then appends IDE channel and > > slot. So for a PCI IDE controller, we get something like > > PCI:0000:00:03.0/piix3-IDE0.master (change this to match qdev info > > strings, I'm just making it up here). > > > If we will do it like you suggest device path will be meaningless > outside of qemu internals. I would like get_dev_path() to build device > path that can be used in a guest to locate the device. In your example > above piix3-IDE0 does not provide any additional information anyway > since PCI:0000:00:03.0 already point at a device that can be easily > identified as piix3-ede0 by looking into PCI config space. > So for IDE I would like it to be like that: > PCI@0000:00:03.0/IDE@0:1 Yep, I agree, just overly verbose in my example I guess. > > An ISA IDE controller is harder since ISA doesn't have a bus enumeration > > mechanism. IIRC, there we some suggestions to use ioports and/or irqs > > to define a path on ISA, but not all ISA qdev devices expose these. So > > perhaps we'd want something like isa.irq14/piix3-IDE0.master. Doing ISA > > correctly probably means standardizing some portion of the qdev info for > > all the ISA devices. Thanks, > Have you looked at my isabus_get_dev_path() patch? It keeps track of > ioports used by ISA device at ISADevice level. No I hadn't, great! It won't be an easy path to parse, but that's probably more a factor of ISA bus architecture than your decision to use ioports. Thanks, Alex
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..3da93df 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -564,7 +564,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 unit); 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 163ffba..907d862 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -67,7 +67,7 @@ static int isa_ide_initfn(ISADevice *dev) { ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev); - ide_bus_new(&s->bus, &s->dev.qdev); + ide_bus_new(&s->bus, &s->dev.qdev, 0); 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 07483e8..d0b04a3 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 0808760..1705a12 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -29,9 +29,10 @@ 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 unit) { qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL); + idebus->unit = unit; } 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);
Without this patch both buses on PIIX3_IDE device have the same unit id. Signed-off-by: Gleb Natapov <gleb@redhat.com> -- Gleb.