[Qemu-devel] [PATCH] initialize unit id of IDE bus

[Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

Without this patch both buses on PIIX3_IDE device have the same unit id.

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Markus Armbruster]

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=...

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Markus Armbruster]

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.

[...]

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Markus Armbruster]

[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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Markus Armbruster]

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

[...]

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Alex Williamson]

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

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH] initialize unit id of IDE bus

[Alex Williamson]

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