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

[Markus Armbruster]

Knut Omang <address@hidden> 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));