* [Qemu-devel] [PATCH v6 1/7] migration: introduce VMSTATE_BUFFER_UNSAFE_INFO_TEST()
2015-06-17 12:44 [Qemu-devel] [PATCH v6 0/7] PXB changes Laszlo Ersek
@ 2015-06-17 12:44 ` Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 2/7] hw/pci-bridge: expose _test parameter in SHPC_VMSTATE() Laszlo Ersek
` (5 subsequent siblings)
6 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 12:44 UTC (permalink / raw)
To: qemu-devel, lersek
Cc: Amit Shah, Marcel Apfelbaum, Michael S. Tsirkin, Juan Quintela
There is no _TEST() variant of VMSTATE_BUFFER_UNSAFE_INFO() yet, but we'll
soon need it. Introduce it and rebase the original
VMSTATE_BUFFER_UNSAFE_INFO() on top.
The parameter order of the new function-like macro follows that of
VMSTATE_SINGLE_TEST(): "_test" is introduced between "_state" and
"_version".
Cc: Juan Quintela <quintela@redhat.com>
Cc: Amit Shah <amit.shah@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v6:
- new in v6
include/migration/vmstate.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 7153b1e..0695d7c 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -500,9 +500,10 @@ extern const VMStateInfo vmstate_info_bitmap;
.start = (_start), \
}
-#define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, _size) { \
+#define VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _state, _test, _version, _info, _size) { \
.name = (stringify(_field)), \
.version_id = (_version), \
+ .field_exists = (_test), \
.size = (_size), \
.info = &(_info), \
.flags = VMS_BUFFER, \
@@ -562,6 +563,10 @@ extern const VMStateInfo vmstate_info_bitmap;
VMSTATE_STRUCT_ARRAY_TEST(_field, _state, _num, NULL, _version, \
_vmsd, _type)
+#define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, _size) \
+ VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _state, NULL, _version, _info, \
+ _size)
+
#define VMSTATE_BOOL_V(_f, _s, _v) \
VMSTATE_SINGLE(_f, _s, _v, vmstate_info_bool, bool)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v6 2/7] hw/pci-bridge: expose _test parameter in SHPC_VMSTATE()
2015-06-17 12:44 [Qemu-devel] [PATCH v6 0/7] PXB changes Laszlo Ersek
2015-06-17 12:44 ` [Qemu-devel] [PATCH v6 1/7] migration: introduce VMSTATE_BUFFER_UNSAFE_INFO_TEST() Laszlo Ersek
@ 2015-06-17 12:45 ` Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property Laszlo Ersek
` (4 subsequent siblings)
6 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 12:45 UTC (permalink / raw)
To: qemu-devel, lersek; +Cc: Marcel Apfelbaum, Michael S. Tsirkin
Change the signature of the function-like macro SHPC_VMSTATE(), so that we
can produce and expect this field conditionally in the migration stream,
starting with the next patch.
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v6:
- new in v6 (replaces "hw/pci-bridge: create interrupt-less,
hotplug-less bridge for PXB" from v5) [Michael]
include/hw/pci/shpc.h | 5 +++--
hw/pci-bridge/pci_bridge_dev.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 9bbea39..14015af 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -51,7 +51,8 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp);
extern VMStateInfo shpc_vmstate_info;
-#define SHPC_VMSTATE(_field, _type) \
- VMSTATE_BUFFER_UNSAFE_INFO(_field, _type, 0, shpc_vmstate_info, 0)
+#define SHPC_VMSTATE(_field, _type, _test) \
+ VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _type, _test, 0, \
+ shpc_vmstate_info, 0)
#endif
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 36f73e1..d697c7b 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -131,7 +131,7 @@ static const VMStateDescription pci_bridge_dev_vmstate = {
.name = "pci_bridge",
.fields = (VMStateField[]) {
VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
- SHPC_VMSTATE(shpc, PCIDevice),
+ SHPC_VMSTATE(shpc, PCIDevice, NULL),
VMSTATE_END_OF_LIST()
}
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
2015-06-17 12:44 [Qemu-devel] [PATCH v6 0/7] PXB changes Laszlo Ersek
2015-06-17 12:44 ` [Qemu-devel] [PATCH v6 1/7] migration: introduce VMSTATE_BUFFER_UNSAFE_INFO_TEST() Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 2/7] hw/pci-bridge: expose _test parameter in SHPC_VMSTATE() Laszlo Ersek
@ 2015-06-17 12:45 ` Laszlo Ersek
2015-06-17 13:42 ` Michael S. Tsirkin
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 4/7] hw/pci-bridge: disable hotplug in PXB Laszlo Ersek
` (3 subsequent siblings)
6 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 12:45 UTC (permalink / raw)
To: qemu-devel, lersek; +Cc: Marcel Apfelbaum, Michael S. Tsirkin
In the PCI expander bridge, we will want to disable those features of
pci-bridge that relate to SHPC (standard hotplug controller):
- SHPC bar and underlying MemoryRegion
- interrupt (INTx or MSI)
- effective hotplug callbacks
- other SHPC hooks (initialization, cleanup, migration etc)
Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
the above if the bit is explicitly cleared.
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v6:
- new in v6 (replaces "hw/pci-bridge: create interrupt-less,
hotplug-less bridge for PXB" from v5) [Michael]
- I tested migration-to-file and migration-from-file, with a single PXB
behind which a virtio-scsi HBA lived
- No idea how to test the hotplug callbacks. I tried the device_add and
device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
monitor reports no error at all; the callbacks don't seem to be
reached.
- Retested with windows 2012; it works
hw/pci-bridge/pci_bridge_dev.c | 90 ++++++++++++++++++++++++++++++++++++------
1 file changed, 78 insertions(+), 12 deletions(-)
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index d697c7b..a0a1ce9 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -40,10 +40,16 @@ struct PCIBridgeDev {
MemoryRegion bar;
uint8_t chassis_nr;
#define PCI_BRIDGE_DEV_F_MSI_REQ 0
+#define PCI_BRIDGE_DEV_F_HOTPLUG 1
uint32_t flags;
};
typedef struct PCIBridgeDev PCIBridgeDev;
+static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
+{
+ return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
+}
+
static int pci_bridge_dev_initfn(PCIDevice *dev)
{
PCIBridge *br = PCI_BRIDGE(dev);
@@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
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;
+
+ if (hotplug_enabled(bridge_dev)) {
+ 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 (!hotplug_enabled(bridge_dev)) {
+ return 0;
+ }
+
if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
msi_supported) {
err = msi_init(dev, 0, 1, true, true);
@@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
msi_error:
slotid_cap_cleanup(dev);
slotid_error:
- shpc_cleanup(dev, &bridge_dev->bar);
+ if (hotplug_enabled(bridge_dev)) {
+ shpc_cleanup(dev, &bridge_dev->bar);
+ }
shpc_error:
pci_bridge_exitfn(dev);
bridge_error:
@@ -89,53 +107,101 @@ bridge_error:
static void pci_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);
+ if (hotplug_enabled(bridge_dev)) {
+ shpc_cleanup(dev, &bridge_dev->bar);
+ }
pci_bridge_exitfn(dev);
}
static void pci_bridge_dev_instance_finalize(Object *obj)
{
+ /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
shpc_free(PCI_DEVICE(obj));
}
static void pci_bridge_dev_write_config(PCIDevice *d,
uint32_t address, uint32_t val, int len)
{
+ PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
+
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);
+ if (hotplug_enabled(bridge_dev)) {
+ shpc_cap_write_config(d, address, val, len);
+ }
}
static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
{
PCIDevice *dev = PCI_DEVICE(qdev);
+ PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
pci_bridge_reset(qdev);
- shpc_reset(dev);
+ if (hotplug_enabled(bridge_dev)) {
+ shpc_reset(dev);
+ }
}
static Property pci_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_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
+ true),
DEFINE_PROP_END_OF_LIST(),
};
+static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
+{
+ PCIDevice *dev = opaque;
+ PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
+
+ return hotplug_enabled(bridge_dev);
+}
+
static const VMStateDescription pci_bridge_dev_vmstate = {
.name = "pci_bridge",
.fields = (VMStateField[]) {
VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
- SHPC_VMSTATE(shpc, PCIDevice, NULL),
+ SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
VMSTATE_END_OF_LIST()
}
};
+static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
+
+ if (!hotplug_enabled(bridge_dev)) {
+ error_setg(errp, "standard hotplug controller has been disabled for "
+ "this %s", TYPE_PCI_BRIDGE_DEV);
+ return;
+ }
+ shpc_device_hotplug_cb(hotplug_dev, dev, errp);
+}
+
+static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
+ DeviceState *dev,
+ Error **errp)
+{
+ PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
+
+ if (!hotplug_enabled(bridge_dev)) {
+ error_setg(errp, "standard hotplug controller has been disabled for "
+ "this %s", TYPE_PCI_BRIDGE_DEV);
+ return;
+ }
+ shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
+}
+
static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
dc->props = pci_bridge_dev_properties;
dc->vmsd = &pci_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;
+ hc->plug = pci_bridge_dev_hotplug_cb;
+ hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
}
static const TypeInfo pci_bridge_dev_info = {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property Laszlo Ersek
@ 2015-06-17 13:42 ` Michael S. Tsirkin
2015-06-17 13:55 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 13:42 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Marcel Apfelbaum, qemu-devel
On Wed, Jun 17, 2015 at 02:45:01PM +0200, Laszlo Ersek wrote:
> In the PCI expander bridge, we will want to disable those features of
> pci-bridge that relate to SHPC (standard hotplug controller):
>
> - SHPC bar and underlying MemoryRegion
> - interrupt (INTx or MSI)
> - effective hotplug callbacks
> - other SHPC hooks (initialization, cleanup, migration etc)
>
> Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
> the above if the bit is explicitly cleared.
>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
I think I would prefer naming the property "shpc".
In particular ACPI hotplug still works fine.
> ---
>
> Notes:
> v6:
> - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
> hotplug-less bridge for PXB" from v5) [Michael]
> - I tested migration-to-file and migration-from-file, with a single PXB
> behind which a virtio-scsi HBA lived
> - No idea how to test the hotplug callbacks. I tried the device_add and
> device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
> monitor reports no error at all; the callbacks don't seem to be
> reached.
> - Retested with windows 2012; it works
>
> hw/pci-bridge/pci_bridge_dev.c | 90 ++++++++++++++++++++++++++++++++++++------
> 1 file changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index d697c7b..a0a1ce9 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -40,10 +40,16 @@ struct PCIBridgeDev {
> MemoryRegion bar;
> uint8_t chassis_nr;
> #define PCI_BRIDGE_DEV_F_MSI_REQ 0
> +#define PCI_BRIDGE_DEV_F_HOTPLUG 1
> uint32_t flags;
> };
> typedef struct PCIBridgeDev PCIBridgeDev;
>
> +static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
> +{
> + return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
> +}
> +
> static int pci_bridge_dev_initfn(PCIDevice *dev)
> {
> PCIBridge *br = PCI_BRIDGE(dev);
> @@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> 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;
> +
> + if (hotplug_enabled(bridge_dev)) {
> + 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 (!hotplug_enabled(bridge_dev)) {
> + return 0;
> + }
> +
> if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> msi_supported) {
> err = msi_init(dev, 0, 1, true, true);
> @@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> msi_error:
> slotid_cap_cleanup(dev);
> slotid_error:
> - shpc_cleanup(dev, &bridge_dev->bar);
> + if (hotplug_enabled(bridge_dev)) {
> + shpc_cleanup(dev, &bridge_dev->bar);
> + }
> shpc_error:
> pci_bridge_exitfn(dev);
> bridge_error:
> @@ -89,53 +107,101 @@ bridge_error:
> static void pci_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);
> + if (hotplug_enabled(bridge_dev)) {
> + shpc_cleanup(dev, &bridge_dev->bar);
> + }
> pci_bridge_exitfn(dev);
> }
>
> static void pci_bridge_dev_instance_finalize(Object *obj)
> {
> + /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
> shpc_free(PCI_DEVICE(obj));
> }
>
> static void pci_bridge_dev_write_config(PCIDevice *d,
> uint32_t address, uint32_t val, int len)
> {
> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
> +
> 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);
> + if (hotplug_enabled(bridge_dev)) {
> + shpc_cap_write_config(d, address, val, len);
> + }
> }
>
> static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
> {
> PCIDevice *dev = PCI_DEVICE(qdev);
> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>
> pci_bridge_reset(qdev);
> - shpc_reset(dev);
> + if (hotplug_enabled(bridge_dev)) {
> + shpc_reset(dev);
> + }
> }
>
> static Property pci_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_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
> + true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
> +{
> + PCIDevice *dev = opaque;
> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> +
> + return hotplug_enabled(bridge_dev);
> +}
> +
> static const VMStateDescription pci_bridge_dev_vmstate = {
> .name = "pci_bridge",
> .fields = (VMStateField[]) {
> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> - SHPC_VMSTATE(shpc, PCIDevice, NULL),
> + SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
> VMSTATE_END_OF_LIST()
> }
> };
>
> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
> +
> + if (!hotplug_enabled(bridge_dev)) {
> + error_setg(errp, "standard hotplug controller has been disabled for "
> + "this %s", TYPE_PCI_BRIDGE_DEV);
> + return;
> + }
> + shpc_device_hotplug_cb(hotplug_dev, dev, errp);
> +}
> +
> +static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> + DeviceState *dev,
> + Error **errp)
> +{
> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
> +
> + if (!hotplug_enabled(bridge_dev)) {
> + error_setg(errp, "standard hotplug controller has been disabled for "
> + "this %s", TYPE_PCI_BRIDGE_DEV);
> + return;
> + }
> + shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
> +}
> +
> static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
> dc->props = pci_bridge_dev_properties;
> dc->vmsd = &pci_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;
> + hc->plug = pci_bridge_dev_hotplug_cb;
> + hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
> }
>
> static const TypeInfo pci_bridge_dev_info = {
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
2015-06-17 13:42 ` Michael S. Tsirkin
@ 2015-06-17 13:55 ` Laszlo Ersek
2015-06-17 14:02 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 13:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, qemu-devel
On 06/17/15 15:42, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 02:45:01PM +0200, Laszlo Ersek wrote:
>> In the PCI expander bridge, we will want to disable those features of
>> pci-bridge that relate to SHPC (standard hotplug controller):
>>
>> - SHPC bar and underlying MemoryRegion
>> - interrupt (INTx or MSI)
>> - effective hotplug callbacks
>> - other SHPC hooks (initialization, cleanup, migration etc)
>>
>> Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
>> the above if the bit is explicitly cleared.
>>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
>
> I think I would prefer naming the property "shpc".
> In particular ACPI hotplug still works fine.
Will do.
Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
what would be your preference?
Should I also rename the helper function hotplug_enabled() to
shpc_enabled()?
Thanks
Laszlo
>
>
>> ---
>>
>> Notes:
>> v6:
>> - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
>> hotplug-less bridge for PXB" from v5) [Michael]
>> - I tested migration-to-file and migration-from-file, with a single PXB
>> behind which a virtio-scsi HBA lived
>> - No idea how to test the hotplug callbacks. I tried the device_add and
>> device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
>> monitor reports no error at all; the callbacks don't seem to be
>> reached.
>> - Retested with windows 2012; it works
>>
>> hw/pci-bridge/pci_bridge_dev.c | 90 ++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 78 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>> index d697c7b..a0a1ce9 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -40,10 +40,16 @@ struct PCIBridgeDev {
>> MemoryRegion bar;
>> uint8_t chassis_nr;
>> #define PCI_BRIDGE_DEV_F_MSI_REQ 0
>> +#define PCI_BRIDGE_DEV_F_HOTPLUG 1
>> uint32_t flags;
>> };
>> typedef struct PCIBridgeDev PCIBridgeDev;
>>
>> +static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
>> +{
>> + return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
>> +}
>> +
>> static int pci_bridge_dev_initfn(PCIDevice *dev)
>> {
>> PCIBridge *br = PCI_BRIDGE(dev);
>> @@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> 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;
>> +
>> + if (hotplug_enabled(bridge_dev)) {
>> + 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 (!hotplug_enabled(bridge_dev)) {
>> + return 0;
>> + }
>> +
>> if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
>> msi_supported) {
>> err = msi_init(dev, 0, 1, true, true);
>> @@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> msi_error:
>> slotid_cap_cleanup(dev);
>> slotid_error:
>> - shpc_cleanup(dev, &bridge_dev->bar);
>> + if (hotplug_enabled(bridge_dev)) {
>> + shpc_cleanup(dev, &bridge_dev->bar);
>> + }
>> shpc_error:
>> pci_bridge_exitfn(dev);
>> bridge_error:
>> @@ -89,53 +107,101 @@ bridge_error:
>> static void pci_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);
>> + if (hotplug_enabled(bridge_dev)) {
>> + shpc_cleanup(dev, &bridge_dev->bar);
>> + }
>> pci_bridge_exitfn(dev);
>> }
>>
>> static void pci_bridge_dev_instance_finalize(Object *obj)
>> {
>> + /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
>> shpc_free(PCI_DEVICE(obj));
>> }
>>
>> static void pci_bridge_dev_write_config(PCIDevice *d,
>> uint32_t address, uint32_t val, int len)
>> {
>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
>> +
>> 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);
>> + if (hotplug_enabled(bridge_dev)) {
>> + shpc_cap_write_config(d, address, val, len);
>> + }
>> }
>>
>> static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>> {
>> PCIDevice *dev = PCI_DEVICE(qdev);
>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>
>> pci_bridge_reset(qdev);
>> - shpc_reset(dev);
>> + if (hotplug_enabled(bridge_dev)) {
>> + shpc_reset(dev);
>> + }
>> }
>>
>> static Property pci_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_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
>> + true),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> +static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
>> +{
>> + PCIDevice *dev = opaque;
>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>> +
>> + return hotplug_enabled(bridge_dev);
>> +}
>> +
>> static const VMStateDescription pci_bridge_dev_vmstate = {
>> .name = "pci_bridge",
>> .fields = (VMStateField[]) {
>> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
>> - SHPC_VMSTATE(shpc, PCIDevice, NULL),
>> + SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
>> VMSTATE_END_OF_LIST()
>> }
>> };
>>
>> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
>> + DeviceState *dev, Error **errp)
>> +{
>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>> +
>> + if (!hotplug_enabled(bridge_dev)) {
>> + error_setg(errp, "standard hotplug controller has been disabled for "
>> + "this %s", TYPE_PCI_BRIDGE_DEV);
>> + return;
>> + }
>> + shpc_device_hotplug_cb(hotplug_dev, dev, errp);
>> +}
>> +
>> +static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>> + DeviceState *dev,
>> + Error **errp)
>> +{
>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>> +
>> + if (!hotplug_enabled(bridge_dev)) {
>> + error_setg(errp, "standard hotplug controller has been disabled for "
>> + "this %s", TYPE_PCI_BRIDGE_DEV);
>> + return;
>> + }
>> + shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
>> +}
>> +
>> static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>> dc->props = pci_bridge_dev_properties;
>> dc->vmsd = &pci_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;
>> + hc->plug = pci_bridge_dev_hotplug_cb;
>> + hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
>> }
>>
>> static const TypeInfo pci_bridge_dev_info = {
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
2015-06-17 13:55 ` Laszlo Ersek
@ 2015-06-17 14:02 ` Michael S. Tsirkin
2015-06-17 14:15 ` Laszlo Ersek
2015-06-18 13:47 ` Paolo Bonzini
0 siblings, 2 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 14:02 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Marcel Apfelbaum, qemu-devel
On Wed, Jun 17, 2015 at 03:55:32PM +0200, Laszlo Ersek wrote:
> On 06/17/15 15:42, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2015 at 02:45:01PM +0200, Laszlo Ersek wrote:
> >> In the PCI expander bridge, we will want to disable those features of
> >> pci-bridge that relate to SHPC (standard hotplug controller):
> >>
> >> - SHPC bar and underlying MemoryRegion
> >> - interrupt (INTx or MSI)
> >> - effective hotplug callbacks
> >> - other SHPC hooks (initialization, cleanup, migration etc)
> >>
> >> Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
> >> the above if the bit is explicitly cleared.
> >>
> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Marcel Apfelbaum <marcel@redhat.com>
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >
> >
> > I think I would prefer naming the property "shpc".
> > In particular ACPI hotplug still works fine.
>
> Will do.
>
> Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
> what would be your preference?
PCI_BRIDGE_DEV_F_SHPC_REQ
Also add macro for "shpc" property string to avoid duplication.
Also clear msi explicitly - no need for that to depend on shpc.
> Should I also rename the helper function hotplug_enabled() to
> shpc_enabled()?
It should be tested only once - before shpc is initialized.
All other users should test QEMU_PCI_CAP_SHPC - if you like,
using a helper shpc_present.
>
> Thanks
> Laszlo
>
> >
> >
> >> ---
> >>
> >> Notes:
> >> v6:
> >> - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
> >> hotplug-less bridge for PXB" from v5) [Michael]
> >> - I tested migration-to-file and migration-from-file, with a single PXB
> >> behind which a virtio-scsi HBA lived
> >> - No idea how to test the hotplug callbacks. I tried the device_add and
> >> device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
> >> monitor reports no error at all; the callbacks don't seem to be
> >> reached.
> >> - Retested with windows 2012; it works
> >>
> >> hw/pci-bridge/pci_bridge_dev.c | 90 ++++++++++++++++++++++++++++++++++++------
> >> 1 file changed, 78 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> >> index d697c7b..a0a1ce9 100644
> >> --- a/hw/pci-bridge/pci_bridge_dev.c
> >> +++ b/hw/pci-bridge/pci_bridge_dev.c
> >> @@ -40,10 +40,16 @@ struct PCIBridgeDev {
> >> MemoryRegion bar;
> >> uint8_t chassis_nr;
> >> #define PCI_BRIDGE_DEV_F_MSI_REQ 0
> >> +#define PCI_BRIDGE_DEV_F_HOTPLUG 1
> >> uint32_t flags;
> >> };
> >> typedef struct PCIBridgeDev PCIBridgeDev;
> >>
> >> +static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
> >> +{
> >> + return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
> >> +}
> >> +
> >> static int pci_bridge_dev_initfn(PCIDevice *dev)
> >> {
> >> PCIBridge *br = PCI_BRIDGE(dev);
> >> @@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> >> 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;
> >> +
> >> + if (hotplug_enabled(bridge_dev)) {
> >> + 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 (!hotplug_enabled(bridge_dev)) {
> >> + return 0;
> >> + }
> >> +
> >> if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> >> msi_supported) {
> >> err = msi_init(dev, 0, 1, true, true);
> >> @@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> >> msi_error:
> >> slotid_cap_cleanup(dev);
> >> slotid_error:
> >> - shpc_cleanup(dev, &bridge_dev->bar);
> >> + if (hotplug_enabled(bridge_dev)) {
> >> + shpc_cleanup(dev, &bridge_dev->bar);
> >> + }
> >> shpc_error:
> >> pci_bridge_exitfn(dev);
> >> bridge_error:
> >> @@ -89,53 +107,101 @@ bridge_error:
> >> static void pci_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);
> >> + if (hotplug_enabled(bridge_dev)) {
> >> + shpc_cleanup(dev, &bridge_dev->bar);
> >> + }
> >> pci_bridge_exitfn(dev);
> >> }
> >>
> >> static void pci_bridge_dev_instance_finalize(Object *obj)
> >> {
> >> + /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
> >> shpc_free(PCI_DEVICE(obj));
> >> }
> >>
> >> static void pci_bridge_dev_write_config(PCIDevice *d,
> >> uint32_t address, uint32_t val, int len)
> >> {
> >> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
> >> +
> >> 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);
> >> + if (hotplug_enabled(bridge_dev)) {
> >> + shpc_cap_write_config(d, address, val, len);
> >> + }
> >> }
> >>
> >> static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
> >> {
> >> PCIDevice *dev = PCI_DEVICE(qdev);
> >> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> >>
> >> pci_bridge_reset(qdev);
> >> - shpc_reset(dev);
> >> + if (hotplug_enabled(bridge_dev)) {
> >> + shpc_reset(dev);
> >> + }
> >> }
> >>
> >> static Property pci_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_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
> >> + true),
> >> DEFINE_PROP_END_OF_LIST(),
> >> };
> >>
> >> +static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
> >> +{
> >> + PCIDevice *dev = opaque;
> >> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> >> +
> >> + return hotplug_enabled(bridge_dev);
> >> +}
> >> +
> >> static const VMStateDescription pci_bridge_dev_vmstate = {
> >> .name = "pci_bridge",
> >> .fields = (VMStateField[]) {
> >> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> >> - SHPC_VMSTATE(shpc, PCIDevice, NULL),
> >> + SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
> >> VMSTATE_END_OF_LIST()
> >> }
> >> };
> >>
> >> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
> >> + DeviceState *dev, Error **errp)
> >> +{
> >> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
> >> +
> >> + if (!hotplug_enabled(bridge_dev)) {
> >> + error_setg(errp, "standard hotplug controller has been disabled for "
> >> + "this %s", TYPE_PCI_BRIDGE_DEV);
> >> + return;
> >> + }
> >> + shpc_device_hotplug_cb(hotplug_dev, dev, errp);
> >> +}
> >> +
> >> +static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> >> + DeviceState *dev,
> >> + Error **errp)
> >> +{
> >> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
> >> +
> >> + if (!hotplug_enabled(bridge_dev)) {
> >> + error_setg(errp, "standard hotplug controller has been disabled for "
> >> + "this %s", TYPE_PCI_BRIDGE_DEV);
> >> + return;
> >> + }
> >> + shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
> >> +}
> >> +
> >> static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
> >> {
> >> DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
> >> dc->props = pci_bridge_dev_properties;
> >> dc->vmsd = &pci_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;
> >> + hc->plug = pci_bridge_dev_hotplug_cb;
> >> + hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
> >> }
> >>
> >> static const TypeInfo pci_bridge_dev_info = {
> >> --
> >> 1.8.3.1
> >>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
2015-06-17 14:02 ` Michael S. Tsirkin
@ 2015-06-17 14:15 ` Laszlo Ersek
2015-06-17 19:54 ` Marcel Apfelbaum
2015-06-18 13:47 ` Paolo Bonzini
1 sibling, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 14:15 UTC (permalink / raw)
To: Michael S. Tsirkin, Marcel Apfelbaum; +Cc: qemu-devel
On 06/17/15 16:02, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 03:55:32PM +0200, Laszlo Ersek wrote:
>> On 06/17/15 15:42, Michael S. Tsirkin wrote:
>>> On Wed, Jun 17, 2015 at 02:45:01PM +0200, Laszlo Ersek wrote:
>>>> In the PCI expander bridge, we will want to disable those features of
>>>> pci-bridge that relate to SHPC (standard hotplug controller):
>>>>
>>>> - SHPC bar and underlying MemoryRegion
>>>> - interrupt (INTx or MSI)
>>>> - effective hotplug callbacks
>>>> - other SHPC hooks (initialization, cleanup, migration etc)
>>>>
>>>> Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
>>>> the above if the bit is explicitly cleared.
>>>>
>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>>
>>> I think I would prefer naming the property "shpc".
>>> In particular ACPI hotplug still works fine.
>>
>> Will do.
>>
>> Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
>> what would be your preference?
>
> PCI_BRIDGE_DEV_F_SHPC_REQ
>
> Also add macro for "shpc" property string to avoid duplication.
> Also clear msi explicitly - no need for that to depend on shpc.
>
>> Should I also rename the helper function hotplug_enabled() to
>> shpc_enabled()?
>
> It should be tested only once - before shpc is initialized.
> All other users should test QEMU_PCI_CAP_SHPC - if you like,
> using a helper shpc_present.
I'll make one attempt (in v7) to get this right. If I miss it, I'll ask
Marcel to please take over the series.
Thanks
Laszlo
>
>>
>> Thanks
>> Laszlo
>>
>>>
>>>
>>>> ---
>>>>
>>>> Notes:
>>>> v6:
>>>> - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
>>>> hotplug-less bridge for PXB" from v5) [Michael]
>>>> - I tested migration-to-file and migration-from-file, with a single PXB
>>>> behind which a virtio-scsi HBA lived
>>>> - No idea how to test the hotplug callbacks. I tried the device_add and
>>>> device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
>>>> monitor reports no error at all; the callbacks don't seem to be
>>>> reached.
>>>> - Retested with windows 2012; it works
>>>>
>>>> hw/pci-bridge/pci_bridge_dev.c | 90 ++++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 78 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>>> index d697c7b..a0a1ce9 100644
>>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>>> @@ -40,10 +40,16 @@ struct PCIBridgeDev {
>>>> MemoryRegion bar;
>>>> uint8_t chassis_nr;
>>>> #define PCI_BRIDGE_DEV_F_MSI_REQ 0
>>>> +#define PCI_BRIDGE_DEV_F_HOTPLUG 1
>>>> uint32_t flags;
>>>> };
>>>> typedef struct PCIBridgeDev PCIBridgeDev;
>>>>
>>>> +static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
>>>> +{
>>>> + return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
>>>> +}
>>>> +
>>>> static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>> {
>>>> PCIBridge *br = PCI_BRIDGE(dev);
>>>> @@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>> 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;
>>>> +
>>>> + if (hotplug_enabled(bridge_dev)) {
>>>> + 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 (!hotplug_enabled(bridge_dev)) {
>>>> + return 0;
>>>> + }
>>>> +
>>>> if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
>>>> msi_supported) {
>>>> err = msi_init(dev, 0, 1, true, true);
>>>> @@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>> msi_error:
>>>> slotid_cap_cleanup(dev);
>>>> slotid_error:
>>>> - shpc_cleanup(dev, &bridge_dev->bar);
>>>> + if (hotplug_enabled(bridge_dev)) {
>>>> + shpc_cleanup(dev, &bridge_dev->bar);
>>>> + }
>>>> shpc_error:
>>>> pci_bridge_exitfn(dev);
>>>> bridge_error:
>>>> @@ -89,53 +107,101 @@ bridge_error:
>>>> static void pci_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);
>>>> + if (hotplug_enabled(bridge_dev)) {
>>>> + shpc_cleanup(dev, &bridge_dev->bar);
>>>> + }
>>>> pci_bridge_exitfn(dev);
>>>> }
>>>>
>>>> static void pci_bridge_dev_instance_finalize(Object *obj)
>>>> {
>>>> + /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
>>>> shpc_free(PCI_DEVICE(obj));
>>>> }
>>>>
>>>> static void pci_bridge_dev_write_config(PCIDevice *d,
>>>> uint32_t address, uint32_t val, int len)
>>>> {
>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
>>>> +
>>>> 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);
>>>> + if (hotplug_enabled(bridge_dev)) {
>>>> + shpc_cap_write_config(d, address, val, len);
>>>> + }
>>>> }
>>>>
>>>> static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>>>> {
>>>> PCIDevice *dev = PCI_DEVICE(qdev);
>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>
>>>> pci_bridge_reset(qdev);
>>>> - shpc_reset(dev);
>>>> + if (hotplug_enabled(bridge_dev)) {
>>>> + shpc_reset(dev);
>>>> + }
>>>> }
>>>>
>>>> static Property pci_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_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
>>>> + true),
>>>> DEFINE_PROP_END_OF_LIST(),
>>>> };
>>>>
>>>> +static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
>>>> +{
>>>> + PCIDevice *dev = opaque;
>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>> +
>>>> + return hotplug_enabled(bridge_dev);
>>>> +}
>>>> +
>>>> static const VMStateDescription pci_bridge_dev_vmstate = {
>>>> .name = "pci_bridge",
>>>> .fields = (VMStateField[]) {
>>>> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
>>>> - SHPC_VMSTATE(shpc, PCIDevice, NULL),
>>>> + SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
>>>> VMSTATE_END_OF_LIST()
>>>> }
>>>> };
>>>>
>>>> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
>>>> + DeviceState *dev, Error **errp)
>>>> +{
>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>>>> +
>>>> + if (!hotplug_enabled(bridge_dev)) {
>>>> + error_setg(errp, "standard hotplug controller has been disabled for "
>>>> + "this %s", TYPE_PCI_BRIDGE_DEV);
>>>> + return;
>>>> + }
>>>> + shpc_device_hotplug_cb(hotplug_dev, dev, errp);
>>>> +}
>>>> +
>>>> +static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>> + DeviceState *dev,
>>>> + Error **errp)
>>>> +{
>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>>>> +
>>>> + if (!hotplug_enabled(bridge_dev)) {
>>>> + error_setg(errp, "standard hotplug controller has been disabled for "
>>>> + "this %s", TYPE_PCI_BRIDGE_DEV);
>>>> + return;
>>>> + }
>>>> + shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
>>>> +}
>>>> +
>>>> static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>> {
>>>> DeviceClass *dc = DEVICE_CLASS(klass);
>>>> @@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>> dc->props = pci_bridge_dev_properties;
>>>> dc->vmsd = &pci_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;
>>>> + hc->plug = pci_bridge_dev_hotplug_cb;
>>>> + hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
>>>> }
>>>>
>>>> static const TypeInfo pci_bridge_dev_info = {
>>>> --
>>>> 1.8.3.1
>>>>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
2015-06-17 14:15 ` Laszlo Ersek
@ 2015-06-17 19:54 ` Marcel Apfelbaum
0 siblings, 0 replies; 41+ messages in thread
From: Marcel Apfelbaum @ 2015-06-17 19:54 UTC (permalink / raw)
To: Laszlo Ersek, Michael S. Tsirkin; +Cc: qemu-devel
On 06/17/2015 05:15 PM, Laszlo Ersek wrote:
> On 06/17/15 16:02, Michael S. Tsirkin wrote:
>> On Wed, Jun 17, 2015 at 03:55:32PM +0200, Laszlo Ersek wrote:
>>> On 06/17/15 15:42, Michael S. Tsirkin wrote:
>>>> On Wed, Jun 17, 2015 at 02:45:01PM +0200, Laszlo Ersek wrote:
>>>>> In the PCI expander bridge, we will want to disable those features of
>>>>> pci-bridge that relate to SHPC (standard hotplug controller):
>>>>>
>>>>> - SHPC bar and underlying MemoryRegion
>>>>> - interrupt (INTx or MSI)
>>>>> - effective hotplug callbacks
>>>>> - other SHPC hooks (initialization, cleanup, migration etc)
>>>>>
>>>>> Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
>>>>> the above if the bit is explicitly cleared.
>>>>>
>>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>>
>>>> I think I would prefer naming the property "shpc".
>>>> In particular ACPI hotplug still works fine.
>>>
>>> Will do.
>>>
>>> Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
>>> what would be your preference?
>>
>> PCI_BRIDGE_DEV_F_SHPC_REQ
>>
>> Also add macro for "shpc" property string to avoid duplication.
>> Also clear msi explicitly - no need for that to depend on shpc.
>>
>>> Should I also rename the helper function hotplug_enabled() to
>>> shpc_enabled()?
>>
>> It should be tested only once - before shpc is initialized.
>> All other users should test QEMU_PCI_CAP_SHPC - if you like,
>> using a helper shpc_present.
>
> I'll make one attempt (in v7) to get this right. If I miss it, I'll ask
> Marcel to please take over the series.
No problem here,
I'll take it from V8 if necessary, but I think it will be just fine. :)
Thanks,
Marcel
>
> Thanks
> Laszlo
>
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>>>>
>>>>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>> v6:
>>>>> - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
>>>>> hotplug-less bridge for PXB" from v5) [Michael]
>>>>> - I tested migration-to-file and migration-from-file, with a single PXB
>>>>> behind which a virtio-scsi HBA lived
>>>>> - No idea how to test the hotplug callbacks. I tried the device_add and
>>>>> device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
>>>>> monitor reports no error at all; the callbacks don't seem to be
>>>>> reached.
>>>>> - Retested with windows 2012; it works
>>>>>
>>>>> hw/pci-bridge/pci_bridge_dev.c | 90 ++++++++++++++++++++++++++++++++++++------
>>>>> 1 file changed, 78 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>>>> index d697c7b..a0a1ce9 100644
>>>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>>>> @@ -40,10 +40,16 @@ struct PCIBridgeDev {
>>>>> MemoryRegion bar;
>>>>> uint8_t chassis_nr;
>>>>> #define PCI_BRIDGE_DEV_F_MSI_REQ 0
>>>>> +#define PCI_BRIDGE_DEV_F_HOTPLUG 1
>>>>> uint32_t flags;
>>>>> };
>>>>> typedef struct PCIBridgeDev PCIBridgeDev;
>>>>>
>>>>> +static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
>>>>> +{
>>>>> + return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
>>>>> +}
>>>>> +
>>>>> static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> {
>>>>> PCIBridge *br = PCI_BRIDGE(dev);
>>>>> @@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> 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;
>>>>> +
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + 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 (!hotplug_enabled(bridge_dev)) {
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
>>>>> msi_supported) {
>>>>> err = msi_init(dev, 0, 1, true, true);
>>>>> @@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> msi_error:
>>>>> slotid_cap_cleanup(dev);
>>>>> slotid_error:
>>>>> - shpc_cleanup(dev, &bridge_dev->bar);
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + shpc_cleanup(dev, &bridge_dev->bar);
>>>>> + }
>>>>> shpc_error:
>>>>> pci_bridge_exitfn(dev);
>>>>> bridge_error:
>>>>> @@ -89,53 +107,101 @@ bridge_error:
>>>>> static void pci_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);
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + shpc_cleanup(dev, &bridge_dev->bar);
>>>>> + }
>>>>> pci_bridge_exitfn(dev);
>>>>> }
>>>>>
>>>>> static void pci_bridge_dev_instance_finalize(Object *obj)
>>>>> {
>>>>> + /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
>>>>> shpc_free(PCI_DEVICE(obj));
>>>>> }
>>>>>
>>>>> static void pci_bridge_dev_write_config(PCIDevice *d,
>>>>> uint32_t address, uint32_t val, int len)
>>>>> {
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
>>>>> +
>>>>> 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);
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + shpc_cap_write_config(d, address, val, len);
>>>>> + }
>>>>> }
>>>>>
>>>>> static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>>>>> {
>>>>> PCIDevice *dev = PCI_DEVICE(qdev);
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>>
>>>>> pci_bridge_reset(qdev);
>>>>> - shpc_reset(dev);
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + shpc_reset(dev);
>>>>> + }
>>>>> }
>>>>>
>>>>> static Property pci_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_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
>>>>> + true),
>>>>> DEFINE_PROP_END_OF_LIST(),
>>>>> };
>>>>>
>>>>> +static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
>>>>> +{
>>>>> + PCIDevice *dev = opaque;
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>> +
>>>>> + return hotplug_enabled(bridge_dev);
>>>>> +}
>>>>> +
>>>>> static const VMStateDescription pci_bridge_dev_vmstate = {
>>>>> .name = "pci_bridge",
>>>>> .fields = (VMStateField[]) {
>>>>> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
>>>>> - SHPC_VMSTATE(shpc, PCIDevice, NULL),
>>>>> + SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
>>>>> VMSTATE_END_OF_LIST()
>>>>> }
>>>>> };
>>>>>
>>>>> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
>>>>> + DeviceState *dev, Error **errp)
>>>>> +{
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>>>>> +
>>>>> + if (!hotplug_enabled(bridge_dev)) {
>>>>> + error_setg(errp, "standard hotplug controller has been disabled for "
>>>>> + "this %s", TYPE_PCI_BRIDGE_DEV);
>>>>> + return;
>>>>> + }
>>>>> + shpc_device_hotplug_cb(hotplug_dev, dev, errp);
>>>>> +}
>>>>> +
>>>>> +static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>>> + DeviceState *dev,
>>>>> + Error **errp)
>>>>> +{
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>>>>> +
>>>>> + if (!hotplug_enabled(bridge_dev)) {
>>>>> + error_setg(errp, "standard hotplug controller has been disabled for "
>>>>> + "this %s", TYPE_PCI_BRIDGE_DEV);
>>>>> + return;
>>>>> + }
>>>>> + shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
>>>>> +}
>>>>> +
>>>>> static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>>> {
>>>>> DeviceClass *dc = DEVICE_CLASS(klass);
>>>>> @@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>>> dc->props = pci_bridge_dev_properties;
>>>>> dc->vmsd = &pci_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;
>>>>> + hc->plug = pci_bridge_dev_hotplug_cb;
>>>>> + hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
>>>>> }
>>>>>
>>>>> static const TypeInfo pci_bridge_dev_info = {
>>>>> --
>>>>> 1.8.3.1
>>>>>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
2015-06-17 14:02 ` Michael S. Tsirkin
2015-06-17 14:15 ` Laszlo Ersek
@ 2015-06-18 13:47 ` Paolo Bonzini
2015-06-18 14:44 ` Michael S. Tsirkin
1 sibling, 1 reply; 41+ messages in thread
From: Paolo Bonzini @ 2015-06-18 13:47 UTC (permalink / raw)
To: Michael S. Tsirkin, Laszlo Ersek; +Cc: Marcel Apfelbaum, qemu-devel
On 17/06/2015 16:02, Michael S. Tsirkin wrote:
> > Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
> > what would be your preference?
>
> PCI_BRIDGE_DEV_F_SHPC_REQ
Do not abbrev unless necessary. :) What about PCI_BRIDGE_DEV_F_HAS_SHPC
or even just PCI_BRIDGE_DEV_F_SHPC?
Paolo
> Also add macro for "shpc" property string to avoid duplication.
> Also clear msi explicitly - no need for that to depend on shpc.
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
2015-06-18 13:47 ` Paolo Bonzini
@ 2015-06-18 14:44 ` Michael S. Tsirkin
0 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-18 14:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Marcel Apfelbaum, Laszlo Ersek, qemu-devel
On Thu, Jun 18, 2015 at 03:47:41PM +0200, Paolo Bonzini wrote:
>
>
> On 17/06/2015 16:02, Michael S. Tsirkin wrote:
> > > Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
> > > what would be your preference?
> >
> > PCI_BRIDGE_DEV_F_SHPC_REQ
>
> Do not abbrev unless necessary. :) What about PCI_BRIDGE_DEV_F_HAS_SHPC
> or even just PCI_BRIDGE_DEV_F_SHPC?
>
> Paolo
The point of _REQ is that it's requested by user.
This avoids confusion with two other flags,
one checking whether a feature is present, another
whether it's enabled.
> > Also add macro for "shpc" property string to avoid duplication.
> > Also clear msi explicitly - no need for that to depend on shpc.
> >
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 4/7] hw/pci-bridge: disable hotplug in PXB
2015-06-17 12:44 [Qemu-devel] [PATCH v6 0/7] PXB changes Laszlo Ersek
` (2 preceding siblings ...)
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property Laszlo Ersek
@ 2015-06-17 12:45 ` Laszlo Ersek
2015-06-17 13:45 ` Michael S. Tsirkin
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 5/7] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf() Laszlo Ersek
` (2 subsequent siblings)
6 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 12:45 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.
The "hotplug" property of "pci-bridge", introduced in the previous
patches, enables us to disable the standard hotplug controller cleanly,
eliminating the SHPC bar and the conflict.
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v6:
- new in v6 (replaces "hw/pci-bridge: create interrupt-less,
hotplug-less bridge for PXB" from v5) [Michael]
hw/pci-bridge/pci_expander_bridge.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index ec2bb45..4398d98 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -176,6 +176,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
bds = qdev_create(BUS(bus), "pci-bridge");
bds->id = dev_name;
qdev_prop_set_uint8(bds, "chassis_nr", pxb->bus_nr);
+ qdev_prop_set_bit(bds, "hotplug", false);
PCI_HOST_BRIDGE(ds)->bus = bus;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 4/7] hw/pci-bridge: disable hotplug in PXB
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 4/7] hw/pci-bridge: disable hotplug in PXB Laszlo Ersek
@ 2015-06-17 13:45 ` Michael S. Tsirkin
2015-06-17 19:52 ` Marcel Apfelbaum
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 13:45 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Marcel Apfelbaum, qemu-devel
On Wed, Jun 17, 2015 at 02:45:02PM +0200, 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.
>
> The "hotplug" property of "pci-bridge", introduced in the previous
> patches, enables us to disable the standard hotplug controller cleanly,
> eliminating the SHPC bar and the conflict.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v6:
> - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
> hotplug-less bridge for PXB" from v5) [Michael]
>
> hw/pci-bridge/pci_expander_bridge.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index ec2bb45..4398d98 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -176,6 +176,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
> bds = qdev_create(BUS(bus), "pci-bridge");
> bds->id = dev_name;
> qdev_prop_set_uint8(bds, "chassis_nr", pxb->bus_nr);
BTW this is a bug: it will conflict with chassis_nr
on another bridge if user set it. Not related to this
patch of course.
> + qdev_prop_set_bit(bds, "hotplug", false);
>
> PCI_HOST_BRIDGE(ds)->bus = bus;
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 4/7] hw/pci-bridge: disable hotplug in PXB
2015-06-17 13:45 ` Michael S. Tsirkin
@ 2015-06-17 19:52 ` Marcel Apfelbaum
0 siblings, 0 replies; 41+ messages in thread
From: Marcel Apfelbaum @ 2015-06-17 19:52 UTC (permalink / raw)
To: Michael S. Tsirkin, Laszlo Ersek; +Cc: qemu-devel
On 06/17/2015 04:45 PM, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 02:45:02PM +0200, 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.
>>
>> The "hotplug" property of "pci-bridge", introduced in the previous
>> patches, enables us to disable the standard hotplug controller cleanly,
>> eliminating the SHPC bar and the conflict.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>> v6:
>> - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
>> hotplug-less bridge for PXB" from v5) [Michael]
>>
>> hw/pci-bridge/pci_expander_bridge.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index ec2bb45..4398d98 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -176,6 +176,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
>> bds = qdev_create(BUS(bus), "pci-bridge");
>> bds->id = dev_name;
>> qdev_prop_set_uint8(bds, "chassis_nr", pxb->bus_nr);
>
> BTW this is a bug: it will conflict with chassis_nr
> on another bridge if user set it. Not related to this
> patch of course.
I will take care of this.
Thanks,
Marcel
>
>> + qdev_prop_set_bit(bds, "hotplug", false);
>>
>> PCI_HOST_BRIDGE(ds)->bus = bus;
>>
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 5/7] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf()
2015-06-17 12:44 [Qemu-devel] [PATCH v6 0/7] PXB changes Laszlo Ersek
` (3 preceding siblings ...)
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 4/7] hw/pci-bridge: disable hotplug in PXB Laszlo Ersek
@ 2015-06-17 12:45 ` Laszlo Ersek
2015-06-17 13:46 ` Michael S. Tsirkin
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 6/7] hw/core: explicit OFW unit address callback for SysBusDeviceClass Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB Laszlo Ersek
6 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 12:45 UTC (permalink / raw)
To: qemu-devel, lersek
Cc: Marcel Apfelbaum, Markus Armbruster, Michael S. Tsirkin
This is done mainly for improving readability, and in preparation for the
next patch, but Markus pointed out another bonus for the string being
returned:
"No arbitrary length limit. Before the patch, it's 39 characters, and the
code breaks catastrophically when qdev_fw_name() is longer: the second
snprintf() is called with its first argument pointing beyond path[], and
its second argument underflowing to a huge size."
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>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
Notes:
v6:
- no changes
v5:
- separate "%s@" from TARGET_FMT_plx with a space [Markus]
- copied Markus's note about "no arbitrary length limit" on the retval
into the commit message
v4:
- unchanged
v3:
- new in v3
hw/core/sysbus.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index b53c351..92eced9 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -281,19 +281,15 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
static char *sysbus_get_fw_dev_path(DeviceState *dev)
{
SysBusDevice *s = SYS_BUS_DEVICE(dev);
- char path[40];
- int off;
-
- off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
if (s->num_mmio) {
- snprintf(path + off, sizeof(path) - off, "@"TARGET_FMT_plx,
- s->mmio[0].addr);
- } else if (s->num_pio) {
- snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);
+ return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
+ s->mmio[0].addr);
}
-
- return g_strdup(path);
+ if (s->num_pio) {
+ return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
+ }
+ return g_strdup(qdev_fw_name(dev));
}
void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 5/7] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf()
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 5/7] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf() Laszlo Ersek
@ 2015-06-17 13:46 ` Michael S. Tsirkin
2015-06-17 13:56 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 13:46 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Marcel Apfelbaum, qemu-devel, Markus Armbruster
On Wed, Jun 17, 2015 at 02:45:03PM +0200, Laszlo Ersek wrote:
> This is done mainly for improving readability, and in preparation for the
> next patch, but Markus pointed out another bonus for the string being
> returned:
>
> "No arbitrary length limit. Before the patch, it's 39 characters, and the
> code breaks catastrophically when qdev_fw_name() is longer: the second
> snprintf() is called with its first argument pointing beyond path[], and
> its second argument underflowing to a huge size."
>
> 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>
> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
In fact, I'd Cc stable on this one.
> ---
>
> Notes:
> v6:
> - no changes
>
> v5:
> - separate "%s@" from TARGET_FMT_plx with a space [Markus]
> - copied Markus's note about "no arbitrary length limit" on the retval
> into the commit message
>
> v4:
> - unchanged
>
> v3:
> - new in v3
>
> hw/core/sysbus.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index b53c351..92eced9 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -281,19 +281,15 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> static char *sysbus_get_fw_dev_path(DeviceState *dev)
> {
> SysBusDevice *s = SYS_BUS_DEVICE(dev);
> - char path[40];
> - int off;
> -
> - off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
>
> if (s->num_mmio) {
> - snprintf(path + off, sizeof(path) - off, "@"TARGET_FMT_plx,
> - s->mmio[0].addr);
> - } else if (s->num_pio) {
> - snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);
> + return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
> + s->mmio[0].addr);
> }
> -
> - return g_strdup(path);
> + if (s->num_pio) {
> + return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
> + }
> + return g_strdup(qdev_fw_name(dev));
> }
>
> void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 5/7] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf()
2015-06-17 13:46 ` Michael S. Tsirkin
@ 2015-06-17 13:56 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 13:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, qemu-devel, Markus Armbruster
On 06/17/15 15:46, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 02:45:03PM +0200, Laszlo Ersek wrote:
>> This is done mainly for improving readability, and in preparation for the
>> next patch, but Markus pointed out another bonus for the string being
>> returned:
>>
>> "No arbitrary length limit. Before the patch, it's 39 characters, and the
>> code breaks catastrophically when qdev_fw_name() is longer: the second
>> snprintf() is called with its first argument pointing beyond path[], and
>> its second argument underflowing to a huge size."
>>
>> 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>
>> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> In fact, I'd Cc stable on this one.
Will do, thanks.
Laszlo
>
>> ---
>>
>> Notes:
>> v6:
>> - no changes
>>
>> v5:
>> - separate "%s@" from TARGET_FMT_plx with a space [Markus]
>> - copied Markus's note about "no arbitrary length limit" on the retval
>> into the commit message
>>
>> v4:
>> - unchanged
>>
>> v3:
>> - new in v3
>>
>> hw/core/sysbus.c | 16 ++++++----------
>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index b53c351..92eced9 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -281,19 +281,15 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>> static char *sysbus_get_fw_dev_path(DeviceState *dev)
>> {
>> SysBusDevice *s = SYS_BUS_DEVICE(dev);
>> - char path[40];
>> - int off;
>> -
>> - off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
>>
>> if (s->num_mmio) {
>> - snprintf(path + off, sizeof(path) - off, "@"TARGET_FMT_plx,
>> - s->mmio[0].addr);
>> - } else if (s->num_pio) {
>> - snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);
>> + return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
>> + s->mmio[0].addr);
>> }
>> -
>> - return g_strdup(path);
>> + if (s->num_pio) {
>> + return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
>> + }
>> + return g_strdup(qdev_fw_name(dev));
>> }
>>
>> void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v6 6/7] hw/core: explicit OFW unit address callback for SysBusDeviceClass
2015-06-17 12:44 [Qemu-devel] [PATCH v6 0/7] PXB changes Laszlo Ersek
` (4 preceding siblings ...)
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 5/7] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf() Laszlo Ersek
@ 2015-06-17 12:45 ` Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB Laszlo Ersek
6 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 12:45 UTC (permalink / raw)
To: qemu-devel, lersek
Cc: Marcel Apfelbaum, Markus Armbruster, Michael S. Tsirkin
The sysbus_get_fw_dev_path() function formats OpenFirmware device path
nodes ("driver-name@unit-address") for sysbus devices. The first choice
for "unit-address" is the base address of the device's first MMIO region.
The second choice is its first IO port.
However, if two sysbus devices with the same "driver-name" lack both MMIO
and PIO resources, then there is no good way to distinguish them based on
their OFW nodes, because in this case unit-address is omitted completely
for both devices. An example is TYPE_PXB_HOST ("pxb-host").
For the sake of such devices, introduce the explicit_ofw_unit_address()
"virtual member function". With this function, each sysbus device in the
same SysBusDeviceClass can state its own address.
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>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
Notes:
v6:
- no changes
v5:
- mention "pxb-host" as an example device in the commit message [Markus]
- reword documentation on explicit_ofw_unit_address(), specifying
headline, preconditions / goal, side effects, retval, errors
separately. [Markus]
- constify parameter of explicit_ofw_unit_address(), after focusing on
side effects [Markus]
- move declaration of "addr" and "fw_dev_path" to the top level block in
sysbus_get_fw_dev_path() [Markus]
- no functional changes, add / keep Markus's R-b
v4:
- Yet another approach. Instead of allowing the creator of the device to
set a string property statically, introduce a class level callback.
v3:
- new in v3
- new approach
include/hw/sysbus.h | 17 +++++++++++++++++
hw/core/sysbus.c | 11 +++++++++++
2 files changed, 28 insertions(+)
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index d1f3f00..34f93c3 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -41,6 +41,23 @@ typedef struct SysBusDeviceClass {
/*< public >*/
int (*init)(SysBusDevice *dev);
+
+ /*
+ * Let the sysbus device format its own non-PIO, non-MMIO unit address.
+ *
+ * Sometimes a class of SysBusDevices has neither MMIO nor PIO resources,
+ * yet instances of it would like to distinguish themselves, in
+ * OpenFirmware device paths, from other instances of the same class on the
+ * sysbus. For that end we expose this callback.
+ *
+ * The implementation is not supposed to change *@dev, or incur other
+ * observable change.
+ *
+ * The function returns a dynamically allocated string. On error, NULL
+ * should be returned; the unit address portion of the OFW node will be
+ * omitted then. (This is not considered a fatal error.)
+ */
+ char *(*explicit_ofw_unit_address)(const SysBusDevice *dev);
} SysBusDeviceClass;
struct SysBusDevice {
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 92eced9..278a2d1 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -281,6 +281,9 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
static char *sysbus_get_fw_dev_path(DeviceState *dev)
{
SysBusDevice *s = SYS_BUS_DEVICE(dev);
+ SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
+ /* for the explicit unit address fallback case: */
+ char *addr, *fw_dev_path;
if (s->num_mmio) {
return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
@@ -289,6 +292,14 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
if (s->num_pio) {
return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
}
+ if (sbc->explicit_ofw_unit_address) {
+ addr = sbc->explicit_ofw_unit_address(s);
+ if (addr) {
+ fw_dev_path = g_strdup_printf("%s@%s", qdev_fw_name(dev), addr);
+ g_free(addr);
+ return fw_dev_path;
+ }
+ }
return g_strdup(qdev_fw_name(dev));
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 12:44 [Qemu-devel] [PATCH v6 0/7] PXB changes Laszlo Ersek
` (5 preceding siblings ...)
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 6/7] hw/core: explicit OFW unit address callback for SysBusDeviceClass Laszlo Ersek
@ 2015-06-17 12:45 ` Laszlo Ersek
2015-06-17 13:57 ` Michael S. Tsirkin
2015-06-17 19:21 ` Michael S. Tsirkin
6 siblings, 2 replies; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 12:45 UTC (permalink / raw)
To: qemu-devel, lersek
Cc: Marcel Apfelbaum, Kevin O'Connor, Michael S. Tsirkin
SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
to follow the pattern
/pci-root@N/pci@i0cf8/...
for devices that live behind an extra root bus. The extra root bus in
question is the N'th among the extra root bridges. (In other words, N
gives the position of the affected extra root bus relative to the other
extra root buses, in bus_nr order.) N starts at 1, and is formatted in
hex.
The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
FW_PCI_DOMAIN).
Cc: Kevin O'Connor <kevin@koconnor.net>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
Notes:
v6:
- no changes
v5:
- constify parameter and local variables of pxb_host_ofw_unit_address(),
in accord with the previous patch
v4:
- new in v4
hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 4398d98..37574ec 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -42,6 +42,8 @@ typedef struct PXBDev {
uint16_t numa_node;
} PXBDev;
+static GList *pxb_dev_list;
+
#define TYPE_PXB_HOST "pxb-host"
static int pxb_bus_num(PCIBus *bus)
@@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
return bus->bus_path;
}
+static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
+{
+ const PCIHostState *host = PCI_HOST_BRIDGE(dev);
+ const PCIBus *bus;
+ const PXBDev *pxb;
+ int position;
+
+ bus = host->bus;
+ pxb = PXB_DEV(bus->parent_dev);
+ position = g_list_index(pxb_dev_list, pxb);
+ assert(position >= 0);
+
+ return g_strdup_printf("%x/pci@i0cf8", position + 1);
+}
+
static void pxb_host_class_init(ObjectClass *class, void *data)
{
DeviceClass *dc = DEVICE_CLASS(class);
+ SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
- dc->fw_name = "pci";
+ dc->fw_name = "pci-root";
+ sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
hc->root_bus_path = pxb_host_root_bus_path;
}
@@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
return pin - PCI_SLOT(pxb->devfn);
}
+static gint pxb_compare(gconstpointer a, gconstpointer b)
+{
+ const PXBDev *pxb_a = a, *pxb_b = b;
+
+ return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
+ pxb_a->bus_nr > pxb_b->bus_nr ? 1 :
+ 0;
+}
+
static int pxb_dev_initfn(PCIDevice *dev)
{
PXBDev *pxb = PXB_DEV(dev);
@@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
+ pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
return 0;
}
+static void pxb_dev_exitfn(PCIDevice *pci_dev)
+{
+ PXBDev *pxb = PXB_DEV(pci_dev);
+
+ pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
+}
+
static Property pxb_dev_properties[] = {
/* Note: 0 is not a legal a PXB bus number. */
DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
@@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
k->init = pxb_dev_initfn;
+ k->exit = pxb_dev_exitfn;
k->vendor_id = PCI_VENDOR_ID_REDHAT;
k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
k->class_id = PCI_CLASS_BRIDGE_HOST;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB Laszlo Ersek
@ 2015-06-17 13:57 ` Michael S. Tsirkin
2015-06-17 14:00 ` Laszlo Ersek
2015-06-17 14:18 ` Kevin O'Connor
2015-06-17 19:21 ` Michael S. Tsirkin
1 sibling, 2 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 13:57 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel
On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
> to follow the pattern
>
> /pci-root@N/pci@i0cf8/...
It's kind of crazy, isn't it?
/pci@i0cf8/pci-root@N would make some sense: access rootN through cf8.
But if bios needs to keep this for compatibility, maybe
we have too, to. Kevin?
>
> for devices that live behind an extra root bus. The extra root bus in
> question is the N'th among the extra root bridges. (In other words, N
> gives the position of the affected extra root bus relative to the other
> extra root buses, in bus_nr order.) N starts at 1, and is formatted in
> hex.
>
> The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
> FW_PCI_DOMAIN).
>
> Cc: Kevin O'Connor <kevin@koconnor.net>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
This makes this a PC-specific device, which is pretty ugly.
You need to hardcode /pci-root@N here, but why not get
/pci@i0cf8 from the real root?
> ---
>
> Notes:
> v6:
> - no changes
>
> v5:
> - constify parameter and local variables of pxb_host_ofw_unit_address(),
> in accord with the previous patch
>
> v4:
> - new in v4
>
> hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 4398d98..37574ec 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -42,6 +42,8 @@ typedef struct PXBDev {
> uint16_t numa_node;
> } PXBDev;
>
> +static GList *pxb_dev_list;
> +
> #define TYPE_PXB_HOST "pxb-host"
>
> static int pxb_bus_num(PCIBus *bus)
> @@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
> return bus->bus_path;
> }
>
> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
> +{
> + const PCIHostState *host = PCI_HOST_BRIDGE(dev);
> + const PCIBus *bus;
> + const PXBDev *pxb;
> + int position;
> +
> + bus = host->bus;
> + pxb = PXB_DEV(bus->parent_dev);
> + position = g_list_index(pxb_dev_list, pxb);
> + assert(position >= 0);
> +
> + return g_strdup_printf("%x/pci@i0cf8", position + 1);
Please document that this reverse order is to match
existing bios behaviour.
> +}
> +
> static void pxb_host_class_init(ObjectClass *class, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(class);
> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>
> - dc->fw_name = "pci";
> + dc->fw_name = "pci-root";
> + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
> hc->root_bus_path = pxb_host_root_bus_path;
> }
>
> @@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
> return pin - PCI_SLOT(pxb->devfn);
> }
>
> +static gint pxb_compare(gconstpointer a, gconstpointer b)
> +{
> + const PXBDev *pxb_a = a, *pxb_b = b;
> +
> + return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
> + pxb_a->bus_nr > pxb_b->bus_nr ? 1 :
> + 0;
> +}
> +
> static int pxb_dev_initfn(PCIDevice *dev)
> {
> PXBDev *pxb = PXB_DEV(dev);
> @@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
> PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>
> + pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
> return 0;
> }
>
> +static void pxb_dev_exitfn(PCIDevice *pci_dev)
> +{
> + PXBDev *pxb = PXB_DEV(pci_dev);
> +
> + pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
> +}
> +
> static Property pxb_dev_properties[] = {
> /* Note: 0 is not a legal a PXB bus number. */
> DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
> @@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> k->init = pxb_dev_initfn;
> + k->exit = pxb_dev_exitfn;
> k->vendor_id = PCI_VENDOR_ID_REDHAT;
> k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
> k->class_id = PCI_CLASS_BRIDGE_HOST;
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 13:57 ` Michael S. Tsirkin
@ 2015-06-17 14:00 ` Laszlo Ersek
2015-06-17 14:03 ` Michael S. Tsirkin
2015-06-17 14:18 ` Kevin O'Connor
1 sibling, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 14:00 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel
On 06/17/15 15:57, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
>> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
>> to follow the pattern
>>
>> /pci-root@N/pci@i0cf8/...
>
> It's kind of crazy, isn't it?
It's... idiosyncratic.
> /pci@i0cf8/pci-root@N would make some sense: access rootN through cf8.
>
> But if bios needs to keep this for compatibility, maybe
> we have too, to. Kevin?
The only reason I'm doing this is because SeaBIOS expects precisely this
OFW devpath fragment.
Thanks
Laszlo
>
>>
>> for devices that live behind an extra root bus. The extra root bus in
>> question is the N'th among the extra root bridges. (In other words, N
>> gives the position of the affected extra root bus relative to the other
>> extra root buses, in bus_nr order.) N starts at 1, and is formatted in
>> hex.
>>
>> The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
>> FW_PCI_DOMAIN).
>>
>> Cc: Kevin O'Connor <kevin@koconnor.net>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>
> This makes this a PC-specific device, which is pretty ugly.
> You need to hardcode /pci-root@N here, but why not get
> /pci@i0cf8 from the real root?
>
>> ---
>>
>> Notes:
>> v6:
>> - no changes
>>
>> v5:
>> - constify parameter and local variables of pxb_host_ofw_unit_address(),
>> in accord with the previous patch
>>
>> v4:
>> - new in v4
>>
>> hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index 4398d98..37574ec 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -42,6 +42,8 @@ typedef struct PXBDev {
>> uint16_t numa_node;
>> } PXBDev;
>>
>> +static GList *pxb_dev_list;
>> +
>> #define TYPE_PXB_HOST "pxb-host"
>>
>> static int pxb_bus_num(PCIBus *bus)
>> @@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
>> return bus->bus_path;
>> }
>>
>> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
>> +{
>> + const PCIHostState *host = PCI_HOST_BRIDGE(dev);
>> + const PCIBus *bus;
>> + const PXBDev *pxb;
>> + int position;
>> +
>> + bus = host->bus;
>> + pxb = PXB_DEV(bus->parent_dev);
>> + position = g_list_index(pxb_dev_list, pxb);
>> + assert(position >= 0);
>> +
>> + return g_strdup_printf("%x/pci@i0cf8", position + 1);
>
> Please document that this reverse order is to match
> existing bios behaviour.
>
>
>> +}
>> +
>> static void pxb_host_class_init(ObjectClass *class, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(class);
>> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
>> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>>
>> - dc->fw_name = "pci";
>> + dc->fw_name = "pci-root";
>> + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
>> hc->root_bus_path = pxb_host_root_bus_path;
>> }
>>
>> @@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
>> return pin - PCI_SLOT(pxb->devfn);
>> }
>>
>> +static gint pxb_compare(gconstpointer a, gconstpointer b)
>> +{
>> + const PXBDev *pxb_a = a, *pxb_b = b;
>> +
>> + return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
>> + pxb_a->bus_nr > pxb_b->bus_nr ? 1 :
>> + 0;
>> +}
>> +
>> static int pxb_dev_initfn(PCIDevice *dev)
>> {
>> PXBDev *pxb = PXB_DEV(dev);
>> @@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
>> PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
>> pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>>
>> + pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
>> return 0;
>> }
>>
>> +static void pxb_dev_exitfn(PCIDevice *pci_dev)
>> +{
>> + PXBDev *pxb = PXB_DEV(pci_dev);
>> +
>> + pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
>> +}
>> +
>> static Property pxb_dev_properties[] = {
>> /* Note: 0 is not a legal a PXB bus number. */
>> DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
>> @@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>
>> k->init = pxb_dev_initfn;
>> + k->exit = pxb_dev_exitfn;
>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>> k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>> k->class_id = PCI_CLASS_BRIDGE_HOST;
>> --
>> 1.8.3.1
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 14:00 ` Laszlo Ersek
@ 2015-06-17 14:03 ` Michael S. Tsirkin
0 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 14:03 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel
On Wed, Jun 17, 2015 at 04:00:38PM +0200, Laszlo Ersek wrote:
> On 06/17/15 15:57, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
> >> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
> >> to follow the pattern
> >>
> >> /pci-root@N/pci@i0cf8/...
> >
> > It's kind of crazy, isn't it?
>
> It's... idiosyncratic.
Yes - hoping for an answer from Kevin.
> > /pci@i0cf8/pci-root@N would make some sense: access rootN through cf8.
> >
> > But if bios needs to keep this for compatibility, maybe
> > we have too, to. Kevin?
>
> The only reason I'm doing this is because SeaBIOS expects precisely this
> OFW devpath fragment.
>
> Thanks
> Laszlo
>
> >
> >>
> >> for devices that live behind an extra root bus. The extra root bus in
> >> question is the N'th among the extra root bridges. (In other words, N
> >> gives the position of the affected extra root bus relative to the other
> >> extra root buses, in bus_nr order.) N starts at 1, and is formatted in
> >> hex.
> >>
> >> The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
> >> FW_PCI_DOMAIN).
> >>
> >> Cc: Kevin O'Connor <kevin@koconnor.net>
> >> Cc: Marcel Apfelbaum <marcel@redhat.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
> >> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> >
> > This makes this a PC-specific device, which is pretty ugly.
> > You need to hardcode /pci-root@N here, but why not get
> > /pci@i0cf8 from the real root?
> >
> >> ---
> >>
> >> Notes:
> >> v6:
> >> - no changes
> >>
> >> v5:
> >> - constify parameter and local variables of pxb_host_ofw_unit_address(),
> >> in accord with the previous patch
> >>
> >> v4:
> >> - new in v4
> >>
> >> hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 38 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> >> index 4398d98..37574ec 100644
> >> --- a/hw/pci-bridge/pci_expander_bridge.c
> >> +++ b/hw/pci-bridge/pci_expander_bridge.c
> >> @@ -42,6 +42,8 @@ typedef struct PXBDev {
> >> uint16_t numa_node;
> >> } PXBDev;
> >>
> >> +static GList *pxb_dev_list;
> >> +
> >> #define TYPE_PXB_HOST "pxb-host"
> >>
> >> static int pxb_bus_num(PCIBus *bus)
> >> @@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
> >> return bus->bus_path;
> >> }
> >>
> >> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
> >> +{
> >> + const PCIHostState *host = PCI_HOST_BRIDGE(dev);
> >> + const PCIBus *bus;
> >> + const PXBDev *pxb;
> >> + int position;
> >> +
> >> + bus = host->bus;
> >> + pxb = PXB_DEV(bus->parent_dev);
> >> + position = g_list_index(pxb_dev_list, pxb);
> >> + assert(position >= 0);
> >> +
> >> + return g_strdup_printf("%x/pci@i0cf8", position + 1);
> >
> > Please document that this reverse order is to match
> > existing bios behaviour.
> >
> >
> >> +}
> >> +
> >> static void pxb_host_class_init(ObjectClass *class, void *data)
> >> {
> >> DeviceClass *dc = DEVICE_CLASS(class);
> >> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
> >> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
> >>
> >> - dc->fw_name = "pci";
> >> + dc->fw_name = "pci-root";
> >> + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
> >> hc->root_bus_path = pxb_host_root_bus_path;
> >> }
> >>
> >> @@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
> >> return pin - PCI_SLOT(pxb->devfn);
> >> }
> >>
> >> +static gint pxb_compare(gconstpointer a, gconstpointer b)
> >> +{
> >> + const PXBDev *pxb_a = a, *pxb_b = b;
> >> +
> >> + return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
> >> + pxb_a->bus_nr > pxb_b->bus_nr ? 1 :
> >> + 0;
> >> +}
> >> +
> >> static int pxb_dev_initfn(PCIDevice *dev)
> >> {
> >> PXBDev *pxb = PXB_DEV(dev);
> >> @@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
> >> PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> >> pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
> >>
> >> + pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
> >> return 0;
> >> }
> >>
> >> +static void pxb_dev_exitfn(PCIDevice *pci_dev)
> >> +{
> >> + PXBDev *pxb = PXB_DEV(pci_dev);
> >> +
> >> + pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
> >> +}
> >> +
> >> static Property pxb_dev_properties[] = {
> >> /* Note: 0 is not a legal a PXB bus number. */
> >> DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
> >> @@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
> >> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>
> >> k->init = pxb_dev_initfn;
> >> + k->exit = pxb_dev_exitfn;
> >> k->vendor_id = PCI_VENDOR_ID_REDHAT;
> >> k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
> >> k->class_id = PCI_CLASS_BRIDGE_HOST;
> >> --
> >> 1.8.3.1
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 13:57 ` Michael S. Tsirkin
2015-06-17 14:00 ` Laszlo Ersek
@ 2015-06-17 14:18 ` Kevin O'Connor
2015-06-17 14:45 ` Laszlo Ersek
1 sibling, 1 reply; 41+ messages in thread
From: Kevin O'Connor @ 2015-06-17 14:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, Laszlo Ersek, qemu-devel
On Wed, Jun 17, 2015 at 03:57:36PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
> > SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
> > to follow the pattern
> >
> > /pci-root@N/pci@i0cf8/...
>
> It's kind of crazy, isn't it?
> /pci@i0cf8/pci-root@N would make some sense: access rootN through cf8.
>
> But if bios needs to keep this for compatibility, maybe
> we have too, to. Kevin?
I have no issue with changing the string in SeaBIOS. In a previous
email we discussed "/pci@i0cf8/pci-root@%x/" as well as
"/pci@i0cf8,%x/", but anything that makes sense is fine with me.
-Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 14:18 ` Kevin O'Connor
@ 2015-06-17 14:45 ` Laszlo Ersek
2015-06-17 15:05 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 14:45 UTC (permalink / raw)
To: Kevin O'Connor, Michael S. Tsirkin
Cc: Marcel Apfelbaum, qemu-devel, Markus Armbruster
On 06/17/15 16:18, Kevin O'Connor wrote:
> On Wed, Jun 17, 2015 at 03:57:36PM +0200, Michael S. Tsirkin wrote:
>> On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
>>> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
>>> to follow the pattern
>>>
>>> /pci-root@N/pci@i0cf8/...
>>
>> It's kind of crazy, isn't it?
>> /pci@i0cf8/pci-root@N would make some sense: access rootN through cf8.
>>
>> But if bios needs to keep this for compatibility, maybe
>> we have too, to. Kevin?
>
> I have no issue with changing the string in SeaBIOS. In a previous
> email we discussed "/pci@i0cf8/pci-root@%x/" as well as
> "/pci@i0cf8,%x/", but anything that makes sense is fine with me.
It is not fine with me.
Every time there is another idea about this format, I get to update and
repost the OVMF series (consisting of 24 patches), which of course
nobody on qemu-devel@ and seabios@ cares about, while it is actually the
*only* thing that matters to me. Plus, this patch appeared in v4 and has
been reposted without changes twice.
Honestly, the format looks outright retarded to me, but I didn't
complain, because adopting it (and not patching SeaBIOS at all) was the
most direct way forward. (Most direct in the sense that we're now at
v6.) I will *not* repeat the entire discussion about the format, and I
won't revisit that outcome. I have spent several nights and weekend days
on implementing SeaBIOS-compatible code in qemu and OVMF, and I won't go
back on that work.
Similarly, the patch "hw/pci-bridge: create interrupt-less, hotplug-less
bridge for PXB" has been present in the QEMU series without functional
changes since v2. I've been aware that it doesn't meet Michael's taste
(that fact was documented in v2), but I'm appalled that it has taken 4
reposts (v3 to v6) to arrive at specifics. Not only did that cause me to
miss 4 opportunities to post an ultimately acceptable patch, it also
wasted the reviews of Marcel and Markus, plus my work to address
Markus's review.
I've been going out of my way to be cooperative, responsive, and just do
whatever I've been told, minimize the impact, etc. As I said, I'm
willing to post a v7 for the SHPC-less pci-bridge device model, but no
more versions, and no other changes.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 14:45 ` Laszlo Ersek
@ 2015-06-17 15:05 ` Michael S. Tsirkin
2015-06-17 18:16 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 15:05 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
On Wed, Jun 17, 2015 at 04:45:45PM +0200, Laszlo Ersek wrote:
> On 06/17/15 16:18, Kevin O'Connor wrote:
> > On Wed, Jun 17, 2015 at 03:57:36PM +0200, Michael S. Tsirkin wrote:
> >> On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
> >>> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
> >>> to follow the pattern
> >>>
> >>> /pci-root@N/pci@i0cf8/...
> >>
> >> It's kind of crazy, isn't it?
> >> /pci@i0cf8/pci-root@N would make some sense: access rootN through cf8.
> >>
> >> But if bios needs to keep this for compatibility, maybe
> >> we have too, to. Kevin?
> >
> > I have no issue with changing the string in SeaBIOS. In a previous
> > email we discussed "/pci@i0cf8/pci-root@%x/" as well as
> > "/pci@i0cf8,%x/", but anything that makes sense is fine with me.
>
> It is not fine with me.
>
> Every time there is another idea about this format, I get to update and
> repost the OVMF series (consisting of 24 patches), which of course
> nobody on qemu-devel@ and seabios@ cares about, while it is actually the
> *only* thing that matters to me. Plus, this patch appeared in v4 and has
> been reposted without changes twice.
>
> Honestly, the format looks outright retarded to me, but I didn't
> complain, because adopting it (and not patching SeaBIOS at all) was the
> most direct way forward. (Most direct in the sense that we're now at
> v6.) I will *not* repeat the entire discussion about the format, and I
> won't revisit that outcome. I have spent several nights and weekend days
> on implementing SeaBIOS-compatible code in qemu and OVMF, and I won't go
> back on that work.
>
> Similarly, the patch "hw/pci-bridge: create interrupt-less, hotplug-less
> bridge for PXB" has been present in the QEMU series without functional
> changes since v2. I've been aware that it doesn't meet Michael's taste
> (that fact was documented in v2), but I'm appalled that it has taken 4
> reposts (v3 to v6) to arrive at specifics. Not only did that cause me to
> miss 4 opportunities to post an ultimately acceptable patch, it also
> wasted the reviews of Marcel and Markus, plus my work to address
> Markus's review.
>
> I've been going out of my way to be cooperative, responsive, and just do
> whatever I've been told, minimize the impact, etc. As I said, I'm
> willing to post a v7 for the SHPC-less pci-bridge device model, but no
> more versions, and no other changes.
>
> Thanks
> Laszlo
Sorry you feel bad. Looks like the patches are pretty close to being
ready.
If you just address the comments about the bridge then I can merge
patches 1-5 directly.
We do need to agree about the correct paths however, this is host/guest
interface which we have to maintain forever, and it's important to get
it right. I kept hoping we can come up with something saner than
the sequence # but oh well. Do you disagree with the statement
that seabios path is currently incorrect? Kevin seems to agree.
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 15:05 ` Michael S. Tsirkin
@ 2015-06-17 18:16 ` Laszlo Ersek
2015-06-17 18:54 ` Michael S. Tsirkin
2015-06-17 19:09 ` Kevin O'Connor
0 siblings, 2 replies; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 18:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
On 06/17/15 17:05, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 04:45:45PM +0200, Laszlo Ersek wrote:
>> On 06/17/15 16:18, Kevin O'Connor wrote:
>>> On Wed, Jun 17, 2015 at 03:57:36PM +0200, Michael S. Tsirkin wrote:
>>>> On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
>>>>> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
>>>>> to follow the pattern
>>>>>
>>>>> /pci-root@N/pci@i0cf8/...
>>>>
>>>> It's kind of crazy, isn't it?
>>>> /pci@i0cf8/pci-root@N would make some sense: access rootN through cf8.
>>>>
>>>> But if bios needs to keep this for compatibility, maybe
>>>> we have too, to. Kevin?
>>>
>>> I have no issue with changing the string in SeaBIOS. In a previous
>>> email we discussed "/pci@i0cf8/pci-root@%x/" as well as
>>> "/pci@i0cf8,%x/", but anything that makes sense is fine with me.
>>
>> It is not fine with me.
>>
>> Every time there is another idea about this format, I get to update and
>> repost the OVMF series (consisting of 24 patches), which of course
>> nobody on qemu-devel@ and seabios@ cares about, while it is actually the
>> *only* thing that matters to me. Plus, this patch appeared in v4 and has
>> been reposted without changes twice.
>>
>> Honestly, the format looks outright retarded to me, but I didn't
>> complain, because adopting it (and not patching SeaBIOS at all) was the
>> most direct way forward. (Most direct in the sense that we're now at
>> v6.) I will *not* repeat the entire discussion about the format, and I
>> won't revisit that outcome. I have spent several nights and weekend days
>> on implementing SeaBIOS-compatible code in qemu and OVMF, and I won't go
>> back on that work.
>>
>> Similarly, the patch "hw/pci-bridge: create interrupt-less, hotplug-less
>> bridge for PXB" has been present in the QEMU series without functional
>> changes since v2. I've been aware that it doesn't meet Michael's taste
>> (that fact was documented in v2), but I'm appalled that it has taken 4
>> reposts (v3 to v6) to arrive at specifics. Not only did that cause me to
>> miss 4 opportunities to post an ultimately acceptable patch, it also
>> wasted the reviews of Marcel and Markus, plus my work to address
>> Markus's review.
>>
>> I've been going out of my way to be cooperative, responsive, and just do
>> whatever I've been told, minimize the impact, etc. As I said, I'm
>> willing to post a v7 for the SHPC-less pci-bridge device model, but no
>> more versions, and no other changes.
>>
>> Thanks
>> Laszlo
>
> Sorry you feel bad. Looks like the patches are pretty close to being
> ready.
I apologize about losing my temper. I had made a resolution to be as
patient as I can (or, preferably, more patient than I can be :)) on
qemu-devel. Looks like I lasted until v6. Sorry about not lasting
longer; it's been a very taxing development for me.
> If you just address the comments about the bridge then I can merge
> patches 1-5 directly.
Thanks. I'll do my best to address your bridge-related comments in v7.
> We do need to agree about the correct paths however, this is host/guest
> interface which we have to maintain forever, and it's important to get
> it right. I kept hoping we can come up with something saner than
> the sequence # but oh well. Do you disagree with the statement
> that seabios path is currently incorrect? Kevin seems to agree.
As discussed earlier, there are two questions to consider about the OFW
devpath pattern
/pci-root@N/pci@i0cf8/...
that SeaBIOS currently recognizes for devices that reside behind extra
PCI root buses.
Q1: everything in that pattern that is not "N"
Q2: what goes into N
These are independent questions.
Marcel's patch for SeaBIOS intended to change SeaBIOS's behavior for
both Q1 and Q2:
- For Q1, the proposed OFW devpath fragment was
/pci-root@N/...
ie. dropping the second node ("pci@i0cf8").
- For Q2, the proposed change was: instead of making N a *serial number*
(where N stood for the N'th extra root bus discovered by SeaBIOS), make
N an actual bus number.
However, applying these changes unconditionally would have broken the
interface between Coreboot and SeaBIOS, in physical hardware
environments (because Coreboot agrees with SeaBIOS on the current syntax
(Q1) and interpretation (Q2) of the devpath pattern). Therefore there
was an idea to make both SeaBIOS changes conditional on runningOnQemu().
As far as I remember, Kevin was more or less okay with that, but (again,
as far as I remember) he did express a mild dislike for such tweaks.
Reluctantly, I agreed, for two reasons: first, it's never a good long
term investment to get a maintainer to commit something he dislikes;
second, such a tweak would have introduced yet another version
dependency between QEMU and SeaBIOS. So I decided to try to write code
for QEMU that is compatible with SeaBIOS out of the box.
Patches #6 and #7 in this v6 series are that code.
To answer your question whether I think the "seabios path is currently
incorrect": that was my initial impression, yes. My opinion is now
different. I think:
- Re Q1: it's just static syntax. No need to bother. Just keep SeaBIOS
happy if we can. (And yes, we can.)
- Re Q2: using N in the sense of "N'th extra root bus" rather than
"extra root bus with bus number N" is peculiar and a little bit
illogical to me. However:
- It is an established interface between Coreboot and SeaBIOS that
works in practice, on physical hardware.
- As long as the enumeration algorithm stays the same in SeaBIOS (ie.
probe extra root buses in increasing order), the "N'th extra root
bus" scheme has identical expressive power, and a bijective
mapping, to the "extra root bus with bus number N".
- Given that the code that transforms between said mappings is ready,
in both QEMU and OVMF, my personal verdict is that the SeaBIOS path
is somewhat unwieldy, and has had me jump through some hoops, but
ultimately it is correct.
It is of course possible to rewind the last seven or so days, return to
Marcel's SeaBIOS patch (and make it depend on runningOnQemu()), but that
will have the following consequences:
- Kevin could be mildly displeased by the SeaBIOS tweaks.
- A new version dependency between SeaBIOS and QEMU will be brought
about.
- I'll have to rework and repost earlier patch set versions for both
QEMU and OVMF, which sort of qualifies as "making a fool out of
Laszlo".
So, please don't. Let's stick with this.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 18:16 ` Laszlo Ersek
@ 2015-06-17 18:54 ` Michael S. Tsirkin
2015-06-17 19:15 ` Laszlo Ersek
2015-06-17 19:09 ` Kevin O'Connor
1 sibling, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 18:54 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
On Wed, Jun 17, 2015 at 08:16:30PM +0200, Laszlo Ersek wrote:
> > We do need to agree about the correct paths however, this is host/guest
> > interface which we have to maintain forever, and it's important to get
> > it right. I kept hoping we can come up with something saner than
> > the sequence # but oh well. Do you disagree with the statement
> > that seabios path is currently incorrect? Kevin seems to agree.
>
> As discussed earlier, there are two questions to consider about the OFW
> devpath pattern
>
> /pci-root@N/pci@i0cf8/...
>
> that SeaBIOS currently recognizes for devices that reside behind extra
> PCI root buses.
>
> Q1: everything in that pattern that is not "N"
> Q2: what goes into N
>
> These are independent questions.
Right. But what I was discussing is a different issue. The point is
that it does not make sense to have /pci@i0cf8 under two hierarchies:
it's the same register. What happens is that you access /pci@i0cf8 and
then *through that* you access another pci root. Not the other way
around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
seabios, unconditionally - not if (QEMU). And I thought Kevin agreed
it's a good idea.
Kevin - is this a good summary of your opinion?
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 18:54 ` Michael S. Tsirkin
@ 2015-06-17 19:15 ` Laszlo Ersek
2015-06-17 19:28 ` Kevin O'Connor
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 19:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
(I'm not trying to answer instead of Kevin, just to comment.)
On 06/17/15 20:54, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 08:16:30PM +0200, Laszlo Ersek wrote:
>>> We do need to agree about the correct paths however, this is host/guest
>>> interface which we have to maintain forever, and it's important to get
>>> it right. I kept hoping we can come up with something saner than
>>> the sequence # but oh well. Do you disagree with the statement
>>> that seabios path is currently incorrect? Kevin seems to agree.
>>
>> As discussed earlier, there are two questions to consider about the OFW
>> devpath pattern
>>
>> /pci-root@N/pci@i0cf8/...
>>
>> that SeaBIOS currently recognizes for devices that reside behind extra
>> PCI root buses.
>>
>> Q1: everything in that pattern that is not "N"
>> Q2: what goes into N
>>
>> These are independent questions.
>
>
> Right. But what I was discussing is a different issue. The point is
> that it does not make sense to have /pci@i0cf8 under two hierarchies:
> it's the same register. What happens is that you access /pci@i0cf8 and
> then *through that* you access another pci root. Not the other way
> around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
> seabios,
For me this is still Question 1 -- 'everything in that pattern that is
not "N"'.
You seem to care about the *semantics* of that OFW device path fragment.
I don't. First, the relevant IEEE spec is prohibitively hard for me to
interpret semantically. Second, there is no known firmware that actually
looks at the "i0cf8" unit-address term and decides *based on that term*
that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
second node is entirely opaque in my interpretation.
> unconditionally - not if (QEMU).
This might qualify as some kind of semantic cleanup, but it will
nonetheless break the SeaBIOS boot options expressed in OFW notation
that are already persistently stored in cbfs, on physical machines. (As
far as I understood.) It might not break the Coreboot-SeaBIOS interface,
but it might invalidate preexistent entries that exist in the prior form
(wherever they exist on physical hardware).
> And I thought Kevin agreed
> it's a good idea.
>
> Kevin - is this a good summary of your opinion?
Kevin, please do answer.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 19:15 ` Laszlo Ersek
@ 2015-06-17 19:28 ` Kevin O'Connor
2015-06-17 19:32 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Kevin O'Connor @ 2015-06-17 19:28 UTC (permalink / raw)
To: Laszlo Ersek, Michael S. Tsirkin
Cc: Marcel Apfelbaum, Markus Armbruster, qemu-devel
On Wed, Jun 17, 2015 at 09:15:24PM +0200, Laszlo Ersek wrote:
> On 06/17/15 20:54, Michael S. Tsirkin wrote:
> > Right. But what I was discussing is a different issue. The point is
> > that it does not make sense to have /pci@i0cf8 under two hierarchies:
> > it's the same register. What happens is that you access /pci@i0cf8 and
> > then *through that* you access another pci root. Not the other way
> > around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
> > seabios,
>
> For me this is still Question 1 -- 'everything in that pattern that is
> not "N"'.
>
> You seem to care about the *semantics* of that OFW device path fragment.
> I don't. First, the relevant IEEE spec is prohibitively hard for me to
> interpret semantically. Second, there is no known firmware that actually
> looks at the "i0cf8" unit-address term and decides *based on that term*
> that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
> second node is entirely opaque in my interpretation.
>
> > unconditionally - not if (QEMU).
>
> This might qualify as some kind of semantic cleanup, but it will
> nonetheless break the SeaBIOS boot options expressed in OFW notation
> that are already persistently stored in cbfs, on physical machines. (As
> far as I understood.) It might not break the Coreboot-SeaBIOS interface,
> but it might invalidate preexistent entries that exist in the prior form
> (wherever they exist on physical hardware).
>
> > And I thought Kevin agreed
> > it's a good idea.
> >
> > Kevin - is this a good summary of your opinion?
>
> Kevin, please do answer.
It is true that it would "invalidate preexistent entries" for
coreboot/seabios users that upgrade, but I think that is manageable.
So I defer the syntax discussion and decisions to the QEMU developers
that are doing the bulk of the work.
-Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 19:28 ` Kevin O'Connor
@ 2015-06-17 19:32 ` Michael S. Tsirkin
2015-06-17 19:44 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 19:32 UTC (permalink / raw)
To: Kevin O'Connor
Cc: Marcel Apfelbaum, Laszlo Ersek, qemu-devel, Markus Armbruster
On Wed, Jun 17, 2015 at 03:28:44PM -0400, Kevin O'Connor wrote:
> On Wed, Jun 17, 2015 at 09:15:24PM +0200, Laszlo Ersek wrote:
> > On 06/17/15 20:54, Michael S. Tsirkin wrote:
> > > Right. But what I was discussing is a different issue. The point is
> > > that it does not make sense to have /pci@i0cf8 under two hierarchies:
> > > it's the same register. What happens is that you access /pci@i0cf8 and
> > > then *through that* you access another pci root. Not the other way
> > > around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
> > > seabios,
> >
> > For me this is still Question 1 -- 'everything in that pattern that is
> > not "N"'.
> >
> > You seem to care about the *semantics* of that OFW device path fragment.
> > I don't. First, the relevant IEEE spec is prohibitively hard for me to
> > interpret semantically. Second, there is no known firmware that actually
> > looks at the "i0cf8" unit-address term and decides *based on that term*
> > that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
> > second node is entirely opaque in my interpretation.
> >
> > > unconditionally - not if (QEMU).
> >
> > This might qualify as some kind of semantic cleanup, but it will
> > nonetheless break the SeaBIOS boot options expressed in OFW notation
> > that are already persistently stored in cbfs, on physical machines. (As
> > far as I understood.) It might not break the Coreboot-SeaBIOS interface,
> > but it might invalidate preexistent entries that exist in the prior form
> > (wherever they exist on physical hardware).
> >
> > > And I thought Kevin agreed
> > > it's a good idea.
> > >
> > > Kevin - is this a good summary of your opinion?
> >
> > Kevin, please do answer.
>
> It is true that it would "invalidate preexistent entries" for
> coreboot/seabios users that upgrade, but I think that is manageable.
> So I defer the syntax discussion and decisions to the QEMU developers
> that are doing the bulk of the work.
>
> -Kevin
I'm fine with either /pci@i0cf8,%x or /pci-root@%x/pci@i0cf8, with a
slight preference to the later - in particular it's easier
to implement in QEMU.
It means old bios won't boot from a pxb, but I think that's
manageable - it works otherwise.
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 19:32 ` Michael S. Tsirkin
@ 2015-06-17 19:44 ` Laszlo Ersek
2015-06-17 21:50 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 19:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
On 06/17/15 21:32, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 03:28:44PM -0400, Kevin O'Connor wrote:
>> On Wed, Jun 17, 2015 at 09:15:24PM +0200, Laszlo Ersek wrote:
>>> On 06/17/15 20:54, Michael S. Tsirkin wrote:
>>>> Right. But what I was discussing is a different issue. The point is
>>>> that it does not make sense to have /pci@i0cf8 under two hierarchies:
>>>> it's the same register. What happens is that you access /pci@i0cf8 and
>>>> then *through that* you access another pci root. Not the other way
>>>> around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
>>>> seabios,
>>>
>>> For me this is still Question 1 -- 'everything in that pattern that is
>>> not "N"'.
>>>
>>> You seem to care about the *semantics* of that OFW device path fragment.
>>> I don't. First, the relevant IEEE spec is prohibitively hard for me to
>>> interpret semantically. Second, there is no known firmware that actually
>>> looks at the "i0cf8" unit-address term and decides *based on that term*
>>> that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
>>> second node is entirely opaque in my interpretation.
>>>
>>>> unconditionally - not if (QEMU).
>>>
>>> This might qualify as some kind of semantic cleanup, but it will
>>> nonetheless break the SeaBIOS boot options expressed in OFW notation
>>> that are already persistently stored in cbfs, on physical machines. (As
>>> far as I understood.) It might not break the Coreboot-SeaBIOS interface,
>>> but it might invalidate preexistent entries that exist in the prior form
>>> (wherever they exist on physical hardware).
>>>
>>>> And I thought Kevin agreed
>>>> it's a good idea.
>>>>
>>>> Kevin - is this a good summary of your opinion?
>>>
>>> Kevin, please do answer.
>>
>> It is true that it would "invalidate preexistent entries" for
>> coreboot/seabios users that upgrade, but I think that is manageable.
>> So I defer the syntax discussion and decisions to the QEMU developers
>> that are doing the bulk of the work.
>>
>> -Kevin
>
> I'm fine with either /pci@i0cf8,%x or /pci-root@%x/pci@i0cf8, with a
> slight preference to the later - in particular it's easier
> to implement in QEMU.
>
> It means old bios won't boot from a pxb, but I think that's
> manageable - it works otherwise.
I don't understand -- the second option you named
("/pci-root@%x/pci@i0cf8") is what this patch implements, and "old" (ie.
current) SeaBIOS does boot from it.
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 19:44 ` Laszlo Ersek
@ 2015-06-17 21:50 ` Michael S. Tsirkin
2015-06-18 13:22 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 21:50 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
On Wed, Jun 17, 2015 at 09:44:07PM +0200, Laszlo Ersek wrote:
> On 06/17/15 21:32, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2015 at 03:28:44PM -0400, Kevin O'Connor wrote:
> >> On Wed, Jun 17, 2015 at 09:15:24PM +0200, Laszlo Ersek wrote:
> >>> On 06/17/15 20:54, Michael S. Tsirkin wrote:
> >>>> Right. But what I was discussing is a different issue. The point is
> >>>> that it does not make sense to have /pci@i0cf8 under two hierarchies:
> >>>> it's the same register. What happens is that you access /pci@i0cf8 and
> >>>> then *through that* you access another pci root. Not the other way
> >>>> around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
> >>>> seabios,
> >>>
> >>> For me this is still Question 1 -- 'everything in that pattern that is
> >>> not "N"'.
> >>>
> >>> You seem to care about the *semantics* of that OFW device path fragment.
> >>> I don't. First, the relevant IEEE spec is prohibitively hard for me to
> >>> interpret semantically. Second, there is no known firmware that actually
> >>> looks at the "i0cf8" unit-address term and decides *based on that term*
> >>> that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
> >>> second node is entirely opaque in my interpretation.
> >>>
> >>>> unconditionally - not if (QEMU).
> >>>
> >>> This might qualify as some kind of semantic cleanup, but it will
> >>> nonetheless break the SeaBIOS boot options expressed in OFW notation
> >>> that are already persistently stored in cbfs, on physical machines. (As
> >>> far as I understood.) It might not break the Coreboot-SeaBIOS interface,
> >>> but it might invalidate preexistent entries that exist in the prior form
> >>> (wherever they exist on physical hardware).
> >>>
> >>>> And I thought Kevin agreed
> >>>> it's a good idea.
> >>>>
> >>>> Kevin - is this a good summary of your opinion?
> >>>
> >>> Kevin, please do answer.
> >>
> >> It is true that it would "invalidate preexistent entries" for
> >> coreboot/seabios users that upgrade, but I think that is manageable.
> >> So I defer the syntax discussion and decisions to the QEMU developers
> >> that are doing the bulk of the work.
> >>
> >> -Kevin
> >
> > I'm fine with either /pci@i0cf8,%x or /pci-root@%x/pci@i0cf8, with a
> > slight preference to the later - in particular it's easier
> > to implement in QEMU.
> >
> > It means old bios won't boot from a pxb, but I think that's
> > manageable - it works otherwise.
>
> I don't understand -- the second option you named
> ("/pci-root@%x/pci@i0cf8") is what this patch implements, and "old" (ie.
> current) SeaBIOS does boot from it.
>
> Laszlo
Ouch. I meant /pci@i0cf8//pci-root@%x.
As you see, it's confusing.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 21:50 ` Michael S. Tsirkin
@ 2015-06-18 13:22 ` Laszlo Ersek
2015-06-18 13:40 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-18 13:22 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
On 06/17/15 23:50, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 09:44:07PM +0200, Laszlo Ersek wrote:
>> On 06/17/15 21:32, Michael S. Tsirkin wrote:
>>> On Wed, Jun 17, 2015 at 03:28:44PM -0400, Kevin O'Connor wrote:
>>>> On Wed, Jun 17, 2015 at 09:15:24PM +0200, Laszlo Ersek wrote:
>>>>> On 06/17/15 20:54, Michael S. Tsirkin wrote:
>>>>>> Right. But what I was discussing is a different issue. The point is
>>>>>> that it does not make sense to have /pci@i0cf8 under two hierarchies:
>>>>>> it's the same register. What happens is that you access /pci@i0cf8 and
>>>>>> then *through that* you access another pci root. Not the other way
>>>>>> around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
>>>>>> seabios,
>>>>>
>>>>> For me this is still Question 1 -- 'everything in that pattern that is
>>>>> not "N"'.
>>>>>
>>>>> You seem to care about the *semantics* of that OFW device path fragment.
>>>>> I don't. First, the relevant IEEE spec is prohibitively hard for me to
>>>>> interpret semantically. Second, there is no known firmware that actually
>>>>> looks at the "i0cf8" unit-address term and decides *based on that term*
>>>>> that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
>>>>> second node is entirely opaque in my interpretation.
>>>>>
>>>>>> unconditionally - not if (QEMU).
>>>>>
>>>>> This might qualify as some kind of semantic cleanup, but it will
>>>>> nonetheless break the SeaBIOS boot options expressed in OFW notation
>>>>> that are already persistently stored in cbfs, on physical machines. (As
>>>>> far as I understood.) It might not break the Coreboot-SeaBIOS interface,
>>>>> but it might invalidate preexistent entries that exist in the prior form
>>>>> (wherever they exist on physical hardware).
>>>>>
>>>>>> And I thought Kevin agreed
>>>>>> it's a good idea.
>>>>>>
>>>>>> Kevin - is this a good summary of your opinion?
>>>>>
>>>>> Kevin, please do answer.
>>>>
>>>> It is true that it would "invalidate preexistent entries" for
>>>> coreboot/seabios users that upgrade, but I think that is manageable.
>>>> So I defer the syntax discussion and decisions to the QEMU developers
>>>> that are doing the bulk of the work.
>>>>
>>>> -Kevin
>>>
>>> I'm fine with either /pci@i0cf8,%x or /pci-root@%x/pci@i0cf8, with a
>>> slight preference to the later - in particular it's easier
>>> to implement in QEMU.
>>>
>>> It means old bios won't boot from a pxb, but I think that's
>>> manageable - it works otherwise.
>>
>> I don't understand -- the second option you named
>> ("/pci-root@%x/pci@i0cf8") is what this patch implements, and "old" (ie.
>> current) SeaBIOS does boot from it.
>>
>> Laszlo
>
> Ouch. I meant /pci@i0cf8//pci-root@%x.
> As you see, it's confusing.
If you insist on /pci@i0cf8/pci-root@%x, then all of SeaBIOS, QEMU, and
OVMF must be (further) modified. Please confirm if this is what you'd like.
(As I said, IMO this change is not warranted for; it just replaces one
opaque string with another opaque string, without semantic effects, but
it causes extra churn and even requires a patch for SeaBIOS.)
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-18 13:22 ` Laszlo Ersek
@ 2015-06-18 13:40 ` Michael S. Tsirkin
2015-06-18 15:42 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-18 13:40 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
On Thu, Jun 18, 2015 at 03:22:59PM +0200, Laszlo Ersek wrote:
> On 06/17/15 23:50, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2015 at 09:44:07PM +0200, Laszlo Ersek wrote:
> >> On 06/17/15 21:32, Michael S. Tsirkin wrote:
> >>> On Wed, Jun 17, 2015 at 03:28:44PM -0400, Kevin O'Connor wrote:
> >>>> On Wed, Jun 17, 2015 at 09:15:24PM +0200, Laszlo Ersek wrote:
> >>>>> On 06/17/15 20:54, Michael S. Tsirkin wrote:
> >>>>>> Right. But what I was discussing is a different issue. The point is
> >>>>>> that it does not make sense to have /pci@i0cf8 under two hierarchies:
> >>>>>> it's the same register. What happens is that you access /pci@i0cf8 and
> >>>>>> then *through that* you access another pci root. Not the other way
> >>>>>> around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
> >>>>>> seabios,
> >>>>>
> >>>>> For me this is still Question 1 -- 'everything in that pattern that is
> >>>>> not "N"'.
> >>>>>
> >>>>> You seem to care about the *semantics* of that OFW device path fragment.
> >>>>> I don't. First, the relevant IEEE spec is prohibitively hard for me to
> >>>>> interpret semantically. Second, there is no known firmware that actually
> >>>>> looks at the "i0cf8" unit-address term and decides *based on that term*
> >>>>> that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
> >>>>> second node is entirely opaque in my interpretation.
> >>>>>
> >>>>>> unconditionally - not if (QEMU).
> >>>>>
> >>>>> This might qualify as some kind of semantic cleanup, but it will
> >>>>> nonetheless break the SeaBIOS boot options expressed in OFW notation
> >>>>> that are already persistently stored in cbfs, on physical machines. (As
> >>>>> far as I understood.) It might not break the Coreboot-SeaBIOS interface,
> >>>>> but it might invalidate preexistent entries that exist in the prior form
> >>>>> (wherever they exist on physical hardware).
> >>>>>
> >>>>>> And I thought Kevin agreed
> >>>>>> it's a good idea.
> >>>>>>
> >>>>>> Kevin - is this a good summary of your opinion?
> >>>>>
> >>>>> Kevin, please do answer.
> >>>>
> >>>> It is true that it would "invalidate preexistent entries" for
> >>>> coreboot/seabios users that upgrade, but I think that is manageable.
> >>>> So I defer the syntax discussion and decisions to the QEMU developers
> >>>> that are doing the bulk of the work.
> >>>>
> >>>> -Kevin
> >>>
> >>> I'm fine with either /pci@i0cf8,%x or /pci-root@%x/pci@i0cf8, with a
> >>> slight preference to the later - in particular it's easier
> >>> to implement in QEMU.
> >>>
> >>> It means old bios won't boot from a pxb, but I think that's
> >>> manageable - it works otherwise.
> >>
> >> I don't understand -- the second option you named
> >> ("/pci-root@%x/pci@i0cf8") is what this patch implements, and "old" (ie.
> >> current) SeaBIOS does boot from it.
> >>
> >> Laszlo
> >
> > Ouch. I meant /pci@i0cf8//pci-root@%x.
> > As you see, it's confusing.
>
> If you insist on /pci@i0cf8/pci-root@%x, then all of SeaBIOS, QEMU, and
> OVMF must be (further) modified. Please confirm if this is what you'd like.
>
> (As I said, IMO this change is not warranted for; it just replaces one
> opaque string with another opaque string, without semantic effects, but
> it causes extra churn and even requires a patch for SeaBIOS.)
>
> Laszlo
I think I prefer the string to match the actual hierarchy (using any
syntax), yes. Current guests don't seem to care but this needs to
be maintained forever, worth being as correct as we can be.
--
MST
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-18 13:40 ` Michael S. Tsirkin
@ 2015-06-18 15:42 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-18 15:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel,
Markus Armbruster
On 06/18/15 15:40, Michael S. Tsirkin wrote:
> On Thu, Jun 18, 2015 at 03:22:59PM +0200, Laszlo Ersek wrote:
>> On 06/17/15 23:50, Michael S. Tsirkin wrote:
>>> On Wed, Jun 17, 2015 at 09:44:07PM +0200, Laszlo Ersek wrote:
>>>> On 06/17/15 21:32, Michael S. Tsirkin wrote:
>>>>> On Wed, Jun 17, 2015 at 03:28:44PM -0400, Kevin O'Connor wrote:
>>>>>> On Wed, Jun 17, 2015 at 09:15:24PM +0200, Laszlo Ersek wrote:
>>>>>>> On 06/17/15 20:54, Michael S. Tsirkin wrote:
>>>>>>>> Right. But what I was discussing is a different issue. The point is
>>>>>>>> that it does not make sense to have /pci@i0cf8 under two hierarchies:
>>>>>>>> it's the same register. What happens is that you access /pci@i0cf8 and
>>>>>>>> then *through that* you access another pci root. Not the other way
>>>>>>>> around. The proposal thus is to switch to /pci@i0cf8/pci-root@N in
>>>>>>>> seabios,
>>>>>>>
>>>>>>> For me this is still Question 1 -- 'everything in that pattern that is
>>>>>>> not "N"'.
>>>>>>>
>>>>>>> You seem to care about the *semantics* of that OFW device path fragment.
>>>>>>> I don't. First, the relevant IEEE spec is prohibitively hard for me to
>>>>>>> interpret semantically. Second, there is no known firmware that actually
>>>>>>> looks at the "i0cf8" unit-address term and decides *based on that term*
>>>>>>> that it has to talk PCI via 0xCF8 and 0xCFC. In other words, the current
>>>>>>> second node is entirely opaque in my interpretation.
>>>>>>>
>>>>>>>> unconditionally - not if (QEMU).
>>>>>>>
>>>>>>> This might qualify as some kind of semantic cleanup, but it will
>>>>>>> nonetheless break the SeaBIOS boot options expressed in OFW notation
>>>>>>> that are already persistently stored in cbfs, on physical machines. (As
>>>>>>> far as I understood.) It might not break the Coreboot-SeaBIOS interface,
>>>>>>> but it might invalidate preexistent entries that exist in the prior form
>>>>>>> (wherever they exist on physical hardware).
>>>>>>>
>>>>>>>> And I thought Kevin agreed
>>>>>>>> it's a good idea.
>>>>>>>>
>>>>>>>> Kevin - is this a good summary of your opinion?
>>>>>>>
>>>>>>> Kevin, please do answer.
>>>>>>
>>>>>> It is true that it would "invalidate preexistent entries" for
>>>>>> coreboot/seabios users that upgrade, but I think that is manageable.
>>>>>> So I defer the syntax discussion and decisions to the QEMU developers
>>>>>> that are doing the bulk of the work.
>>>>>>
>>>>>> -Kevin
>>>>>
>>>>> I'm fine with either /pci@i0cf8,%x or /pci-root@%x/pci@i0cf8, with a
>>>>> slight preference to the later - in particular it's easier
>>>>> to implement in QEMU.
>>>>>
>>>>> It means old bios won't boot from a pxb, but I think that's
>>>>> manageable - it works otherwise.
>>>>
>>>> I don't understand -- the second option you named
>>>> ("/pci-root@%x/pci@i0cf8") is what this patch implements, and "old" (ie.
>>>> current) SeaBIOS does boot from it.
>>>>
>>>> Laszlo
>>>
>>> Ouch. I meant /pci@i0cf8//pci-root@%x.
>>> As you see, it's confusing.
>>
>> If you insist on /pci@i0cf8/pci-root@%x, then all of SeaBIOS, QEMU, and
>> OVMF must be (further) modified. Please confirm if this is what you'd like.
>>
>> (As I said, IMO this change is not warranted for; it just replaces one
>> opaque string with another opaque string, without semantic effects, but
>> it causes extra churn and even requires a patch for SeaBIOS.)
>>
>> Laszlo
>
> I think I prefer the string to match the actual hierarchy (using any
> syntax), yes. Current guests don't seem to care but this needs to
> be maintained forever, worth being as correct as we can be.
Alright. When I find the drive in myself to do so, I'll post a v7 with
patches v6 #1 through #4 included, addressing your pci-bridge comments
on top. (If Marcel would prefer to take over those patches immediately,
I'm game.)
Patch #5 you have already included in a pull request, Cc'ing stable;
thanks for that.
Patches #6 and #7 I am hereby dropping (the boot order sub-feature). I
might revoke the related OVMF-side patches from my latest (v2) OVMF
series, or just let them go in in their current form. Once this
sub-feature is sorted out between QEMU and SeaBIOS, I might revisit the
related OVMF patches. Since we discussed this topic several times over,
I trust whatever we'll find in QEMU at that point shall be possible to
support in OVMF.
I don't necessarily want to sneak patches #6 and #7 onto Marcel's plate
-- because they are a feature not intimately related to the expander
bridge's core functionality --, so I guess #6 and #7 are free for the
taking, for anyone who cares enough (including you).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 18:16 ` Laszlo Ersek
2015-06-17 18:54 ` Michael S. Tsirkin
@ 2015-06-17 19:09 ` Kevin O'Connor
1 sibling, 0 replies; 41+ messages in thread
From: Kevin O'Connor @ 2015-06-17 19:09 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Marcel Apfelbaum, Markus Armbruster, qemu-devel,
Michael S. Tsirkin
On Wed, Jun 17, 2015 at 08:16:30PM +0200, Laszlo Ersek wrote:
> As discussed earlier, there are two questions to consider about the OFW
> devpath pattern
>
> /pci-root@N/pci@i0cf8/...
>
> that SeaBIOS currently recognizes for devices that reside behind extra
> PCI root buses.
>
> Q1: everything in that pattern that is not "N"
> Q2: what goes into N
>
> These are independent questions.
>
> Marcel's patch for SeaBIOS intended to change SeaBIOS's behavior for
> both Q1 and Q2:
>
> - For Q1, the proposed OFW devpath fragment was
>
> /pci-root@N/...
>
> ie. dropping the second node ("pci@i0cf8").
>
> - For Q2, the proposed change was: instead of making N a *serial number*
> (where N stood for the N'th extra root bus discovered by SeaBIOS), make
> N an actual bus number.
>
> However, applying these changes unconditionally would have broken the
> interface between Coreboot and SeaBIOS, in physical hardware
> environments (because Coreboot agrees with SeaBIOS on the current syntax
> (Q1) and interpretation (Q2) of the devpath pattern). Therefore there
> was an idea to make both SeaBIOS changes conditional on runningOnQemu().
Just to be clear, there is no syntax requirement between coreboot and
SeaBIOS with respect to Q1. The only requirement is with respect to
Q2. Coreboot code doesn't read or write the bootorder file - it's
treated as a static user provided config file.
> As far as I remember, Kevin was more or less okay with that, but (again,
> as far as I remember) he did express a mild dislike for such tweaks.
I agree with previous comments about this email discussion going on
too long. I apologize if my earlier comments drove excess
development. My only requirement is that we not break a
seabios/coreboot feature (ie, not change Q2 if not on qemu).
The rest of your email seems like an accurate synopsis to me.
-Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB Laszlo Ersek
2015-06-17 13:57 ` Michael S. Tsirkin
@ 2015-06-17 19:21 ` Michael S. Tsirkin
2015-06-17 19:35 ` Laszlo Ersek
2015-06-17 19:38 ` Kevin O'Connor
1 sibling, 2 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 19:21 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel
On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
> to follow the pattern
>
> /pci-root@N/pci@i0cf8/...
>
> for devices that live behind an extra root bus. The extra root bus in
> question is the N'th among the extra root bridges. (In other words, N
> gives the position of the affected extra root bus relative to the other
> extra root buses, in bus_nr order.) N starts at 1, and is formatted in
> hex.
>
> The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
> FW_PCI_DOMAIN).
>
> Cc: Kevin O'Connor <kevin@koconnor.net>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>
> Notes:
> v6:
> - no changes
>
> v5:
> - constify parameter and local variables of pxb_host_ofw_unit_address(),
> in accord with the previous patch
>
> v4:
> - new in v4
>
> hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 4398d98..37574ec 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -42,6 +42,8 @@ typedef struct PXBDev {
> uint16_t numa_node;
> } PXBDev;
>
> +static GList *pxb_dev_list;
> +
> #define TYPE_PXB_HOST "pxb-host"
>
> static int pxb_bus_num(PCIBus *bus)
So this trick breaks if we ever do have multiple roots, with expanders
under each.
I guess you could go to parent bus, scan child devices of parent and
count TYPE_PXB_DEVICE devices there.
BTW does this work if we have pci bridges under the default root in
addition to PXBs? Does seabios know to skip regular bridges when it's
counting roots?
> @@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
> return bus->bus_path;
> }
>
> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
> +{
> + const PCIHostState *host = PCI_HOST_BRIDGE(dev);
> + const PCIBus *bus;
> + const PXBDev *pxb;
> + int position;
> +
> + bus = host->bus;
> + pxb = PXB_DEV(bus->parent_dev);
> + position = g_list_index(pxb_dev_list, pxb);
> + assert(position >= 0);
> +
> + return g_strdup_printf("%x/pci@i0cf8", position + 1);
> +}
> +
> static void pxb_host_class_init(ObjectClass *class, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(class);
> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>
> - dc->fw_name = "pci";
> + dc->fw_name = "pci-root";
> + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
> hc->root_bus_path = pxb_host_root_bus_path;
> }
>
> @@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
> return pin - PCI_SLOT(pxb->devfn);
> }
>
> +static gint pxb_compare(gconstpointer a, gconstpointer b)
> +{
> + const PXBDev *pxb_a = a, *pxb_b = b;
> +
> + return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
> + pxb_a->bus_nr > pxb_b->bus_nr ? 1 :
> + 0;
> +}
> +
> static int pxb_dev_initfn(PCIDevice *dev)
> {
> PXBDev *pxb = PXB_DEV(dev);
> @@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
> PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>
> + pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
> return 0;
> }
>
> +static void pxb_dev_exitfn(PCIDevice *pci_dev)
> +{
> + PXBDev *pxb = PXB_DEV(pci_dev);
> +
> + pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
> +}
> +
> static Property pxb_dev_properties[] = {
> /* Note: 0 is not a legal a PXB bus number. */
> DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
> @@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> k->init = pxb_dev_initfn;
> + k->exit = pxb_dev_exitfn;
> k->vendor_id = PCI_VENDOR_ID_REDHAT;
> k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
> k->class_id = PCI_CLASS_BRIDGE_HOST;
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 19:21 ` Michael S. Tsirkin
@ 2015-06-17 19:35 ` Laszlo Ersek
2015-06-17 21:49 ` Michael S. Tsirkin
2015-06-17 19:38 ` Kevin O'Connor
1 sibling, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-17 19:35 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel
On 06/17/15 21:21, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
>> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
>> to follow the pattern
>>
>> /pci-root@N/pci@i0cf8/...
>>
>> for devices that live behind an extra root bus. The extra root bus in
>> question is the N'th among the extra root bridges. (In other words, N
>> gives the position of the affected extra root bus relative to the other
>> extra root buses, in bus_nr order.) N starts at 1, and is formatted in
>> hex.
>>
>> The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
>> FW_PCI_DOMAIN).
>>
>> Cc: Kevin O'Connor <kevin@koconnor.net>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>
>> Notes:
>> v6:
>> - no changes
>>
>> v5:
>> - constify parameter and local variables of pxb_host_ofw_unit_address(),
>> in accord with the previous patch
>>
>> v4:
>> - new in v4
>>
>> hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index 4398d98..37574ec 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -42,6 +42,8 @@ typedef struct PXBDev {
>> uint16_t numa_node;
>> } PXBDev;
>>
>> +static GList *pxb_dev_list;
>> +
>> #define TYPE_PXB_HOST "pxb-host"
>>
>> static int pxb_bus_num(PCIBus *bus)
>
>
> So this trick breaks if we ever do have multiple roots, with expanders
> under each.
Assuming I understand what you mean by "multiple roots", I guess the
trick could be extended to maintain a separate list of expanders for
each root.
> I guess you could go to parent bus, scan child devices of parent and
> count TYPE_PXB_DEVICE devices there.
>
>
> BTW does this work if we have pci bridges under the default root in
> addition to PXBs?
I have no clue how to configure that on the QEMU command line, sorry.
> Does seabios know to skip regular bridges when it's
> counting roots?
Something for Kevin to answer :)
The initial scan in OVMF doesn't look for regular bridges specifically.
When a device in 0x00..0x1f, function 0, responds from a bus in
0x01..0xff, that bus is considered a live extra root bus. The bridges
that hang off the expanders come to life much later, when the generic
edk2 PCI bus driver enumerates devices (on the root buses that the OVMF
PCI host bridge / root bridge driver produced). No clue about regular
bridges.
Thanks
Laszlo
>> @@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
>> return bus->bus_path;
>> }
>>
>> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
>> +{
>> + const PCIHostState *host = PCI_HOST_BRIDGE(dev);
>> + const PCIBus *bus;
>> + const PXBDev *pxb;
>> + int position;
>> +
>> + bus = host->bus;
>> + pxb = PXB_DEV(bus->parent_dev);
>> + position = g_list_index(pxb_dev_list, pxb);
>> + assert(position >= 0);
>> +
>> + return g_strdup_printf("%x/pci@i0cf8", position + 1);
>> +}
>> +
>> static void pxb_host_class_init(ObjectClass *class, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(class);
>> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
>> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>>
>> - dc->fw_name = "pci";
>> + dc->fw_name = "pci-root";
>> + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
>> hc->root_bus_path = pxb_host_root_bus_path;
>> }
>>
>> @@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
>> return pin - PCI_SLOT(pxb->devfn);
>> }
>>
>> +static gint pxb_compare(gconstpointer a, gconstpointer b)
>> +{
>> + const PXBDev *pxb_a = a, *pxb_b = b;
>> +
>> + return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
>> + pxb_a->bus_nr > pxb_b->bus_nr ? 1 :
>> + 0;
>> +}
>> +
>> static int pxb_dev_initfn(PCIDevice *dev)
>> {
>> PXBDev *pxb = PXB_DEV(dev);
>> @@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
>> PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
>> pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>>
>> + pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
>> return 0;
>> }
>>
>> +static void pxb_dev_exitfn(PCIDevice *pci_dev)
>> +{
>> + PXBDev *pxb = PXB_DEV(pci_dev);
>> +
>> + pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
>> +}
>> +
>> static Property pxb_dev_properties[] = {
>> /* Note: 0 is not a legal a PXB bus number. */
>> DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
>> @@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>
>> k->init = pxb_dev_initfn;
>> + k->exit = pxb_dev_exitfn;
>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>> k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>> k->class_id = PCI_CLASS_BRIDGE_HOST;
>> --
>> 1.8.3.1
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 19:35 ` Laszlo Ersek
@ 2015-06-17 21:49 ` Michael S. Tsirkin
2015-06-18 13:18 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 21:49 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel
On Wed, Jun 17, 2015 at 09:35:20PM +0200, Laszlo Ersek wrote:
> On 06/17/15 21:21, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
> >> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
> >> to follow the pattern
> >>
> >> /pci-root@N/pci@i0cf8/...
> >>
> >> for devices that live behind an extra root bus. The extra root bus in
> >> question is the N'th among the extra root bridges. (In other words, N
> >> gives the position of the affected extra root bus relative to the other
> >> extra root buses, in bus_nr order.) N starts at 1, and is formatted in
> >> hex.
> >>
> >> The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
> >> FW_PCI_DOMAIN).
> >>
> >> Cc: Kevin O'Connor <kevin@koconnor.net>
> >> Cc: Marcel Apfelbaum <marcel@redhat.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
> >> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> >> ---
> >>
> >> Notes:
> >> v6:
> >> - no changes
> >>
> >> v5:
> >> - constify parameter and local variables of pxb_host_ofw_unit_address(),
> >> in accord with the previous patch
> >>
> >> v4:
> >> - new in v4
> >>
> >> hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 38 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> >> index 4398d98..37574ec 100644
> >> --- a/hw/pci-bridge/pci_expander_bridge.c
> >> +++ b/hw/pci-bridge/pci_expander_bridge.c
> >> @@ -42,6 +42,8 @@ typedef struct PXBDev {
> >> uint16_t numa_node;
> >> } PXBDev;
> >>
> >> +static GList *pxb_dev_list;
> >> +
> >> #define TYPE_PXB_HOST "pxb-host"
> >>
> >> static int pxb_bus_num(PCIBus *bus)
> >
> >
> > So this trick breaks if we ever do have multiple roots, with expanders
> > under each.
>
> Assuming I understand what you mean by "multiple roots", I guess the
> trick could be extended to maintain a separate list of expanders for
> each root.
You don't need an extra list: expander are already linked
under root.
> > I guess you could go to parent bus, scan child devices of parent and
> > count TYPE_PXB_DEVICE devices there.
> >
> >
> > BTW does this work if we have pci bridges under the default root in
> > addition to PXBs?
>
> I have no clue how to configure that on the QEMU command line, sorry.
Simple -device pvi-bridge-device,chassis_nr=XXX,id=foo then attach
devices there.
> > Does seabios know to skip regular bridges when it's
> > counting roots?
>
> Something for Kevin to answer :)
>
> The initial scan in OVMF doesn't look for regular bridges specifically.
> When a device in 0x00..0x1f, function 0, responds from a bus in
> 0x01..0xff, that bus is considered a live extra root bus.
Do you do it before or after configuring bridges under root bus 0?
If after, then it's a bug - you will discover regular bridges this way.
> The bridges
> that hang off the expanders come to life much later, when the generic
> edk2 PCI bus driver enumerates devices (on the root buses that the OVMF
> PCI host bridge / root bridge driver produced). No clue about regular
> bridges.
>
> Thanks
> Laszlo
>
> >> @@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
> >> return bus->bus_path;
> >> }
> >>
> >> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
> >> +{
> >> + const PCIHostState *host = PCI_HOST_BRIDGE(dev);
> >> + const PCIBus *bus;
> >> + const PXBDev *pxb;
> >> + int position;
> >> +
> >> + bus = host->bus;
> >> + pxb = PXB_DEV(bus->parent_dev);
> >> + position = g_list_index(pxb_dev_list, pxb);
> >> + assert(position >= 0);
> >> +
> >> + return g_strdup_printf("%x/pci@i0cf8", position + 1);
> >> +}
> >> +
> >> static void pxb_host_class_init(ObjectClass *class, void *data)
> >> {
> >> DeviceClass *dc = DEVICE_CLASS(class);
> >> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
> >> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
> >>
> >> - dc->fw_name = "pci";
> >> + dc->fw_name = "pci-root";
> >> + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
> >> hc->root_bus_path = pxb_host_root_bus_path;
> >> }
> >>
> >> @@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
> >> return pin - PCI_SLOT(pxb->devfn);
> >> }
> >>
> >> +static gint pxb_compare(gconstpointer a, gconstpointer b)
> >> +{
> >> + const PXBDev *pxb_a = a, *pxb_b = b;
> >> +
> >> + return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
> >> + pxb_a->bus_nr > pxb_b->bus_nr ? 1 :
> >> + 0;
> >> +}
> >> +
> >> static int pxb_dev_initfn(PCIDevice *dev)
> >> {
> >> PXBDev *pxb = PXB_DEV(dev);
> >> @@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
> >> PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> >> pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
> >>
> >> + pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
> >> return 0;
> >> }
> >>
> >> +static void pxb_dev_exitfn(PCIDevice *pci_dev)
> >> +{
> >> + PXBDev *pxb = PXB_DEV(pci_dev);
> >> +
> >> + pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
> >> +}
> >> +
> >> static Property pxb_dev_properties[] = {
> >> /* Note: 0 is not a legal a PXB bus number. */
> >> DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
> >> @@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
> >> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>
> >> k->init = pxb_dev_initfn;
> >> + k->exit = pxb_dev_exitfn;
> >> k->vendor_id = PCI_VENDOR_ID_REDHAT;
> >> k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
> >> k->class_id = PCI_CLASS_BRIDGE_HOST;
> >> --
> >> 1.8.3.1
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 21:49 ` Michael S. Tsirkin
@ 2015-06-18 13:18 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2015-06-18 13:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, Kevin O'Connor, qemu-devel
On 06/17/15 23:49, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 09:35:20PM +0200, Laszlo Ersek wrote:
>> On 06/17/15 21:21, Michael S. Tsirkin wrote:
>>> On Wed, Jun 17, 2015 at 02:45:05PM +0200, Laszlo Ersek wrote:
>>>> SeaBIOS expects OpenFirmware device paths in the "bootorder" fw_cfg file
>>>> to follow the pattern
>>>>
>>>> /pci-root@N/pci@i0cf8/...
>>>>
>>>> for devices that live behind an extra root bus. The extra root bus in
>>>> question is the N'th among the extra root bridges. (In other words, N
>>>> gives the position of the affected extra root bus relative to the other
>>>> extra root buses, in bus_nr order.) N starts at 1, and is formatted in
>>>> hex.
>>>>
>>>> The "pci@i0cf8" node text is hardcoded in SeaBIOS (see the macro
>>>> FW_PCI_DOMAIN).
>>>>
>>>> Cc: Kevin O'Connor <kevin@koconnor.net>
>>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> Tested-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>>
>>>> Notes:
>>>> v6:
>>>> - no changes
>>>>
>>>> v5:
>>>> - constify parameter and local variables of pxb_host_ofw_unit_address(),
>>>> in accord with the previous patch
>>>>
>>>> v4:
>>>> - new in v4
>>>>
>>>> hw/pci-bridge/pci_expander_bridge.c | 39 ++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 38 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>>>> index 4398d98..37574ec 100644
>>>> --- a/hw/pci-bridge/pci_expander_bridge.c
>>>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>>>> @@ -42,6 +42,8 @@ typedef struct PXBDev {
>>>> uint16_t numa_node;
>>>> } PXBDev;
>>>>
>>>> +static GList *pxb_dev_list;
>>>> +
>>>> #define TYPE_PXB_HOST "pxb-host"
>>>>
>>>> static int pxb_bus_num(PCIBus *bus)
>>>
>>>
>>> So this trick breaks if we ever do have multiple roots, with expanders
>>> under each.
>>
>> Assuming I understand what you mean by "multiple roots", I guess the
>> trick could be extended to maintain a separate list of expanders for
>> each root.
>
> You don't need an extra list: expander are already linked
> under root.
>
>>> I guess you could go to parent bus, scan child devices of parent and
>>> count TYPE_PXB_DEVICE devices there.
>>>
>>>
>>> BTW does this work if we have pci bridges under the default root in
>>> addition to PXBs?
>>
>> I have no clue how to configure that on the QEMU command line, sorry.
>
> Simple -device pvi-bridge-device,chassis_nr=XXX,id=foo then attach
> devices there.
>
>>> Does seabios know to skip regular bridges when it's
>>> counting roots?
>>
>> Something for Kevin to answer :)
>>
>> The initial scan in OVMF doesn't look for regular bridges specifically.
>> When a device in 0x00..0x1f, function 0, responds from a bus in
>> 0x01..0xff, that bus is considered a live extra root bus.
>
> Do you do it before or after configuring bridges under root bus 0?
Before.
The driver that scans the roots and produces
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances is OVMF's own (cloned and
modified from another platform's similar driver). The driver that
configures bridges and does the rest of the enumeration is the generic
PCI bus driver that OVMF only includes without modifications. That
driver depends on the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances produced
by the former.
Thanks
Laszlo
> If after, then it's a bug - you will discover regular bridges this way.
>
>
>> The bridges
>> that hang off the expanders come to life much later, when the generic
>> edk2 PCI bus driver enumerates devices (on the root buses that the OVMF
>> PCI host bridge / root bridge driver produced). No clue about regular
>> bridges.
>>
>> Thanks
>> Laszlo
>>
>>>> @@ -88,12 +90,29 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
>>>> return bus->bus_path;
>>>> }
>>>>
>>>> +static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
>>>> +{
>>>> + const PCIHostState *host = PCI_HOST_BRIDGE(dev);
>>>> + const PCIBus *bus;
>>>> + const PXBDev *pxb;
>>>> + int position;
>>>> +
>>>> + bus = host->bus;
>>>> + pxb = PXB_DEV(bus->parent_dev);
>>>> + position = g_list_index(pxb_dev_list, pxb);
>>>> + assert(position >= 0);
>>>> +
>>>> + return g_strdup_printf("%x/pci@i0cf8", position + 1);
>>>> +}
>>>> +
>>>> static void pxb_host_class_init(ObjectClass *class, void *data)
>>>> {
>>>> DeviceClass *dc = DEVICE_CLASS(class);
>>>> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
>>>> PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
>>>>
>>>> - dc->fw_name = "pci";
>>>> + dc->fw_name = "pci-root";
>>>> + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address;
>>>> hc->root_bus_path = pxb_host_root_bus_path;
>>>> }
>>>>
>>>> @@ -148,6 +167,15 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
>>>> return pin - PCI_SLOT(pxb->devfn);
>>>> }
>>>>
>>>> +static gint pxb_compare(gconstpointer a, gconstpointer b)
>>>> +{
>>>> + const PXBDev *pxb_a = a, *pxb_b = b;
>>>> +
>>>> + return pxb_a->bus_nr < pxb_b->bus_nr ? -1 :
>>>> + pxb_a->bus_nr > pxb_b->bus_nr ? 1 :
>>>> + 0;
>>>> +}
>>>> +
>>>> static int pxb_dev_initfn(PCIDevice *dev)
>>>> {
>>>> PXBDev *pxb = PXB_DEV(dev);
>>>> @@ -191,9 +219,17 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>>> PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
>>>> pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>>>>
>>>> + pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
>>>> return 0;
>>>> }
>>>>
>>>> +static void pxb_dev_exitfn(PCIDevice *pci_dev)
>>>> +{
>>>> + PXBDev *pxb = PXB_DEV(pci_dev);
>>>> +
>>>> + pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
>>>> +}
>>>> +
>>>> static Property pxb_dev_properties[] = {
>>>> /* Note: 0 is not a legal a PXB bus number. */
>>>> DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
>>>> @@ -207,6 +243,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
>>>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>>
>>>> k->init = pxb_dev_initfn;
>>>> + k->exit = pxb_dev_exitfn;
>>>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>> k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>>>> k->class_id = PCI_CLASS_BRIDGE_HOST;
>>>> --
>>>> 1.8.3.1
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB
2015-06-17 19:21 ` Michael S. Tsirkin
2015-06-17 19:35 ` Laszlo Ersek
@ 2015-06-17 19:38 ` Kevin O'Connor
1 sibling, 0 replies; 41+ messages in thread
From: Kevin O'Connor @ 2015-06-17 19:38 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, Laszlo Ersek, qemu-devel
On Wed, Jun 17, 2015 at 09:21:00PM +0200, Michael S. Tsirkin wrote:
> BTW does this work if we have pci bridges under the default root in
> addition to PXBs? Does seabios know to skip regular bridges when it's
> counting roots?
Yes - SeaBIOS will skip regular pci-to-pci bridges in its scan for
extra roots.
-Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread