| Message ID | 20220719080123.553981-1-rvkagan@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/pci/pci_bridge: ensure PCIe slots have only one slot | expand |
On 19/07/2022 10.01, Roman Kagan wrote: > It's possible to create non-working configurations by attaching a device > to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and > specifying a slot number other that zero, e.g.: > > -device pcie-root-port,id=s0,... \ > -device virtio-blk-pci,bus=s0,addr=4,... > > Make QEMU reject such configurations and only allow addr=0 on the > secondary bus of a PCIe slot. > > To verify this new behavior, add two basic qtests for the PCIe bridges > that may be affected by change: pcie-root-port and x3130. For the > former, two testcases are included, one positive for slot #0 and one > negative for (arbitrary) slot #4; for the latter, only a positive > testcase for slot #4 is included. > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> > --- > v1 -> v2: > - use object_dynamic_cast (without assert) [Vladimir] > - add explaining comment [Michael] > - add tests > (I've only had a chance to run tests against x86; hope I didn't mess > them up for other arches) With my s390x hat on: This should not affect s390x - we're not using PCIe there. > hw/pci/pci_bridge.c | 6 +++ > tests/qtest/pcie-root-port-test.c | 77 +++++++++++++++++++++++++++++++ > tests/qtest/xio3130-test.c | 54 ++++++++++++++++++++++ > tests/qtest/meson.build | 2 + > 4 files changed, 139 insertions(+) > create mode 100644 tests/qtest/pcie-root-port-test.c > create mode 100644 tests/qtest/xio3130-test.c > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c > index da34c8ebcd..23e1701d06 100644 > --- a/hw/pci/pci_bridge.c > +++ b/hw/pci/pci_bridge.c > @@ -33,6 +33,7 @@ > #include "qemu/units.h" > #include "hw/pci/pci_bridge.h" > #include "hw/pci/pci_bus.h" > +#include "hw/pci/pcie_port.h" > #include "qemu/module.h" > #include "qemu/range.h" > #include "qapi/error.h" > @@ -386,6 +387,11 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename) > br->windows = pci_bridge_region_init(br); > QLIST_INIT(&sec_bus->child); > QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling); > + > + /* PCIe slot derivatives are bridges with a single slot; enforce that */ > + if (object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT)) { > + sec_bus->slot_reserved_mask = ~1u; > + } > } > > /* default qdev clean up function for PCI-to-PCI bridge */ > diff --git a/tests/qtest/pcie-root-port-test.c b/tests/qtest/pcie-root-port-test.c > new file mode 100644 > index 0000000000..a1f3d84d75 > --- /dev/null > +++ b/tests/qtest/pcie-root-port-test.c > @@ -0,0 +1,77 @@ > +/* > + * QTest testcase for generic PCIe root port > + * > + * Copyright (c) 2022 Yandex N.V. > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > + > +#include "qemu/osdep.h" > +#include "libqtest-single.h" Do you really need libqtest-single.h here? libqtest.h should be enough, shouldn't it? > +/* > + * Let QEMU choose the bus and slot for the device under test. It may even be > + * a non-PCIe bus but it's ok for the purpose of the test. > + */ > +static const char *common_args = "-device pcie-root-port,id=s0" > + ",port=1,chassis=1,multifunction=on"; > + > +static void test_slot0(void) > +{ > + QTestState *qts; > + QDict *resp; > + > + /* attach a PCIe device into slot0 of the root port */ > + qts = qtest_init(common_args); > + /* PCIe root port is known to be supported, use it as a leaf device too */ > + resp = qtest_qmp(qts, "{'execute': 'device_add', 'arguments': {" > + "'driver': 'pcie-root-port', " > + "'id': 'port1', " > + "'bus': 's0', " > + "'chassis': 5, " > + "'addr': '0'" > + "} }"); > + g_assert_nonnull(resp); > + g_assert(!qdict_haskey(resp, "event")); > + g_assert(!qdict_haskey(resp, "error")); > + qobject_unref(resp); > + > + qtest_quit(qts); > +} > + > +static void test_slot4(void) > +{ > + QTestState *qts; > + QDict *resp; > + > + /* attach a PCIe device into slot4 of the root port should be rejected */ > + qts = qtest_init(common_args); > + /* PCIe root port is known to be supported, use it as a leaf device too */ > + resp = qtest_qmp(qts, "{'execute': 'device_add', 'arguments': {" > + "'driver': 'pcie-root-port', " > + "'id': 'port1', " > + "'bus': 's0', " > + "'chassis': 5, " > + "'addr': '4'" > + "} }"); > + qmp_expect_error_and_unref(resp, "GenericError"); > + > + qtest_quit(qts); > +} > + > +int main(int argc, char **argv) > +{ > + int ret; > + > + g_test_init(&argc, &argv, NULL); > + > + qtest_add_func("/pcie-root-port/slot0", test_slot0); > + qtest_add_func("/pcie-root-port/slot4", test_slot4); > + > + ret = g_test_run(); > + > + qtest_end(); Please drop the qtest_end() - you're already using qtest_quit in the individual tests. > + return ret; > +} > diff --git a/tests/qtest/xio3130-test.c b/tests/qtest/xio3130-test.c > new file mode 100644 > index 0000000000..3a937889b9 > --- /dev/null > +++ b/tests/qtest/xio3130-test.c > @@ -0,0 +1,54 @@ > +/* > + * QTest testcase for TI X3130 PCIe switch > + * > + * Copyright (c) 2022 Yandex N.V. > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > + > +#include "qemu/osdep.h" > +#include "libqtest-single.h" dito - please switch to libqtest.h > +/* > + * Let QEMU choose the bus and slot for the device under test. It may even be > + * a non-PCIe bus but it's ok for the purpose of the test. > + */ > +static const char *common_args = "-device x3130-upstream,id=s0"; > + > +static void test_slot4(void) > +{ > + QTestState *qts; > + QDict *resp; > + > + /* attach a downstream port into slot4 of the upstream port */ > + qts = qtest_init(common_args); > + resp = qtest_qmp(qts, "{'execute': 'device_add', 'arguments': {" > + "'driver': 'xio3130-downstream', " > + "'id': 'port1', " > + "'bus': 's0', " > + "'chassis': 5, " > + "'addr': '4'" > + "} }"); > + g_assert_nonnull(resp); > + g_assert(!qdict_haskey(resp, "event")); > + g_assert(!qdict_haskey(resp, "error")); > + qobject_unref(resp); > + > + qtest_quit(qts); > +} > + > +int main(int argc, char **argv) > +{ > + int ret; > + > + g_test_init(&argc, &argv, NULL); > + > + qtest_add_func("/x3130/slot4", test_slot4); > + > + ret = g_test_run(); > + > + qtest_end(); Drop qtest_end() also here. > + return ret; > +} > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build > index 31287a9173..19cab1bc35 100644 > --- a/tests/qtest/meson.build > +++ b/tests/qtest/meson.build > @@ -54,6 +54,7 @@ qtests_i386 = \ > (config_all_devices.has_key('CONFIG_I82801B11') ? ['i82801b11-test'] : []) + \ > (config_all_devices.has_key('CONFIG_IOH3420') ? ['ioh3420-test'] : []) + \ > (config_all_devices.has_key('CONFIG_LPC_ICH9') ? ['lpc-ich9-test'] : []) + \ > + (config_all_devices.has_key('CONFIG_PCIE_PORT') ? ['pcie-root-port-test'] : []) + \ > (config_all_devices.has_key('CONFIG_USB_UHCI') ? ['usb-hcd-uhci-test'] : []) + \ > (config_all_devices.has_key('CONFIG_USB_UHCI') and \ > config_all_devices.has_key('CONFIG_USB_EHCI') ? ['usb-hcd-ehci-test'] : []) + \ > @@ -63,6 +64,7 @@ qtests_i386 = \ > (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) + \ > (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : []) + \ > (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + \ > + (config_all_devices.has_key('CONFIG_XIO3130') ? ['xio3130-test'] : []) + \ > (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) + \ > (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? ['fuzz-megasas-test'] : []) + \ > (config_all_devices.has_key('CONFIG_LSI_SCSI_PCI') ? ['fuzz-lsi53c895a-test'] : []) + \ Thomas
On Tue, Jul 19, 2022 at 12:42:47PM +0200, Thomas Huth wrote: > On 19/07/2022 10.01, Roman Kagan wrote: > > +#include "qemu/osdep.h" > > +#include "libqtest-single.h" > > Do you really need libqtest-single.h here? libqtest.h should be enough, > shouldn't it? Indeed, will replace with libqtest.h > > + > > + qtest_end(); > > Please drop the qtest_end() - you're already using qtest_quit in the > individual tests. ok Thanks, Roman.
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index da34c8ebcd..23e1701d06 100644 --- a/hw/pci/pci_bridge.c +++ b/hw/pci/pci_bridge.c @@ -33,6 +33,7 @@ #include "qemu/units.h" #include "hw/pci/pci_bridge.h" #include "hw/pci/pci_bus.h" +#include "hw/pci/pcie_port.h" #include "qemu/module.h" #include "qemu/range.h" #include "qapi/error.h" @@ -386,6 +387,11 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename) br->windows = pci_bridge_region_init(br); QLIST_INIT(&sec_bus->child); QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling); + + /* PCIe slot derivatives are bridges with a single slot; enforce that */ + if (object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT)) { + sec_bus->slot_reserved_mask = ~1u; + } } /* default qdev clean up function for PCI-to-PCI bridge */ diff --git a/tests/qtest/pcie-root-port-test.c b/tests/qtest/pcie-root-port-test.c new file mode 100644 index 0000000000..a1f3d84d75 --- /dev/null +++ b/tests/qtest/pcie-root-port-test.c @@ -0,0 +1,77 @@ +/* + * QTest testcase for generic PCIe root port + * + * Copyright (c) 2022 Yandex N.V. + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "libqtest-single.h" + +/* + * Let QEMU choose the bus and slot for the device under test. It may even be + * a non-PCIe bus but it's ok for the purpose of the test. + */ +static const char *common_args = "-device pcie-root-port,id=s0" + ",port=1,chassis=1,multifunction=on"; + +static void test_slot0(void) +{ + QTestState *qts; + QDict *resp; + + /* attach a PCIe device into slot0 of the root port */ + qts = qtest_init(common_args); + /* PCIe root port is known to be supported, use it as a leaf device too */ + resp = qtest_qmp(qts, "{'execute': 'device_add', 'arguments': {" + "'driver': 'pcie-root-port', " + "'id': 'port1', " + "'bus': 's0', " + "'chassis': 5, " + "'addr': '0'" + "} }"); + g_assert_nonnull(resp); + g_assert(!qdict_haskey(resp, "event")); + g_assert(!qdict_haskey(resp, "error")); + qobject_unref(resp); + + qtest_quit(qts); +} + +static void test_slot4(void) +{ + QTestState *qts; + QDict *resp; + + /* attach a PCIe device into slot4 of the root port should be rejected */ + qts = qtest_init(common_args); + /* PCIe root port is known to be supported, use it as a leaf device too */ + resp = qtest_qmp(qts, "{'execute': 'device_add', 'arguments': {" + "'driver': 'pcie-root-port', " + "'id': 'port1', " + "'bus': 's0', " + "'chassis': 5, " + "'addr': '4'" + "} }"); + qmp_expect_error_and_unref(resp, "GenericError"); + + qtest_quit(qts); +} + +int main(int argc, char **argv) +{ + int ret; + + g_test_init(&argc, &argv, NULL); + + qtest_add_func("/pcie-root-port/slot0", test_slot0); + qtest_add_func("/pcie-root-port/slot4", test_slot4); + + ret = g_test_run(); + + qtest_end(); + + return ret; +} diff --git a/tests/qtest/xio3130-test.c b/tests/qtest/xio3130-test.c new file mode 100644 index 0000000000..3a937889b9 --- /dev/null +++ b/tests/qtest/xio3130-test.c @@ -0,0 +1,54 @@ +/* + * QTest testcase for TI X3130 PCIe switch + * + * Copyright (c) 2022 Yandex N.V. + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "libqtest-single.h" + +/* + * Let QEMU choose the bus and slot for the device under test. It may even be + * a non-PCIe bus but it's ok for the purpose of the test. + */ +static const char *common_args = "-device x3130-upstream,id=s0"; + +static void test_slot4(void) +{ + QTestState *qts; + QDict *resp; + + /* attach a downstream port into slot4 of the upstream port */ + qts = qtest_init(common_args); + resp = qtest_qmp(qts, "{'execute': 'device_add', 'arguments': {" + "'driver': 'xio3130-downstream', " + "'id': 'port1', " + "'bus': 's0', " + "'chassis': 5, " + "'addr': '4'" + "} }"); + g_assert_nonnull(resp); + g_assert(!qdict_haskey(resp, "event")); + g_assert(!qdict_haskey(resp, "error")); + qobject_unref(resp); + + qtest_quit(qts); +} + +int main(int argc, char **argv) +{ + int ret; + + g_test_init(&argc, &argv, NULL); + + qtest_add_func("/x3130/slot4", test_slot4); + + ret = g_test_run(); + + qtest_end(); + + return ret; +} diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 31287a9173..19cab1bc35 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -54,6 +54,7 @@ qtests_i386 = \ (config_all_devices.has_key('CONFIG_I82801B11') ? ['i82801b11-test'] : []) + \ (config_all_devices.has_key('CONFIG_IOH3420') ? ['ioh3420-test'] : []) + \ (config_all_devices.has_key('CONFIG_LPC_ICH9') ? ['lpc-ich9-test'] : []) + \ + (config_all_devices.has_key('CONFIG_PCIE_PORT') ? ['pcie-root-port-test'] : []) + \ (config_all_devices.has_key('CONFIG_USB_UHCI') ? ['usb-hcd-uhci-test'] : []) + \ (config_all_devices.has_key('CONFIG_USB_UHCI') and \ config_all_devices.has_key('CONFIG_USB_EHCI') ? ['usb-hcd-ehci-test'] : []) + \ @@ -63,6 +64,7 @@ qtests_i386 = \ (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) + \ (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : []) + \ (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + \ + (config_all_devices.has_key('CONFIG_XIO3130') ? ['xio3130-test'] : []) + \ (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) + \ (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? ['fuzz-megasas-test'] : []) + \ (config_all_devices.has_key('CONFIG_LSI_SCSI_PCI') ? ['fuzz-lsi53c895a-test'] : []) + \
It's possible to create non-working configurations by attaching a device to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and specifying a slot number other that zero, e.g.: -device pcie-root-port,id=s0,... \ -device virtio-blk-pci,bus=s0,addr=4,... Make QEMU reject such configurations and only allow addr=0 on the secondary bus of a PCIe slot. To verify this new behavior, add two basic qtests for the PCIe bridges that may be affected by change: pcie-root-port and x3130. For the former, two testcases are included, one positive for slot #0 and one negative for (arbitrary) slot #4; for the latter, only a positive testcase for slot #4 is included. Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru> --- v1 -> v2: - use object_dynamic_cast (without assert) [Vladimir] - add explaining comment [Michael] - add tests (I've only had a chance to run tests against x86; hope I didn't mess them up for other arches) hw/pci/pci_bridge.c | 6 +++ tests/qtest/pcie-root-port-test.c | 77 +++++++++++++++++++++++++++++++ tests/qtest/xio3130-test.c | 54 ++++++++++++++++++++++ tests/qtest/meson.build | 2 + 4 files changed, 139 insertions(+) create mode 100644 tests/qtest/pcie-root-port-test.c create mode 100644 tests/qtest/xio3130-test.c