[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an i
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function |
Date: |
Wed, 20 Aug 2014 15:03:59 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
"Michael S. Tsirkin" <address@hidden> writes:
> 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.
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.
- [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function, Knut Omang, 2014/08/20
- Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function, Paolo Bonzini, 2014/08/20
- Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function, Markus Armbruster, 2014/08/20
- Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function, Michael S. Tsirkin, 2014/08/20
- Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function, Paolo Bonzini, 2014/08/20
- Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function, Markus Armbruster, 2014/08/20
Re: [Qemu-devel] [PATCH 2/3] ioh3420: Provide a unique bus name and an interrupt mapping function, Andreas Färber, 2014/08/20