From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58976) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2lva-00050o-BL for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:44:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2lvX-0007ol-0S for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:44:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49202) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2lvW-0007nv-NS for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:44:42 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 3319735C10C for ; Wed, 10 Jun 2015 19:44:42 +0000 (UTC) Message-ID: <557893A5.5040805@redhat.com> Date: Wed, 10 Jun 2015 21:44:37 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1433956033-11584-1-git-send-email-lersek@redhat.com> <1433956033-11584-5-git-send-email-lersek@redhat.com> <55788F56.4000607@redhat.com> In-Reply-To: <55788F56.4000607@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum , qemu-devel@nongnu.org Cc: Markus Armbruster , "Michael S. Tsirkin" On 06/10/15 21:26, Marcel Apfelbaum wrote: > On 06/10/2015 08:07 PM, Laszlo Ersek wrote: >> The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot >> off devices behind the PXB. This happens because the >> sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have >> enough information to format a unique identifier for the PXB in question, >> and consequently the OpenFirmware device path passed down to the guest >> firmware in the "bootorder" fw_cfg file is unusable for identifying the >> boot device. >> >> For example, the command line fragment >> >> -device pxb,id=bridge1,bus_nr=4 \ >> \ >> -netdev user,id=netdev0 \ >> -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0 >> >> results in the following "bootorder" entry: >> >> /pci/pci-bridge@0/ethernet@2/ethernet-phy@0 >> >> The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and the >> resultant OpenFirmware device path is independent of bus_nr=4 -- and >> therefore it is useless for identifying the device. >> >> In this patch we insert a dummy bus between the main sysbus and the >> TYPE_PXB_HOST device. Formatting child addresses is a bus class level >> responsibility, which is exactly what we'll use here. >> >> After the patch, the same command line fragment results in the following >> OpenFirmware device path in the "bootorder" fw_cfg file: >> >> /extra-pci-roots/pxbhost@4/pci-bridge@0/ethernet@2/ethernet-phy@0 > Here I have a little comment: > Thinking of it again, using pxbhost here is too specific. *Any kind* of > PCI root > bridge will benefit from this notation, not only PXB. > I hope you are OK with it. > /extra-pci-roots/pci-root@4/ maybe is more generic. Sure, absolutely. > >> >> The original, initial "/pci" fragment has been replaced with >> "/extra-pci-roots/pxbhost@4", which (a) looks better, (b) provides all >> the >> necessary information. sysbus_get_fw_dev_path() formats the first node >> ("extra-pci-roots") as always, and the new function >> extra_pci_roots_bus_get_fw_dev_path() formats the second one >> ("pxbhost@4"). > The "extra-pci-roots" is a nice touch. > >> >> Here's the comparison ("diff -u -b -U28") between the "info qtree" >> outputs, before and after (the hpet device is the first common line): >> >>> --- qtree.before 2015-06-10 18:16:44.903359633 +0200 >>> +++ qtree.after 2015-06-10 18:19:01.904139538 +0200 >>> @@ -1,30 +1,33 @@ >>> bus: main-system-bus >>> type System >>> + dev: extra-pci-roots, id "" >>> + bus: extra-pci-roots-bus.0 >>> + type extra-pci-roots-bus >>> dev: pxb-host, id "" >>> bus: pxb-internal >>> type pxb-bus >>> dev: pci-basic-bridge, id "bridge1" >>> chassis_nr = 4 (0x4) >>> addr = 00.0 >>> romfile = "" >>> rombar = 1 (0x1) >>> multifunction = false >>> command_serr_enable = true >>> class PCI bridge, addr 04:00.0, pci id 1b36:000a (sub >>> 0000:0000) >>> bus: bridge1 >>> type PCI >>> dev: e1000, id "" >>> mac = "52:54:00:12:34:56" >>> vlan = >>> netdev = "netdev0" >>> autonegotiation = true >>> mitigation = true >>> addr = 02.0 >>> romfile = "" >>> rombar = 1 (0x1) >>> multifunction = false >>> command_serr_enable = true >>> class Ethernet controller, addr 05:02.0, pci id >>> 8086:100e (sub 1af4:1100) >>> bar 0: mem at 0x88100000 [0x8811ffff] >>> bar 1: i/o at 0xd000 [0xd03f] >>> dev: hpet, id "" >> >> Cc: Markus Armbruster >> Cc: Marcel Apfelbaum >> Cc: Michael S. Tsirkin >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> v2: >> - new in v2, addressing the "bootorder" fw_cfg issue discussed >> earlier >> - Cc'd Markus, because I probably butchered a handful of QOM >> rules in >> this patch :) >> >> hw/pci-bridge/pci_expander_bridge.c | 45 >> +++++++++++++++++++++++++++++++++++-- >> 1 file changed, 43 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pci-bridge/pci_expander_bridge.c >> b/hw/pci-bridge/pci_expander_bridge.c >> index c7a085d..30e93d6 100644 >> --- a/hw/pci-bridge/pci_expander_bridge.c >> +++ b/hw/pci-bridge/pci_expander_bridge.c >> @@ -19,6 +19,15 @@ >> #include "qemu/error-report.h" >> #include "sysemu/numa.h" >> >> +/* >> + * We'll insert a dummy bus between the main sysbus and >> TYPE_PXB_HOST, because >> + * the main sysbus doesn't know how to format the bus number of the >> extra root >> + * bus in sysbus_get_fw_dev_path(). We'll use this dummy bus just for >> + * formatting that. >> + */ >> +#define TYPE_EXTRA_PCI_ROOTS_DEV "extra-pci-roots" >> +#define TYPE_EXTRA_PCI_ROOTS_BUS "extra-pci-roots-bus" >> + >> #define TYPE_PXB_BUS "pxb-bus" >> #define PXB_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_BUS) >> >> @@ -93,7 +102,8 @@ static void pxb_host_class_init(ObjectClass *class, >> void *data) >> DeviceClass *dc = DEVICE_CLASS(class); >> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class); >> >> - dc->fw_name = "pci"; >> + dc->fw_name = "pxbhost"; >> + dc->bus_type = TYPE_EXTRA_PCI_ROOTS_BUS; >> hc->root_bus_path = pxb_host_root_bus_path; >> } >> >> @@ -151,6 +161,8 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int >> pin) >> static int pxb_dev_initfn(PCIDevice *dev) >> { >> PXBDev *pxb = PXB_DEV(dev); >> + DeviceState *extra_pci_roots_dev; >> + BusState *extra_pci_roots_bus; >> DeviceState *ds, *bds; >> PCIBus *bus; >> const char *dev_name = NULL; >> @@ -165,7 +177,10 @@ static int pxb_dev_initfn(PCIDevice *dev) >> dev_name = dev->qdev.id; >> } >> >> - ds = qdev_create(NULL, TYPE_PXB_HOST); >> + extra_pci_roots_dev = qdev_create(NULL, TYPE_EXTRA_PCI_ROOTS_DEV); >> + extra_pci_roots_bus = qbus_create(TYPE_EXTRA_PCI_ROOTS_BUS, >> + extra_pci_roots_dev, NULL); > Yep, this is the answer. > >> + ds = qdev_create(extra_pci_roots_bus, TYPE_PXB_HOST); >> bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS); >> >> bus->parent_dev = dev; >> @@ -221,11 +236,37 @@ static const TypeInfo pxb_dev_info = { >> .class_init = pxb_dev_class_init, >> }; >> >> +static char *extra_pci_roots_bus_get_fw_dev_path(DeviceState *dev) >> +{ >> + return g_strdup_printf("%s@%x", qdev_fw_name(dev), >> + pxb_bus_num(PCI_HOST_BRIDGE(dev)->bus)); >> +} >> + >> +static void extra_pci_roots_bus_class_init(ObjectClass *klass, void >> *data) >> +{ >> + BusClass *k = BUS_CLASS(klass); >> + >> + k->get_fw_dev_path = extra_pci_roots_bus_get_fw_dev_path; > I tried to add this to the existing PXB Bus, but it didn't work of course. > The PXB Bus is *behind* the PXB device. Right. The PXB bus class (TYPE_PXB_BUS) is derived from TYPE_PCI_BUS, which has its own "child-formatter" callback: pcibus_get_fw_dev_path(). One can override that member in a derived class, but that's not useful, for two reason: - it's already too far from the qtree root (ie. past the node where the needed info is available), like you said; and - pcibus_get_fw_dev_path() is *correct* in the way it formats its children's addresses. Namely, pcibus_get_fw_dev_path() takes credit for formatting the pci-bridge@0 node, which is just right, because the bridge is in slot 0 of the parent TYPE_PXB_HOST. We should not change that; the current method mirrors the counterpart UEFI device paths precisely. Thanks for the feedback, if Markus and Michael are otherwise okay with this patch, I'll replace "pxbhost" with "pci-root". Thanks! Laszlo > >> +} >> + >> +static const TypeInfo extra_pci_roots_bus_info = { >> + .name = TYPE_EXTRA_PCI_ROOTS_BUS, >> + .parent = TYPE_BUS, >> + .class_init = extra_pci_roots_bus_class_init, >> +}; >> + >> +static const TypeInfo extra_pci_roots_bus_dev_info = { >> + .name = TYPE_EXTRA_PCI_ROOTS_DEV, >> + .parent = TYPE_SYS_BUS_DEVICE, >> +}; >> + >> static void pxb_register_types(void) >> { >> type_register_static(&pxb_bus_info); >> type_register_static(&pxb_host_info); >> type_register_static(&pxb_dev_info); >> + type_register_static(&extra_pci_roots_bus_info); >> + type_register_static(&extra_pci_roots_bus_dev_info); >> } >> >> type_init(pxb_register_types) >> > > Thanks, > Marcel >