From e2a6290aab578b2170c1f5909fa556385dc0d820 Mon Sep 17 00:00:00 2001 From: Marcel Apfelbaum Date: Mon, 2 Aug 2021 12:00:57 +0300 Subject: [PATCH 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO Q35 has now ACPI hotplug enabled by default for PCI(e) devices. As opposed to native PCIe hotplug, guests like Fedora 34 will not assign IO range to pcie-root-ports not supporting native hotplug, resulting into a regression. Reproduce by: qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio device_add e1000,bus=p1 In the Guest OS the respective pcie-root-port will have the IO range disabled. Fix it by setting the "reserve-io" hint capability of the pcie-root-ports so the firmware will allocate the IO range instead. Acked-by: Igor Mammedov Signed-off-by: Marcel Apfelbaum Message-Id: <20210802090057.1709775-1-marcel@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci-bridge/gen_pcie_root_port.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c index ec9907917e..20099a8ae3 100644 --- a/hw/pci-bridge/gen_pcie_root_port.c +++ b/hw/pci-bridge/gen_pcie_root_port.c @@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort, GEN_PCIE_ROOT_PORT) (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF) #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1 +#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE 4096 struct GenPCIERootPort { /*< private >*/ @@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque, int version_id) static void gen_rp_realize(DeviceState *dev, Error **errp) { PCIDevice *d = PCI_DEVICE(dev); + PCIESlot *s = PCIE_SLOT(d); GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d); PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d); Error *local_err = NULL; @@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error **errp) return; } + if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug) { + grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE; + } int rc = pci_bridge_qemu_reserve_cap_init(d, 0, grp->res_reserve, errp); From d7346e614f4ec353f9b24bb69bfeaf1ade287e07 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 23 Jul 2021 05:04:24 -0400 Subject: [PATCH 2/5] acpi: x86: pcihp: add support hotplug on multifunction bridges Commit [1] switched PCI hotplug from native to ACPI one by default. That however breaks hotplug on following CLI that used to work: -nodefaults -machine q35 \ -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 where PCI device is hotplugged to pcie-root-port-1 with error on guest side: ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) cause is that QEMU's ACPI hotplug never supported functions other then 0 and due to bug it was generating notification entries for not described functions. Technically there is no reason not to describe cold-plugged bridges (root ports) on functions other then 0, as they similarly to bridge on function 0 are unpluggable. So since we need to describe multifunction devices iterate over fuctions as well. But describe only cold-plugged bridges[root ports] on functions other than 0 as well. 1) Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) Signed-off-by: Igor Mammedov Reported-by: Laurent Vivier Message-Id: <20210723090424.2092226-1-imammedo@redhat.com> Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reported-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 17836149fe..a33ac8b91e 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, Aml *dev, *notify_method = NULL, *method; QObject *bsel; PCIBus *sec; - int i; + int devfn; bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL); if (bsel) { @@ -384,23 +384,31 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED); } - for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) { + for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { DeviceClass *dc; PCIDeviceClass *pc; - PCIDevice *pdev = bus->devices[i]; - int slot = PCI_SLOT(i); + PCIDevice *pdev = bus->devices[devfn]; + int slot = PCI_SLOT(devfn); + int func = PCI_FUNC(devfn); + /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ + int adr = slot << 16 | func; bool hotplug_enabled_dev; bool bridge_in_acpi; bool cold_plugged_bridge; if (!pdev) { - if (bsel) { /* add hotplug slots for non present devices */ + /* + * add hotplug slots for non present devices. + * hotplug is supported only for non-multifunction device + * so generate device description only for function 0 + */ + if (bsel && !func) { if (pci_bus_is_express(bus) && slot > 0) { break; } - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); + dev = aml_device("S%.02X", devfn); aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); aml_append(method, aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) @@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, continue; } - /* start to compose PCI slot descriptor */ - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); + /* + * allow describing coldplugged bridges in ACPI even if they are not + * on function 0, as they are not unpluggable, for all other devices + * generate description only for function 0 per slot + */ + if (func && !bridge_in_acpi) { + continue; + } + + /* start to compose PCI device descriptor */ + dev = aml_device("S%.02X", devfn); + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); if (bsel) { /* @@ -496,7 +513,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en); } - /* slot descriptor has been composed, add it into parent context */ + /* device descriptor has been composed, add it into parent context */ aml_append(parent_scope, dev); } @@ -525,13 +542,12 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, /* Notify about child bus events in any case */ if (pcihp_bridge_en) { QLIST_FOREACH(sec, &bus->child, sibling) { - int32_t devfn = sec->parent_dev->devfn; - if (pci_bus_is_root(sec)) { continue; } - aml_append(method, aml_name("^S%.02X.PCNT", devfn)); + aml_append(method, aml_name("^S%.02X.PCNT", + sec->parent_dev->devfn)); } } From 5cd4a8d4e567a6d52553c2133bf1c9b008d80481 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 3 Aug 2021 16:13:10 -0400 Subject: [PATCH 3/5] arm/acpi: allow DSDT changes We are going to commit ccee1a8140 ("acpi: Update _DSM method in expected files"). Allow changes to DSDT on ARM. Only configs with pci are affected thus all virt variants but for microvm only the pcie variant. Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test-allowed-diff.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..8a7fe463c5 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,6 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/virt/DSDT", +"tests/data/acpi/virt/DSDT.memhp", +"tests/data/acpi/virt/DSDT.numamem", +"tests/data/acpi/virt/DSDT.pxb", +"tests/data/acpi/microvm/DSDT.pcie", From 40c3472a29c9a1fd65255fc196aa6feb99aaec9e Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 27 Jul 2021 05:18:47 -0400 Subject: [PATCH 4/5] Revert "acpi/gpex: Inform os to keep firmware resource map" This reverts commit 0cf8882fd06ba0aeb1e90fa6f23fce85504d7e14. Which this commit, with aarch64 when using efi PCI devices with IO ports do not work. The reason is that EFI creates I/O port mappings below 0x1000 (in fact, at 0). However Linux, for legacy reasons, does not support I/O ports <= 0x1000 on PCI, so the I/O assignment created by EFI is rejected. EFI creates the mappings primarily for itself, and up until DSM #5 started to be enforced, all PCI resource allocations that existed at boot were ignored by Linux and recreated from scratch. Also, the commit in question looks dubious - it seems unlikely that Linux would fail to create a resource tree. What does happen is that BARs get moved around, which may cause trouble in some cases: for instance, Linux had to add special code to the EFI framebuffer driver to copy with framebuffer BARs being relocated. DSM #5 has a long history of debate and misinterpretation. Link: https://lore.kernel.org/r/20210724185234.GA2265457@roeck-us.net/ Fixes: 0cf8882fd06 ("acpi/gpex: Inform os to keep firmware resource map") Reported-by: Guenter Roeck Suggested-by: Ard Biesheuvel Acked-by: Ard Biesheuvel Tested-by: Guenter Roeck Signed-off-by: Michael S. Tsirkin --- hw/pci-host/gpex-acpi.c | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c index 0f01f13a6e..e7e162a00a 100644 --- a/hw/pci-host/gpex-acpi.c +++ b/hw/pci-host/gpex-acpi.c @@ -112,26 +112,10 @@ static void acpi_dsdt_add_pci_osc(Aml *dev) UUID = aml_touuid("E5C937D0-3553-4D7A-9117-EA4D19C3434D"); ifctx = aml_if(aml_equal(aml_arg(0), UUID)); ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(0))); - uint8_t byte_list[] = { - 0x1 << 0 /* support for functions other than function 0 */ | - 0x1 << 5 /* support for function 5 */ - }; - buf = aml_buffer(ARRAY_SIZE(byte_list), byte_list); + uint8_t byte_list[1] = {1}; + buf = aml_buffer(1, byte_list); aml_append(ifctx1, aml_return(buf)); aml_append(ifctx, ifctx1); - - /* - * PCI Firmware Specification 3.1 - * 4.6.5. _DSM for Ignoring PCI Boot Configurations - */ - /* Arg2: Function Index: 5 */ - ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(5))); - /* - * 0 - The operating system must not ignore the PCI configuration that - * firmware has done at boot time. - */ - aml_append(ifctx1, aml_return(aml_int(0))); - aml_append(ifctx, ifctx1); aml_append(method, ifctx); byte_list[0] = 0; From 62a4db5522cbbd8b5309a2949c22f00e5b0138e3 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 3 Aug 2021 16:20:22 -0400 Subject: [PATCH 5/5] Drop _DSM 5 from expected DSDTs on ARM diff -rup /tmp/old/tests/data/acpi/microvm/DSDT.pcie.dsl /tmp/new/tests/data/acpi/microvm/DSDT.pcie.dsl --- /tmp/old/tests/data/acpi/microvm/DSDT.pcie.dsl 2021-08-03 16:22:52.289295442 -0400 +++ /tmp/new/tests/data/acpi/microvm/DSDT.pcie.dsl 2021-08-03 16:22:40.102286317 -0400 @@ -1302,14 +1302,9 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS " { Return (Buffer (One) { - 0x21 // ! + 0x01 // . }) } - - If ((Arg2 == 0x05)) - { - Return (Zero) - } } Return (Buffer (One) Signed-off-by: Michael S. Tsirkin --- tests/data/acpi/microvm/DSDT.pcie | Bin 3031 -> 3023 bytes tests/data/acpi/virt/DSDT | Bin 5204 -> 5196 bytes tests/data/acpi/virt/DSDT.memhp | Bin 6565 -> 6557 bytes tests/data/acpi/virt/DSDT.numamem | Bin 5204 -> 5196 bytes tests/data/acpi/virt/DSDT.pxb | Bin 7695 -> 7679 bytes tests/qtest/bios-tables-test-allowed-diff.h | 5 ----- 6 files changed, 5 deletions(-) diff --git a/tests/data/acpi/microvm/DSDT.pcie b/tests/data/acpi/microvm/DSDT.pcie index 3fb373fd970f0a33f30f57b1917720754396f0e9..765f14ef3d1e54d3cadccbf0a880f8adb73b3f1f 100644 GIT binary patch delta 51 zcmcaEeqNl*CD#U6GCFLIXMD`bp&RcK?8~x1ak3Y;JR{@e HBJNZGj^hr8 delta 59 zcmX>veqEf)CDA8`aGj89g?~Gd||zFpYN!_GMY1IoXR_o>OrF P`{XPx)+G#+v$#_M_<|6J diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT index 134d8ae5b602e0aaade6756b99c9abca45279284..c47503990715d389914fdf9c8bccb510761741ac 100644 GIT binary patch delta 50 zcmcbjaYlp7CD?thI$T+!B G_%Q%n84Z^J delta 58 zcmX@3aYcj6CD diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp index 140976b23ebea792bec12435a2a547ac5f5cd8f9..bae36cdd397473afe3923c52f030641a5ab19d5d 100644 GIT binary patch delta 52 zcmZ2#JlB}ZCDyi;GWjiE?8wQ*p&RcK?8~x1ak8hdJR{@g ILSYj&0ETJ})&Kwi delta 60 zcmbPhywsS>CDOrF Q`{XPx)+G#^Glfmq0PR!{)&Kwi diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem index 134d8ae5b602e0aaade6756b99c9abca45279284..c47503990715d389914fdf9c8bccb510761741ac 100644 GIT binary patch delta 50 zcmcbjaYlp7CD?thI$T+!B G_%Q%n84Z^J delta 58 zcmX@3aYcj6CD diff --git a/tests/data/acpi/virt/DSDT.pxb b/tests/data/acpi/virt/DSDT.pxb index 46b9c4cad5d65cb1b578410fb3168b70a05021be..fbd78f44c4785d19759daea909fe6d6f9a6e6b01 100644 GIT binary patch delta 78 zcmeCT`ESkT66_N4UzUM^Y5znn8OFOC)g?F?9XC60Hgj_5#=8XjvMf-Xd|F7Ji*bn{ YGb2NEli%{ie}uSD&QL^0Gn_Z9smFU delta 94 zcmexw-EYI?66_MfFUP>ZG;bo84CB3x>Jprco|_#wn>jg5<6VM%Sr%wc{v#tVq_}{6 hauyfs5{4y$%!~}tO>Qd|e-YwBQNsyWGg(IVF#z_a8;k$| diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index 8a7fe463c5..dfb8523c8b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1,6 +1 @@ /* List of comma-separated changed AML files to ignore */ -"tests/data/acpi/virt/DSDT", -"tests/data/acpi/virt/DSDT.memhp", -"tests/data/acpi/virt/DSDT.numamem", -"tests/data/acpi/virt/DSDT.pxb", -"tests/data/acpi/microvm/DSDT.pcie",