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

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