From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43682) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XK4fQ-0003KT-4z for qemu-devel@nongnu.org; Wed, 20 Aug 2014 08:07:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XK4fL-0000oK-Df for qemu-devel@nongnu.org; Wed, 20 Aug 2014 08:07:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8270) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XK4fL-0000oE-59 for qemu-devel@nongnu.org; Wed, 20 Aug 2014 08:06:59 -0400 From: Markus Armbruster References: <1408517593.25437.102.camel@ori.omang.mine.nu> <53F461E6.5020506@redhat.com> <1408527055.14053.107.camel@abi.no.oracle.com> Date: Wed, 20 Aug 2014 14:06:38 +0200 In-Reply-To: <1408527055.14053.107.camel@abi.no.oracle.com> (Knut Omang's message of "Wed, 20 Aug 2014 11:30:55 +0200") Message-ID: <87k3631i29.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Knut Omang Cc: Marcel Apfelbaum , Alexey Kardashevskiy , Juan Quintela , qemu-devel@nongnu.org, Gonglei , "Michael S.Tsirkin" , Igor Mammedov , Paolo Bonzini Knut Omang 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));