From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53201) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2leU-0002bU-OC for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:27:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2leP-0006fH-OQ for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:27:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56198) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2leP-0006fA-FN for qemu-devel@nongnu.org; Wed, 10 Jun 2015 15:27:01 -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 1A7EA2DC41A for ; Wed, 10 Jun 2015 19:27:01 +0000 (UTC) Message-ID: <55788F80.2060404@redhat.com> Date: Wed, 10 Jun 2015 22:26:56 +0300 From: Marcel Apfelbaum MIME-Version: 1.0 References: <1433956033-11584-1-git-send-email-lersek@redhat.com> <1433956033-11584-5-git-send-email-lersek@redhat.com> In-Reply-To: <1433956033-11584-5-git-send-email-lersek@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Laszlo Ersek , qemu-devel@nongnu.org Cc: Markus Armbruster , "Michael S. Tsirkin" 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 > > 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"). > > Here's the comparison ("diff -u -b -U28") between the "info qtree" > outputs, before and after (the hpet device is the first common line): BTW, did you try to boot from it :) ? > >> --- 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); > + 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; > +} > + > +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) >