* [Qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes
@ 2015-06-10 17:07 Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 1/4] i386/acpi-build: more traditional _UID and _HID for PXB root buses Laszlo Ersek
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 17:07 UTC (permalink / raw)
To: qemu-devel, lersek
Cc: Marcel Apfelbaum, Markus Armbruster, Michael S. Tsirkin
Version 2 of:
http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342208
- Patches #1 and #2 have not been changed.
- Patch #3 replaces #3 and #4 from v1.
- Patch #4 addresses a separate issue, raised originally in bullet (7)
of <http://thread.gmane.org/gmane.comp.emulators.qemu/342206>, and
then discussed further in that thread. I'm Cc'ing Markus for this
patch.
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Thanks
Laszlo
Laszlo Ersek (4):
i386/acpi-build: more traditional _UID and _HID for PXB root buses
i386/acpi-build: fix PXB workarounds for unsupported BIOSes
hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB
hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
hw/pci-bridge/Makefile.objs | 1 +
include/hw/pci/pci.h | 1 +
hw/i386/acpi-build.c | 13 +--
.../{pci_bridge_dev.c => pci_basic_bridge_dev.c} | 128 ++++++---------------
hw/pci-bridge/pci_expander_bridge.c | 47 +++++++-
5 files changed, 84 insertions(+), 106 deletions(-)
copy hw/pci-bridge/{pci_bridge_dev.c => pci_basic_bridge_dev.c} (35%)
--
1.8.3.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 1/4] i386/acpi-build: more traditional _UID and _HID for PXB root buses
2015-06-10 17:07 [Qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes Laszlo Ersek
@ 2015-06-10 17:07 ` Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 2/4] i386/acpi-build: fix PXB workarounds for unsupported BIOSes Laszlo Ersek
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 17:07 UTC (permalink / raw)
To: qemu-devel, lersek; +Cc: Marcel Apfelbaum, Michael S. Tsirkin
The ACPI specification permits the _HID and _UID objects to evaluate to
strings. (See "6.1.5 _HID (Hardware ID)" and "6.1.12 _UID (Unique ID)" in
the ACPI v6.0 spec.)
With regard to related standards, the UEFI specification can also express
a device address composed from string _HID and _UID identifiers, inside
the Expanded ACPI Device Path Node. (See "9.3.3 ACPI Device Path", Table
49, in the UEFI v2.5 spec.)
However, numeric (integer) contents for both _HID and _UID are more
traditional. They are recommended by the UEFI spec for size reasons:
[...] the ACPI Device Path node is smaller and should be used if
possible to reduce the size of device paths that may potentially be
stored in nonvolatile storage [...]
External tools support them better (for example the --acpi_hid and
--acpi_uid options of "efibootmgr" only take numeric identifiers).
Finally, numeric _HID and _UID contents are existing practice in the QEMU
source.
This patch was tested with a Fedora 20 LiveCD and a preexistent Windows
Server 2012 R2 guest. Using "acpidump" and "iasl" in the Fedora guest, we
get, in the SSDT:
> Scope (\_SB)
> {
> Device (PC04)
> {
> Name (_UID, 0x04) // _UID: Unique ID
> Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
Notes:
v2:
- no changes, added Marcel's R-b
hw/i386/acpi-build.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5593e41..52c2591 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -945,9 +945,8 @@ build_ssdt(GArray *table_data, GArray *linker,
scope = aml_scope("\\_SB");
dev = aml_device("PC%.02X", bus_num);
- aml_append(dev,
- aml_name_decl("_UID", aml_string("PC%.02X", bus_num)));
- aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03")));
+ aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
+ aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
if (numa_node != NUMA_NODE_UNASSIGNED) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] i386/acpi-build: fix PXB workarounds for unsupported BIOSes
2015-06-10 17:07 [Qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 1/4] i386/acpi-build: more traditional _UID and _HID for PXB root buses Laszlo Ersek
@ 2015-06-10 17:07 ` Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 3/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer Laszlo Ersek
3 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 17:07 UTC (permalink / raw)
To: qemu-devel, lersek; +Cc: Marcel Apfelbaum, Michael S. Tsirkin
The patch
apci: fix PXB behaviour if used with unsupported BIOS
uses the following condition to see if a "PXB mem/IO chunk" has *not* been
configured by the BIOS:
(!range_base || range_base > range_limit)
When this condition evaluates to true, said patch *omits* the
corresponding entry from the _CRS.
Later on the patch checks for the opposite condition (with the intent of
*adding* entries to the _CRS if the "PXB mem/IO chunks" *have* been
configured). Unfortunately, the condition was negated incorrectly: only
the first ! operator was removed, which led to the nonsensical expression
(range_base || range_base > range_limit)
leading to bogus entries in the _CRS, and causing BSOD in Windows Server
2012 R2 when it runs on OVMF.
The correct negative of the condition seen at the top is
(range_base && range_base <= range_limit)
Fix the expressions.
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
Notes:
v2:
- no changes, added Marcel's R-b
hw/i386/acpi-build.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 52c2591..b71e942 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -833,7 +833,7 @@ static Aml *build_crs(PCIHostState *host,
* Work-around for old bioses
* that do not support multiple root buses
*/
- if (range_base || range_base > range_limit) {
+ if (range_base && range_base <= range_limit) {
aml_append(crs,
aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
AML_POS_DECODE, AML_ENTIRE_RANGE,
@@ -854,7 +854,7 @@ static Aml *build_crs(PCIHostState *host,
* Work-around for old bioses
* that do not support multiple root buses
*/
- if (range_base || range_base > range_limit) {
+ if (range_base && range_base <= range_limit) {
aml_append(crs,
aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
AML_MAX_FIXED, AML_NON_CACHEABLE,
@@ -865,7 +865,7 @@ static Aml *build_crs(PCIHostState *host,
0,
range_limit - range_base + 1));
crs_range_insert(mem_ranges, range_base, range_limit);
- }
+ }
range_base =
pci_bridge_get_base(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
@@ -876,7 +876,7 @@ static Aml *build_crs(PCIHostState *host,
* Work-around for old bioses
* that do not support multiple root buses
*/
- if (range_base || range_base > range_limit) {
+ if (range_base && range_base <= range_limit) {
aml_append(crs,
aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
AML_MAX_FIXED, AML_NON_CACHEABLE,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB
2015-06-10 17:07 [Qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 1/4] i386/acpi-build: more traditional _UID and _HID for PXB root buses Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 2/4] i386/acpi-build: fix PXB workarounds for unsupported BIOSes Laszlo Ersek
@ 2015-06-10 17:07 ` Laszlo Ersek
2015-06-10 18:22 ` Marcel Apfelbaum
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer Laszlo Ersek
3 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 17:07 UTC (permalink / raw)
To: qemu-devel, lersek; +Cc: Marcel Apfelbaum, Michael S. Tsirkin
OVMF downloads the ACPI linker/loader script from QEMU when the edk2 PCI
Bus driver globally signals the firmware that PCI enumeration and resource
allocation have completed. At this point QEMU regenerates the ACPI payload
in an fw_cfg read callback, and this is when the PXB's _CRS gets
populated.
Unfortunately, when this happens, the PCI_COMMAND_MEMORY bit is clear in
the root bus's command register, *unlike* under SeaBIOS. The consequences
unfold as follows:
- When build_crs() fetches dev->io_regions[i].addr, it is all-bits-one,
because pci_update_mappings() --> pci_bar_address() calculated it as
PCI_BAR_UNMAPPED, due to the PCI_COMMAND_MEMORY bit being clear.
- Consequently, the SHPC MMIO BAR (bar 0) of the bridge is not added to
the _CRS, *despite* having been programmed in PCI config space.
- Similarly, the SHPC MMIO BAR of the PXB is not removed from the main
root bus's DWordMemory descriptor.
- Guest OSes (Linux and Windows alike) notice the pre-programmed SHPC BAR
within the PXB's config space, and notice that it conflicts with the
main root bus's memory resource descriptors. Linux reports
pci 0000:04:00.0: BAR 0: can't assign mem (size 0x100)
pci 0000:04:00.0: BAR 0: trying firmware assignment [mem
0x88200000-0x882000ff 64bit]
pci 0000:04:00.0: BAR 0: [mem 0x88200000-0x882000ff 64bit] conflicts
with PCI Bus 0000:00 [mem
0x88200000-0xfebfffff]
While Windows Server 2012 R2 reports
https://technet.microsoft.com/en-us/library/cc732199%28v=ws.10%29.aspx
This device cannot find enough free resources that it can use. If you
want to use this device, you will need to disable one of the other
devices on this system. (Code 12)
This issue was apparently encountered earlier, see the "hack" in:
https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02983.html
and the current hole-punching logic in build_crs() and build_ssdt() is
probably supposed to remedy exactly that problem -- however, for OVMF they
don't work, because at the end of the PCI enumeration and resource
allocation, which cues the ACPI linker/loader client, the command register
is clear.
It has been suggested to implement the above "hack" more cleanly and
permanently. Unfortunately, we can't just disable the SHPC bar of
TYPE_PCI_BRIDGE_DEV based on a new property, because this device model has
class-level ties to hotplug, and the SHPC bar (and the interrupt)
originate from there.
Therefore implement a more basic bridge device type, to be used by PXB,
that has no SHPC / hotplug support at all, and consequently (see commit
c008ac0c), no need for an interrupt / MSI either.
Suggested-by: Marcel Apfelbaum <marcel@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- Replaced the corresponding v1 patch with a new approach. Instead of
considering the PXB's disabled SHPC BAR in the PXB's _CRS (and
correspondingly, punching it out of the main _CRS), this patch creates
a separate device model that lacks the SHPC functionality completely.
I already know from IRC that Michael doesn't like this, but that's
okay: I'm formatting this with "-C35% --find-copies-harder", so that
the differences are obvious against the clone origin, and Michael can
pinpoint the locations where and how I should use a new device
property instead, in the original device model.
(That said, I did test this code, with both Windows and Linux, and
compared the dumped / decompiled SSDTs too.)
hw/pci-bridge/Makefile.objs | 1 +
include/hw/pci/pci.h | 1 +
.../{pci_bridge_dev.c => pci_basic_bridge_dev.c} | 128 ++++++---------------
hw/pci-bridge/pci_expander_bridge.c | 2 +-
4 files changed, 35 insertions(+), 97 deletions(-)
copy hw/pci-bridge/{pci_bridge_dev.c => pci_basic_bridge_dev.c} (35%)
diff --git a/hw/pci-bridge/Makefile.objs b/hw/pci-bridge/Makefile.objs
index f2adfe3..bcfe753 100644
--- a/hw/pci-bridge/Makefile.objs
+++ b/hw/pci-bridge/Makefile.objs
@@ -1,4 +1,5 @@
common-obj-y += pci_bridge_dev.o
+common-obj-y += pci_basic_bridge_dev.o
common-obj-y += pci_expander_bridge.o
common-obj-$(CONFIG_XIO3130) += xio3130_upstream.o xio3130_downstream.o
common-obj-$(CONFIG_IOH3420) += ioh3420.o
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d44bc84..619c31a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -92,6 +92,7 @@
#define PCI_DEVICE_ID_REDHAT_SDHCI 0x0007
#define PCI_DEVICE_ID_REDHAT_PCIE_HOST 0x0008
#define PCI_DEVICE_ID_REDHAT_PXB 0x0009
+#define PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE 0x000A
#define PCI_DEVICE_ID_REDHAT_QXL 0x0100
#define FMT_PCIBUS PRIx64
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_basic_bridge_dev.c
similarity index 35%
copy from hw/pci-bridge/pci_bridge_dev.c
copy to hw/pci-bridge/pci_basic_bridge_dev.c
index 36f73e1..d8bfb6c 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_basic_bridge_dev.c
@@ -1,7 +1,9 @@
/*
- * Standard PCI Bridge Device
+ * PCI Bridge Device without interrupt and SHPC / hotplug support.
*
- * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin <mst@redhat.com>
+ * Copyright (c) 2011, 2015 Red Hat Inc.
+ * Authors: Michael S. Tsirkin <mst@redhat.com>
+ * Laszlo Ersek <lersek@redhat.com>
*
* http://www.pcisig.com/specifications/conventional/pci_to_pci_bridge_architecture/
*
@@ -21,158 +23,92 @@
#include "hw/pci/pci_bridge.h"
#include "hw/pci/pci_ids.h"
-#include "hw/pci/msi.h"
-#include "hw/pci/shpc.h"
#include "hw/pci/slotid_cap.h"
#include "exec/memory.h"
#include "hw/pci/pci_bus.h"
-#include "hw/hotplug.h"
-#define TYPE_PCI_BRIDGE_DEV "pci-bridge"
-#define PCI_BRIDGE_DEV(obj) \
- OBJECT_CHECK(PCIBridgeDev, (obj), TYPE_PCI_BRIDGE_DEV)
+#define TYPE_PCI_BASIC_BRIDGE_DEV "pci-basic-bridge"
+#define PCI_BASIC_BRIDGE_DEV(obj) \
+ OBJECT_CHECK(PCIBasicBridgeDev, (obj), TYPE_PCI_BASIC_BRIDGE_DEV)
-struct PCIBridgeDev {
+struct PCIBasicBridgeDev {
/*< private >*/
PCIBridge parent_obj;
/*< public >*/
- MemoryRegion bar;
uint8_t chassis_nr;
-#define PCI_BRIDGE_DEV_F_MSI_REQ 0
- uint32_t flags;
};
-typedef struct PCIBridgeDev PCIBridgeDev;
+typedef struct PCIBasicBridgeDev PCIBasicBridgeDev;
-static int pci_bridge_dev_initfn(PCIDevice *dev)
+static int pci_basic_bridge_dev_initfn(PCIDevice *dev)
{
- PCIBridge *br = PCI_BRIDGE(dev);
- PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
+ PCIBasicBridgeDev *bridge_dev = PCI_BASIC_BRIDGE_DEV(dev);
int err;
err = pci_bridge_initfn(dev, TYPE_PCI_BUS);
if (err) {
goto bridge_error;
}
- dev->config[PCI_INTERRUPT_PIN] = 0x1;
- memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", shpc_bar_size(dev));
- err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
- if (err) {
- goto shpc_error;
- }
err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
if (err) {
goto slotid_error;
}
- if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
- msi_supported) {
- err = msi_init(dev, 0, 1, true, true);
- if (err < 0) {
- goto msi_error;
- }
- }
- /* TODO: spec recommends using 64 bit prefetcheable BAR.
- * Check whether that works well. */
- pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
- PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
return 0;
-msi_error:
- slotid_cap_cleanup(dev);
slotid_error:
- shpc_cleanup(dev, &bridge_dev->bar);
-shpc_error:
pci_bridge_exitfn(dev);
bridge_error:
return err;
}
-static void pci_bridge_dev_exitfn(PCIDevice *dev)
+static void pci_basic_bridge_dev_exitfn(PCIDevice *dev)
{
- PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
- if (msi_present(dev)) {
- msi_uninit(dev);
- }
slotid_cap_cleanup(dev);
- shpc_cleanup(dev, &bridge_dev->bar);
pci_bridge_exitfn(dev);
}
-static void pci_bridge_dev_instance_finalize(Object *obj)
-{
- shpc_free(PCI_DEVICE(obj));
-}
-
-static void pci_bridge_dev_write_config(PCIDevice *d,
- uint32_t address, uint32_t val, int len)
-{
- pci_bridge_write_config(d, address, val, len);
- if (msi_present(d)) {
- msi_write_config(d, address, val, len);
- }
- shpc_cap_write_config(d, address, val, len);
-}
-
-static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
-{
- PCIDevice *dev = PCI_DEVICE(qdev);
-
- pci_bridge_reset(qdev);
- shpc_reset(dev);
-}
-
-static Property pci_bridge_dev_properties[] = {
+static Property pci_basic_bridge_dev_properties[] = {
/* Note: 0 is not a legal chassis number. */
- DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
- DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, true),
+ DEFINE_PROP_UINT8("chassis_nr", PCIBasicBridgeDev, chassis_nr, 0),
DEFINE_PROP_END_OF_LIST(),
};
-static const VMStateDescription pci_bridge_dev_vmstate = {
- .name = "pci_bridge",
+static const VMStateDescription pci_basic_bridge_dev_vmstate = {
+ .name = "pci_basic_bridge",
.fields = (VMStateField[]) {
VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
- SHPC_VMSTATE(shpc, PCIDevice),
VMSTATE_END_OF_LIST()
}
};
-static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
+static void pci_basic_bridge_dev_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
- HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
- k->init = pci_bridge_dev_initfn;
- k->exit = pci_bridge_dev_exitfn;
- k->config_write = pci_bridge_dev_write_config;
+ k->init = pci_basic_bridge_dev_initfn;
+ k->exit = pci_basic_bridge_dev_exitfn;
+ k->config_write = pci_bridge_write_config;
k->vendor_id = PCI_VENDOR_ID_REDHAT;
- k->device_id = PCI_DEVICE_ID_REDHAT_BRIDGE;
+ k->device_id = PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE;
k->class_id = PCI_CLASS_BRIDGE_PCI;
k->is_bridge = 1,
- dc->desc = "Standard PCI Bridge";
- dc->reset = qdev_pci_bridge_dev_reset;
- dc->props = pci_bridge_dev_properties;
- dc->vmsd = &pci_bridge_dev_vmstate;
+ dc->desc = "Basic PCI Bridge (no interrupt, no hotplug)";
+ dc->reset = pci_bridge_reset;
+ dc->props = pci_basic_bridge_dev_properties;
+ dc->vmsd = &pci_basic_bridge_dev_vmstate;
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
- hc->plug = shpc_device_hotplug_cb;
- hc->unplug_request = shpc_device_hot_unplug_request_cb;
}
-static const TypeInfo pci_bridge_dev_info = {
- .name = TYPE_PCI_BRIDGE_DEV,
+static const TypeInfo pci_basic_bridge_dev_info = {
+ .name = TYPE_PCI_BASIC_BRIDGE_DEV,
.parent = TYPE_PCI_BRIDGE,
- .instance_size = sizeof(PCIBridgeDev),
- .class_init = pci_bridge_dev_class_init,
- .instance_finalize = pci_bridge_dev_instance_finalize,
- .interfaces = (InterfaceInfo[]) {
- { TYPE_HOTPLUG_HANDLER },
- { }
- }
+ .instance_size = sizeof(PCIBasicBridgeDev),
+ .class_init = pci_basic_bridge_dev_class_init,
};
-static void pci_bridge_dev_register(void)
+static void pci_basic_bridge_dev_register(void)
{
- type_register_static(&pci_bridge_dev_info);
+ type_register_static(&pci_basic_bridge_dev_info);
}
-type_init(pci_bridge_dev_register);
+type_init(pci_basic_bridge_dev_register);
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index ec2bb45..c7a085d 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -173,7 +173,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
bus->address_space_io = dev->bus->address_space_io;
bus->map_irq = pxb_map_irq_fn;
- bds = qdev_create(BUS(bus), "pci-bridge");
+ bds = qdev_create(BUS(bus), "pci-basic-bridge");
bds->id = dev_name;
qdev_prop_set_uint8(bds, "chassis_nr", pxb->bus_nr);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 17:07 [Qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes Laszlo Ersek
` (2 preceding siblings ...)
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 3/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB Laszlo Ersek
@ 2015-06-10 17:07 ` Laszlo Ersek
2015-06-10 19:26 ` Marcel Apfelbaum
2015-06-10 19:26 ` Marcel Apfelbaum
3 siblings, 2 replies; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 17:07 UTC (permalink / raw)
To: qemu-devel, lersek
Cc: Marcel Apfelbaum, Markus Armbruster, Michael S. Tsirkin
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):
> --- 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 = <null>
> 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 <armbru@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
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)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 3/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB Laszlo Ersek
@ 2015-06-10 18:22 ` Marcel Apfelbaum
2015-06-10 19:04 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Marcel Apfelbaum @ 2015-06-10 18:22 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: Michael S. Tsirkin
On 06/10/2015 08:07 PM, Laszlo Ersek wrote:
> OVMF downloads the ACPI linker/loader script from QEMU when the edk2 PCI
> Bus driver globally signals the firmware that PCI enumeration and resource
> allocation have completed. At this point QEMU regenerates the ACPI payload
> in an fw_cfg read callback, and this is when the PXB's _CRS gets
> populated.
>
> Unfortunately, when this happens, the PCI_COMMAND_MEMORY bit is clear in
> the root bus's command register, *unlike* under SeaBIOS. The consequences
> unfold as follows:
>
> - When build_crs() fetches dev->io_regions[i].addr, it is all-bits-one,
> because pci_update_mappings() --> pci_bar_address() calculated it as
> PCI_BAR_UNMAPPED, due to the PCI_COMMAND_MEMORY bit being clear.
>
> - Consequently, the SHPC MMIO BAR (bar 0) of the bridge is not added to
> the _CRS, *despite* having been programmed in PCI config space.
>
> - Similarly, the SHPC MMIO BAR of the PXB is not removed from the main
> root bus's DWordMemory descriptor.
>
> - Guest OSes (Linux and Windows alike) notice the pre-programmed SHPC BAR
> within the PXB's config space, and notice that it conflicts with the
> main root bus's memory resource descriptors. Linux reports
>
> pci 0000:04:00.0: BAR 0: can't assign mem (size 0x100)
> pci 0000:04:00.0: BAR 0: trying firmware assignment [mem
> 0x88200000-0x882000ff 64bit]
> pci 0000:04:00.0: BAR 0: [mem 0x88200000-0x882000ff 64bit] conflicts
> with PCI Bus 0000:00 [mem
> 0x88200000-0xfebfffff]
>
> While Windows Server 2012 R2 reports
>
> https://technet.microsoft.com/en-us/library/cc732199%28v=ws.10%29.aspx
>
> This device cannot find enough free resources that it can use. If you
> want to use this device, you will need to disable one of the other
> devices on this system. (Code 12)
>
> This issue was apparently encountered earlier, see the "hack" in:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02983.html
>
> and the current hole-punching logic in build_crs() and build_ssdt() is
> probably supposed to remedy exactly that problem -- however, for OVMF they
> don't work, because at the end of the PCI enumeration and resource
> allocation, which cues the ACPI linker/loader client, the command register
> is clear.
>
> It has been suggested to implement the above "hack" more cleanly and
> permanently. Unfortunately, we can't just disable the SHPC bar of
> TYPE_PCI_BRIDGE_DEV based on a new property, because this device model has
> class-level ties to hotplug, and the SHPC bar (and the interrupt)
> originate from there.
>
> Therefore implement a more basic bridge device type, to be used by PXB,
> that has no SHPC / hotplug support at all, and consequently (see commit
> c008ac0c), no need for an interrupt / MSI either.
>
> Suggested-by: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v2:
> - Replaced the corresponding v1 patch with a new approach. Instead of
> considering the PXB's disabled SHPC BAR in the PXB's _CRS (and
> correspondingly, punching it out of the main _CRS), this patch creates
> a separate device model that lacks the SHPC functionality completely.
>
> I already know from IRC that Michael doesn't like this, but that's
> okay: I'm formatting this with "-C35% --find-copies-harder", so that
> the differences are obvious against the clone origin
Nice, I'll use that.
, and Michael can
> pinpoint the locations where and how I should use a new device
> property instead, in the original device model.
>
> (That said, I did test this code, with both Windows and Linux, and
> compared the dumped / decompiled SSDTs too.)
>
> hw/pci-bridge/Makefile.objs | 1 +
> include/hw/pci/pci.h | 1 +
> .../{pci_bridge_dev.c => pci_basic_bridge_dev.c} | 128 ++++++---------------
> hw/pci-bridge/pci_expander_bridge.c | 2 +-
> 4 files changed, 35 insertions(+), 97 deletions(-)
> copy hw/pci-bridge/{pci_bridge_dev.c => pci_basic_bridge_dev.c} (35%)
>
> diff --git a/hw/pci-bridge/Makefile.objs b/hw/pci-bridge/Makefile.objs
> index f2adfe3..bcfe753 100644
> --- a/hw/pci-bridge/Makefile.objs
> +++ b/hw/pci-bridge/Makefile.objs
> @@ -1,4 +1,5 @@
> common-obj-y += pci_bridge_dev.o
> +common-obj-y += pci_basic_bridge_dev.o
> common-obj-y += pci_expander_bridge.o
> common-obj-$(CONFIG_XIO3130) += xio3130_upstream.o xio3130_downstream.o
> common-obj-$(CONFIG_IOH3420) += ioh3420.o
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index d44bc84..619c31a 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -92,6 +92,7 @@
> #define PCI_DEVICE_ID_REDHAT_SDHCI 0x0007
> #define PCI_DEVICE_ID_REDHAT_PCIE_HOST 0x0008
> #define PCI_DEVICE_ID_REDHAT_PXB 0x0009
> +#define PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE 0x000A
> #define PCI_DEVICE_ID_REDHAT_QXL 0x0100
>
> #define FMT_PCIBUS PRIx64
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_basic_bridge_dev.c
> similarity index 35%
> copy from hw/pci-bridge/pci_bridge_dev.c
> copy to hw/pci-bridge/pci_basic_bridge_dev.c
> index 36f73e1..d8bfb6c 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_basic_bridge_dev.c
> @@ -1,7 +1,9 @@
> /*
> - * Standard PCI Bridge Device
> + * PCI Bridge Device without interrupt and SHPC / hotplug support.
> *
> - * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin <mst@redhat.com>
> + * Copyright (c) 2011, 2015 Red Hat Inc.
> + * Authors: Michael S. Tsirkin <mst@redhat.com>
> + * Laszlo Ersek <lersek@redhat.com>
> *
> * http://www.pcisig.com/specifications/conventional/pci_to_pci_bridge_architecture/
> *
> @@ -21,158 +23,92 @@
>
> #include "hw/pci/pci_bridge.h"
> #include "hw/pci/pci_ids.h"
> -#include "hw/pci/msi.h"
> -#include "hw/pci/shpc.h"
> #include "hw/pci/slotid_cap.h"
> #include "exec/memory.h"
> #include "hw/pci/pci_bus.h"
> -#include "hw/hotplug.h"
>
> -#define TYPE_PCI_BRIDGE_DEV "pci-bridge"
> -#define PCI_BRIDGE_DEV(obj) \
> - OBJECT_CHECK(PCIBridgeDev, (obj), TYPE_PCI_BRIDGE_DEV)
> +#define TYPE_PCI_BASIC_BRIDGE_DEV "pci-basic-bridge"
> +#define PCI_BASIC_BRIDGE_DEV(obj) \
> + OBJECT_CHECK(PCIBasicBridgeDev, (obj), TYPE_PCI_BASIC_BRIDGE_DEV)
>
> -struct PCIBridgeDev {
> +struct PCIBasicBridgeDev {
> /*< private >*/
> PCIBridge parent_obj;
> /*< public >*/
>
> - MemoryRegion bar;
> uint8_t chassis_nr;
> -#define PCI_BRIDGE_DEV_F_MSI_REQ 0
> - uint32_t flags;
> };
> -typedef struct PCIBridgeDev PCIBridgeDev;
> +typedef struct PCIBasicBridgeDev PCIBasicBridgeDev;
>
> -static int pci_bridge_dev_initfn(PCIDevice *dev)
> +static int pci_basic_bridge_dev_initfn(PCIDevice *dev)
> {
> - PCIBridge *br = PCI_BRIDGE(dev);
> - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> + PCIBasicBridgeDev *bridge_dev = PCI_BASIC_BRIDGE_DEV(dev);
> int err;
>
> err = pci_bridge_initfn(dev, TYPE_PCI_BUS);
> if (err) {
> goto bridge_error;
> }
> - dev->config[PCI_INTERRUPT_PIN] = 0x1;
> - memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", shpc_bar_size(dev));
> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
> - if (err) {
> - goto shpc_error;
> - }
> err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
> if (err) {
> goto slotid_error;
> }
> - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> - msi_supported) {
> - err = msi_init(dev, 0, 1, true, true);
> - if (err < 0) {
> - goto msi_error;
> - }
> - }
> - /* TODO: spec recommends using 64 bit prefetcheable BAR.
> - * Check whether that works well. */
> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
> - PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
> return 0;
> -msi_error:
> - slotid_cap_cleanup(dev);
> slotid_error:
> - shpc_cleanup(dev, &bridge_dev->bar);
> -shpc_error:
> pci_bridge_exitfn(dev);
> bridge_error:
> return err;
> }
>
> -static void pci_bridge_dev_exitfn(PCIDevice *dev)
> +static void pci_basic_bridge_dev_exitfn(PCIDevice *dev)
> {
> - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> - if (msi_present(dev)) {
> - msi_uninit(dev);
> - }
> slotid_cap_cleanup(dev);
> - shpc_cleanup(dev, &bridge_dev->bar);
> pci_bridge_exitfn(dev);
> }
>
> -static void pci_bridge_dev_instance_finalize(Object *obj)
> -{
> - shpc_free(PCI_DEVICE(obj));
> -}
> -
> -static void pci_bridge_dev_write_config(PCIDevice *d,
> - uint32_t address, uint32_t val, int len)
> -{
> - pci_bridge_write_config(d, address, val, len);
> - if (msi_present(d)) {
> - msi_write_config(d, address, val, len);
> - }
> - shpc_cap_write_config(d, address, val, len);
> -}
> -
> -static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
> -{
> - PCIDevice *dev = PCI_DEVICE(qdev);
> -
> - pci_bridge_reset(qdev);
> - shpc_reset(dev);
> -}
> -
> -static Property pci_bridge_dev_properties[] = {
> +static Property pci_basic_bridge_dev_properties[] = {
> /* Note: 0 is not a legal chassis number. */
> - DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
> - DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, true),
> + DEFINE_PROP_UINT8("chassis_nr", PCIBasicBridgeDev, chassis_nr, 0),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> -static const VMStateDescription pci_bridge_dev_vmstate = {
> - .name = "pci_bridge",
> +static const VMStateDescription pci_basic_bridge_dev_vmstate = {
> + .name = "pci_basic_bridge",
> .fields = (VMStateField[]) {
> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> - SHPC_VMSTATE(shpc, PCIDevice),
> VMSTATE_END_OF_LIST()
> }
> };
>
> -static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
> +static void pci_basic_bridge_dev_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>
> - k->init = pci_bridge_dev_initfn;
> - k->exit = pci_bridge_dev_exitfn;
> - k->config_write = pci_bridge_dev_write_config;
> + k->init = pci_basic_bridge_dev_initfn;
> + k->exit = pci_basic_bridge_dev_exitfn;
> + k->config_write = pci_bridge_write_config;
> k->vendor_id = PCI_VENDOR_ID_REDHAT;
> - k->device_id = PCI_DEVICE_ID_REDHAT_BRIDGE;
> + k->device_id = PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE;
> k->class_id = PCI_CLASS_BRIDGE_PCI;
> k->is_bridge = 1,
> - dc->desc = "Standard PCI Bridge";
> - dc->reset = qdev_pci_bridge_dev_reset;
> - dc->props = pci_bridge_dev_properties;
> - dc->vmsd = &pci_bridge_dev_vmstate;
> + dc->desc = "Basic PCI Bridge (no interrupt, no hotplug)";
> + dc->reset = pci_bridge_reset;
> + dc->props = pci_basic_bridge_dev_properties;
> + dc->vmsd = &pci_basic_bridge_dev_vmstate;
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> - hc->plug = shpc_device_hotplug_cb;
> - hc->unplug_request = shpc_device_hot_unplug_request_cb;
> }
>
> -static const TypeInfo pci_bridge_dev_info = {
> - .name = TYPE_PCI_BRIDGE_DEV,
> +static const TypeInfo pci_basic_bridge_dev_info = {
> + .name = TYPE_PCI_BASIC_BRIDGE_DEV,
> .parent = TYPE_PCI_BRIDGE,
> - .instance_size = sizeof(PCIBridgeDev),
> - .class_init = pci_bridge_dev_class_init,
> - .instance_finalize = pci_bridge_dev_instance_finalize,
> - .interfaces = (InterfaceInfo[]) {
> - { TYPE_HOTPLUG_HANDLER },
> - { }
> - }
> + .instance_size = sizeof(PCIBasicBridgeDev),
> + .class_init = pci_basic_bridge_dev_class_init,
> };
>
> -static void pci_bridge_dev_register(void)
> +static void pci_basic_bridge_dev_register(void)
> {
> - type_register_static(&pci_bridge_dev_info);
> + type_register_static(&pci_basic_bridge_dev_info);
> }
>
> -type_init(pci_bridge_dev_register);
> +type_init(pci_basic_bridge_dev_register);
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index ec2bb45..c7a085d 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -173,7 +173,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
> bus->address_space_io = dev->bus->address_space_io;
> bus->map_irq = pxb_map_irq_fn;
>
> - bds = qdev_create(BUS(bus), "pci-bridge");
> + bds = qdev_create(BUS(bus), "pci-basic-bridge");
> bds->id = dev_name;
> qdev_prop_set_uint8(bds, "chassis_nr", pxb->bus_nr);
>
>
In my opinion this is the cleanest solution.
If Michael doesn't like the idea of a new "public" device,
we can incorporate it (hide it) into the PXB device, the same
I did with the PXB bus type. In this way it will be a PXB 'internal'.
What do you think?
In the meantime:
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB
2015-06-10 18:22 ` Marcel Apfelbaum
@ 2015-06-10 19:04 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 19:04 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: Michael S. Tsirkin
On 06/10/15 20:22, Marcel Apfelbaum wrote:
> On 06/10/2015 08:07 PM, Laszlo Ersek wrote:
>> OVMF downloads the ACPI linker/loader script from QEMU when the edk2 PCI
>> Bus driver globally signals the firmware that PCI enumeration and
>> resource
>> allocation have completed. At this point QEMU regenerates the ACPI
>> payload
>> in an fw_cfg read callback, and this is when the PXB's _CRS gets
>> populated.
>>
>> Unfortunately, when this happens, the PCI_COMMAND_MEMORY bit is clear in
>> the root bus's command register, *unlike* under SeaBIOS. The consequences
>> unfold as follows:
>>
>> - When build_crs() fetches dev->io_regions[i].addr, it is all-bits-one,
>> because pci_update_mappings() --> pci_bar_address() calculated it as
>> PCI_BAR_UNMAPPED, due to the PCI_COMMAND_MEMORY bit being clear.
>>
>> - Consequently, the SHPC MMIO BAR (bar 0) of the bridge is not added to
>> the _CRS, *despite* having been programmed in PCI config space.
>>
>> - Similarly, the SHPC MMIO BAR of the PXB is not removed from the main
>> root bus's DWordMemory descriptor.
>>
>> - Guest OSes (Linux and Windows alike) notice the pre-programmed SHPC BAR
>> within the PXB's config space, and notice that it conflicts with the
>> main root bus's memory resource descriptors. Linux reports
>>
>> pci 0000:04:00.0: BAR 0: can't assign mem (size 0x100)
>> pci 0000:04:00.0: BAR 0: trying firmware assignment [mem
>> 0x88200000-0x882000ff 64bit]
>> pci 0000:04:00.0: BAR 0: [mem 0x88200000-0x882000ff 64bit] conflicts
>> with PCI Bus 0000:00 [mem
>> 0x88200000-0xfebfffff]
>>
>> While Windows Server 2012 R2 reports
>>
>>
>> https://technet.microsoft.com/en-us/library/cc732199%28v=ws.10%29.aspx
>>
>> This device cannot find enough free resources that it can use. If
>> you
>> want to use this device, you will need to disable one of the other
>> devices on this system. (Code 12)
>>
>> This issue was apparently encountered earlier, see the "hack" in:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02983.html
>>
>> and the current hole-punching logic in build_crs() and build_ssdt() is
>> probably supposed to remedy exactly that problem -- however, for OVMF
>> they
>> don't work, because at the end of the PCI enumeration and resource
>> allocation, which cues the ACPI linker/loader client, the command
>> register
>> is clear.
>>
>> It has been suggested to implement the above "hack" more cleanly and
>> permanently. Unfortunately, we can't just disable the SHPC bar of
>> TYPE_PCI_BRIDGE_DEV based on a new property, because this device model
>> has
>> class-level ties to hotplug, and the SHPC bar (and the interrupt)
>> originate from there.
>>
>> Therefore implement a more basic bridge device type, to be used by PXB,
>> that has no SHPC / hotplug support at all, and consequently (see commit
>> c008ac0c), no need for an interrupt / MSI either.
>>
>> Suggested-by: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>> v2:
>> - Replaced the corresponding v1 patch with a new approach.
>> Instead of
>> considering the PXB's disabled SHPC BAR in the PXB's _CRS (and
>> correspondingly, punching it out of the main _CRS), this patch
>> creates
>> a separate device model that lacks the SHPC functionality
>> completely.
>>
>> I already know from IRC that Michael doesn't like this, but that's
>> okay: I'm formatting this with "-C35% --find-copies-harder", so
>> that
>> the differences are obvious against the clone origin
> Nice, I'll use that.
>
> , and Michael can
>> pinpoint the locations where and how I should use a new device
>> property instead, in the original device model.
>>
>> (That said, I did test this code, with both Windows and Linux, and
>> compared the dumped / decompiled SSDTs too.)
>>
>> hw/pci-bridge/Makefile.objs | 1 +
>> include/hw/pci/pci.h | 1 +
>> .../{pci_bridge_dev.c => pci_basic_bridge_dev.c} | 128
>> ++++++---------------
>> hw/pci-bridge/pci_expander_bridge.c | 2 +-
>> 4 files changed, 35 insertions(+), 97 deletions(-)
>> copy hw/pci-bridge/{pci_bridge_dev.c => pci_basic_bridge_dev.c} (35%)
>>
>> diff --git a/hw/pci-bridge/Makefile.objs b/hw/pci-bridge/Makefile.objs
>> index f2adfe3..bcfe753 100644
>> --- a/hw/pci-bridge/Makefile.objs
>> +++ b/hw/pci-bridge/Makefile.objs
>> @@ -1,4 +1,5 @@
>> common-obj-y += pci_bridge_dev.o
>> +common-obj-y += pci_basic_bridge_dev.o
>> common-obj-y += pci_expander_bridge.o
>> common-obj-$(CONFIG_XIO3130) += xio3130_upstream.o xio3130_downstream.o
>> common-obj-$(CONFIG_IOH3420) += ioh3420.o
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index d44bc84..619c31a 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -92,6 +92,7 @@
>> #define PCI_DEVICE_ID_REDHAT_SDHCI 0x0007
>> #define PCI_DEVICE_ID_REDHAT_PCIE_HOST 0x0008
>> #define PCI_DEVICE_ID_REDHAT_PXB 0x0009
>> +#define PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE 0x000A
>> #define PCI_DEVICE_ID_REDHAT_QXL 0x0100
>>
>> #define FMT_PCIBUS PRIx64
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>> b/hw/pci-bridge/pci_basic_bridge_dev.c
>> similarity index 35%
>> copy from hw/pci-bridge/pci_bridge_dev.c
>> copy to hw/pci-bridge/pci_basic_bridge_dev.c
>> index 36f73e1..d8bfb6c 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_basic_bridge_dev.c
>> @@ -1,7 +1,9 @@
>> /*
>> - * Standard PCI Bridge Device
>> + * PCI Bridge Device without interrupt and SHPC / hotplug support.
>> *
>> - * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin
>> <mst@redhat.com>
>> + * Copyright (c) 2011, 2015 Red Hat Inc.
>> + * Authors: Michael S. Tsirkin <mst@redhat.com>
>> + * Laszlo Ersek <lersek@redhat.com>
>> *
>> *
>> http://www.pcisig.com/specifications/conventional/pci_to_pci_bridge_architecture/
>>
>> *
>> @@ -21,158 +23,92 @@
>>
>> #include "hw/pci/pci_bridge.h"
>> #include "hw/pci/pci_ids.h"
>> -#include "hw/pci/msi.h"
>> -#include "hw/pci/shpc.h"
>> #include "hw/pci/slotid_cap.h"
>> #include "exec/memory.h"
>> #include "hw/pci/pci_bus.h"
>> -#include "hw/hotplug.h"
>>
>> -#define TYPE_PCI_BRIDGE_DEV "pci-bridge"
>> -#define PCI_BRIDGE_DEV(obj) \
>> - OBJECT_CHECK(PCIBridgeDev, (obj), TYPE_PCI_BRIDGE_DEV)
>> +#define TYPE_PCI_BASIC_BRIDGE_DEV "pci-basic-bridge"
>> +#define PCI_BASIC_BRIDGE_DEV(obj) \
>> + OBJECT_CHECK(PCIBasicBridgeDev, (obj), TYPE_PCI_BASIC_BRIDGE_DEV)
>>
>> -struct PCIBridgeDev {
>> +struct PCIBasicBridgeDev {
>> /*< private >*/
>> PCIBridge parent_obj;
>> /*< public >*/
>>
>> - MemoryRegion bar;
>> uint8_t chassis_nr;
>> -#define PCI_BRIDGE_DEV_F_MSI_REQ 0
>> - uint32_t flags;
>> };
>> -typedef struct PCIBridgeDev PCIBridgeDev;
>> +typedef struct PCIBasicBridgeDev PCIBasicBridgeDev;
>>
>> -static int pci_bridge_dev_initfn(PCIDevice *dev)
>> +static int pci_basic_bridge_dev_initfn(PCIDevice *dev)
>> {
>> - PCIBridge *br = PCI_BRIDGE(dev);
>> - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>> + PCIBasicBridgeDev *bridge_dev = PCI_BASIC_BRIDGE_DEV(dev);
>> int err;
>>
>> err = pci_bridge_initfn(dev, TYPE_PCI_BUS);
>> if (err) {
>> goto bridge_error;
>> }
>> - dev->config[PCI_INTERRUPT_PIN] = 0x1;
>> - memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
>> shpc_bar_size(dev));
>> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>> - if (err) {
>> - goto shpc_error;
>> - }
>> err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>> if (err) {
>> goto slotid_error;
>> }
>> - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
>> - msi_supported) {
>> - err = msi_init(dev, 0, 1, true, true);
>> - if (err < 0) {
>> - goto msi_error;
>> - }
>> - }
>> - /* TODO: spec recommends using 64 bit prefetcheable BAR.
>> - * Check whether that works well. */
>> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
>> - PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
>> return 0;
>> -msi_error:
>> - slotid_cap_cleanup(dev);
>> slotid_error:
>> - shpc_cleanup(dev, &bridge_dev->bar);
>> -shpc_error:
>> pci_bridge_exitfn(dev);
>> bridge_error:
>> return err;
>> }
>>
>> -static void pci_bridge_dev_exitfn(PCIDevice *dev)
>> +static void pci_basic_bridge_dev_exitfn(PCIDevice *dev)
>> {
>> - PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>> - if (msi_present(dev)) {
>> - msi_uninit(dev);
>> - }
>> slotid_cap_cleanup(dev);
>> - shpc_cleanup(dev, &bridge_dev->bar);
>> pci_bridge_exitfn(dev);
>> }
>>
>> -static void pci_bridge_dev_instance_finalize(Object *obj)
>> -{
>> - shpc_free(PCI_DEVICE(obj));
>> -}
>> -
>> -static void pci_bridge_dev_write_config(PCIDevice *d,
>> - uint32_t address, uint32_t
>> val, int len)
>> -{
>> - pci_bridge_write_config(d, address, val, len);
>> - if (msi_present(d)) {
>> - msi_write_config(d, address, val, len);
>> - }
>> - shpc_cap_write_config(d, address, val, len);
>> -}
>> -
>> -static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>> -{
>> - PCIDevice *dev = PCI_DEVICE(qdev);
>> -
>> - pci_bridge_reset(qdev);
>> - shpc_reset(dev);
>> -}
>> -
>> -static Property pci_bridge_dev_properties[] = {
>> +static Property pci_basic_bridge_dev_properties[] = {
>> /* Note: 0 is not a legal chassis number. */
>> - DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
>> - DEFINE_PROP_BIT("msi", PCIBridgeDev, flags,
>> PCI_BRIDGE_DEV_F_MSI_REQ, true),
>> + DEFINE_PROP_UINT8("chassis_nr", PCIBasicBridgeDev, chassis_nr, 0),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> -static const VMStateDescription pci_bridge_dev_vmstate = {
>> - .name = "pci_bridge",
>> +static const VMStateDescription pci_basic_bridge_dev_vmstate = {
>> + .name = "pci_basic_bridge",
>> .fields = (VMStateField[]) {
>> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
>> - SHPC_VMSTATE(shpc, PCIDevice),
>> VMSTATE_END_OF_LIST()
>> }
>> };
>>
>> -static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>> +static void pci_basic_bridge_dev_class_init(ObjectClass *klass, void
>> *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>
>> - k->init = pci_bridge_dev_initfn;
>> - k->exit = pci_bridge_dev_exitfn;
>> - k->config_write = pci_bridge_dev_write_config;
>> + k->init = pci_basic_bridge_dev_initfn;
>> + k->exit = pci_basic_bridge_dev_exitfn;
>> + k->config_write = pci_bridge_write_config;
>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>> - k->device_id = PCI_DEVICE_ID_REDHAT_BRIDGE;
>> + k->device_id = PCI_DEVICE_ID_REDHAT_BASIC_BRIDGE;
>> k->class_id = PCI_CLASS_BRIDGE_PCI;
>> k->is_bridge = 1,
>> - dc->desc = "Standard PCI Bridge";
>> - dc->reset = qdev_pci_bridge_dev_reset;
>> - dc->props = pci_bridge_dev_properties;
>> - dc->vmsd = &pci_bridge_dev_vmstate;
>> + dc->desc = "Basic PCI Bridge (no interrupt, no hotplug)";
>> + dc->reset = pci_bridge_reset;
>> + dc->props = pci_basic_bridge_dev_properties;
>> + dc->vmsd = &pci_basic_bridge_dev_vmstate;
>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> - hc->plug = shpc_device_hotplug_cb;
>> - hc->unplug_request = shpc_device_hot_unplug_request_cb;
>> }
>>
>> -static const TypeInfo pci_bridge_dev_info = {
>> - .name = TYPE_PCI_BRIDGE_DEV,
>> +static const TypeInfo pci_basic_bridge_dev_info = {
>> + .name = TYPE_PCI_BASIC_BRIDGE_DEV,
>> .parent = TYPE_PCI_BRIDGE,
>> - .instance_size = sizeof(PCIBridgeDev),
>> - .class_init = pci_bridge_dev_class_init,
>> - .instance_finalize = pci_bridge_dev_instance_finalize,
>> - .interfaces = (InterfaceInfo[]) {
>> - { TYPE_HOTPLUG_HANDLER },
>> - { }
>> - }
>> + .instance_size = sizeof(PCIBasicBridgeDev),
>> + .class_init = pci_basic_bridge_dev_class_init,
>> };
>>
>> -static void pci_bridge_dev_register(void)
>> +static void pci_basic_bridge_dev_register(void)
>> {
>> - type_register_static(&pci_bridge_dev_info);
>> + type_register_static(&pci_basic_bridge_dev_info);
>> }
>>
>> -type_init(pci_bridge_dev_register);
>> +type_init(pci_basic_bridge_dev_register);
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c
>> b/hw/pci-bridge/pci_expander_bridge.c
>> index ec2bb45..c7a085d 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -173,7 +173,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
>> bus->address_space_io = dev->bus->address_space_io;
>> bus->map_irq = pxb_map_irq_fn;
>>
>> - bds = qdev_create(BUS(bus), "pci-bridge");
>> + bds = qdev_create(BUS(bus), "pci-basic-bridge");
>> bds->id = dev_name;
>> qdev_prop_set_uint8(bds, "chassis_nr", pxb->bus_nr);
>>
>>
>
> In my opinion this is the cleanest solution.
> If Michael doesn't like the idea of a new "public" device,
> we can incorporate it (hide it) into the PXB device, the same
> I did with the PXB bus type. In this way it will be a PXB 'internal'.
> What do you think?
It would work for me, of course.
> In the meantime:
>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks!
NB, I'll mention that I *did* start implementing the "new flag"-based
approach, before opting for this avenue. Basically I added a new bit to
the "flags" bitmap, and tried to make everything conditional on that. I
named the two spots earlier where that idea broke down for me:
(1) migration
(2) hotplug interface (the shpc_device_hotplug_cb() and
shpc_device_hot_unplug_request_cb() callbacks, and the
TYPE_HOTPLUG_HANDLER interface).
Regarding the hotplug interface, that's problematic because these
settings are class-level, not instance level (and the property would be
instance level). But, if I understood Michael on IRC correctly, I could
simply add different (locally defined) hotplug callbacks that always
failed. Probably doable, but it's (a different kind of) overkill IMO --
the current patch is "overkill" because it adds a new device model,
while this "advertize hotplug, but always fail it" is overkill IMO
exactly because of that. :) Why advertize it if it always fails?
Regarding migration, I must not have done a good job describing my
concern, in
<http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342905>.
So let me try again:
The flag approach would go like this: in pci_bridge_dev_initfn(), I'd
check the new "shpc" bit in the flags bitmask (same as it is done now
for "msi"), and it if is set, I'd omit all of the following:
- the (dev->config[PCI_INTERRUPT_PIN] = 0x1) assignment
- the memory_region_init() call
- the shpc_init() call
- the msi_init() call
- the pci_register_bar() call
Straightforward, right? Accordingly, the error paths in the initfn, and
pci_bridge_dev_exitfn() would be updated in sync. Etc etc etc.
Now, the problem is this:
static const VMStateDescription pci_bridge_dev_vmstate = {
.name = "pci_bridge",
.fields = (VMStateField[]) {
VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
SHPC_VMSTATE(shpc, PCIDevice),
VMSTATE_END_OF_LIST()
}
};
I *cannot* make SHPC_VMSTATE() conditional on the new flag. It is always
there. I don't know what it does. What if it includes a callback of some
kind that *depends* on shpc_init()? If that's the case, then *outgoing*
migration with "shpc=off" might crash.
Therefore, I'd have to turn this SHPC_VMSTATE() vmstate field into a
subsection, and introduce an "is this needed" function for it, which
would of course key off of the "shpc" flag.
But that would break incoming migration *from* qemu-2.3! That's why in
sync with introducing the subsection, I'd have bump *both*
"minimum_version_id" and "version_id" in pci_bridge_dev_vmstate. Because
this way the v2.3 -> v2.4 migration would be cleanly rejected.
Of course, if SHPC_VMSTATE(), as-is, simply decays to a no-op, if
shpc_init() is omitted, then migration is not a question at all, under
Michael's proposal. But, I don't know if that's the case.
... I like this patch because it eliminates both questions (hotplug and
migration) at once.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer Laszlo Ersek
@ 2015-06-10 19:26 ` Marcel Apfelbaum
2015-06-10 19:44 ` Laszlo Ersek
2015-06-10 19:26 ` Marcel Apfelbaum
1 sibling, 1 reply; 15+ messages in thread
From: Marcel Apfelbaum @ 2015-06-10 19:26 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +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
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.
>
> 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 = <null>
>> 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 <armbru@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> 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.
> +}
> +
> +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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer Laszlo Ersek
2015-06-10 19:26 ` Marcel Apfelbaum
@ 2015-06-10 19:26 ` Marcel Apfelbaum
2015-06-10 19:34 ` Laszlo Ersek
1 sibling, 1 reply; 15+ messages in thread
From: Marcel Apfelbaum @ 2015-06-10 19:26 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +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 = <null>
>> 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 <armbru@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> 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)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 19:26 ` Marcel Apfelbaum
@ 2015-06-10 19:34 ` Laszlo Ersek
2015-06-10 20:11 ` Laszlo Ersek
2015-06-11 4:43 ` Marcel Apfelbaum
0 siblings, 2 replies; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 19:34 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +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
>>
>> 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 :) ?
No, not yet; I'll have to write additional OVMF code for that; but the
syntax is OK, and the information is there, so I'm fairly sure I can
write that code. :)
The SeaBIOS side I'll probably leave to you... /me ducks :)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 19:26 ` Marcel Apfelbaum
@ 2015-06-10 19:44 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 19:44 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +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 = <null>
>>> 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 <armbru@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> 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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 19:34 ` Laszlo Ersek
@ 2015-06-10 20:11 ` Laszlo Ersek
2015-06-10 21:29 ` Laszlo Ersek
2015-06-11 4:43 ` Marcel Apfelbaum
1 sibling, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 20:11 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: Markus Armbruster, Michael S. Tsirkin
On 06/10/15 21:34, Laszlo Ersek wrote:
> 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
>>>
>>> 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 :) ?
>
> No, not yet; I'll have to write additional OVMF code for that; but the
> syntax is OK, and the information is there, so I'm fairly sure I can
> write that code. :)
On a second thought -- I *will* write the OVMF code for this now, or
tomorrow, because the syntax is *still* a little bit off. I think we'll
need:
/extra-pci-roots@0/pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
^^
The part I marked (ie. the @ and the UnitAddress after it) are
mandatory. I'll just hardcode the "@0" string there in QEMU, as written
above.
Thanks
Laszlo
> The SeaBIOS side I'll probably leave to you... /me ducks :)
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 20:11 ` Laszlo Ersek
@ 2015-06-10 21:29 ` Laszlo Ersek
2015-06-11 4:44 ` Marcel Apfelbaum
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2015-06-10 21:29 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: Markus Armbruster, Michael S. Tsirkin
On 06/10/15 22:11, Laszlo Ersek wrote:
> On 06/10/15 21:34, Laszlo Ersek wrote:
>> On 06/10/15 21:26, Marcel Apfelbaum wrote:
>>> BTW, did you try to boot from it :) ?
>>
>> No, not yet; I'll have to write additional OVMF code for that; but the
>> syntax is OK, and the information is there, so I'm fairly sure I can
>> write that code. :)
>
> On a second thought -- I *will* write the OVMF code for this now, or
> tomorrow, because the syntax is *still* a little bit off. I think we'll
> need:
>
> /extra-pci-roots@0/pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
> ^^
>
> The part I marked (ie. the @ and the UnitAddress after it) are
> mandatory. I'll just hardcode the "@0" string there in QEMU, as written
> above.
Yes, that works; I can PXE-boot with OVMF from a virtio-net-pci NIC that
is located at the above OFW devpath.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 19:34 ` Laszlo Ersek
2015-06-10 20:11 ` Laszlo Ersek
@ 2015-06-11 4:43 ` Marcel Apfelbaum
1 sibling, 0 replies; 15+ messages in thread
From: Marcel Apfelbaum @ 2015-06-11 4:43 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: Markus Armbruster, Michael S. Tsirkin
On 06/10/2015 10:34 PM, Laszlo Ersek wrote:
> 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
>>>
>>> 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 :) ?
>
> No, not yet; I'll have to write additional OVMF code for that; but the
> syntax is OK, and the information is there, so I'm fairly sure I can
> write that code. :)
>
> The SeaBIOS side I'll probably leave to you... /me ducks :)
I'll take it, sure, you are doing all the work anyway :)
Thanks,
Marcel
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer
2015-06-10 21:29 ` Laszlo Ersek
@ 2015-06-11 4:44 ` Marcel Apfelbaum
0 siblings, 0 replies; 15+ messages in thread
From: Marcel Apfelbaum @ 2015-06-11 4:44 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: Markus Armbruster, Michael S. Tsirkin
On 06/11/2015 12:29 AM, Laszlo Ersek wrote:
> On 06/10/15 22:11, Laszlo Ersek wrote:
>> On 06/10/15 21:34, Laszlo Ersek wrote:
>>> On 06/10/15 21:26, Marcel Apfelbaum wrote:
>
>>>> BTW, did you try to boot from it :) ?
>>>
>>> No, not yet; I'll have to write additional OVMF code for that; but the
>>> syntax is OK, and the information is there, so I'm fairly sure I can
>>> write that code. :)
>>
>> On a second thought -- I *will* write the OVMF code for this now, or
>> tomorrow, because the syntax is *still* a little bit off. I think we'll
>> need:
>>
>> /extra-pci-roots@0/pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
>> ^^
>>
>> The part I marked (ie. the @ and the UnitAddress after it) are
>> mandatory. I'll just hardcode the "@0" string there in QEMU, as written
>> above.
>
> Yes, that works; I can PXE-boot with OVMF from a virtio-net-pci NIC that
> is located at the above OFW devpath.
Good!
I'll work now on Seabios
.
Thanks,
Marcel
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-06-11 4:44 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-10 17:07 [Qemu-devel] [PATCH v2 0/4] PXB tweaks and fixes Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 1/4] i386/acpi-build: more traditional _UID and _HID for PXB root buses Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 2/4] i386/acpi-build: fix PXB workarounds for unsupported BIOSes Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 3/4] hw/pci-bridge: create interrupt-less, hotplug-less bridge for PXB Laszlo Ersek
2015-06-10 18:22 ` Marcel Apfelbaum
2015-06-10 19:04 ` Laszlo Ersek
2015-06-10 17:07 ` [Qemu-devel] [PATCH v2 4/4] hw/pci-bridge: push down PXB in qtree in order to format PXB bus numer Laszlo Ersek
2015-06-10 19:26 ` Marcel Apfelbaum
2015-06-10 19:44 ` Laszlo Ersek
2015-06-10 19:26 ` Marcel Apfelbaum
2015-06-10 19:34 ` Laszlo Ersek
2015-06-10 20:11 ` Laszlo Ersek
2015-06-10 21:29 ` Laszlo Ersek
2015-06-11 4:44 ` Marcel Apfelbaum
2015-06-11 4:43 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).