| Message ID | 1408517593.25437.102.camel@ori.omang.mine.nu |
|---|---|
| State | New |
| Headers | show |
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
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
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
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,
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
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
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.
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 :(
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
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,
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(+)