[Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Knut Omang]

A unique bus name is necessary to be able to refer to each instance
from the command line and monitors.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/pci-bridge/ioh3420.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 7cd87fc..8f6c8b0 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -95,6 +95,9 @@ static int ioh3420_initfn(PCIDevice *d)
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
+    char tmp[100];
+    sprintf(tmp, "pcie_port.%d", s->slot);
+    pci_bridge_map_irq(br, g_strdup(tmp), pci_swizzle_map_irq_fn);
 
     rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
     if (rc < 0) {
@@ -154,6 +157,7 @@ static void ioh3420_exitfn(PCIDevice *d)
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+    g_free((char*)br->bus_name);
 }
 
 PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction,
-- 
1.9.0

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Paolo Bonzini]

Il 20/08/2014 08:53, Knut Omang ha scritto:
> A unique bus name is necessary to be able to refer to each instance
> from the command line and monitors.

Is it needed?  Can't you just add id= to the -device option?

Paolo

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Knut Omang]

On Wed, 2014-08-20 at 10:52 +0200, Paolo Bonzini wrote:
> Il 20/08/2014 08:53, Knut Omang ha scritto:
> > A unique bus name is necessary to be able to refer to each instance
> > from the command line and monitors.
> 
> Is it needed?  Can't you just add id= to the -device option?

Yes, as far as I understand the problem is that the id= would work on
the ioh3420 device itself, while what is needed here is to name the
secondary bus of the ioh3420, which I haven't found a way to name from
the command line.

Maybe an even better solution would be to have default names for
everything, if not specified, from a user friendliness perspective? 

I suppose this is a more general issue of sensible default values
though, but the fact that it is easy to create devices which cannot be
referred has caused me some confusion from time to time.

> Paolo

Thanks,

Knut

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Michael S. Tsirkin]

On Wed, Aug 20, 2014 at 11:30:55AM +0200, Knut Omang wrote:
> On Wed, 2014-08-20 at 10:52 +0200, Paolo Bonzini wrote:
> > Il 20/08/2014 08:53, Knut Omang ha scritto:
> > > A unique bus name is necessary to be able to refer to each instance
> > > from the command line and monitors.
> > 
> > Is it needed?  Can't you just add id= to the -device option?
> 
> Yes, as far as I understand the problem is that the id= would work on
> the ioh3420 device itself, while what is needed here is to name the
> secondary bus of the ioh3420, which I haven't found a way to name from
> the command line.

Did you try using the device name?

For pci bridges, unless you set bus_name, bus name will
match device itself. See this code:

     * If we don't specify the name, the bus will be addressed as
     * <id>.0, where id is the device id.
     * Since PCI Bridge devices have a single bus each, we don't need
     * the index:
     * let users address the bus using the device name.
     */
    if (!br->bus_name && dev->qdev.id && *dev->qdev.id) {
            br->bus_name = dev->qdev.id;
    }



> Maybe an even better solution would be to have default names for
> everything, if not specified, from a user friendliness perspective? 
>
> I suppose this is a more general issue of sensible default values
> though, but the fact that it is easy to create devices which cannot be
> referred has caused me some confusion from time to time.
> 
> > Paolo
> 
> Thanks,
> 
> Knut

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Andreas Färber]

Hi Knut,

Am 20.08.2014 08:53, schrieb Knut Omang:
> 
> A unique bus name is necessary to be able to refer to each instance
> from the command line and monitors.
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  hw/pci-bridge/ioh3420.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index 7cd87fc..8f6c8b0 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -95,6 +95,9 @@ static int ioh3420_initfn(PCIDevice *d)
>      PCIEPort *p = PCIE_PORT(d);
>      PCIESlot *s = PCIE_SLOT(d);
>      int rc;
> +    char tmp[100];
> +    sprintf(tmp, "pcie_port.%d", s->slot);
> +    pci_bridge_map_irq(br, g_strdup(tmp), pci_swizzle_map_irq_fn);

Style issues apart (white line being dropped), this is a rather
convoluted way of providing a bus name to something. ;)
That makes me think something else is going wrong, or APIs may need to
be improved. Could you elaborate for those not intimately familiar with
this bridge what name you're seeing before, given a particular command
line, and explain why IRQ mapping affects the bus name?

Thanks,
Andreas

>  
>      rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
>      if (rc < 0) {
> @@ -154,6 +157,7 @@ static void ioh3420_exitfn(PCIDevice *d)
>      pcie_cap_exit(d);
>      msi_uninit(d);
>      pci_bridge_exitfn(d);
> +    g_free((char*)br->bus_name);
>  }
>  
>  PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction,

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Markus Armbruster]

Knut Omang <knut.omang@oracle.com> writes:

> On Wed, 2014-08-20 at 10:52 +0200, Paolo Bonzini wrote:
>> Il 20/08/2014 08:53, Knut Omang ha scritto:
>> > A unique bus name is necessary to be able to refer to each instance
>> > from the command line and monitors.
>> 
>> Is it needed?  Can't you just add id= to the -device option?
>
> Yes, as far as I understand the problem is that the id= would work on
> the ioh3420 device itself, while what is needed here is to name the
> secondary bus of the ioh3420, which I haven't found a way to name from
> the command line.

Bus names in qdev are a mess.  Here are the rules:

1. If code provides a name, that's the name.

2. Else, if the device has an ID, the name is ID.N, where N counts the
device's buses from zero.

3. Else, the name is BUS-TYPE-NAME.N, where N counts the buses of this
type from zero.

This results in a usable bus name unless device IDs collide with bus
type names, or the code provides names that collide.

The user needs to take care to use IDs that don't collide with bus
names.  Adding new bus names may screw some users.

The user needs to take further care to use IDs whenever the code
provides a bus name that collides.  Adding code that provides bus names
may screw some users.

Broken by design.

The problem here is "code provides names that collide": device
q35-pcihost provides the name "pcie.0".  Bound to collide with the first
PCIE bus named under rule 3.  For instance, if you next add an ioh3420
without ID, its bus is also named "pcie.0".

Rule 1 should be taken out and shot.  Unfortunately, that'll break ABI
left & right.  Instead, we can try to reduce its use.  The appended does
exactly that for q35-pcihost.  With it applied, the bus provided by
q35-pcihost still gets the same name "pcie.0", but under rule 3 instead
of rule 1.  Rule 3 then names further PCIE buses "pcie.1", "pcie.2", ...
instead of "pcie.0", "pcie.1", ...  Better, but it's still an ABI break.

> Maybe an even better solution would be to have default names for
> everything, if not specified, from a user friendliness perspective? 

Buses *have* a default name!  You're confusing this with device IDs,
which exist only when the user sets one.

Changes in this area are difficult, because the names are all ABI.
Names that cannot be used are fair game, of course.

> I suppose this is a more general issue of sensible default values
> though, but the fact that it is easy to create devices which cannot be
> referred has caused me some confusion from time to time.

Picking default qdev IDs risks collisions with the user's IDs.  We
shouldn't do that.  We do it anyway in a few places, for historical
reasons.

QOM paths might be a sane way to let users refer to devices without IDs.


While writing the above, I stumbled another rule 1 screwup: pci_bridge.c
attempts to "improve" the boring standard bus names chosen via rule 2 or
3.

pci_bridge_initfn() provides a bus name of its own (commit 8a3d80f
pci_bridge: user-friendly default bus name):

a. If pci_bridge_map_irq() set a bus name, that's the name.

b. Else, if the device has an ID, that's the name.  Thus, ID.N is
"improved" to just ID, at the cost of a special case: now users have to
avoid not just IDs of the form BUS-TYPE-NAME.N, but also plain
BUS-TYPE-NAME.

Callers of pci_bridge_map_irq() generally provide a name.  Some names
contain spaces, thus can't collide (but would be bloody inconvenient on
the command line or in the monitor).  Others don't, but thankfully the
ones I checked are in dead code.  Craptastic.


diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 37f228e..469aafd 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -47,7 +47,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
     sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
 
-    pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
+    pci->bus = pci_bus_new(DEVICE(s), NULL,
                            s->mch.pci_address_space, s->mch.address_space_io,
                            0, TYPE_PCIE_BUS);
     qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Michael S. Tsirkin]

On Wed, Aug 20, 2014 at 02:06:38PM +0200, Markus Armbruster wrote:
> Knut Omang <knut.omang@oracle.com> writes:
> 
> > On Wed, 2014-08-20 at 10:52 +0200, Paolo Bonzini wrote:
> >> Il 20/08/2014 08:53, Knut Omang ha scritto:
> >> > A unique bus name is necessary to be able to refer to each instance
> >> > from the command line and monitors.
> >> 
> >> Is it needed?  Can't you just add id= to the -device option?
> >
> > Yes, as far as I understand the problem is that the id= would work on
> > the ioh3420 device itself, while what is needed here is to name the
> > secondary bus of the ioh3420, which I haven't found a way to name from
> > the command line.
> 
> Bus names in qdev are a mess.  Here are the rules:
> 
> 1. If code provides a name, that's the name.
> 
> 2. Else, if the device has an ID, the name is ID.N, where N counts the
> device's buses from zero.
> 
> 3. Else, the name is BUS-TYPE-NAME.N, where N counts the buses of this
> type from zero.
> 
> This results in a usable bus name unless device IDs collide with bus
> type names, or the code provides names that collide.
> 
> The user needs to take care to use IDs that don't collide with bus
> names.  Adding new bus names may screw some users.
> 
> The user needs to take further care to use IDs whenever the code
> provides a bus name that collides.  Adding code that provides bus names
> may screw some users.
> 
> Broken by design.
> 
> The problem here is "code provides names that collide": device
> q35-pcihost provides the name "pcie.0".  Bound to collide with the first
> PCIE bus named under rule 3.  For instance, if you next add an ioh3420
> without ID, its bus is also named "pcie.0".
> 
> Rule 1 should be taken out and shot.  Unfortunately, that'll break ABI
> left & right.  Instead, we can try to reduce its use.  The appended does
> exactly that for q35-pcihost.  With it applied, the bus provided by
> q35-pcihost still gets the same name "pcie.0", but under rule 3 instead
> of rule 1.  Rule 3 then names further PCIE buses "pcie.1", "pcie.2", ...
> instead of "pcie.0", "pcie.1", ...  Better, but it's still an ABI break.
> 
> > Maybe an even better solution would be to have default names for
> > everything, if not specified, from a user friendliness perspective? 
> 
> Buses *have* a default name!  You're confusing this with device IDs,
> which exist only when the user sets one.
> 
> Changes in this area are difficult, because the names are all ABI.
> Names that cannot be used are fair game, of course.
> 
> > I suppose this is a more general issue of sensible default values
> > though, but the fact that it is easy to create devices which cannot be
> > referred has caused me some confusion from time to time.
> 
> Picking default qdev IDs risks collisions with the user's IDs.  We
> shouldn't do that.  We do it anyway in a few places, for historical
> reasons.
> 
> QOM paths might be a sane way to let users refer to devices without IDs.
> 
> 
> While writing the above, I stumbled another rule 1 screwup: pci_bridge.c
> attempts to "improve" the boring standard bus names chosen via rule 2 or
> 3.
> 
> pci_bridge_initfn() provides a bus name of its own (commit 8a3d80f
> pci_bridge: user-friendly default bus name):
> a. If pci_bridge_map_irq() set a bus name, that's the name.
> 
> b. Else, if the device has an ID, that's the name.  Thus, ID.N is
> "improved" to just ID, at the cost of a special case: now users have to
> avoid not just IDs of the form BUS-TYPE-NAME.N, but also plain
> BUS-TYPE-NAME.
> 
> Callers of pci_bridge_map_irq() generally provide a name.  Some names
> contain spaces, thus can't collide (but would be bloody inconvenient on
> the command line or in the monitor).  Others don't, but thankfully the
> ones I checked are in dead code.  Craptastic.
> 
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 37f228e..469aafd 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -47,7 +47,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>      sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
>      sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
>  
> -    pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
> +    pci->bus = pci_bus_new(DEVICE(s), NULL,
>                             s->mch.pci_address_space, s->mch.address_space_io,
>                             0, TYPE_PCIE_BUS);
>      qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));

This is for the root bus, I think it won't help Knut who's trying to
add devices behind root ports.

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Aug 20, 2014 at 02:06:38PM +0200, Markus Armbruster wrote:
>> Knut Omang <knut.omang@oracle.com> writes:
>> 
>> > On Wed, 2014-08-20 at 10:52 +0200, Paolo Bonzini wrote:
>> >> Il 20/08/2014 08:53, Knut Omang ha scritto:
>> >> > A unique bus name is necessary to be able to refer to each instance
>> >> > from the command line and monitors.
>> >> 
>> >> Is it needed?  Can't you just add id= to the -device option?
>> >
>> > Yes, as far as I understand the problem is that the id= would work on
>> > the ioh3420 device itself, while what is needed here is to name the
>> > secondary bus of the ioh3420, which I haven't found a way to name from
>> > the command line.
>> 
>> Bus names in qdev are a mess.  Here are the rules:
>> 
>> 1. If code provides a name, that's the name.
>> 
>> 2. Else, if the device has an ID, the name is ID.N, where N counts the
>> device's buses from zero.
>> 
>> 3. Else, the name is BUS-TYPE-NAME.N, where N counts the buses of this
>> type from zero.
>> 
>> This results in a usable bus name unless device IDs collide with bus
>> type names, or the code provides names that collide.
>> 
>> The user needs to take care to use IDs that don't collide with bus
>> names.  Adding new bus names may screw some users.
>> 
>> The user needs to take further care to use IDs whenever the code
>> provides a bus name that collides.  Adding code that provides bus names
>> may screw some users.
>> 
>> Broken by design.
>> 
>> The problem here is "code provides names that collide": device
>> q35-pcihost provides the name "pcie.0".  Bound to collide with the first
>> PCIE bus named under rule 3.  For instance, if you next add an ioh3420
>> without ID, its bus is also named "pcie.0".
>> 
>> Rule 1 should be taken out and shot.  Unfortunately, that'll break ABI
>> left & right.  Instead, we can try to reduce its use.  The appended does
>> exactly that for q35-pcihost.  With it applied, the bus provided by
>> q35-pcihost still gets the same name "pcie.0", but under rule 3 instead
>> of rule 1.  Rule 3 then names further PCIE buses "pcie.1", "pcie.2", ...
>> instead of "pcie.0", "pcie.1", ...  Better, but it's still an ABI break.
>> 
>> > Maybe an even better solution would be to have default names for
>> > everything, if not specified, from a user friendliness perspective? 
>> 
>> Buses *have* a default name!  You're confusing this with device IDs,
>> which exist only when the user sets one.
>> 
>> Changes in this area are difficult, because the names are all ABI.
>> Names that cannot be used are fair game, of course.
>> 
>> > I suppose this is a more general issue of sensible default values
>> > though, but the fact that it is easy to create devices which cannot be
>> > referred has caused me some confusion from time to time.
>> 
>> Picking default qdev IDs risks collisions with the user's IDs.  We
>> shouldn't do that.  We do it anyway in a few places, for historical
>> reasons.
>> 
>> QOM paths might be a sane way to let users refer to devices without IDs.
>> 
>> 
>> While writing the above, I stumbled another rule 1 screwup: pci_bridge.c
>> attempts to "improve" the boring standard bus names chosen via rule 2 or
>> 3.
>> 
>> pci_bridge_initfn() provides a bus name of its own (commit 8a3d80f
>> pci_bridge: user-friendly default bus name):
>> a. If pci_bridge_map_irq() set a bus name, that's the name.
>> 
>> b. Else, if the device has an ID, that's the name.  Thus, ID.N is
>> "improved" to just ID, at the cost of a special case: now users have to
>> avoid not just IDs of the form BUS-TYPE-NAME.N, but also plain
>> BUS-TYPE-NAME.
>> 
>> Callers of pci_bridge_map_irq() generally provide a name.  Some names
>> contain spaces, thus can't collide (but would be bloody inconvenient on
>> the command line or in the monitor).  Others don't, but thankfully the
>> ones I checked are in dead code.  Craptastic.
>> 
>> 
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 37f228e..469aafd 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -47,7 +47,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>>      sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
>>      sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
>>  
>> -    pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
>> +    pci->bus = pci_bus_new(DEVICE(s), NULL,
>>                             s->mch.pci_address_space, s->mch.address_space_io,
>>                             0, TYPE_PCIE_BUS);
>>      qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));
>
> This is for the root bus, I think it won't help Knut who's trying to
> add devices behind root ports.

Read again, more slowly :)

Yes, I null the name of the root bus.  That makes the qdev machinery
derive the very same "pcie.0" name via rule 3 instead of rule 1, with
the side effect that future (non-root) PCIE buses get different names.
In particular, the next one named via rule 3 will be called "pcie.1"
instead of "pcie.0", making it actually accessible.

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Knut Omang]

On Wed, 2014-08-20 at 13:36 +0200, Michael S. Tsirkin wrote:
> On Wed, Aug 20, 2014 at 11:30:55AM +0200, Knut Omang wrote:
> > On Wed, 2014-08-20 at 10:52 +0200, Paolo Bonzini wrote:
> > > Il 20/08/2014 08:53, Knut Omang ha scritto:
> > > > A unique bus name is necessary to be able to refer to each instance
> > > > from the command line and monitors.
> > > 
> > > Is it needed?  Can't you just add id= to the -device option?
> > 
> > Yes, as far as I understand the problem is that the id= would work on
> > the ioh3420 device itself, while what is needed here is to name the
> > secondary bus of the ioh3420, which I haven't found a way to name from
> > the command line.
> 
> Did you try using the device name?

I believe I did, I tried a lot back and forth back then...

> For pci bridges, unless you set bus_name, bus name will
> match device itself. See this code:
> 
>      * If we don't specify the name, the bus will be addressed as
>      * <id>.0, where id is the device id.
>      * Since PCI Bridge devices have a single bus each, we don't need
>      * the index:
>      * let users address the bus using the device name.
>      */
>     if (!br->bus_name && dev->qdev.id && *dev->qdev.id) {
>             br->bus_name = dev->qdev.id;
>     }

but my testing of this may well have been before your patch with this
logic - the cost of being so slow with my patches - it won't happen
again...
 
Both with the somewhat counterintuitive second re-provision of the bus
name in

-    pci_bridge_map_irq(br, g_strdup(tmp), pci_swizzle_map_irq_fn);
+    pci_bridge_map_irq(br, br->bus_name, pci_swizzle_map_irq_fn);

and actually with removing it due to Alex's commit 659fefee which was
also not in when this was conceived, I am now able to instantiate two
devices on two ports by means of:

-device ioh3420,slot=0,id=pcie_port.0
-device ioh3420,slot=1,id=pcie_port.1
-device <ari_capable_device1>,bus=pcie_port.0
-device <ari_capable_device2>,bus=pcie_port.1

In light of Markus' enlightening description of the mess here, unless
there are more issues, I'll repost the rest of the changes without this
commit,

Thanks,

Knut

> > Maybe an even better solution would be to have default names for
> > everything, if not specified, from a user friendliness perspective? 
> >
> > I suppose this is a more general issue of sensible default values
> > though, but the fact that it is easy to create devices which cannot be
> > referred has caused me some confusion from time to time.
> > 
> > > Paolo
> > 
> > Thanks,
> > 
> > Knut

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Paolo Bonzini]

Il 20/08/2014 15:03, Markus Armbruster ha scritto:
>> >
>> > This is for the root bus, I think it won't help Knut who's trying to
>> > add devices behind root ports.
> Read again, more slowly :)
> 
> Yes, I null the name of the root bus.  That makes the qdev machinery
> derive the very same "pcie.0" name via rule 3 instead of rule 1, with
> the side effect that future (non-root) PCIE buses get different names.
> In particular, the next one named via rule 3 will be called "pcie.1"
> instead of "pcie.0", making it actually accessible.

I agree that this is a big improvement.

Paolo

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Paolo Bonzini]

Il 20/08/2014 13:36, Michael S. Tsirkin ha scritto:
> 
> For pci bridges, unless you set bus_name, bus name will
> match device itself. See this code:
> 
>      * If we don't specify the name, the bus will be addressed as
>      * <id>.0, where id is the device id.
>      * Since PCI Bridge devices have a single bus each, we don't need
>      * the index:
>      * let users address the bus using the device name.
>      */
>     if (!br->bus_name && dev->qdev.id && *dev->qdev.id) {
>             br->bus_name = dev->qdev.id;
>     }

Is libvirt using this rule?  If not, I'd rather slash it since the
<id>.0 name is shared with all other buses and not PCI-bridge-specific.

Paolo

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 20/08/2014 15:03, Markus Armbruster ha scritto:
>>> >
>>> > This is for the root bus, I think it won't help Knut who's trying to
>>> > add devices behind root ports.
>> Read again, more slowly :)
>> 
>> Yes, I null the name of the root bus.  That makes the qdev machinery
>> derive the very same "pcie.0" name via rule 3 instead of rule 1, with
>> the side effect that future (non-root) PCIE buses get different names.
>> In particular, the next one named via rule 3 will be called "pcie.1"
>> instead of "pcie.0", making it actually accessible.
>
> I agree that this is a big improvement.

It's also an ABI break.  I'm not saying we can't do it, just that we
better consider it carefully.

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 20/08/2014 13:36, Michael S. Tsirkin ha scritto:
>> 
>> For pci bridges, unless you set bus_name, bus name will
>> match device itself. See this code:
>> 
>>      * If we don't specify the name, the bus will be addressed as
>>      * <id>.0, where id is the device id.
>>      * Since PCI Bridge devices have a single bus each, we don't need
>>      * the index:
>>      * let users address the bus using the device name.
>>      */
>>     if (!br->bus_name && dev->qdev.id && *dev->qdev.id) {
>>             br->bus_name = dev->qdev.id;
>>     }
>
> Is libvirt using this rule?  If not, I'd rather slash it since the
> <id>.0 name is shared with all other buses and not PCI-bridge-specific.

br->bus_name is null unless pci_bridge_map_irq() set it.  Only caller
for ioh3420 is ioh3420_init(), and that's dead code.  Therefore,
br->bus_name is null here.

Libvirt always sets a device ID.  Slashing this this special case would
change the bus name from ID.0 to just ID.  That'll break libvirt, as far
as I can tell from its source.

Sad.

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

On 08/20/2014 08:57 AM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 20/08/2014 13:36, Michael S. Tsirkin ha scritto:
>>>
>>> For pci bridges, unless you set bus_name, bus name will
>>> match device itself. See this code:
>>>
>>>      * If we don't specify the name, the bus will be addressed as
>>>      * <id>.0, where id is the device id.
>>>      * Since PCI Bridge devices have a single bus each, we don't need
>>>      * the index:
>>>      * let users address the bus using the device name.
>>>      */
>>>     if (!br->bus_name && dev->qdev.id && *dev->qdev.id) {
>>>             br->bus_name = dev->qdev.id;
>>>     }
>>
>> Is libvirt using this rule?  If not, I'd rather slash it since the
>> <id>.0 name is shared with all other buses and not PCI-bridge-specific.
> 
> br->bus_name is null unless pci_bridge_map_irq() set it.  Only caller
> for ioh3420 is ioh3420_init(), and that's dead code.  Therefore,
> br->bus_name is null here.
> 
> Libvirt always sets a device ID.  Slashing this this special case would
> change the bus name from ID.0 to just ID.  That'll break libvirt, as far
> as I can tell from its source.

Libvirt has had to deal with shenanigans like this before:

http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_capabilities.c;h=b758b5a0d4;hb=HEAD#l1982

> 
> Sad.

Although libvirt could deal with the fallout (especially if it is made
introspectible, instead of just randomly changing with no witness that
can be probed via QMP), you are correct that it would be nicer to not
break existing clients :(

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function

[Paolo Bonzini]

Il 20/08/2014 20:32, Eric Blake ha scritto:
> On 08/20/2014 08:57 AM, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 20/08/2014 13:36, Michael S. Tsirkin ha scritto:
>>>>
>>>> For pci bridges, unless you set bus_name, bus name will
>>>> match device itself. See this code:
>>>>
>>>>      * If we don't specify the name, the bus will be addressed as
>>>>      * <id>.0, where id is the device id.
>>>>      * Since PCI Bridge devices have a single bus each, we don't need
>>>>      * the index:
>>>>      * let users address the bus using the device name.
>>>>      */
>>>>     if (!br->bus_name && dev->qdev.id && *dev->qdev.id) {
>>>>             br->bus_name = dev->qdev.id;
>>>>     }
>>>
>>> Is libvirt using this rule?  If not, I'd rather slash it since the
>>> <id>.0 name is shared with all other buses and not PCI-bridge-specific.
>>
>> br->bus_name is null unless pci_bridge_map_irq() set it.  Only caller
>> for ioh3420 is ioh3420_init(), and that's dead code.  Therefore,
>> br->bus_name is null here.
>>
>> Libvirt always sets a device ID.  Slashing this this special case would
>> change the bus name from ID.0 to just ID.  That'll break libvirt, as far
>> as I can tell from its source.
> 
> Libvirt has had to deal with shenanigans like this before:
> 
> http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_capabilities.c;h=b758b5a0d4;hb=HEAD#l1982

While I applaud your attempts, I still maintain that it is pointless to
support anything but a very recent QEMU (let's say 2.0+ right now) for
non-x86 architecture.

I think that for q35 we should still be able to do modifications (and
perhaps should not even be providing backwards-compatible machine
types), but pci bridges affect piix4 too.

Paolo