* [PATCH v2] hw/pci/pci_bridge: ensure PCIe slots have only one slot
@ 2022-07-19 8:01 Roman Kagan
2022-07-19 10:42 ` Thomas Huth
0 siblings, 1 reply; 3+ messages in thread
From: Roman Kagan @ 2022-07-19 8:01 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
Laurent Vivier, yc-core, Thomas Huth
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
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'] : []) + \
--
2.36.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] hw/pci/pci_bridge: ensure PCIe slots have only one slot
2022-07-19 8:01 [PATCH v2] hw/pci/pci_bridge: ensure PCIe slots have only one slot Roman Kagan
@ 2022-07-19 10:42 ` Thomas Huth
2022-07-20 10:18 ` Roman Kagan
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Huth @ 2022-07-19 10:42 UTC (permalink / raw)
To: Roman Kagan, qemu-devel
Cc: Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
Laurent Vivier, yc-core, qemu-ppc@nongnu.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] hw/pci/pci_bridge: ensure PCIe slots have only one slot
2022-07-19 10:42 ` Thomas Huth
@ 2022-07-20 10:18 ` Roman Kagan
0 siblings, 0 replies; 3+ messages in thread
From: Roman Kagan @ 2022-07-20 10:18 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Paolo Bonzini, Marcel Apfelbaum, Michael S. Tsirkin,
Laurent Vivier, yc-core, qemu-ppc@nongnu.org
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-20 10:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-19 8:01 [PATCH v2] hw/pci/pci_bridge: ensure PCIe slots have only one slot Roman Kagan
2022-07-19 10:42 ` Thomas Huth
2022-07-20 10:18 ` Roman Kagan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).