From 9ce75d4d5eaf30333797fd6f8b5c6ee7f917951f Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 17 Nov 2022 00:44:58 +0300 Subject: [PATCH 01/56] shpc: disallow unplug when power indicator is blinking Pressing attention button has special meaning when power indicator is blinking. Better just not do it. For example, trying to remove device immediately after hotplug leads to both commands succeded but device not actually unrealized. Same thing for PCIE hotplug was done in 81124b3c7a5dae "pcie: add power indicator blink check" Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20221116214458.82090-1-vsementsov@yandex-team.ru> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci/shpc.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c index e71f3a7483..fca7f6691a 100644 --- a/hw/pci/shpc.c +++ b/hw/pci/shpc.c @@ -568,6 +568,13 @@ void shpc_device_unplug_request_cb(HotplugHandler *hotplug_dev, state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK); led = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK); + + if (led == SHPC_LED_BLINK) { + error_setg(errp, "Hot-unplug failed: " + "guest is busy (power indicator blinking)"); + return; + } + if (state == SHPC_STATE_DISABLED && led == SHPC_LED_OFF) { shpc_free_devices_in_slot(shpc, slot); shpc_set_status(shpc, slot, 1, SHPC_SLOT_STATUS_MRL_OPEN); From 1f1b30af753288c176ddff2bc62b6d3d37aa8a6d Mon Sep 17 00:00:00 2001 From: Bernhard Beschow Date: Sat, 21 Jan 2023 16:19:35 +0100 Subject: [PATCH 02/56] hw/i386/acpi-build: Remove unused attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ammends commit 3db119da7915 'pc: acpi: switch to AML API composed DSDT'. Signed-off-by: Bernhard Beschow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Igor Mammedov Message-Id: <20230121151941.24120-2-shentey@gmail.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 127c4e2d50..8c333973f9 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -117,8 +117,6 @@ typedef struct AcpiMiscInfo { #ifdef CONFIG_TPM TPMVersion tpm_version; #endif - const unsigned char *dsdt_code; - unsigned dsdt_size; } AcpiMiscInfo; typedef struct FwCfgTPMConfig { From 9c6c0aeacda7d9c3e3d5743f4927d31f5ac76888 Mon Sep 17 00:00:00 2001 From: Bernhard Beschow Date: Sat, 21 Jan 2023 16:19:36 +0100 Subject: [PATCH 03/56] hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Frees isa-bus.c from implicit ACPI dependency. While at it, resolve open coding of qbus_build_aml() in piix3 and ich9. Signed-off-by: Bernhard Beschow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Igor Mammedov Message-Id: <20230121151941.24120-3-shentey@gmail.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/acpi/acpi_interface.c | 10 ++++++++++ hw/i2c/smbus_ich9.c | 5 +---- hw/i386/acpi-microvm.c | 3 ++- hw/isa/isa-bus.c | 10 ---------- hw/isa/lpc_ich9.c | 5 +---- hw/isa/piix3.c | 5 +---- include/hw/acpi/acpi_aml_interface.h | 3 +++ include/hw/isa/isa.h | 1 - 8 files changed, 18 insertions(+), 24 deletions(-) diff --git a/hw/acpi/acpi_interface.c b/hw/acpi/acpi_interface.c index c668d361f6..8637ff18fc 100644 --- a/hw/acpi/acpi_interface.c +++ b/hw/acpi/acpi_interface.c @@ -2,6 +2,7 @@ #include "hw/acpi/acpi_dev_interface.h" #include "hw/acpi/acpi_aml_interface.h" #include "qemu/module.h" +#include "qemu/queue.h" void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event) { @@ -12,6 +13,15 @@ void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event) } } +void qbus_build_aml(BusState *bus, Aml *scope) +{ + BusChild *kid; + + QTAILQ_FOREACH(kid, &bus->children, sibling) { + call_dev_aml_func(DEVICE(kid->child), scope); + } +} + static void register_types(void) { static const TypeInfo acpi_dev_if_info = { diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c index ee50ba1f2c..52ba77f3fc 100644 --- a/hw/i2c/smbus_ich9.c +++ b/hw/i2c/smbus_ich9.c @@ -97,13 +97,10 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp) static void build_ich9_smb_aml(AcpiDevAmlIf *adev, Aml *scope) { - BusChild *kid; ICH9SMBState *s = ICH9_SMB_DEVICE(adev); BusState *bus = BUS(s->smb.smbus); - QTAILQ_FOREACH(kid, &bus->children, sibling) { - call_dev_aml_func(DEVICE(kid->child), scope); - } + qbus_build_aml(bus, scope); } static void ich9_smb_class_init(ObjectClass *klass, void *data) diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c index fb09185cbd..a075360d85 100644 --- a/hw/i386/acpi-microvm.c +++ b/hw/i386/acpi-microvm.c @@ -26,6 +26,7 @@ #include "exec/memory.h" #include "hw/acpi/acpi.h" +#include "hw/acpi/acpi_aml_interface.h" #include "hw/acpi/aml-build.h" #include "hw/acpi/bios-linker-loader.h" #include "hw/acpi/generic_event_device.h" @@ -129,7 +130,7 @@ build_dsdt_microvm(GArray *table_data, BIOSLinker *linker, sb_scope = aml_scope("_SB"); fw_cfg_add_acpi_dsdt(sb_scope, x86ms->fw_cfg); - isa_build_aml(ISA_BUS(isabus), sb_scope); + qbus_build_aml(BUS(isabus), sb_scope); build_ged_aml(sb_scope, GED_DEVICE, x86ms->acpi_dev, GED_MMIO_IRQ, AML_SYSTEM_MEMORY, GED_MMIO_BASE); acpi_dsdt_add_power_button(sb_scope); diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c index 1bee1a47f1..f155b80010 100644 --- a/hw/isa/isa-bus.c +++ b/hw/isa/isa-bus.c @@ -24,7 +24,6 @@ #include "hw/sysbus.h" #include "sysemu/sysemu.h" #include "hw/isa/isa.h" -#include "hw/acpi/acpi_aml_interface.h" static ISABus *isabus; @@ -188,15 +187,6 @@ ISADevice *isa_vga_init(ISABus *bus) } } -void isa_build_aml(ISABus *bus, Aml *scope) -{ - BusChild *kid; - - QTAILQ_FOREACH(kid, &bus->parent_obj.children, sibling) { - call_dev_aml_func(DEVICE(kid->child), scope); - } -} - static void isabus_bridge_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c index 8d541e2b54..1fba3c210c 100644 --- a/hw/isa/lpc_ich9.c +++ b/hw/isa/lpc_ich9.c @@ -813,7 +813,6 @@ static void ich9_send_gpe(AcpiDeviceIf *adev, AcpiEventStatusBits ev) static void build_ich9_isa_aml(AcpiDevAmlIf *adev, Aml *scope) { Aml *field; - BusChild *kid; ICH9LPCState *s = ICH9_LPC_DEVICE(adev); BusState *bus = BUS(s->isa_bus); Aml *sb_scope = aml_scope("\\_SB"); @@ -835,9 +834,7 @@ static void build_ich9_isa_aml(AcpiDevAmlIf *adev, Aml *scope) aml_append(sb_scope, field); aml_append(scope, sb_scope); - QTAILQ_FOREACH(kid, &bus->children, sibling) { - call_dev_aml_func(DEVICE(kid->child), scope); - } + qbus_build_aml(bus, scope); } static void ich9_lpc_class_init(ObjectClass *klass, void *data) diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c index 283b971ec4..a9cb39bf21 100644 --- a/hw/isa/piix3.c +++ b/hw/isa/piix3.c @@ -306,7 +306,6 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp) static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope) { Aml *field; - BusChild *kid; Aml *sb_scope = aml_scope("\\_SB"); BusState *bus = qdev_get_child_bus(DEVICE(adev), "isa.0"); @@ -322,9 +321,7 @@ static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope) aml_append(sb_scope, field); aml_append(scope, sb_scope); - QTAILQ_FOREACH(kid, &bus->children, sibling) { - call_dev_aml_func(DEVICE(kid->child), scope); - } + qbus_build_aml(bus, scope); } static void pci_piix3_class_init(ObjectClass *klass, void *data) diff --git a/include/hw/acpi/acpi_aml_interface.h b/include/hw/acpi/acpi_aml_interface.h index 436da069d6..11748a8866 100644 --- a/include/hw/acpi/acpi_aml_interface.h +++ b/include/hw/acpi/acpi_aml_interface.h @@ -3,6 +3,7 @@ #include "qom/object.h" #include "hw/acpi/aml-build.h" +#include "hw/qdev-core.h" #define TYPE_ACPI_DEV_AML_IF "acpi-dev-aml-interface" typedef struct AcpiDevAmlIfClass AcpiDevAmlIfClass; @@ -46,4 +47,6 @@ static inline void call_dev_aml_func(DeviceState *dev, Aml *scope) } } +void qbus_build_aml(BusState *bus, Aml *scope); + #endif diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h index 6c8a8a92cb..25acd5c34c 100644 --- a/include/hw/isa/isa.h +++ b/include/hw/isa/isa.h @@ -86,7 +86,6 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp); ISADevice *isa_create_simple(ISABus *bus, const char *name); ISADevice *isa_vga_init(ISABus *bus); -void isa_build_aml(ISABus *bus, Aml *scope); /** * isa_register_ioport: Install an I/O port region on the ISA bus. From edfa7180106162765a75f7be17be8df0e4ab823e Mon Sep 17 00:00:00 2001 From: Bernhard Beschow Date: Sat, 21 Jan 2023 16:19:37 +0100 Subject: [PATCH 04/56] hw/acpi/piix4: No need to #include "hw/southbridge/piix.h" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hw/acpi/piix4 has its own header with its structure definition etc. Ammends commit 2bfd0845f0 'hw/acpi/piix4: move PIIX4PMState into separate piix4.h header'. Signed-off-by: Bernhard Beschow Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230121151941.24120-4-shentey@gmail.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/acpi/piix4.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 0a81f1ad93..2ab4930f11 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -21,7 +21,6 @@ #include "qemu/osdep.h" #include "hw/i386/pc.h" -#include "hw/southbridge/piix.h" #include "hw/irq.h" #include "hw/isa/apm.h" #include "hw/i2c/pm_smbus.h" From d395b18dce82855d03d934e30a515caf5a10a885 Mon Sep 17 00:00:00 2001 From: Bernhard Beschow Date: Sat, 21 Jan 2023 16:19:38 +0100 Subject: [PATCH 05/56] hw/acpi/acpi_dev_interface: Remove unused parameter from AcpiDeviceIfClass::madt_cpu The only function ever assigned to AcpiDeviceIfClass::madt_cpu is pc_madt_cpu_entry() which doesn't use the AcpiDeviceIf parameter. Signed-off-by: Bernhard Beschow Reviewed-by: Igor Mammedov Message-Id: <20230121151941.24120-5-shentey@gmail.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/acpi/acpi-x86-stub.c | 5 ++--- hw/acpi/cpu.c | 3 +-- hw/i386/acpi-common.c | 7 +++---- include/hw/acpi/acpi_dev_interface.h | 3 +-- include/hw/i386/pc.h | 6 ++---- 5 files changed, 9 insertions(+), 15 deletions(-) diff --git a/hw/acpi/acpi-x86-stub.c b/hw/acpi/acpi-x86-stub.c index 3df1e090f4..d0d399d26b 100644 --- a/hw/acpi/acpi-x86-stub.c +++ b/hw/acpi/acpi-x86-stub.c @@ -2,9 +2,8 @@ #include "hw/i386/pc.h" #include "hw/i386/acpi-build.h" -void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid, - const CPUArchIdList *apic_ids, GArray *entry, - bool force_enabled) +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, + GArray *entry, bool force_enabled) { } diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index 4e580959a2..19c154d78f 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -355,7 +355,6 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts, char *cphp_res_path = g_strdup_printf("%s." CPUHP_RES_DEVICE, res_root); Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL); AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(obj); - AcpiDeviceIf *adev = ACPI_DEVICE_IF(obj); cpu_ctrl_dev = aml_device("%s", cphp_res_path); { @@ -666,7 +665,7 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts, /* build _MAT object */ assert(adevc && adevc->madt_cpu); - adevc->madt_cpu(adev, i, arch_ids, madt_buf, + adevc->madt_cpu(i, arch_ids, madt_buf, true); /* set enabled flag */ aml_append(dev, aml_name_decl("_MAT", aml_buffer(madt_buf->len, (uint8_t *)madt_buf->data))); diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c index 4aaafbdd7b..52e5c1439a 100644 --- a/hw/i386/acpi-common.c +++ b/hw/i386/acpi-common.c @@ -33,9 +33,8 @@ #include "acpi-build.h" #include "acpi-common.h" -void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid, - const CPUArchIdList *apic_ids, GArray *entry, - bool force_enabled) +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, + GArray *entry, bool force_enabled) { uint32_t apic_id = apic_ids->cpus[uid].arch_id; /* Flags – Local APIC Flags */ @@ -112,7 +111,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker, build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags */ for (i = 0; i < apic_ids->len; i++) { - adevc->madt_cpu(adev, i, apic_ids, table_data, false); + adevc->madt_cpu(i, apic_ids, table_data, false); if (apic_ids->cpus[i].arch_id > 254) { x2apic_mode = true; } diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h index ea6056ab92..a1648220ff 100644 --- a/include/hw/acpi/acpi_dev_interface.h +++ b/include/hw/acpi/acpi_dev_interface.h @@ -52,8 +52,7 @@ struct AcpiDeviceIfClass { /* */ void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list); void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev); - void (*madt_cpu)(AcpiDeviceIf *adev, int uid, - const CPUArchIdList *apic_ids, GArray *entry, + void (*madt_cpu)(int uid, const CPUArchIdList *apic_ids, GArray *entry, bool force_enabled); }; #endif diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 88a120bc23..66e3d059ef 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -9,7 +9,6 @@ #include "hw/block/flash.h" #include "hw/i386/x86.h" -#include "hw/acpi/acpi_dev_interface.h" #include "hw/hotplug.h" #include "qom/object.h" #include "hw/i386/sgx-epc.h" @@ -193,9 +192,8 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size); /* hw/i386/acpi-common.c */ -void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid, - const CPUArchIdList *apic_ids, GArray *entry, - bool force_enabled); +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, + GArray *entry, bool force_enabled); /* sgx.c */ void pc_machine_init_sgx_epc(PCMachineState *pcms); From 744734ccc9eff28394a453de462b2a155f364118 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Mon, 9 Jan 2023 15:31:30 +0900 Subject: [PATCH 06/56] vhost-user: Correct a reference of TARGET_AARCH64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Presumably TARGET_ARM_64 should be a mistake of TARGET_AARCH64. Signed-off-by: Akihiko Odaki Message-Id: <20230109063130.81296-1-akihiko.odaki@daynix.com> Fixes: 27598393a2 ("Lift max memory slots limit imposed by vhost-user") Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/vhost-user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index d9ce0501b2..6c79da953b 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -48,7 +48,7 @@ * hardware plaform. */ #if defined(TARGET_X86) || defined(TARGET_X86_64) || \ - defined(TARGET_ARM) || defined(TARGET_ARM_64) + defined(TARGET_ARM) || defined(TARGET_AARCH64) #include "hw/acpi/acpi.h" #define VHOST_USER_MAX_RAM_SLOTS ACPI_MAX_RAM_SLOTS From 8a8c9c3a747f77e664fa2288735b45a9d750be75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 5 Jan 2023 18:37:02 +0100 Subject: [PATCH 07/56] hw/pci-host: Use register definitions from PCI standard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No need to document magic values when the definition names from "standard-headers/linux/pci_regs.h" are self-explicit. Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20230105173702.56610-1-philmd@linaro.org> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: BALATON Zoltan Reviewed-by: Richard Henderson Reviewed-by: Bernhard Beschow --- hw/pci-host/grackle.c | 2 +- hw/pci-host/raven.c | 6 +++--- hw/pci-host/uninorth.c | 33 +++++++++++---------------------- 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c index 8cf318cb80..8e589ff2c9 100644 --- a/hw/pci-host/grackle.c +++ b/hw/pci-host/grackle.c @@ -91,7 +91,7 @@ static void grackle_init(Object *obj) static void grackle_pci_realize(PCIDevice *d, Error **errp) { - d->config[0x09] = 0x01; + d->config[PCI_CLASS_PROG] = 0x01; } static void grackle_pci_class_init(ObjectClass *klass, void *data) diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c index 5b00b4e462..cdfb62ac2e 100644 --- a/hw/pci-host/raven.c +++ b/hw/pci-host/raven.c @@ -330,9 +330,9 @@ static void raven_realize(PCIDevice *d, Error **errp) char *filename; int bios_size = -1; - d->config[0x0C] = 0x08; // cache_line_size - d->config[0x0D] = 0x10; // latency_timer - d->config[0x34] = 0x00; // capabilities_pointer + d->config[PCI_CACHE_LINE_SIZE] = 0x08; + d->config[PCI_LATENCY_TIMER] = 0x10; + d->config[PCI_CAPABILITY_LIST] = 0x00; memory_region_init_rom_nomigrate(&s->bios, OBJECT(s), "bios", BIOS_SIZE, &error_fatal); diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c index e3abe3c0f9..e4c1abd871 100644 --- a/hw/pci-host/uninorth.c +++ b/hw/pci-host/uninorth.c @@ -276,12 +276,9 @@ static void pci_unin_internal_init(Object *obj) static void unin_main_pci_host_realize(PCIDevice *d, Error **errp) { - /* cache_line_size */ - d->config[0x0C] = 0x08; - /* latency_timer */ - d->config[0x0D] = 0x10; - /* capabilities_pointer */ - d->config[0x34] = 0x00; + d->config[PCI_CACHE_LINE_SIZE] = 0x08; + d->config[PCI_LATENCY_TIMER] = 0x10; + d->config[PCI_CAPABILITY_LIST] = 0x00; /* * Set kMacRISCPCIAddressSelect (0x48) register to indicate PCI @@ -296,30 +293,22 @@ static void unin_main_pci_host_realize(PCIDevice *d, Error **errp) static void unin_agp_pci_host_realize(PCIDevice *d, Error **errp) { - /* cache_line_size */ - d->config[0x0C] = 0x08; - /* latency_timer */ - d->config[0x0D] = 0x10; - /* capabilities_pointer - d->config[0x34] = 0x80; */ + d->config[PCI_CACHE_LINE_SIZE] = 0x08; + d->config[PCI_LATENCY_TIMER] = 0x10; + /* d->config[PCI_CAPABILITY_LIST] = 0x80; */ } static void u3_agp_pci_host_realize(PCIDevice *d, Error **errp) { - /* cache line size */ - d->config[0x0C] = 0x08; - /* latency timer */ - d->config[0x0D] = 0x10; + d->config[PCI_CACHE_LINE_SIZE] = 0x08; + d->config[PCI_LATENCY_TIMER] = 0x10; } static void unin_internal_pci_host_realize(PCIDevice *d, Error **errp) { - /* cache_line_size */ - d->config[0x0C] = 0x08; - /* latency_timer */ - d->config[0x0D] = 0x10; - /* capabilities_pointer */ - d->config[0x34] = 0x00; + d->config[PCI_CACHE_LINE_SIZE] = 0x08; + d->config[PCI_LATENCY_TIMER] = 0x10; + d->config[PCI_CAPABILITY_LIST] = 0x00; } static void unin_main_pci_host_class_init(ObjectClass *klass, void *data) From bad9c5a5166fd5e3a892b7b0477cf2f4bd3a959a Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Mon, 9 Jan 2023 10:58:09 +0000 Subject: [PATCH 08/56] virtio-rng-pci: fix migration compat for vectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixup the migration compatibility for existing machine types so that they do not enable msi-x. Symptom: (qemu) qemu: get_pci_config_device: Bad config data: i=0x34 read: 84 device: 98 cmask: ff wmask: 0 w1cmask:0 qemu: Failed to load PCIDevice:config qemu: Failed to load virtio-rng:virtio qemu: error while loading state for instance 0x0 of device '0000:00:03.0/virtio-rng' qemu: load of migration failed: Invalid argument Note: This fix will break migration from 7.2->7.2-fixed with this patch bz: https://bugzilla.redhat.com/show_bug.cgi?id=2155749 Fixes: 9ea02e8f1 ("virtio-rng-pci: Allow setting nvectors, so we can use MSI-X") Signed-off-by: Dr. David Alan Gilbert Message-Id: <20230109105809.163975-1-dgilbert@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Thomas Huth Acked-by: David Daney Fixes: 9ea02e8f1 ("virtio-rng-pci: Allow setting nvectors, so we can use MSI-X")
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé --- hw/core/machine.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/core/machine.c b/hw/core/machine.c index 616f3a207c..f7761baab5 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -46,6 +46,7 @@ const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2); GlobalProperty hw_compat_7_1[] = { { "virtio-device", "queue_reset", "false" }, + { "virtio-rng-pci", "vectors", "0" }, }; const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1); From 8a7c606016d283a1716290c657f6f45bc7c4d817 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 9 Jan 2023 14:37:27 -0500 Subject: [PATCH 09/56] intel-iommu: Document iova_tree It seems not super clear on when iova_tree is used, and why. Add a rich comment above iova_tree to track why we needed the iova_tree, and when we need it. Also comment for the map/unmap messages, on how they're used and implications (e.g. unmap can be larger than the mapped ranges). Suggested-by: Jason Wang Signed-off-by: Peter Xu Message-Id: <20230109193727.1360190-1-peterx@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- include/exec/memory.h | 26 ++++++++++++++++++++++++ include/hw/i386/intel_iommu.h | 38 ++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index c37ffdbcd1..2e602a2fad 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -129,6 +129,32 @@ struct IOMMUTLBEntry { /* * Bitmap for different IOMMUNotifier capabilities. Each notifier can * register with one or multiple IOMMU Notifier capability bit(s). + * + * Normally there're two use cases for the notifiers: + * + * (1) When the device needs accurate synchronizations of the vIOMMU page + * tables, it needs to register with both MAP|UNMAP notifies (which + * is defined as IOMMU_NOTIFIER_IOTLB_EVENTS below). + * + * Regarding to accurate synchronization, it's when the notified + * device maintains a shadow page table and must be notified on each + * guest MAP (page table entry creation) and UNMAP (invalidation) + * events (e.g. VFIO). Both notifications must be accurate so that + * the shadow page table is fully in sync with the guest view. + * + * (2) When the device doesn't need accurate synchronizations of the + * vIOMMU page tables, it needs to register only with UNMAP or + * DEVIOTLB_UNMAP notifies. + * + * It's when the device maintains a cache of IOMMU translations + * (IOTLB) and is able to fill that cache by requesting translations + * from the vIOMMU through a protocol similar to ATS (Address + * Translation Service). + * + * Note that in this mode the vIOMMU will not maintain a shadowed + * page table for the address space, and the UNMAP messages can cover + * more than the pages that used to get mapped. The IOMMU notifiee + * should be able to take care of over-sized invalidations. */ typedef enum { IOMMU_NOTIFIER_NONE = 0, diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 46d973e629..89dcbc5e1e 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -109,7 +109,43 @@ struct VTDAddressSpace { QLIST_ENTRY(VTDAddressSpace) next; /* Superset of notifier flags that this address space has */ IOMMUNotifierFlag notifier_flags; - IOVATree *iova_tree; /* Traces mapped IOVA ranges */ + /* + * @iova_tree traces mapped IOVA ranges. + * + * The tree is not needed if no MAP notifier is registered with current + * VTD address space, because all guest invalidate commands can be + * directly passed to the IOMMU UNMAP notifiers without any further + * reshuffling. + * + * The tree OTOH is required for MAP typed iommu notifiers for a few + * reasons. + * + * Firstly, there's no way to identify whether an PSI (Page Selective + * Invalidations) or DSI (Domain Selective Invalidations) event is an + * MAP or UNMAP event within the message itself. Without having prior + * knowledge of existing state vIOMMU doesn't know whether it should + * notify MAP or UNMAP for a PSI message it received when caching mode + * is enabled (for MAP notifiers). + * + * Secondly, PSI messages received from guest driver can be enlarged in + * range, covers but not limited to what the guest driver wanted to + * invalidate. When the range to invalidates gets bigger than the + * limit of a PSI message, it can even become a DSI which will + * invalidate the whole domain. If the vIOMMU directly notifies the + * registered device with the unmodified range, it may confuse the + * registered drivers (e.g. vfio-pci) on either: + * + * (1) Trying to map the same region more than once (for + * VFIO_IOMMU_MAP_DMA, -EEXIST will trigger), or, + * + * (2) Trying to UNMAP a range that is still partially mapped. + * + * That accuracy is not required for UNMAP-only notifiers, but it is a + * must-to-have for notifiers registered with MAP events, because the + * vIOMMU needs to make sure the shadow page table is always in sync + * with the guest IOMMU pgtables for a device. + */ + IOVATree *iova_tree; }; struct VTDIOTLBEntry { From eac7a7791bb6d719233deed750034042318ffd56 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Fri, 30 Dec 2022 23:07:25 +0100 Subject: [PATCH 10/56] x86: don't let decompressed kernel image clobber setup_data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The setup_data links are appended to the compressed kernel image. Since the kernel image is typically loaded at 0x100000, setup_data lives at `0x100000 + compressed_size`, which does not get relocated during the kernel's boot process. The kernel typically decompresses the image starting at address 0x1000000 (note: there's one more zero there than the compressed image above). This usually is fine for most kernels. However, if the compressed image is actually quite large, then setup_data will live at a `0x100000 + compressed_size` that extends into the decompressed zone at 0x1000000. In other words, if compressed_size is larger than `0x1000000 - 0x100000`, then the decompression step will clobber setup_data, resulting in crashes. Visually, what happens now is that QEMU appends setup_data to the kernel image: kernel image setup_data |--------------------------||----------------| 0x100000 0x100000+l1 0x100000+l1+l2 The problem is that this decompresses to 0x1000000 (one more zero). So if l1 is > (0x1000000-0x100000), then this winds up looking like: kernel image setup_data |--------------------------||----------------| 0x100000 0x100000+l1 0x100000+l1+l2 d e c o m p r e s s e d k e r n e l |-------------------------------------------------------------| 0x1000000 0x1000000+l3 The decompressed kernel seemingly overwriting the compressed kernel image isn't a problem, because that gets relocated to a higher address early on in the boot process, at the end of startup_64. setup_data, however, stays in the same place, since those links are self referential and nothing fixes them up. So the decompressed kernel clobbers it. Fix this by appending setup_data to the cmdline blob rather than the kernel image blob, which remains at a lower address that won't get clobbered. This could have been done by overwriting the initrd blob instead, but that poses big difficulties, such as no longer being able to use memory mapped files for initrd, hurting performance, and, more importantly, the initrd address calculation is hard coded in qboot, and it always grows down rather than up, which means lots of brittle semantics would have to be changed around, incurring more complexity. In contrast, using cmdline is simple and doesn't interfere with anything. The microvm machine has a gross hack where it fiddles with fw_cfg data after the fact. So this hack is updated to account for this appending, by reserving some bytes. Fixup-by: Michael S. Tsirkin Cc: x86@kernel.org Cc: Philippe Mathieu-Daudé Cc: H. Peter Anvin Cc: Borislav Petkov Cc: Eric Biggers Signed-off-by: Jason A. Donenfeld Message-Id: <20221230220725.618763-1-Jason@zx2c4.com> Message-ID: <20230128061015-mutt-send-email-mst@kernel.org> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Tested-by: Eric Biggers Tested-by: Mathias Krause --- hw/i386/microvm.c | 15 +++++++---- hw/i386/x86.c | 52 +++++++++++++++++++++------------------ hw/nvram/fw_cfg.c | 9 +++++++ include/hw/i386/microvm.h | 5 ++-- include/hw/nvram/fw_cfg.h | 9 +++++++ 5 files changed, 59 insertions(+), 31 deletions(-) diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c index 170a331e3f..29f30dd6d3 100644 --- a/hw/i386/microvm.c +++ b/hw/i386/microvm.c @@ -378,7 +378,8 @@ static void microvm_fix_kernel_cmdline(MachineState *machine) MicrovmMachineState *mms = MICROVM_MACHINE(machine); BusState *bus; BusChild *kid; - char *cmdline; + char *cmdline, *existing_cmdline; + size_t len; /* * Find MMIO transports with attached devices, and add them to the kernel @@ -387,7 +388,8 @@ static void microvm_fix_kernel_cmdline(MachineState *machine) * Yes, this is a hack, but one that heavily improves the UX without * introducing any significant issues. */ - cmdline = g_strdup(machine->kernel_cmdline); + existing_cmdline = fw_cfg_read_bytes_ptr(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA); + cmdline = g_strdup(existing_cmdline); bus = sysbus_get_default(); QTAILQ_FOREACH(kid, &bus->children, sibling) { DeviceState *dev = kid->child; @@ -411,9 +413,12 @@ static void microvm_fix_kernel_cmdline(MachineState *machine) } } - fw_cfg_modify_i32(x86ms->fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(cmdline) + 1); - fw_cfg_modify_string(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA, cmdline); - + len = strlen(cmdline); + if (len > VIRTIO_CMDLINE_TOTAL_MAX_LEN + strlen(existing_cmdline)) { + fprintf(stderr, "qemu: virtio mmio cmdline too large, skipping\n"); + } else { + memcpy(existing_cmdline, cmdline, len + 1); + } g_free(cmdline); } diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 78cc131926..eaff4227bd 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -50,6 +50,7 @@ #include "hw/intc/i8259.h" #include "hw/rtc/mc146818rtc.h" #include "target/i386/sev.h" +#include "hw/i386/microvm.h" #include "hw/acpi/cpu_hotplug.h" #include "hw/irq.h" @@ -813,12 +814,18 @@ void x86_load_linux(X86MachineState *x86ms, const char *kernel_filename = machine->kernel_filename; const char *initrd_filename = machine->initrd_filename; const char *dtb_filename = machine->dtb; - const char *kernel_cmdline = machine->kernel_cmdline; + char *kernel_cmdline; SevKernelLoaderContext sev_load_ctx = {}; enum { RNG_SEED_LENGTH = 32 }; - /* Align to 16 bytes as a paranoia measure */ - cmdline_size = (strlen(kernel_cmdline) + 16) & ~15; + /* + * Add the NUL terminator, some padding for the microvm cmdline fiddling + * hack, and then align to 16 bytes as a paranoia measure + */ + cmdline_size = (strlen(machine->kernel_cmdline) + 1 + + VIRTIO_CMDLINE_TOTAL_MAX_LEN + 16) & ~15; + /* Make a copy, since we might append arbitrary bytes to it later. */ + kernel_cmdline = g_strndup(machine->kernel_cmdline, cmdline_size); /* load the kernel header */ f = fopen(kernel_filename, "rb"); @@ -959,12 +966,6 @@ void x86_load_linux(X86MachineState *x86ms, initrd_max = x86ms->below_4g_mem_size - acpi_data_size - 1; } - fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr); - fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1); - fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline); - sev_load_ctx.cmdline_data = (char *)kernel_cmdline; - sev_load_ctx.cmdline_size = strlen(kernel_cmdline) + 1; - if (protocol >= 0x202) { stl_p(header + 0x228, cmdline_addr); } else { @@ -1091,27 +1092,24 @@ void x86_load_linux(X86MachineState *x86ms, exit(1); } - setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16); - kernel_size = setup_data_offset + sizeof(SetupData) + dtb_size; - kernel = g_realloc(kernel, kernel_size); - - - setup_data = (SetupData *)(kernel + setup_data_offset); + setup_data_offset = cmdline_size; + cmdline_size += sizeof(SetupData) + dtb_size; + kernel_cmdline = g_realloc(kernel_cmdline, cmdline_size); + setup_data = (void *)kernel_cmdline + setup_data_offset; setup_data->next = cpu_to_le64(first_setup_data); - first_setup_data = prot_addr + setup_data_offset; + first_setup_data = cmdline_addr + setup_data_offset; setup_data->type = cpu_to_le32(SETUP_DTB); setup_data->len = cpu_to_le32(dtb_size); - load_image_size(dtb_filename, setup_data->data, dtb_size); } - if (!legacy_no_rng_seed) { - setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16); - kernel_size = setup_data_offset + sizeof(SetupData) + RNG_SEED_LENGTH; - kernel = g_realloc(kernel, kernel_size); - setup_data = (SetupData *)(kernel + setup_data_offset); + if (!legacy_no_rng_seed && protocol >= 0x209) { + setup_data_offset = cmdline_size; + cmdline_size += sizeof(SetupData) + RNG_SEED_LENGTH; + kernel_cmdline = g_realloc(kernel_cmdline, cmdline_size); + setup_data = (void *)kernel_cmdline + setup_data_offset; setup_data->next = cpu_to_le64(first_setup_data); - first_setup_data = prot_addr + setup_data_offset; + first_setup_data = cmdline_addr + setup_data_offset; setup_data->type = cpu_to_le32(SETUP_RNG_SEED); setup_data->len = cpu_to_le32(RNG_SEED_LENGTH); qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH); @@ -1122,6 +1120,12 @@ void x86_load_linux(X86MachineState *x86ms, fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size); } + fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr); + fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, cmdline_size); + fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline, cmdline_size); + sev_load_ctx.cmdline_data = (char *)kernel_cmdline; + sev_load_ctx.cmdline_size = cmdline_size; + fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr); fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size); sev_load_ctx.kernel_data = (char *)kernel; @@ -1134,7 +1138,7 @@ void x86_load_linux(X86MachineState *x86ms, * kernel on the other side of the fw_cfg interface matches the hash of the * file the user passed in. */ - if (!sev_enabled()) { + if (!sev_enabled() && first_setup_data) { SetupDataFixup *fixup = g_malloc(sizeof(*fixup)); memcpy(setup, header, MIN(sizeof(header), setup_size)); diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index a00881bc64..432754eda4 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -741,6 +741,15 @@ void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len) fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true); } +void *fw_cfg_read_bytes_ptr(FWCfgState *s, uint16_t key) +{ + int arch = !!(key & FW_CFG_ARCH_LOCAL); + + key &= FW_CFG_ENTRY_MASK; + assert(key < fw_cfg_max_entry(s)); + return s->entries[arch][key].data; +} + void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value) { size_t sz = strlen(value) + 1; diff --git a/include/hw/i386/microvm.h b/include/hw/i386/microvm.h index fad97a891d..e8af61f194 100644 --- a/include/hw/i386/microvm.h +++ b/include/hw/i386/microvm.h @@ -50,8 +50,9 @@ */ /* Platform virtio definitions */ -#define VIRTIO_MMIO_BASE 0xfeb00000 -#define VIRTIO_CMDLINE_MAXLEN 64 +#define VIRTIO_MMIO_BASE 0xfeb00000 +#define VIRTIO_CMDLINE_MAXLEN 64 +#define VIRTIO_CMDLINE_TOTAL_MAX_LEN ((VIRTIO_CMDLINE_MAXLEN + 1) * 16) #define GED_MMIO_BASE 0xfea00000 #define GED_MMIO_BASE_MEMHP (GED_MMIO_BASE + 0x100) diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h index 2e503904dc..990dcdbb2e 100644 --- a/include/hw/nvram/fw_cfg.h +++ b/include/hw/nvram/fw_cfg.h @@ -139,6 +139,15 @@ void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key, void *data, size_t len, bool read_only); +/** + * fw_cfg_read_bytes_ptr: + * @s: fw_cfg device being modified + * @key: selector key value for new fw_cfg item + * + * Reads an existing fw_cfg data pointer. + */ +void *fw_cfg_read_bytes_ptr(FWCfgState *s, uint16_t key); + /** * fw_cfg_add_string: * @s: fw_cfg device being modified From 0711c2849735eb77b5082803c05cbaed99ccd852 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:33 +0100 Subject: [PATCH 11/56] tests: qtest: print device_add error before failing test Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-2-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/libqtest.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 6b2216cb20..d658222a19 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -1435,6 +1435,10 @@ void qtest_qmp_device_add_qdict(QTestState *qts, const char *drv, resp = qtest_qmp(qts, "{'execute': 'device_add', 'arguments': %p}", args); g_assert(resp); g_assert(!qdict_haskey(resp, "event")); /* We don't expect any events */ + if (qdict_haskey(resp, "error")) { + fprintf(stderr, "error: %s\n", + qdict_get_str(qdict_get_qdict(resp, "error"), "desc")); + } g_assert(!qdict_haskey(resp, "error")); qobject_unref(resp); } From 36773faeebbc965e6063802ae3c215f1a5f01bda Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:34 +0100 Subject: [PATCH 12/56] tests: acpi: cleanup arguments to make them more readable no functional change Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-3-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 8608408213..08b8aee76b 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -951,8 +951,7 @@ static void test_acpi_q35_tcg_bridge(void) data.variant = ".bridge"; data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); - test_acpi_one("-device pci-bridge,chassis_nr=1", - &data); + test_acpi_one("-device pci-bridge,chassis_nr=1", &data); free_test_data(&data); } @@ -962,14 +961,12 @@ static void test_acpi_q35_multif_bridge(void) .machine = MACHINE_Q35, .variant = ".multi-bridge", }; - test_acpi_one("-device pcie-root-port,id=pcie-root-port-0," - "multifunction=on," - "port=0x0,chassis=1,addr=0x2,bus=pcie.0 " - "-device pcie-root-port,id=pcie-root-port-1," - "port=0x1,chassis=2,addr=0x3.0x1,bus=pcie.0 " - "-device virtio-balloon,id=balloon0," - "bus=pcie.0,addr=0x4.0x2", - &data); + test_acpi_one( + " -device virtio-balloon,id=balloon0,addr=0x4.0x2" + " -device pcie-root-port,id=rp0,multifunction=on," + "port=0x0,chassis=1,addr=0x2" + " -device pcie-root-port,id=rp1,port=0x1,chassis=2,addr=0x3.0x1", + &data); free_test_data(&data); } From 89b36fd8614a7d02e15a72c6aa25a3682fc7fcef Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:35 +0100 Subject: [PATCH 13/56] tests: acpi: whitelist DSDT blobs for tests that use pci-bridges Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-4-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test-allowed-diff.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..2602a57c9b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,5 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/q35/DSDT.multi-bridge", +"tests/data/acpi/pc/DSDT.bridge", +"tests/data/acpi/pc/DSDT.roothp", +"tests/data/acpi/pc/DSDT.hpbridge", From 9ebb74d61486abdc1183aeb2852ffe0587d35fea Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:36 +0100 Subject: [PATCH 14/56] tests: acpi: extend pcihp with nested bridges add nested bridges/root-ports to pcihp tests, to make sure follow up patches don't break nested enumeration of bridges in DSDT. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-5-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 08b8aee76b..6a99b10384 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -863,7 +863,8 @@ static void test_acpi_piix4_tcg_bridge(void) data.variant = ".bridge"; data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); - test_acpi_one("-device pci-bridge,chassis_nr=1", &data); + test_acpi_one("-device pci-bridge,chassis_nr=1 " + "-device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 ", &data); free_test_data(&data); } @@ -877,7 +878,8 @@ static void test_acpi_piix4_no_root_hotplug(void) data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); test_acpi_one("-global PIIX4_PM.acpi-root-pci-hotplug=off " - "-device pci-bridge,chassis_nr=1", &data); + "-device pci-bridge,chassis_nr=1 " + "-device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 ", &data); free_test_data(&data); } @@ -891,7 +893,8 @@ static void test_acpi_piix4_no_bridge_hotplug(void) data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); test_acpi_one("-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off " - "-device pci-bridge,chassis_nr=1", &data); + "-device pci-bridge,chassis_nr=1 " + "-device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 ", &data); free_test_data(&data); } @@ -965,8 +968,14 @@ static void test_acpi_q35_multif_bridge(void) " -device virtio-balloon,id=balloon0,addr=0x4.0x2" " -device pcie-root-port,id=rp0,multifunction=on," "port=0x0,chassis=1,addr=0x2" - " -device pcie-root-port,id=rp1,port=0x1,chassis=2,addr=0x3.0x1", + " -device pcie-root-port,id=rp1,port=0x1,chassis=2,addr=0x3.0x1" + " -device pcie-root-port,id=rp2,port=0x0,chassis=3,bus=rp1,addr=0.0" + " -device pci-bridge,bus=rp2,chassis_nr=4,id=br1" + " -device pcie-root-port,id=rphptgt1,port=0x0,chassis=5,addr=2.1" + " -device pcie-root-port,id=rphptgt2,port=0x0,chassis=6,addr=2.2" + " -device pcie-root-port,id=rphptgt3,port=0x0,chassis=7,addr=2.3", &data); + free_test_data(&data); } From 48dde093d30dc6d09e832e478cb6661cf633a882 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:37 +0100 Subject: [PATCH 15/56] tests: acpi: update expected blobs add extra nested bridges/root ports to blobs so it would be posible to check how follow up patches would affect it. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-6-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/data/acpi/pc/DSDT.bridge | Bin 9532 -> 12608 bytes tests/data/acpi/pc/DSDT.roothp | Bin 6656 -> 9732 bytes tests/data/acpi/q35/DSDT.multi-bridge | Bin 8630 -> 12301 bytes tests/qtest/bios-tables-test-allowed-diff.h | 4 ---- 4 files changed, 4 deletions(-) diff --git a/tests/data/acpi/pc/DSDT.bridge b/tests/data/acpi/pc/DSDT.bridge index 75016fd4b72aae544658e51c06a2940da31c81da..d1b019754bb03fa7d815ab57304ddb2376a4c8af 100644 GIT binary patch delta 153 zcmdnvbs&k$CD$rprJH)ru3^cR4nzD~mD#0L(Zn ArT_o{ delta 67 zcmV-J0KEUeV!TQUL{mgmJS6}C0c5cXcMA-QmjPT~Q$k-5f|miajSFi60z$=;`wfP( Z2@k6a2p>>GPE-JJ0h8Y_7qd<#@(RdT6?p&v diff --git a/tests/data/acpi/pc/DSDT.roothp b/tests/data/acpi/pc/DSDT.roothp index 545512adfa0f9af81a7fafd353679f64f75e501e..14473ab4c91d68af88fff45e703f572c387d0af7 100644 GIT binary patch delta 137 zcmZoLY4PE533dr#QDb0W{I-#+oSUb431duruv5H1*J8%a-Q3?9nfsdGC3Zd z{G8XA)hXE3hjH@^-v8VZj81-`zAj;YApy>wp^Q+53m5ZdBfff0K}T7T2-5=Y>I_DP jwqOH;&F94rGBRpTo+v5D6{i<$U=bhS>=!b5y<{K&C!r|o delta 55 zcmZqiX)xh(33dr#kYZq9WZ%eD&dufjI3_;WDPF++@#bFc?~F{ohbFi4CT`B>+r&M2 LqN>E?&8mR_$Sx8n diff --git a/tests/data/acpi/q35/DSDT.multi-bridge b/tests/data/acpi/q35/DSDT.multi-bridge index 3dba4d84369f1f2850fbdc771072519d34f58072..f045438b4e794406316418074c6d319261bfcd9e 100644 GIT binary patch delta 2396 zcmZ|R&2rLU6b9fV1R4UAzqCLp368Fuu|nw&UD+1sqAFH{Zt66SE8MwtSb!{WTy({_ z?=3jacm?jc|p2))*Ftr_b@&8YT?Q@uhip} z#y=|!SlLK#^D1Nc+r7+mcuHu-@`LxOKKSLg590SpaAV?pe)@Cy$9-srAHY1HKl_1W zDjbv0^0<0Fe`@-zE?YP!%{ua54)x`=d-s9vl!C!&OF!!kPSFIAz?+!2P7i@?pxCkv zC6NyJd?85?NvO!Se2FF#lZdG#(voD7B$8B-j$C zC6SgilQfaEitI{@&BP{RD~YsZm}H1#RAf(DvP`l>vPvQ?IVL$GIThKLmQ^OJL{^nV zTJlWtMDi-qm6igN0+E7}NK27Pkw{TR4y2{Tq(r2oB+^o5QYKPXkt1nwm^efnC6Sgj zCTm32RODD%)|spmSyvKisW7P!si;U#TB=N{M5;<6Ej1=JA~hB1OUnk64I&##A}w_$ zbs}{Y8A!_}lT9Lmx)WnRT63On0QF0w)Y?63>*Dd!;x{7nT&O9 z;kR1ByQ}PEY#TUqc`#vsyB{SM4pCBo0k(8Xnm9zsAq=n$Q!>*QQ@?{zZZgim4Qb)S z=E)OQ?Zg>&!IsAvmhHwFw!ucl8P@H^8TP^U!Wk6$`^c<87UomBXPJNNma#l)lai!J==@zCu& gfw?XDw`=c0zt;^l9Bp?(g|D{bSMBiC^6j0`U&gLNmH+?% delta 97 zcmeB8*yhaT66_MPO_70t@xex}Mma8Ty_oo5r+5Kpy~)$%?8RI-gAEPhIpQ5%f_NAh wm>4*mf?a(WC%=`8pIoh=%54diVq{>Ryjr1sv!z}PBb!u!vtJ0q Date: Thu, 12 Jan 2023 15:02:38 +0100 Subject: [PATCH 16/56] tests: acpi: cleanup use_uefi argument usage 'use_uefi' is used for the flag is a part of 'test_data *data' argument that is passed to the same functions, which makes use_uefi argument redundant. Drop it and use 'data::uefi_*' directly, instead. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-7-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test.c | 35 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 6a99b10384..cb95f687fe 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -748,9 +748,9 @@ static void test_smbios_structs(test_data *data, SmbiosEntryPointType ep_type) } } -static void test_acpi_load_tables(test_data *data, bool use_uefi) +static void test_acpi_load_tables(test_data *data) { - if (use_uefi) { + if (data->uefi_fl1 && data->uefi_fl2) { /* use UEFI */ g_assert(data->scan_len); data->rsdp_addr = acpi_find_rsdp_address_uefi(data->qts, data->ram_start, data->scan_len); @@ -766,12 +766,11 @@ static void test_acpi_load_tables(test_data *data, bool use_uefi) test_acpi_fadt_table(data); } -static char *test_acpi_create_args(test_data *data, const char *params, - bool use_uefi) +static char *test_acpi_create_args(test_data *data, const char *params) { char *args; - if (use_uefi) { + if (data->uefi_fl1 && data->uefi_fl2) { /* use UEFI */ /* * TODO: convert '-drive if=pflash' to new syntax (see e33763be7cd3) * when arm/virt boad starts to support it. @@ -809,11 +808,10 @@ static char *test_acpi_create_args(test_data *data, const char *params, static void test_acpi_one(const char *params, test_data *data) { char *args; - bool use_uefi = data->uefi_fl1 && data->uefi_fl2; - args = test_acpi_create_args(data, params, use_uefi); + args = test_acpi_create_args(data, params); data->qts = qtest_init(args); - test_acpi_load_tables(data, use_uefi); + test_acpi_load_tables(data); if (getenv(ACPI_REBUILD_EXPECTED_AML)) { dump_aml_files(data, true); @@ -826,7 +824,7 @@ static void test_acpi_one(const char *params, test_data *data) * Bug on uefi-test-tools to provide entry point: * https://bugs.launchpad.net/qemu/+bug/1821884 */ - if (!use_uefi) { + if (!(data->uefi_fl1 && data->uefi_fl2)) { SmbiosEntryPointType ep_type = test_smbios_entry_point(data); test_smbios_structs(data, ep_type); } @@ -1904,10 +1902,9 @@ static void test_acpi_piix4_oem_fields(void) data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); - args = test_acpi_create_args(&data, - OEM_TEST_ARGS, false); + args = test_acpi_create_args(&data, OEM_TEST_ARGS); data.qts = qtest_init(args); - test_acpi_load_tables(&data, false); + test_acpi_load_tables(&data); test_oem_fields(&data); qtest_quit(data.qts); free_test_data(&data); @@ -1924,10 +1921,9 @@ static void test_acpi_q35_oem_fields(void) data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); - args = test_acpi_create_args(&data, - OEM_TEST_ARGS, false); + args = test_acpi_create_args(&data, OEM_TEST_ARGS); data.qts = qtest_init(args); - test_acpi_load_tables(&data, false); + test_acpi_load_tables(&data); test_oem_fields(&data); qtest_quit(data.qts); free_test_data(&data); @@ -1942,9 +1938,9 @@ static void test_acpi_microvm_oem_fields(void) test_acpi_microvm_prepare(&data); args = test_acpi_create_args(&data, - OEM_TEST_ARGS",acpi=on", false); + OEM_TEST_ARGS",acpi=on"); data.qts = qtest_init(args); - test_acpi_load_tables(&data, false); + test_acpi_load_tables(&data); test_oem_fields(&data); qtest_quit(data.qts); free_test_data(&data); @@ -1964,10 +1960,9 @@ static void test_acpi_virt_oem_fields(void) }; char *args; - args = test_acpi_create_args(&data, - "-cpu cortex-a57 "OEM_TEST_ARGS, true); + args = test_acpi_create_args(&data, "-cpu cortex-a57 "OEM_TEST_ARGS); data.qts = qtest_init(args); - test_acpi_load_tables(&data, true); + test_acpi_load_tables(&data); test_oem_fields(&data); qtest_quit(data.qts); free_test_data(&data); From 025cfbbac81d350d58599f4f2d00c51770068058 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:39 +0100 Subject: [PATCH 17/56] pci_bridge: remove whitespace Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-8-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci-bridge/pci_bridge_dev.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index 3435df8d73..4b2696ea7f 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -186,7 +186,6 @@ static Property pci_bridge_dev_properties[] = { res_reserve.mem_pref_32, -1), DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev, res_reserve.mem_pref_64, -1), - DEFINE_PROP_END_OF_LIST(), }; From f7b35824b1247d1b32b0b1001ac481d6338891fa Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:40 +0100 Subject: [PATCH 18/56] x86: acpi: pcihp: clean up duplicate bridge_in_acpi assignment Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-9-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 8c333973f9..8ba34d8fde 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -464,7 +464,6 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, call_dev_aml_func(DEVICE(pdev), dev); - bridge_in_acpi = cold_plugged_bridge && pcihp_bridge_en; if (bridge_in_acpi) { /* * device is coldplugged bridge, From 1d77e15718c83b84aa46cfb12493a1dafa2a3252 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:41 +0100 Subject: [PATCH 19/56] pci: acpi hotplug: rename x-native-hotplug to x-do-not-expose-native-hotplug-cap When ACPI PCI hotplug for Q35 was introduced (6.1), it was implemented by hiding HPC capability on PCIE slot. That however led to a number of regressions and to fix it, it was decided to keep HPC cap exposed in ACPI PCI hotplug case and force guest in ACPI PCI hotplug mode by other means [1]. That reduced meaning of x-native-hotplug to a compat knob [2] for broken 6.1 machine type. Rename property to match its current purpose. 1) 211afe5c69 (hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC) 2) c318bef762 (hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type) Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-10-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/pc_q35.c | 5 +++-- hw/pci-bridge/gen_pcie_root_port.c | 7 ++++++- hw/pci/pcie.c | 6 +++--- hw/pci/pcie_port.c | 3 ++- include/hw/pci/pcie_port.h | 3 ++- 5 files changed, 16 insertions(+), 8 deletions(-) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 83c57c6eb1..66cd718b70 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -257,8 +257,9 @@ static void pc_q35_init(MachineState *machine) NULL); if (!keep_pci_slot_hpc && acpi_pcihp) { - object_register_sugar_prop(TYPE_PCIE_SLOT, "x-native-hotplug", - "false", true); + object_register_sugar_prop(TYPE_PCIE_SLOT, + "x-do-not-expose-native-hotplug-cap", + "true", true); } /* irq lines */ diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c index 20099a8ae3..1ce4e7beba 100644 --- a/hw/pci-bridge/gen_pcie_root_port.c +++ b/hw/pci-bridge/gen_pcie_root_port.c @@ -87,7 +87,12 @@ static void gen_rp_realize(DeviceState *dev, Error **errp) return; } - if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug) { + /* + * reserving IO space led to worse issues in 6.1, when this hunk was + * introduced. (see commit: 211afe5c69b59). Keep this broken for 6.1 + * machine type ABI compatibility only + */ + if (s->hide_native_hotplug_cap && grp->res_reserve.io == -1 && s->hotplug) { grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE; } int rc = pci_bridge_qemu_reserve_cap_init(d, 0, diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 68a62da0b5..924fdabd15 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -611,11 +611,11 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s) PCI_EXP_SLTCAP_ABP); /* - * Enable native hot-plug on all hot-plugged bridges unless - * hot-plug is disabled on the slot. + * Expose native hot-plug on all bridges if hot-plug is enabled on the slot. + * (unless broken 6.1 ABI is enforced for compat reasons) */ if (s->hotplug && - (s->native_hotplug || DEVICE(dev)->hotplugged)) { + (!s->hide_native_hotplug_cap || DEVICE(dev)->hotplugged)) { pci_long_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCAP, PCI_EXP_SLTCAP_HPS | PCI_EXP_SLTCAP_HPC); diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c index 687e4e763a..65a397ad23 100644 --- a/hw/pci/pcie_port.c +++ b/hw/pci/pcie_port.c @@ -173,7 +173,8 @@ static Property pcie_slot_props[] = { DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0), DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0), DEFINE_PROP_BOOL("hotplug", PCIESlot, hotplug, true), - DEFINE_PROP_BOOL("x-native-hotplug", PCIESlot, native_hotplug, true), + DEFINE_PROP_BOOL("x-do-not-expose-native-hotplug-cap", PCIESlot, + hide_native_hotplug_cap, false), DEFINE_PROP_END_OF_LIST() }; diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h index fd484afb30..6c40e3733f 100644 --- a/include/hw/pci/pcie_port.h +++ b/include/hw/pci/pcie_port.h @@ -63,7 +63,8 @@ struct PCIESlot { /* Indicates whether any type of hot-plug is allowed on the slot */ bool hotplug; - bool native_hotplug; + /* broken ACPI hotplug compat knob to preserve 6.1 ABI intact */ + bool hide_native_hotplug_cap; QLIST_ENTRY(PCIESlot) next; }; From 45284cfb49a47bb4536e29b4965a41a0ecb63149 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:42 +0100 Subject: [PATCH 20/56] pcihp: piix4: do not call acpi_pcihp_reset() when ACPI PCI hotplug is disabled piix4_pm_reset() is calling acpi_pcihp_reset() when ACPI PCI hotplug is disabled, which leads to assigning BSEL properties to bridges on path acpi_set_bsel() ... if (qbus_is_hotpluggable(BUS(bus))) { // above happens to be true by default (though it's SHPC hotplug handler) // set BSEL } At the moment the issue is masked by the fact that we use not only BSEL, to decide if we should generated hoplug AML but also pcihp_bridge_en knob. However the later patches will drop dependency on pcihp_bridge_en, and use only BSEL exclusively to decide if hotplug AML for slots should be built, which exposes issue. We should not ever call acpi_pcihp_reset() if ACPI PCI hotplug is disabled, make it so. PS: * Q35 does the right thing (i.e. it calls acpi_pcihp_reset only when pcihp is enabled) * the issue also makes acpi_pcihp_update() logic run on SHPC enabled bridges, which seems to be harmless Fixes: 3d7e78aa777 ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-11-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/acpi/piix4.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 2ab4930f11..724294b378 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -304,7 +304,9 @@ static void piix4_pm_reset(DeviceState *dev) acpi_update_sci(&s->ar, s->irq); pm_io_space_update(s); - acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug); + if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug) { + acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug); + } } static void piix4_pm_powerdown_req(Notifier *n, void *opaque) From 2940a4b9e3d206cc759c7630dde2fb7ded3e9ec2 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:43 +0100 Subject: [PATCH 21/56] pci: acpihp: assign BSEL only to coldplugged bridges ACPI PCI hotplug would broken after bridge hotplug and then migration if hotplugged bridge were specified on target at command line. Currently it's not possible since, 'hotplugged' property was made read-only for some time now. The issue would happen due to BSEL being assigned to all bridges during 1st 'reset': source seq: 1. start 'pc' machine => sets BSEL to 0 on pci.0 (host-bridge) 2. hotplug bridge, no bsel is assigned (so far is ok) target seq: 1. start 'pc' machine with -S -device pci-bridge,id=hp_br,hotplugged=on BSEL gets assigned to as follows hp_br: 0 pci.0: 1 as result hotplug requests with migrated AML generated on source would be misdirected to 'hp_br' instead of intended pci.0 While it's not issue at the moment, it's based on implicit assumptions * 'hotplugged' property is read-only * 1st reset happens before QEMU drops into monitor mode which lets add hotplugged on source bridges as hotplugged ones (anything added at that stage counts as hotplugged (yet another assumption)) All of it looks too fragile to me, so lets restrict BSEL only to cold-plugged bridges explicitly. Migration wise it shouldn't break anything since assignment order stays the same: * user can't specify 'hotplugged=on' on CLI * user can't specify 'hotplugged=off' at monitor stage or later on older QEMU versions where 'hotplugged' is RW, hotplug is broken after migration anyways and we cannot do anything to fix that. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-12-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/acpi/pcihp.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index 99a898d9ae..5dc7377411 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -85,31 +85,40 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) } } -/* Assign BSEL property to all buses. In the future, this can be changed - * to only assign to buses that support hotplug. - */ +typedef struct { + unsigned bsel_alloc; + bool has_bridge_hotplug; +} BSELInfo; + +/* Assign BSEL property only to buses that support hotplug. */ static void *acpi_set_bsel(PCIBus *bus, void *opaque) { - unsigned *bsel_alloc = opaque; + BSELInfo *info = opaque; unsigned *bus_bsel; + DeviceState *br = bus->qbus.parent; + bool is_bridge = IS_PCI_BRIDGE(br); + /* hotplugged bridges can't be described in ACPI ignore them */ if (qbus_is_hotpluggable(BUS(bus))) { - bus_bsel = g_malloc(sizeof *bus_bsel); + if (!is_bridge || (!br->hotplugged && info->has_bridge_hotplug)) { + bus_bsel = g_malloc(sizeof *bus_bsel); - *bus_bsel = (*bsel_alloc)++; - object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, - bus_bsel, OBJ_PROP_FLAG_READ); + *bus_bsel = info->bsel_alloc++; + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, + bus_bsel, OBJ_PROP_FLAG_READ); + } } - return bsel_alloc; + return info; } -static void acpi_set_pci_info(void) +static void acpi_set_pci_info(bool has_bridge_hotplug) { static bool bsel_is_set; Object *host = acpi_get_i386_pci_host(); PCIBus *bus; - unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; + BSELInfo info = { .bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT, + .has_bridge_hotplug = has_bridge_hotplug }; if (bsel_is_set) { return; @@ -123,7 +132,7 @@ static void acpi_set_pci_info(void) bus = PCI_HOST_BRIDGE(host)->bus; if (bus) { /* Scan all PCI buses. Set property to enable acpi based hotplug. */ - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &info); } } @@ -287,7 +296,7 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off) if (acpihp_root_off) { acpi_pcihp_disable_root_bus(); } - acpi_set_pci_info(); + acpi_set_pci_info(!s->legacy_piix); acpi_pcihp_update(s); } From debbda1c67eac6c4b44d07fe4301ff9b57c82afa Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:44 +0100 Subject: [PATCH 22/56] x86: pcihp: fix invalid AML PCNT calls to hotplugged bridges When QEMU is started with hotplugged bridges (think migration): QEMU -S -monitor stdio \ -device pci-bridge,chassis_nr=1 \ -device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 (qemu) device_add pci-bridge,id=hpbr,bus=pci.1,addr=2.0,chassis_nr=3 (qemu) cont it will generate AML calls to hpbr's PCNT, which doesn't exists since it's hotplugged bridge. As result DSDT becomes malformed, with consequences that hotplug might stop working at best or crash guest OS at worst, when it attempts to call non existing PCNT method or during OS guest reboot when parsing DSDT again. IASL de-compiles malformed AML of above config DSDT as: + External (_SB_.PCI0.S18_.S10_.PCNT, MethodObj) // Warning: Unknown method, guessing 1 arguments + External (_SB_.PCI0.S18_.S19_.PCNT, MethodObj) // Warning: Unknown method, guessing 2 arguments ... BNUM = One DVNT (PCIU, One) DVNT (PCID, 0x03) - ^S08.PCNT () + ^S19.PCNT (^S10.PCNT (^S08.PCNT ())) } } With BSEL assignment limited only to coldplugged bridges [1], it should be possible to add PCNT call to a child bridge only if the child has BSEL property, otherwise ignore it since it's hotplugged. Which should fix the issue. 1) ("pci: acpihp: assign BSEL only to coldplugged bridges") Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-13-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 8ba34d8fde..1c51ab01fc 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -515,7 +515,8 @@ 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) { - if (pci_bus_is_root(sec)) { + if (pci_bus_is_root(sec) || + !object_property_find(OBJECT(sec), ACPI_PCIHP_PROP_BSEL)) { continue; } From 6dfcb0e797b7607d4f4ee98a7a93d01c5cb10bbc Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:45 +0100 Subject: [PATCH 23/56] tests: boot_sector_test: avoid crashing if status is not available yet If test case was started in paused mode (-S CLI option) and then allowed to continue via QMP, boot_sector_test could assert on transient state with following error: assertion failed (qdict_get_try_str(qret, "status") == "running"): (NULL == "running") Instead of crashing test if 'status' is not available yet, skip check and repeat iteration again after TEST_DELAY has elapsed. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-14-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/boot-sector.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qtest/boot-sector.c b/tests/qtest/boot-sector.c index 44a109abd8..d3f68018e7 100644 --- a/tests/qtest/boot-sector.c +++ b/tests/qtest/boot-sector.c @@ -160,7 +160,9 @@ void boot_sector_test(QTestState *qts) qrsp = qtest_qmp(qts, "{ 'execute': 'query-status' }"); qret = qdict_get_qdict(qrsp, "return"); g_assert_nonnull(qret); - g_assert_cmpstr(qdict_get_try_str(qret, "status"), ==, "running"); + if (qdict_get_try_str(qret, "status")) { + g_assert_cmpstr(qdict_get_try_str(qret, "status"), ==, "running"); + } qobject_unref(qrsp); g_usleep(TEST_DELAY); From 2f447a36e7336129886db224661f9151f27b853c Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:46 +0100 Subject: [PATCH 24/56] tests: acpi: extend bridge tests with hotplugged bridges with previous commit fixing malformed PCNT calls to hotplugged bridges, it should be possible add coldplug/hotplug test when describing PCI topology in DSDT without breeaking CI. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-15-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test.c | 48 ++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index cb95f687fe..b65e864a9c 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -805,12 +805,15 @@ static char *test_acpi_create_args(test_data *data, const char *params) return args; } -static void test_acpi_one(const char *params, test_data *data) +static void test_vm_prepare(const char *params, test_data *data) { - char *args; - - args = test_acpi_create_args(data, params); + char *args = test_acpi_create_args(data, params); data->qts = qtest_init(args); + g_free(args); +} + +static void process_acpi_tables(test_data *data) +{ test_acpi_load_tables(data); if (getenv(ACPI_REBUILD_EXPECTED_AML)) { @@ -830,7 +833,12 @@ static void test_acpi_one(const char *params, test_data *data) } qtest_quit(data->qts); - g_free(args); +} + +static void test_acpi_one(const char *params, test_data *data) +{ + test_vm_prepare(params, data); + process_acpi_tables(data); } static uint8_t base_required_struct_types[] = { @@ -861,8 +869,21 @@ static void test_acpi_piix4_tcg_bridge(void) data.variant = ".bridge"; data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); - test_acpi_one("-device pci-bridge,chassis_nr=1 " - "-device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 ", &data); + test_vm_prepare("-S" + " -device pci-bridge,chassis_nr=1" + " -device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2", &data); + + /* hotplugged bridges section */ + qtest_qmp_device_add(data.qts, "pci-bridge", "hpbr", + "{'bus': 'pci.1', 'addr': '2.0', 'chassis_nr': 3 }"); + qtest_qmp_device_add(data.qts, "pci-bridge", "hpbr_multifunc", + "{'bus': 'pci.1', 'addr': '0xf.1', 'chassis_nr': 4 }"); + qtest_qmp_device_add(data.qts, "pci-bridge", "hpbrhost", + "{'bus': 'pci.0', 'addr': '4.0', 'chassis_nr': 5 }"); + qtest_qmp_send(data.qts, "{'execute':'cont' }"); + qtest_qmp_eventwait(data.qts, "RESUME"); + + process_acpi_tables(&data); free_test_data(&data); } @@ -962,7 +983,7 @@ static void test_acpi_q35_multif_bridge(void) .machine = MACHINE_Q35, .variant = ".multi-bridge", }; - test_acpi_one( + test_vm_prepare("-S" " -device virtio-balloon,id=balloon0,addr=0x4.0x2" " -device pcie-root-port,id=rp0,multifunction=on," "port=0x0,chassis=1,addr=0x2" @@ -974,6 +995,17 @@ static void test_acpi_q35_multif_bridge(void) " -device pcie-root-port,id=rphptgt3,port=0x0,chassis=7,addr=2.3", &data); + /* hotplugged bridges section */ + qtest_qmp_device_add(data.qts, "pci-bridge", "hpbr1", + "{'bus': 'br1', 'addr': '6.0', 'chassis_nr': 128 }"); + qtest_qmp_device_add(data.qts, "pci-bridge", "hpbr2-multiif", + "{ 'bus': 'br1', 'addr': '2.2', 'chassis_nr': 129 }"); + qtest_qmp_device_add(data.qts, "pcie-pci-bridge", "hpbr3", + "{'bus': 'rp0', 'addr': '0.0' }"); + qtest_qmp_send(data.qts, "{'execute':'cont' }"); + qtest_qmp_eventwait(data.qts, "RESUME"); + + process_acpi_tables(&data); free_test_data(&data); } From 2efe88a94863d5ca6ec04126b9090c2c2cd64f97 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:47 +0100 Subject: [PATCH 25/56] tests: boot_sector_test(): make it multi-shot if the function is called the 2nd time within the same qtest session, it will prematurely return before boot sector is executed due to remaining signature. Follow up patch will add VM reboot to a test case and will call boot_sector_test() again within the same qtest env, which may lead to above issue. To fix it make sure signature in VM RAM is no more before exiting boot_sector_test(), so next time it's called it will wait boot sector is completed again. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-16-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/boot-sector.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/qtest/boot-sector.c b/tests/qtest/boot-sector.c index d3f68018e7..679ee17e2a 100644 --- a/tests/qtest/boot-sector.c +++ b/tests/qtest/boot-sector.c @@ -153,6 +153,8 @@ void boot_sector_test(QTestState *qts) signature_high = qtest_readb(qts, SIGNATURE_ADDR + 1); signature = (signature_high << 8) | signature_low; if (signature == SIGNATURE) { + /* wipe signature */ + qtest_writeb(qts, SIGNATURE_ADDR, 0x00); break; } From c0d19126f39000181a007371b9200fd2e2b0dcc8 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:48 +0100 Subject: [PATCH 26/56] tests: acpi: add reboot cycle to bridge test hotplugged bridges should not be described in DSDT, while it works on cold boot, some ACPPI PCI code are invoked during reboot. This patch will let us catch unexpected AML if hotplug checks are broken. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-17-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index b65e864a9c..a8c17461c8 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -812,7 +812,7 @@ static void test_vm_prepare(const char *params, test_data *data) g_free(args); } -static void process_acpi_tables(test_data *data) +static void process_acpi_tables_noexit(test_data *data) { test_acpi_load_tables(data); @@ -831,7 +831,11 @@ static void process_acpi_tables(test_data *data) SmbiosEntryPointType ep_type = test_smbios_entry_point(data); test_smbios_structs(data, ep_type); } +} +static void process_acpi_tables(test_data *data) +{ + process_acpi_tables_noexit(data); qtest_quit(data->qts); } @@ -883,6 +887,11 @@ static void test_acpi_piix4_tcg_bridge(void) qtest_qmp_send(data.qts, "{'execute':'cont' }"); qtest_qmp_eventwait(data.qts, "RESUME"); + process_acpi_tables_noexit(&data); + free_test_data(&data); + + /* check that reboot/reset doesn't change any ACPI tables */ + qtest_qmp_send(data.qts, "{'execute':'system_reset' }"); process_acpi_tables(&data); free_test_data(&data); } @@ -1005,6 +1014,11 @@ static void test_acpi_q35_multif_bridge(void) qtest_qmp_send(data.qts, "{'execute':'cont' }"); qtest_qmp_eventwait(data.qts, "RESUME"); + process_acpi_tables_noexit(&data); + free_test_data(&data); + + /* check that reboot/reset doesn't change any ACPI tables */ + qtest_qmp_send(data.qts, "{'execute':'system_reset' }"); process_acpi_tables(&data); free_test_data(&data); } From 54836748fc32ea4c564585fad58a2a7e1fdae522 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:49 +0100 Subject: [PATCH 27/56] tests: acpi: whitelist DSDT before refactoring acpi based PCI hotplug machinery Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-18-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test-allowed-diff.h | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..dea61d94f1 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,2 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/pc/DSDT.hpbrroot", From 19f5052cebe46a6faef3e0065e40a622a8798473 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:50 +0100 Subject: [PATCH 28/56] pcihp: drop pcihp_bridge_en dependency when composing PCNT method .. and use only BSEL presence to decide on how PCNT should be composed. That simplifies possible combinations to consider, but mainly it makes PCIHP AML be governed only by BSEL, which is property of PCIBus (aka part of bridge) and as result it opens possibility to convert build_append_pci_bus_devices() into AcpiDevAmlIf::build_dev_aml callback to make bridges self describing. PS: used approach leaves unused PCNT, when ACPI hotplug is completely disabled but that's harmless and followup commits will get rid of it later. Scope (PCI0) ... Method (PCNT, 0, NotSerialized) { } ... } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-19-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 53 ++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 1c51ab01fc..27f2cc4180 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -494,39 +494,34 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, aml_append(parent_scope, notify_method); } - /* Append PCNT method to notify about events on local and child buses. - * Add this method for root bus only when hotplug is enabled since DSDT - * expects it. + /* + * Append PCNT method to notify about events on local and child buses. */ - if (bsel || pcihp_bridge_en) { - method = aml_method("PCNT", 0, AML_NOTSERIALIZED); + method = aml_method("PCNT", 0, AML_NOTSERIALIZED); - /* If bus supports hotplug select it and notify about local events */ - if (bsel) { - uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel)); + /* If bus supports hotplug select it and notify about local events */ + if (bsel) { + uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel)); - aml_append(method, aml_store(aml_int(bsel_val), aml_name("BNUM"))); - aml_append(method, aml_call2("DVNT", aml_name("PCIU"), - aml_int(1))); /* Device Check */ - aml_append(method, aml_call2("DVNT", aml_name("PCID"), - aml_int(3))); /* Eject Request */ - } - - /* Notify about child bus events in any case */ - if (pcihp_bridge_en) { - QLIST_FOREACH(sec, &bus->child, sibling) { - if (pci_bus_is_root(sec) || - !object_property_find(OBJECT(sec), ACPI_PCIHP_PROP_BSEL)) { - continue; - } - - aml_append(method, aml_name("^S%.02X.PCNT", - sec->parent_dev->devfn)); - } - } - - aml_append(parent_scope, method); + aml_append(method, aml_store(aml_int(bsel_val), aml_name("BNUM"))); + aml_append(method, aml_call2("DVNT", aml_name("PCIU"), + aml_int(1))); /* Device Check */ + aml_append(method, aml_call2("DVNT", aml_name("PCID"), + aml_int(3))); /* Eject Request */ } + + /* Notify about child bus events in any case */ + QLIST_FOREACH(sec, &bus->child, sibling) { + if (pci_bus_is_root(sec) || + !object_property_find(OBJECT(sec), ACPI_PCIHP_PROP_BSEL)) { + continue; + } + + aml_append(method, aml_name("^S%.02X.PCNT", sec->parent_dev->devfn)); + } + + aml_append(parent_scope, method); + qobject_unref(bsel); } From 54f82b6461703611b0a0a08c98a25ba226f6a07a Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:51 +0100 Subject: [PATCH 29/56] tests: acpi: update expected blobs expected change: Scope (PCI0) ... Method (PCNT, 0, NotSerialized) { } ... } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-20-imammedo@redhat.com> --- tests/data/acpi/pc/DSDT.hpbrroot | Bin 3064 -> 3071 bytes tests/qtest/bios-tables-test-allowed-diff.h | 1 - 2 files changed, 1 deletion(-) diff --git a/tests/data/acpi/pc/DSDT.hpbrroot b/tests/data/acpi/pc/DSDT.hpbrroot index 578468f4f00a9373366c92926b512c192dd6675b..42d923ef3fcc17898955ff30a1dda1bfd7da0947 100644 GIT binary patch delta 42 ycmew%{$HHSCD Date: Thu, 12 Jan 2023 15:02:52 +0100 Subject: [PATCH 30/56] tests: acpi: whitelist DSDT before refactoring acpi based PCI hotplug machinery Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-21-imammedo@redhat.com> --- tests/qtest/bios-tables-test-allowed-diff.h | 36 +++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..4be20b2cd1 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,37 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/pc/DSDT", +"tests/data/acpi/pc/DSDT.bridge", +"tests/data/acpi/pc/DSDT.ipmikcs", +"tests/data/acpi/pc/DSDT.cphp", +"tests/data/acpi/pc/DSDT.memhp", +"tests/data/acpi/pc/DSDT.numamem", +"tests/data/acpi/pc/DSDT.nohpet", +"tests/data/acpi/pc/DSDT.dimmpxm", +"tests/data/acpi/pc/DSDT.acpihmat", +"tests/data/acpi/pc/DSDT.acpierst", +"tests/data/acpi/pc/DSDT.roothp", +"tests/data/acpi/pc/DSDT.hpbrroot", +"tests/data/acpi/pc/DSDT.hpbridge", +"tests/data/acpi/q35/DSDT", +"tests/data/acpi/q35/DSDT.tis.tpm2", +"tests/data/acpi/q35/DSDT.tis.tpm12", +"tests/data/acpi/q35/DSDT.bridge", +"tests/data/acpi/q35/DSDT.multi-bridge", +"tests/data/acpi/q35/DSDT.mmio64", +"tests/data/acpi/q35/DSDT.ipmibt", +"tests/data/acpi/q35/DSDT.cphp", +"tests/data/acpi/q35/DSDT.memhp", +"tests/data/acpi/q35/DSDT.numamem", +"tests/data/acpi/q35/DSDT.nohpet", +"tests/data/acpi/q35/DSDT.dimmpxm", +"tests/data/acpi/q35/DSDT.acpihmat", +"tests/data/acpi/q35/DSDT.acpierst", +"tests/data/acpi/q35/DSDT.applesmc", +"tests/data/acpi/q35/DSDT.pvpanic-isa", +"tests/data/acpi/q35/DSDT.ivrs", +"tests/data/acpi/q35/DSDT.viot", +"tests/data/acpi/q35/DSDT.cxl", +"tests/data/acpi/q35/DSDT.ipmismbus", +"tests/data/acpi/q35/DSDT.xapic", +"tests/data/acpi/q35/DSDT.acpihmat-noinitiator", +"tests/data/acpi/q35/DSDT.core-count2", From ddab4d3fae4e8cb3b1d70c9f2364987ddc18c6a3 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:53 +0100 Subject: [PATCH 31/56] pcihp: compose PCNT callchain right before its user _GPE._E01 it's a stepping stone to making build_append_pci_bus_devices() suitable for AcpiDevAmlIfClass:build_dev_aml callback and lets further simplify it by separating PCNT generation from slots descriptions. It also makes PCNT callchain ASL much more readable since callchain not longer cluttered by slots descriptors. Plus, move will let next patch easily drop empty PCNT (pc/q35) when there is nothing hotpluggable. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-22-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 27f2cc4180..d434ad9189 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -388,7 +388,6 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, { Aml *dev, *notify_method = NULL, *method; QObject *bsel; - PCIBus *sec; int devfn; bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL); @@ -494,12 +493,35 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, aml_append(parent_scope, notify_method); } + qobject_unref(bsel); +} + +static void build_append_notfication_callback(Aml *parent_scope, + const PCIBus *bus) +{ + Aml *method; + PCIBus *sec; + QObject *bsel; + + QLIST_FOREACH(sec, &bus->child, sibling) { + Aml *br_scope = aml_scope("S%.02X", sec->parent_dev->devfn); + if (pci_bus_is_root(sec) || + !object_property_find(OBJECT(sec), ACPI_PCIHP_PROP_BSEL)) { + continue; + } + build_append_notfication_callback(br_scope, sec); + aml_append(parent_scope, br_scope); + } + /* * Append PCNT method to notify about events on local and child buses. + * ps: hostbridge might not have hotplug (bsel) enabled but might have + * child bridges that do have bsel. */ method = aml_method("PCNT", 0, AML_NOTSERIALIZED); /* If bus supports hotplug select it and notify about local events */ + bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL); if (bsel) { uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel)); @@ -521,7 +543,6 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, } aml_append(parent_scope, method); - qobject_unref(bsel); } @@ -1721,6 +1742,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, aml_append(dsdt, sb_scope); if (pm->pcihp_bridge_en || pm->pcihp_root_en) { + Object *pci_host = acpi_get_i386_pci_host(); + PCIBus *bus = PCI_HOST_BRIDGE(pci_host)->bus; + + scope = aml_scope("\\_SB.PCI0"); + build_append_notfication_callback(scope, bus); + aml_append(dsdt, scope); + scope = aml_scope("_GPE"); { method = aml_method("_E01", 0, AML_NOTSERIALIZED); From 219e638f3b3f3e34b5cf00c0a0d536a7e0155f70 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:54 +0100 Subject: [PATCH 32/56] pcihp: do not put empty PCNT in DSDT count number of PCNT methods that actually call Notify and if there aren't any, drop PCNT altogether. It mostly affects 'Q35' tests where there is no root-ports /bridges attached and 'PC' machine when ACPI PCI hotplug is completely disabled. Expected ASL change: - Method (PCNT, 0, NotSerialized) - { - } ... Method (_E01, 0, NotSerialized) // _Exx: Edge-Triggered GPE { - Acquire (\_SB.PCI0.BLCK, 0xFFFF) - \_SB.PCI0.PCNT () - Release (\_SB.PCI0.BLCK) } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-23-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index d434ad9189..6368fcefa3 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -496,12 +496,13 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, qobject_unref(bsel); } -static void build_append_notfication_callback(Aml *parent_scope, +static bool build_append_notfication_callback(Aml *parent_scope, const PCIBus *bus) { Aml *method; PCIBus *sec; QObject *bsel; + int nr_notifiers = 0; QLIST_FOREACH(sec, &bus->child, sibling) { Aml *br_scope = aml_scope("S%.02X", sec->parent_dev->devfn); @@ -509,7 +510,8 @@ static void build_append_notfication_callback(Aml *parent_scope, !object_property_find(OBJECT(sec), ACPI_PCIHP_PROP_BSEL)) { continue; } - build_append_notfication_callback(br_scope, sec); + nr_notifiers = nr_notifiers + + build_append_notfication_callback(br_scope, sec); aml_append(parent_scope, br_scope); } @@ -530,6 +532,7 @@ static void build_append_notfication_callback(Aml *parent_scope, aml_int(1))); /* Device Check */ aml_append(method, aml_call2("DVNT", aml_name("PCID"), aml_int(3))); /* Eject Request */ + nr_notifiers++; } /* Notify about child bus events in any case */ @@ -544,6 +547,7 @@ static void build_append_notfication_callback(Aml *parent_scope, aml_append(parent_scope, method); qobject_unref(bsel); + return !!nr_notifiers; } static Aml *aml_pci_pdsm(void) @@ -1742,20 +1746,26 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, aml_append(dsdt, sb_scope); if (pm->pcihp_bridge_en || pm->pcihp_root_en) { + bool has_pcnt; + Object *pci_host = acpi_get_i386_pci_host(); PCIBus *bus = PCI_HOST_BRIDGE(pci_host)->bus; scope = aml_scope("\\_SB.PCI0"); - build_append_notfication_callback(scope, bus); - aml_append(dsdt, scope); + has_pcnt = build_append_notfication_callback(scope, bus); + if (has_pcnt) { + aml_append(dsdt, scope); + } scope = aml_scope("_GPE"); { method = aml_method("_E01", 0, AML_NOTSERIALIZED); - aml_append(method, - aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF)); - aml_append(method, aml_call0("\\_SB.PCI0.PCNT")); - aml_append(method, aml_release(aml_name("\\_SB.PCI0.BLCK"))); + if (has_pcnt) { + aml_append(method, + aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF)); + aml_append(method, aml_call0("\\_SB.PCI0.PCNT")); + aml_append(method, aml_release(aml_name("\\_SB.PCI0.BLCK"))); + } aml_append(scope, method); } aml_append(dsdt, scope); From 15dcfb197ee6c2c84a0a3d1a926bec81203e42dc Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:55 +0100 Subject: [PATCH 33/56] tests: acpi: update expected blobs Expected changes: * pc/bridge testcase due to ("pcihp: compose PCNT callchain right before its user _GPE._E01") ... + Scope (\_SB.PCI0) + { + Scope (S18) + { + Scope (S08) + { + Method (PCNT, 0, NotSerialized) + { + BNUM = 0x02 + DVNT (PCIU, One) + DVNT (PCID, 0x03) + } + } Method (PCNT, 0, NotSerialized) { - BNUM = Zero + BNUM = One DVNT (PCIU, One) DVNT (PCID, 0x03) - ^S18.PCNT () + ^S08.PCNT () } } + + Method (PCNT, 0, NotSerialized) + { + BNUM = Zero + DVNT (PCIU, One) + DVNT (PCID, 0x03) + ^S18.PCNT () + } } Scope (_GPE) * due to ("pcihp: do not put empty PCNT in DSDT") in the most Q35 tests ... { Name (_ADR, 0x001F0003) // _ADR: Address } - - Method (PCNT, 0, NotSerialized) - { - } } } ... { Method (_E01, 0, NotSerialized) // _Exx: Edge-Triggered GPE { - Acquire (\_SB.PCI0.BLCK, 0xFFFF) - \_SB.PCI0.PCNT () - Release (\_SB.PCI0.BLCK) } } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-24-imammedo@redhat.com> --- tests/data/acpi/pc/DSDT | Bin 6458 -> 6470 bytes tests/data/acpi/pc/DSDT.acpierst | Bin 6418 -> 6430 bytes tests/data/acpi/pc/DSDT.acpihmat | Bin 7783 -> 7795 bytes tests/data/acpi/pc/DSDT.bridge | Bin 12608 -> 12634 bytes tests/data/acpi/pc/DSDT.cphp | Bin 6922 -> 6934 bytes tests/data/acpi/pc/DSDT.dimmpxm | Bin 8112 -> 8124 bytes tests/data/acpi/pc/DSDT.hpbridge | Bin 6418 -> 6430 bytes tests/data/acpi/pc/DSDT.hpbrroot | Bin 3071 -> 3064 bytes tests/data/acpi/pc/DSDT.ipmikcs | Bin 6530 -> 6542 bytes tests/data/acpi/pc/DSDT.memhp | Bin 7817 -> 7829 bytes tests/data/acpi/pc/DSDT.nohpet | Bin 6316 -> 6328 bytes tests/data/acpi/pc/DSDT.numamem | Bin 6464 -> 6476 bytes tests/data/acpi/pc/DSDT.roothp | Bin 9732 -> 9758 bytes tests/data/acpi/q35/DSDT | Bin 8310 -> 8252 bytes tests/data/acpi/q35/DSDT.acpierst | Bin 8327 -> 8269 bytes tests/data/acpi/q35/DSDT.acpihmat | Bin 9635 -> 9577 bytes tests/data/acpi/q35/DSDT.acpihmat-noinitiator | Bin 8589 -> 8531 bytes tests/data/acpi/q35/DSDT.applesmc | Bin 8356 -> 8298 bytes tests/data/acpi/q35/DSDT.bridge | Bin 11439 -> 11458 bytes tests/data/acpi/q35/DSDT.core-count2 | Bin 32450 -> 32392 bytes tests/data/acpi/q35/DSDT.cphp | Bin 8774 -> 8716 bytes tests/data/acpi/q35/DSDT.cxl | Bin 9636 -> 9578 bytes tests/data/acpi/q35/DSDT.dimmpxm | Bin 9964 -> 9906 bytes tests/data/acpi/q35/DSDT.ipmibt | Bin 8385 -> 8327 bytes tests/data/acpi/q35/DSDT.ipmismbus | Bin 8398 -> 8340 bytes tests/data/acpi/q35/DSDT.ivrs | Bin 8327 -> 8269 bytes tests/data/acpi/q35/DSDT.memhp | Bin 9669 -> 9611 bytes tests/data/acpi/q35/DSDT.mmio64 | Bin 9440 -> 9382 bytes tests/data/acpi/q35/DSDT.multi-bridge | Bin 12301 -> 12358 bytes tests/data/acpi/q35/DSDT.nohpet | Bin 8168 -> 8110 bytes tests/data/acpi/q35/DSDT.numamem | Bin 8316 -> 8258 bytes tests/data/acpi/q35/DSDT.pvpanic-isa | Bin 8411 -> 8353 bytes tests/data/acpi/q35/DSDT.tis.tpm12 | Bin 8916 -> 8858 bytes tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8942 -> 8884 bytes tests/data/acpi/q35/DSDT.viot | Bin 9419 -> 9361 bytes tests/data/acpi/q35/DSDT.xapic | Bin 35673 -> 35615 bytes tests/qtest/bios-tables-test-allowed-diff.h | 36 ------------------ 37 files changed, 36 deletions(-) diff --git a/tests/data/acpi/pc/DSDT b/tests/data/acpi/pc/DSDT index b688686dc3614f56582991c0974f6ef1964ee6ce..c99179b35254725daeebb416400b1b6f9f1d74c4 100644 GIT binary patch delta 51 zcmdmGbj*m$CDWCDp4a&GRf0H&DuV5fM2#(>Q|+&N54-3^m}@wPKL9iDuK z+i3GvzJJ`C!}#ktHwUX+WZdkh*1@GB;Kvc82htPZ>}eq2&KhiJ5ig(?Y+w;Dq7dNh f7s61$#WeZ8fjFm#CWOlfG9=Lflg2PAv-l6)2(#cWs{xbnFltGUA!z^r diff --git a/tests/data/acpi/pc/DSDT.cphp b/tests/data/acpi/pc/DSDT.cphp index 53eb0dd7d422e880a668cf3ea01b8b288004042a..659ad3d6b9026c090e0d8d8e21ece5df44249ec0 100644 GIT binary patch delta 51 zcmeA&n`Xx466_KpCe6UWxPBv-2QTNT(W8#CI;sqS9Ze|qn=j9R5j?n`N1UP#dOb(Dw F005}751ar1 delta 38 ucmdmEzrmi%CDsxPLG*^*2p!<4tBfI=PU~ zc(W|uA8rK^`2c6X5QYLqC%;f%moUGO0B6roMkvFDi+S@NzIx8hGn6kfPF}294FFm9 BAYA|e diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT index 2771bcea89b531549557a19538606219a8e222b1..d68c472b460e4609a64ea67de3c4cebfca76164d 100644 GIT binary patch delta 49 zcmez7u*ZSRCDqa2sFR7`xZQ@nt))aI#jvl!h)*aDpWLKp<>(C*0P66_MvuE4;+D7ulWQI5+;Iwn5YDPF)udh=Ad)r{^UYyr-GAq)cc@$Lbx nB9`&428ImL$}#%PAXNd*o(4`n&ffq3BME@iN2?CD_ diff --git a/tests/data/acpi/q35/DSDT.core-count2 b/tests/data/acpi/q35/DSDT.core-count2 index 375aceed6b16528f7986fad46b045eba76af9760..0603db8cc63cfc562f83e55eaf5162e7c29bf4d1 100644 GIT binary patch delta 51 zcmX@~m$Bn7BbQ6COGrl@0|VoRja+AIxLl-S;)9*y1$-noKdhO>$Sc4T?;hYP!WQpp HV8{Rf*RK!t delta 109 zcmeD9%XsK7BbQ6COUR)*1_s6n8@bNbaCu9`#0NXY3ph({epoY$(OrZsz}YW^LBKxV pJ-}7OGTzm|kRe(*MxPm^D!|#(z{$ti`~QC=0g(D=btD0h3IKv5Auj*` diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp index a0ecafc36c57c6d4791b511f3febe210713d253c..beeb83c33b385fc8b41d44f299b8d9ba7203d935 100644 GIT binary patch delta 49 zcmX@+(&NJA66_Mfqr||#_;VvypaPeRR7`xZQ@ntW-&MH0rDEcPo#F+Yr8aY`&SG>IVGD5f3tf66_LkP=SGgan?qzMmbJ5`Iz`%r+5Ls%~Rx_F}jPe1vvYKFbLSky9c<6 mSjM{=7&1gF$LKSI6a_eY8aVkld;kBBBmhz$t&SuBQUL&4{~kpE diff --git a/tests/data/acpi/q35/DSDT.ipmismbus b/tests/data/acpi/q35/DSDT.ipmismbus index 3f32dffdbf3cd7e3791155530cf89417d8f2ec90..6c5d1afe443d9261d3b93801711f8d5b267696f3 100644 GIT binary patch delta 48 zcmX@-IK`36CD(C*0P66_MvuE4;+D7ulWQI5+;Iwn5YDPF)udh=Ad)r{^UYyr-GAq)cc@$Lbx nB9`&428ImL$}#%PAXNd*o(4`n&ffq3BME@iN2?(3`;J66_MfYrw$3D7cZUQI5MwgfS*Q*ePD1Q+V<;IR_>;&dE3AiYKqpS7eE9 z0&@K2izolm194n9Cm)k9ntVl10m89SD4M)ZPeC@i$@N08p=CTryrWAH4%f@R`r4cAbYmGO IpVhAf00jg!7ytkO diff --git a/tests/data/acpi/q35/DSDT.nohpet b/tests/data/acpi/q35/DSDT.nohpet index b116947dacd4fe9b563ecc7e1510cdb2474011cb..9ff9983a80a7487470ccd02ce587200444675816 100644 GIT binary patch delta 49 zcmaE1zs{b^CDpumw2#g)j)%$GZo( nide?G8W=J}E63g4F Cg$w8a delta 105 zcmZ4Jc-xW7CDeV`_A&9nPVoYMcAFQeW>_A&9nPVoY+_L~>Vyd~0-XIq7zFI&-2+@j mEaP1b3>l)8WAvFpiUOQH4V-+Oz5oA55&)@>R!0&5sQ>^^$sO$g diff --git a/tests/data/acpi/q35/DSDT.viot b/tests/data/acpi/q35/DSDT.viot index 6b436f9cd95776c26bec09066eb621bf97219dc6..eeb40b360f7c1de93501e1ddcd7dab306a51113b 100644 GIT binary patch delta 48 zcmX@@Ink5LCD$Fl2~Uj?rfZDGG4*G;s29_Wu7LNdTliS{+FMqyhj-gB}zB diff --git a/tests/data/acpi/q35/DSDT.xapic b/tests/data/acpi/q35/DSDT.xapic index f47f09122287bdd20d7762d3d6dee6e05d944285..3aa86f07243f0449c7dc245650715d729744e3ee 100644 GIT binary patch delta 51 zcmcaPjcNWgCN7s?mk{}G1_nm&ja(_6TrN^E@xe~<0zQ(PD>`Q}@(S?8y9c<6u*JI? H7%~6=i=GYM delta 109 zcmbO~jp^nzCN7s?mypPA1_nm$ja(_6T;5VK@xe~<0?tyKD>`Q}x{I&{IQxY#2-wHF p2e^t@#=9CAGDIuK=re;<1vq;eIQckx|NoC908$^VjwAq50RX(s9~b}t diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index 4be20b2cd1..dfb8523c8b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1,37 +1 @@ /* List of comma-separated changed AML files to ignore */ -"tests/data/acpi/pc/DSDT", -"tests/data/acpi/pc/DSDT.bridge", -"tests/data/acpi/pc/DSDT.ipmikcs", -"tests/data/acpi/pc/DSDT.cphp", -"tests/data/acpi/pc/DSDT.memhp", -"tests/data/acpi/pc/DSDT.numamem", -"tests/data/acpi/pc/DSDT.nohpet", -"tests/data/acpi/pc/DSDT.dimmpxm", -"tests/data/acpi/pc/DSDT.acpihmat", -"tests/data/acpi/pc/DSDT.acpierst", -"tests/data/acpi/pc/DSDT.roothp", -"tests/data/acpi/pc/DSDT.hpbrroot", -"tests/data/acpi/pc/DSDT.hpbridge", -"tests/data/acpi/q35/DSDT", -"tests/data/acpi/q35/DSDT.tis.tpm2", -"tests/data/acpi/q35/DSDT.tis.tpm12", -"tests/data/acpi/q35/DSDT.bridge", -"tests/data/acpi/q35/DSDT.multi-bridge", -"tests/data/acpi/q35/DSDT.mmio64", -"tests/data/acpi/q35/DSDT.ipmibt", -"tests/data/acpi/q35/DSDT.cphp", -"tests/data/acpi/q35/DSDT.memhp", -"tests/data/acpi/q35/DSDT.numamem", -"tests/data/acpi/q35/DSDT.nohpet", -"tests/data/acpi/q35/DSDT.dimmpxm", -"tests/data/acpi/q35/DSDT.acpihmat", -"tests/data/acpi/q35/DSDT.acpierst", -"tests/data/acpi/q35/DSDT.applesmc", -"tests/data/acpi/q35/DSDT.pvpanic-isa", -"tests/data/acpi/q35/DSDT.ivrs", -"tests/data/acpi/q35/DSDT.viot", -"tests/data/acpi/q35/DSDT.cxl", -"tests/data/acpi/q35/DSDT.ipmismbus", -"tests/data/acpi/q35/DSDT.xapic", -"tests/data/acpi/q35/DSDT.acpihmat-noinitiator", -"tests/data/acpi/q35/DSDT.core-count2", From 5d1aee56676c82c3bf2f678a816c0821d362a77b Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:56 +0100 Subject: [PATCH 34/56] whitelist DSDT before adding endpoint devices to bridge testcases Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-25-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test-allowed-diff.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..571f14fd59 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,5 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/pc/DSDT.roothp", +"tests/data/acpi/pc/DSDT.hpbrroot", +"tests/data/acpi/q35/DSDT.bridge", +"tests/data/acpi/q35/DSDT.multi-bridge", From be8e333138d8e95602b0e1343a95a376785f6dc3 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:57 +0100 Subject: [PATCH 35/56] tests: acpi: add endpoint devices to bridges to make sure that they are enumerated or ignored as expected Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-26-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test.c | 37 ++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index a8c17461c8..22b22c403d 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -875,7 +875,9 @@ static void test_acpi_piix4_tcg_bridge(void) data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); test_vm_prepare("-S" " -device pci-bridge,chassis_nr=1" - " -device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2", &data); + " -device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2" + " -device pci-testdev,bus=pci.0,addr=5.0" + " -device pci-testdev,bus=pci.1", &data); /* hotplugged bridges section */ qtest_qmp_device_add(data.qts, "pci-bridge", "hpbr", @@ -884,6 +886,10 @@ static void test_acpi_piix4_tcg_bridge(void) "{'bus': 'pci.1', 'addr': '0xf.1', 'chassis_nr': 4 }"); qtest_qmp_device_add(data.qts, "pci-bridge", "hpbrhost", "{'bus': 'pci.0', 'addr': '4.0', 'chassis_nr': 5 }"); + qtest_qmp_device_add(data.qts, "pci-testdev", "d1", "{'bus': 'pci.0' }"); + qtest_qmp_device_add(data.qts, "pci-testdev", "d2", "{'bus': 'pci.1' }"); + qtest_qmp_device_add(data.qts, "pci-testdev", "d3", "{'bus': 'hpbr', " + "'addr': '1.0' }"); qtest_qmp_send(data.qts, "{'execute':'cont' }"); qtest_qmp_eventwait(data.qts, "RESUME"); @@ -907,7 +913,9 @@ static void test_acpi_piix4_no_root_hotplug(void) data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); test_acpi_one("-global PIIX4_PM.acpi-root-pci-hotplug=off " "-device pci-bridge,chassis_nr=1 " - "-device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 ", &data); + "-device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 " + "-device pci-testdev,bus=pci.0 " + "-device pci-testdev,bus=pci.1", &data); free_test_data(&data); } @@ -922,7 +930,9 @@ static void test_acpi_piix4_no_bridge_hotplug(void) data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); test_acpi_one("-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off " "-device pci-bridge,chassis_nr=1 " - "-device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 ", &data); + "-device pci-bridge,bus=pci.1,addr=1.0,chassis_nr=2 " + "-device pci-testdev,bus=pci.0 " + "-device pci-testdev,bus=pci.1,addr=2.0", &data); free_test_data(&data); } @@ -937,7 +947,9 @@ static void test_acpi_piix4_no_acpi_pci_hotplug(void) data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); test_acpi_one("-global PIIX4_PM.acpi-root-pci-hotplug=off " "-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off " - "-device pci-bridge,chassis_nr=1", &data); + "-device pci-bridge,chassis_nr=1 " + "-device pci-testdev,bus=pci.0 " + "-device pci-testdev,bus=pci.1", &data); free_test_data(&data); } @@ -982,7 +994,9 @@ static void test_acpi_q35_tcg_bridge(void) data.variant = ".bridge"; data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); - test_acpi_one("-device pci-bridge,chassis_nr=1", &data); + test_acpi_one("-device pci-bridge,chassis_nr=1,id=br1" + " -device pci-testdev,bus=pcie.0" + " -device pci-testdev,bus=br1", &data); free_test_data(&data); } @@ -1001,8 +1015,11 @@ static void test_acpi_q35_multif_bridge(void) " -device pci-bridge,bus=rp2,chassis_nr=4,id=br1" " -device pcie-root-port,id=rphptgt1,port=0x0,chassis=5,addr=2.1" " -device pcie-root-port,id=rphptgt2,port=0x0,chassis=6,addr=2.2" - " -device pcie-root-port,id=rphptgt3,port=0x0,chassis=7,addr=2.3", - &data); + " -device pcie-root-port,id=rphptgt3,port=0x0,chassis=7,addr=2.3" + " -device pci-testdev,bus=pcie.0,addr=2.4" + " -device pci-testdev,bus=pcie.0,addr=5.0" + " -device pci-testdev,bus=rp0,addr=0.0" + " -device pci-testdev,bus=br1", &data); /* hotplugged bridges section */ qtest_qmp_device_add(data.qts, "pci-bridge", "hpbr1", @@ -1010,7 +1027,11 @@ static void test_acpi_q35_multif_bridge(void) qtest_qmp_device_add(data.qts, "pci-bridge", "hpbr2-multiif", "{ 'bus': 'br1', 'addr': '2.2', 'chassis_nr': 129 }"); qtest_qmp_device_add(data.qts, "pcie-pci-bridge", "hpbr3", - "{'bus': 'rp0', 'addr': '0.0' }"); + "{'bus': 'rphptgt1', 'addr': '0.0' }"); + qtest_qmp_device_add(data.qts, "pcie-root-port", "hprp", + "{'bus': 'rphptgt2', 'addr': '0.0' }"); + qtest_qmp_device_add(data.qts, "pci-testdev", "hpnic", + "{'bus': 'rphptgt3', 'addr': '0.0' }"); qtest_qmp_send(data.qts, "{'execute':'cont' }"); qtest_qmp_eventwait(data.qts, "RESUME"); From 65e414a9dda6ea1bd52a74dc6e75003f3ca92003 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:58 +0100 Subject: [PATCH 36/56] tests: acpi: update expected blobs previous commit added endpoint devices to bridge testcases, which exposes extra non-hotpluggable slot in DSDT on bus where hotplug is not available. It should look like this (numbers may vary): + Device (S28) + { + Name (_ADR, 0x00050000) // _ADR: Address + } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-27-imammedo@redhat.com> --- tests/data/acpi/pc/DSDT.hpbrroot | Bin 3064 -> 3081 bytes tests/data/acpi/pc/DSDT.roothp | Bin 9758 -> 9775 bytes tests/data/acpi/q35/DSDT.bridge | Bin 11458 -> 11475 bytes tests/data/acpi/q35/DSDT.multi-bridge | Bin 12358 -> 12375 bytes tests/qtest/bios-tables-test-allowed-diff.h | 4 ---- 5 files changed, 4 deletions(-) diff --git a/tests/data/acpi/pc/DSDT.hpbrroot b/tests/data/acpi/pc/DSDT.hpbrroot index 578468f4f00a9373366c92926b512c192dd6675b..a71ed4fbaa14be655c28a5e03e50157b4476e480 100644 GIT binary patch delta 53 zcmew%-YLQ566_Mf$-}_Fcyc4xJx(r1rI`3&r+5KR#m%2M*_Z^QoA`r`4B|QB9bJNe Hs#q8RhyD!~ delta 35 qcmeB__#w{a66_N4gPVbYQFkNPJx(qM#hCbDr+5Jmh0UKh*_Z&l3JL`P diff --git a/tests/data/acpi/pc/DSDT.roothp b/tests/data/acpi/pc/DSDT.roothp index fe502ed97751950cc245d728c873065f062c76b2..d58f4d2f0adbb86f8f6403a1cf9b13e1cabed035 100644 GIT binary patch delta 58 zcmbQ|v)+fxCDaNm`jT&21d00Qm}$^ZZW diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge index c38b121ad90ecb896a906a50340ad5bd7d5453f9..3a01bb196b047b875be07be28d07f3139716e82f 100644 GIT binary patch delta 56 zcmX>Uc{!5HCDM9f^W)*?4>q)j=ZJT73F2X3 MU}o6-Q0pNh002i2`~Uy| delta 40 wcmcZ{c_@<0CDa6N&`CD)hUqa1ewBV$Z_uv5H1JHzH_a;x|@2kNph0{{nL3+(^^ diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index 571f14fd59..dfb8523c8b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1,5 +1 @@ /* List of comma-separated changed AML files to ignore */ -"tests/data/acpi/pc/DSDT.roothp", -"tests/data/acpi/pc/DSDT.hpbrroot", -"tests/data/acpi/q35/DSDT.bridge", -"tests/data/acpi/q35/DSDT.multi-bridge", From ab84fc1c353cd396b420e3c3360508ada594f6a9 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:02:59 +0100 Subject: [PATCH 37/56] x86: pcihp: acpi: prepare slot ignore rule to work with self describing bridges Before switching pci bridges to AcpiDevAmlIf interface, ensure that ignored slots are handled correctly. (existing rule works but only if bridge doesn't have AcpiDevAmlIf interface). While at it rewrite related comments to be less confusing (hopefully). Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-28-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 6368fcefa3..8045b20713 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -423,14 +423,22 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, hotpluggbale_slot = bsel && dc->hotpluggable && !cold_plugged_bridge; - /* - * 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, and for other - * functions if device on function provides its own AML - */ - if (func && !bridge_in_acpi && !get_dev_aml_func(DEVICE(pdev))) { - continue; + if (func) { + if (IS_PCI_BRIDGE(pdev)) { + /* + * Ignore only hotplugged PCI bridges on !0 functions, but + * allow describing cold plugged bridges on all functions + */ + if (DEVICE(pdev)->hotplugged) { + continue; + } + } else if (!get_dev_aml_func(DEVICE(pdev))) { + /* + * Ignore all other devices on !0 functions unless they + * have AML description (i.e have get_dev_aml_func() != 0) + */ + continue; + } } } else { /* From d78644c7817617ea99b05ff30738580c56a6194f Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:00 +0100 Subject: [PATCH 38/56] pci: acpi: wire up AcpiDevAmlIf interface to generic bridge ... so that the concrete impl. won't has to duplicate it every time. By default it doesn't do anything unless leaf class defines and sets AcpiDevAmlIfClass::build_dev_aml handler. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-29-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci/pci_bridge.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index b2b180edd6..a1a1cc861e 100644 --- a/hw/pci/pci_bridge.c +++ b/hw/pci/pci_bridge.c @@ -36,6 +36,7 @@ #include "qemu/module.h" #include "qemu/range.h" #include "qapi/error.h" +#include "hw/acpi/acpi_aml_interface.h" /* PCI bridge subsystem vendor ID helper functions */ #define PCI_SSVID_SIZEOF 8 @@ -472,6 +473,10 @@ static const TypeInfo pci_bridge_type_info = { .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(PCIBridge), .abstract = true, + .interfaces = (InterfaceInfo[]) { + { TYPE_ACPI_DEV_AML_IF }, + { }, + }, }; static void pci_bridge_register_types(void) From 6c36ec46b0d28f682eed1ce1278989535c1307dc Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:01 +0100 Subject: [PATCH 39/56] pcihp: make bridge describe itself using AcpiDevAmlIfClass:build_dev_aml simplify build_append_pci_bus_devices() a bit by handling bridge specific logic in bridge dedicated AcpiDevAmlIfClass::build_dev_aml callback. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-30-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/acpi/Kconfig | 4 ++++ hw/acpi/meson.build | 4 +++- hw/acpi/pci-bridge-stub.c | 20 ++++++++++++++++++++ hw/acpi/pci-bridge.c | 27 +++++++++++++++++++++++++++ hw/i386/Kconfig | 1 + hw/i386/acpi-build.c | 17 ++--------------- hw/pci/pci_bridge.c | 9 +++++++++ include/hw/acpi/pci.h | 4 ++++ 8 files changed, 70 insertions(+), 16 deletions(-) create mode 100644 hw/acpi/pci-bridge-stub.c create mode 100644 hw/acpi/pci-bridge.c diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig index 1f7803fdab..e07d3204eb 100644 --- a/hw/acpi/Kconfig +++ b/hw/acpi/Kconfig @@ -39,6 +39,10 @@ config ACPI_PCIHP bool depends on ACPI +config ACPI_PCI_BRIDGE + bool + depends on ACPI && PCI && ACPI_PCIHP + config ACPI_HMAT bool depends on ACPI diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build index 30054a8cdc..50b73129b4 100644 --- a/hw/acpi/meson.build +++ b/hw/acpi/meson.build @@ -19,6 +19,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c')) acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c')) acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c')) +acpi_ss.add(when: 'CONFIG_ACPI_PCI_BRIDGE', if_true: files('pci-bridge.c')) acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_true: files('pcihp.c')) acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_false: files('acpi-pci-hotplug-stub.c')) acpi_ss.add(when: 'CONFIG_ACPI_VIOT', if_true: files('viot.c')) @@ -30,9 +31,10 @@ if have_tpm acpi_ss.add(files('tpm.c')) endif softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c', 'ghes-stub.c', 'acpi_interface.c')) +softmmu_ss.add(when: 'CONFIG_ACPI_PCI_BRIDGE', if_false: files('pci-bridge-stub.c')) softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss) softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-build-stub.c', 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c', 'acpi-mem-hotplug-stub.c', 'acpi-cpu-hotplug-stub.c', 'acpi-pci-hotplug-stub.c', 'acpi-nvdimm-stub.c', - 'cxl-stub.c')) + 'cxl-stub.c', 'pci-bridge-stub.c')) diff --git a/hw/acpi/pci-bridge-stub.c b/hw/acpi/pci-bridge-stub.c new file mode 100644 index 0000000000..9d78638c48 --- /dev/null +++ b/hw/acpi/pci-bridge-stub.c @@ -0,0 +1,20 @@ +/* + * QEMU ACPI PCI bridge stub + * + * Copyright (c) 2023 Red Hat, Inc. + * + * Author: + * Igor Mammedov + * + * SPDX-License-Identifier: GPL-2.0-or-later + * + * 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 "hw/acpi/pci.h" + +void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope) +{ +} diff --git a/hw/acpi/pci-bridge.c b/hw/acpi/pci-bridge.c new file mode 100644 index 0000000000..5f3ee5157f --- /dev/null +++ b/hw/acpi/pci-bridge.c @@ -0,0 +1,27 @@ +/* + * QEMU ACPI PCI bridge + * + * Copyright (c) 2023 Red Hat, Inc. + * + * Author: + * Igor Mammedov + * + * SPDX-License-Identifier: GPL-2.0-or-later + * + * 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 "hw/acpi/pci.h" +#include "hw/pci/pci_bridge.h" +#include "hw/acpi/pcihp.h" + +void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope) +{ + PCIBridge *br = PCI_BRIDGE(adev); + + if (object_property_find(OBJECT(&br->sec_bus), ACPI_PCIHP_PROP_BSEL)) { + build_append_pci_bus_devices(scope, pci_bridge_get_sec_bus(br)); + } +} diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig index c4fb5b49bd..1bf47b0b0b 100644 --- a/hw/i386/Kconfig +++ b/hw/i386/Kconfig @@ -58,6 +58,7 @@ config PC_ACPI select ACPI_X86 select ACPI_CPU_HOTPLUG select ACPI_MEMORY_HOTPLUG + select ACPI_PCI_BRIDGE select ACPI_VIOT select SMBUS_EEPROM select PFLASH_CFI01 diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 8045b20713..49181a55b1 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -383,8 +383,7 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot) aml_append(method, if_ctx); } -static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, - bool pcihp_bridge_en) +void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) { Aml *dev, *notify_method = NULL, *method; QObject *bsel; @@ -406,7 +405,6 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ int adr = slot << 16 | func; bool hotpluggbale_slot = false; - bool bridge_in_acpi = false; bool cold_plugged_bridge = false; if (pdev) { @@ -418,7 +416,6 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, */ cold_plugged_bridge = IS_PCI_BRIDGE(pdev) && !DEVICE(pdev)->hotplugged; - bridge_in_acpi = cold_plugged_bridge && pcihp_bridge_en; hotpluggbale_slot = bsel && dc->hotpluggable && !cold_plugged_bridge; @@ -471,16 +468,6 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, call_dev_aml_func(DEVICE(pdev), dev); - if (bridge_in_acpi) { - /* - * device is coldplugged bridge, - * add child device descriptions into its scope - */ - PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev)); - - build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en); - } - if (hotpluggbale_slot) { aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); /* add _EJ0 to make slot hotpluggable */ @@ -1704,7 +1691,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, PCIBus *bus = PCI_HOST_BRIDGE(pci_host)->bus; Aml *scope = aml_scope("PCI0"); /* Scan all PCI buses. Generate tables to support hotplug. */ - build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); + build_append_pci_bus_devices(scope, bus); aml_append(sb_scope, scope); } } diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index a1a1cc861e..dd5af508f9 100644 --- a/hw/pci/pci_bridge.c +++ b/hw/pci/pci_bridge.c @@ -37,6 +37,7 @@ #include "qemu/range.h" #include "qapi/error.h" #include "hw/acpi/acpi_aml_interface.h" +#include "hw/acpi/pci.h" /* PCI bridge subsystem vendor ID helper functions */ #define PCI_SSVID_SIZEOF 8 @@ -468,10 +469,18 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset, return 0; } +static void pci_bridge_class_init(ObjectClass *klass, void *data) +{ + AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass); + + adevc->build_dev_aml = build_pci_bridge_aml; +} + static const TypeInfo pci_bridge_type_info = { .name = TYPE_PCI_BRIDGE, .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(PCIBridge), + .class_init = pci_bridge_class_init, .abstract = true, .interfaces = (InterfaceInfo[]) { { TYPE_ACPI_DEV_AML_IF }, diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h index b5deee0a9d..467a99461c 100644 --- a/include/hw/acpi/pci.h +++ b/include/hw/acpi/pci.h @@ -27,6 +27,7 @@ #define HW_ACPI_PCI_H #include "hw/acpi/bios-linker-loader.h" +#include "hw/acpi/acpi_aml_interface.h" typedef struct AcpiMcfgInfo { uint64_t base; @@ -36,4 +37,7 @@ typedef struct AcpiMcfgInfo { void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info, const char *oem_id, const char *oem_table_id); Aml *aml_pci_device_dsm(void); + +void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus); +void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope); #endif From c6f16471959e49db40a41371134240a8bd464450 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:02 +0100 Subject: [PATCH 40/56] =?UTF-8?q?pci:=20make=20sure=20pci=5Fbus=5Fis=5Fexp?= =?UTF-8?q?ress()=20won't=20error=20out=20with=20"discards=20=E2=80=98cons?= =?UTF-8?q?t=E2=80=99=20qualifier"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit function doesn't need RW aceess to passed in bus pointer, make it const. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-31-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci/pci.c | 2 +- include/hw/pci/pci.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 39a7bb32aa..208c16f450 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -483,7 +483,7 @@ static void pci_bus_uninit(PCIBus *bus) pci_host_bus_unregister(BUS(bus)->parent); } -bool pci_bus_is_express(PCIBus *bus) +bool pci_bus_is_express(const PCIBus *bus) { return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS); } diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 85ee458cd2..d5a40cd058 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -270,7 +270,7 @@ typedef void (*pci_bus_dev_fn)(PCIBus *b, PCIDevice *d, void *opaque); typedef void (*pci_bus_fn)(PCIBus *b, void *opaque); typedef void *(*pci_bus_ret_fn)(PCIBus *b, void *opaque); -bool pci_bus_is_express(PCIBus *bus); +bool pci_bus_is_express(const PCIBus *bus); void pci_root_bus_init(PCIBus *bus, size_t bus_size, DeviceState *parent, const char *name, From a06c15a3b0778848c61b1bc3f03e41a3b585ea3d Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:03 +0100 Subject: [PATCH 41/56] pcihp: isolate rule whether slot should be described in DSDT Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-32-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 83 +++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 40 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 49181a55b1..b4c9ff4794 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -383,6 +383,42 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot) aml_append(method, if_ctx); } +static bool is_devfn_ignored(const int devfn, const PCIBus *bus, + bool bus_has_hotplug) +{ + const PCIDevice *pdev = bus->devices[devfn]; + + if (pdev) { + if (PCI_FUNC(devfn)) { + if (IS_PCI_BRIDGE(pdev)) { + /* + * Ignore only hotplugged PCI bridges on !0 functions, but + * allow describing cold plugged bridges on all functions + */ + if (DEVICE(pdev)->hotplugged) { + return true; + } + } else if (!get_dev_aml_func(DEVICE(pdev))) { + /* + * Ignore all other devices on !0 functions unless they + * have AML description (i.e have get_dev_aml_func() != 0) + */ + return true; + } + } + } else { /* non populated slots */ + /* + * hotplug is supported only for non-multifunction device + * so generate device description only for function 0 + */ + if (!bus_has_hotplug || PCI_FUNC(devfn) || + (pci_bus_is_express(bus) && PCI_SLOT(devfn) > 0)) { + return true; + } + } + return false; +} + void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) { Aml *dev, *notify_method = NULL, *method; @@ -398,59 +434,26 @@ void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) } for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { - DeviceClass *dc; 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 hotpluggbale_slot = false; - bool cold_plugged_bridge = false; + bool hotpluggbale_slot = true; + + if (is_devfn_ignored(devfn, bus, !!bsel)) { + continue; + } if (pdev) { - dc = DEVICE_GET_CLASS(pdev); - /* * Cold plugged bridges aren't themselves hot-pluggable. * Hotplugged bridges *are* hot-pluggable. */ - cold_plugged_bridge = IS_PCI_BRIDGE(pdev) && + bool cold_plugged_bridge = IS_PCI_BRIDGE(pdev) && !DEVICE(pdev)->hotplugged; - - hotpluggbale_slot = bsel && dc->hotpluggable && + hotpluggbale_slot = bsel && DEVICE_GET_CLASS(pdev)->hotpluggable && !cold_plugged_bridge; - - if (func) { - if (IS_PCI_BRIDGE(pdev)) { - /* - * Ignore only hotplugged PCI bridges on !0 functions, but - * allow describing cold plugged bridges on all functions - */ - if (DEVICE(pdev)->hotplugged) { - continue; - } - } else if (!get_dev_aml_func(DEVICE(pdev))) { - /* - * Ignore all other devices on !0 functions unless they - * have AML description (i.e have get_dev_aml_func() != 0) - */ - continue; - } - } - } else { - /* - * 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; - } - /* mark it as empty hotpluggable slot */ - hotpluggbale_slot = true; - } else { - continue; - } } /* start to compose PCI device descriptor */ From 2e827356dfa87156a944a83217f675f212294398 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:04 +0100 Subject: [PATCH 42/56] tests: acpi: whitelist DSDT before decoupling PCI hotplug code from basic slots description Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-33-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test-allowed-diff.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..1983fa596b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,15 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/pc/DSDT", +"tests/data/acpi/pc/DSDT.acpierst", +"tests/data/acpi/pc/DSDT.acpihmat", +"tests/data/acpi/pc/DSDT.bridge", +"tests/data/acpi/pc/DSDT.cphp", +"tests/data/acpi/pc/DSDT.dimmpxm", +"tests/data/acpi/pc/DSDT.hpbridge", +"tests/data/acpi/pc/DSDT.ipmikcs", +"tests/data/acpi/pc/DSDT.memhp", +"tests/data/acpi/pc/DSDT.nohpet", +"tests/data/acpi/pc/DSDT.numamem", +"tests/data/acpi/pc/DSDT.roothp", +"tests/data/acpi/q35/DSDT.bridge", +"tests/data/acpi/q35/DSDT.multi-bridge", From 6fe5518e4fb75f6b3ae08d4b58da87fe8734a5de Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:05 +0100 Subject: [PATCH 43/56] pcihp: acpi: decouple hotplug and generic slots description Split build_append_pci_bus_devices() onto generic part that builds AML descriptions only for populated slots which is applicable to both hotplug disabled and enabled bridges. And a hotplug only part that complements generic AML with hotplug depended bits (that depend on BSEL), like _SUN/_EJ0 entries, dynamic _DSM. Hotplug part, will generate full 'Device' descriptors for non-populated slots (like it used to be) and complementary 'Scope' descriptors for populated slots that are hotplug capable. i.e. something like this: - ... + Name (BSEL, 0x03) + Scope (S00) + { + Name (ASUN, Zero) + Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method + { + Local0 = Package (0x02) + { + BSEL, + ASUN + } + Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0)) + } + [ ... other hotplug depended bits ] + } While generic build_append_pci_bus_devices() still calls hotplug part at its end it doesn't really depend on any hotplug bits anymore and later both could be completely separated when it's necessary. Main benefit though is that both build_append_pci_bus_devices() and build_append_pcihp_slots() become more readable and it makes easier to modify them with less risk of affecting another part. Also it opens possibility to re-use generic part elsewhere (microvm, arm/virt). Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-34-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 121 +++++++++++++++++++++++++------------------ 1 file changed, 72 insertions(+), 49 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index b4c9ff4794..2077efbee4 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -383,35 +383,40 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot) aml_append(method, if_ctx); } -static bool is_devfn_ignored(const int devfn, const PCIBus *bus, - bool bus_has_hotplug) +static bool is_devfn_ignored_generic(const int devfn, const PCIBus *bus) { const PCIDevice *pdev = bus->devices[devfn]; - if (pdev) { - if (PCI_FUNC(devfn)) { - if (IS_PCI_BRIDGE(pdev)) { - /* - * Ignore only hotplugged PCI bridges on !0 functions, but - * allow describing cold plugged bridges on all functions - */ - if (DEVICE(pdev)->hotplugged) { - return true; - } - } else if (!get_dev_aml_func(DEVICE(pdev))) { - /* - * Ignore all other devices on !0 functions unless they - * have AML description (i.e have get_dev_aml_func() != 0) - */ + if (PCI_FUNC(devfn)) { + if (IS_PCI_BRIDGE(pdev)) { + /* + * Ignore only hotplugged PCI bridges on !0 functions, but + * allow describing cold plugged bridges on all functions + */ + if (DEVICE(pdev)->hotplugged) { return true; } + } else if (!get_dev_aml_func(DEVICE(pdev))) { + /* + * Ignore all other devices on !0 functions unless they + * have AML description (i.e have get_dev_aml_func() != 0) + */ + return true; } + } + return false; +} + +static bool is_devfn_ignored_hotplug(const int devfn, const PCIBus *bus) +{ + if (bus->devices[devfn]) { + return is_devfn_ignored_generic(devfn, bus); } else { /* non populated slots */ - /* + /* * hotplug is supported only for non-multifunction device * so generate device description only for function 0 */ - if (!bus_has_hotplug || PCI_FUNC(devfn) || + if (PCI_FUNC(devfn) || (pci_bus_is_express(bus) && PCI_SLOT(devfn) > 0)) { return true; } @@ -419,29 +424,23 @@ static bool is_devfn_ignored(const int devfn, const PCIBus *bus, return false; } -void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) +static void build_append_pcihp_slots(Aml *parent_scope, PCIBus *bus, + QObject *bsel) { - Aml *dev, *notify_method = NULL, *method; - QObject *bsel; int devfn; + Aml *dev, *notify_method = NULL, *method; + uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel)); - bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL); - if (bsel) { - uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel)); - - aml_append(parent_scope, aml_name_decl("BSEL", aml_int(bsel_val))); - notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED); - } + aml_append(parent_scope, aml_name_decl("BSEL", aml_int(bsel_val))); + notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED); for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { 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; + int adr = slot << 16 | PCI_FUNC(devfn); bool hotpluggbale_slot = true; - if (is_devfn_ignored(devfn, bus, !!bsel)) { + if (is_devfn_ignored_hotplug(devfn, bus)) { continue; } @@ -452,24 +451,20 @@ void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) */ bool cold_plugged_bridge = IS_PCI_BRIDGE(pdev) && !DEVICE(pdev)->hotplugged; - hotpluggbale_slot = bsel && DEVICE_GET_CLASS(pdev)->hotpluggable && + hotpluggbale_slot = DEVICE_GET_CLASS(pdev)->hotpluggable && !cold_plugged_bridge; + dev = aml_scope("S%.02X", devfn); + } else { + dev = aml_device("S%.02X", devfn); + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); } - /* start to compose PCI device descriptor */ - dev = aml_device("S%.02X", devfn); - aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); - - if (bsel) { - /* - * Can't declare _SUN here for every device as it changes 'slot' - * enumeration order in linux kernel, so use another variable for it - */ - aml_append(dev, aml_name_decl("ASUN", aml_int(slot))); - aml_append(dev, aml_pci_device_dsm()); - } - - call_dev_aml_func(DEVICE(pdev), dev); + /* + * Can't declare _SUN here for every device as it changes 'slot' + * enumeration order in linux kernel, so use another variable for it + */ + aml_append(dev, aml_name_decl("ASUN", aml_int(slot))); + aml_append(dev, aml_pci_device_dsm()); if (hotpluggbale_slot) { aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); @@ -486,9 +481,37 @@ void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) /* device descriptor has been composed, add it into parent context */ aml_append(parent_scope, dev); } + aml_append(parent_scope, notify_method); +} + +void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) +{ + QObject *bsel; + int devfn; + Aml *dev; + + bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL); + + for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { + /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ + int adr = PCI_SLOT(devfn) << 16 | PCI_FUNC(devfn); + + if (!bus->devices[devfn] || is_devfn_ignored_generic(devfn, bus)) { + continue; + } + + /* start to compose PCI device descriptor */ + dev = aml_device("S%.02X", devfn); + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); + + call_dev_aml_func(DEVICE(bus->devices[devfn]), dev); + + /* device descriptor has been composed, add it into parent context */ + aml_append(parent_scope, dev); + } if (bsel) { - aml_append(parent_scope, notify_method); + build_append_pcihp_slots(parent_scope, bus, bsel); } qobject_unref(bsel); From 912a5cf142e41110ec9cf0d0df3f412f24f32f12 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:06 +0100 Subject: [PATCH 44/56] tests: acpi: update expected blobs Expected change for non-populated slots is that thay are moved after non-hotpluggable PCI tree description. And expected change for hotplug capable populated slots is: - ... + Name (BSEL, 0x03) + Scope (S00) + { + Name (ASUN, Zero) + Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method + { + Local0 = Package (0x02) + { + BSEL, + ASUN + } + Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0)) + } [ ... other hotplug depended bits ] + } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-35-imammedo@redhat.com> --- tests/data/acpi/pc/DSDT | Bin 6470 -> 6487 bytes tests/data/acpi/pc/DSDT.acpierst | Bin 6430 -> 6453 bytes tests/data/acpi/pc/DSDT.acpihmat | Bin 7795 -> 7812 bytes tests/data/acpi/pc/DSDT.bridge | Bin 12634 -> 12699 bytes tests/data/acpi/pc/DSDT.cphp | Bin 6934 -> 6951 bytes tests/data/acpi/pc/DSDT.dimmpxm | Bin 8124 -> 8141 bytes tests/data/acpi/pc/DSDT.hpbridge | Bin 6430 -> 6459 bytes tests/data/acpi/pc/DSDT.ipmikcs | Bin 6542 -> 6559 bytes tests/data/acpi/pc/DSDT.memhp | Bin 7829 -> 7846 bytes tests/data/acpi/pc/DSDT.nohpet | Bin 6328 -> 6345 bytes tests/data/acpi/pc/DSDT.numamem | Bin 6476 -> 6493 bytes tests/data/acpi/pc/DSDT.roothp | Bin 9775 -> 9787 bytes tests/data/acpi/q35/DSDT.bridge | Bin 11475 -> 11481 bytes tests/data/acpi/q35/DSDT.multi-bridge | Bin 12375 -> 12423 bytes tests/qtest/bios-tables-test-allowed-diff.h | 14 -------------- 15 files changed, 14 deletions(-) diff --git a/tests/data/acpi/pc/DSDT b/tests/data/acpi/pc/DSDT index c99179b35254725daeebb416400b1b6f9f1d74c4..1bc656f2a4897d2932d593e8768173e0d2597d45 100644 GIT binary patch delta 153 zcmX?Rblr%{CDvjwd^<$VE$S=nO c5&#+i5?}-g@CraQg9NykKq8yJ@CI=M05Z-dWdHyG delta 145 zcmca^bj*m$CDVFY6sgBY7x`GU9s%t#|s diff --git a/tests/data/acpi/pc/DSDT.acpierst b/tests/data/acpi/pc/DSDT.acpierst index b0ae8c2cf52616836dae14c0a971f56fcfa7cdc8..0d4639906ddce689b3dcd9d749c79e3a511d548a 100644 GIT binary patch delta 186 zcmbPdwAF~qCD3oGPVoW`R|1?p4WgU4gAEMgIpQ5%f*7Kk z+@ymIEFe4{pbm!3f4EmL@@oYfLX`jonHVPDm+7WM<%S3U>8j5YPl0<`^96 p$1r)LkQ@(40H_Qkzz7oH6@Zup65wKji$IldF@r=lYw>;I0st<`Evf(j delta 162 zcmdmLG|!03CD2gC##=olR8$2hrARE|vmY}n+7LaLh;g@d>OKjtUM delta 151 zcmZp%{cOYK66_LEEXTmW$gz=YiGaH2)tLBTr+5L!s{ziQ1{_You09OWP5QwG2Jsy6 zjxIqA9FD=EehiZhMdc)-o4jO$4J;rMJU}BLl8hk9&9?(0_B+)Ci4ob bN{evB2OGLDEMWjKj9?675M#5Ea1b{De|;nj diff --git a/tests/data/acpi/pc/DSDT.bridge b/tests/data/acpi/pc/DSDT.bridge index 783a9d7b2964612626268905837d108679603432..4c2d77b8051de2ed21fe43c8283003d8083747f7 100644 GIT binary patch delta 429 zcmcbWG&`BgCDcc4D%mQ|kV{oV+!(>4L<;mH6B5VShAOj~)OosSeO|ky!i?LM~=-#Dwi3Z`GbuReqaH*7ASzQniVWyj8MVG z0Co!l$X1{z%qen6&fpb*gbUmia$pf;2gtF5oC>sfGM9iogbnmUG?WdrZu1%eO^(eE H^s1NuJu71d delta 124 zcmbQ8d@G5|CDp4a&F140H&DuV5fM2#sH=OXHNqTr(jnfhUg~!$@_Wr zSfZP}WG8d+8ExLcy@HX+fpu~upYh~$URmbu2FA&&_)J(qsu?Hm=JjQAIt)~3ym>3{ bf9}oUe6^gLH!5Fd-29X8HpgZ~{VFB^@bxCy diff --git a/tests/data/acpi/pc/DSDT.cphp b/tests/data/acpi/pc/DSDT.cphp index 659ad3d6b9026c090e0d8d8e21ece5df44249ec0..ef487176e117acf8f271d0a54bfad8dfa33ef696 100644 GIT binary patch delta 153 zcmbPcw%m-%CDvjwd^<$WFU40m$oAiSX4B|QB z9bJMLCdcrrNJKYz$p#x(K!kXJni)78gG2ooCqERB+g!)Hf>FSMHP{fU1t`zNFnI&N csC5E4mL1|=ZJT731WzD za+3}=uz>J*fI1j97YVIkvjwd^<$W9C?dxO Z60nG$>?fqcCIHqpSxZE9bCyUDHvq0iBSHWG delta 128 zcmX?WzsH`-CDm_q PNZ;h!!qS_oMS{2i7_T9@ diff --git a/tests/data/acpi/pc/DSDT.hpbridge b/tests/data/acpi/pc/DSDT.hpbridge index b0ae8c2cf52616836dae14c0a971f56fcfa7cdc8..ce2e1430a38b467b212573a896b94c306caa12fb 100644 GIT binary patch delta 211 zcmbPdwA+ZwCDmiY DjxaJ> delta 203 zcmdmOG|!03CD=r+5L+n*q+A1{_You09OWP5QwG2Jsy6 zjxIqA9FD=EehiZz3d#vZH#sQ<8(2Ujcz{MQO#a8KvUwf%2S#xR)?h=Z7N86hL>m_q PNZ;hOywaO}`GU9s3acV^ diff --git a/tests/data/acpi/pc/DSDT.memhp b/tests/data/acpi/pc/DSDT.memhp index b2a7c042a902d1bbac79961639e27d302ad8799f..45b434d485444750cf00ebc1b2658f2fa40f0884 100644 GIT binary patch delta 169 zcmbPgyUdo$CDo#F+Yt_3)I8bmj72OAi~bHqEk1TjQ6 zxk(2bSU`9@KphO5y#-e=nrQ_aLX`jonHWSk;)4xc7?yw;Mlgmkh{53$?CQfHpb0W_ l@<#y`9uN~~pkr{TALHbYB67R}5W_$MTudO5&CbF>+yIwfDo6kT delta 145 zcmZ2xJJpuUCDB^zvD0TJQ>YG&YY3=Z{UocvHoZu4J(6^sH7tigs*EkJoDhROQE cs?s7H@xg{J3`-b53?mrB7{u5dA{@jG06rHa#Q*>R diff --git a/tests/data/acpi/pc/DSDT.nohpet b/tests/data/acpi/pc/DSDT.nohpet index b64da36b14edd13270dfd9db040a3b99219a36a0..dbed1404bb70eebf1c3cf0f882d3b4b7cccd53a8 100644 GIT binary patch delta 153 zcmdmCc+!x|CDo#F+Yt_3)I8bmj72OAi~bHqEk1TjQ6 zxk(2bSU`9@KphO5^SD=ZM2{3{Lcm*JuK>}P%Ad$^!yg}Rm8F(bN delta 152 zcmX?UxWkakCDB^zvD0TJQ>YG&YY3=Z{UocxeiZnFaS3Py1U)?h=Z7N9&6M4pQY cq(EAPBR<&Bg<%N;h+za{7=sv_%Xx#i0ZK(CPXGV_ diff --git a/tests/data/acpi/pc/DSDT.numamem b/tests/data/acpi/pc/DSDT.numamem index f554b0b09db33fa90d65267c2687e90d4ab7d92e..6ee52f1230445c0dff01c77e72a74ca37e5864f1 100644 GIT binary patch delta 140 zcmX?Obk~T>CDbjFCwCDFU40m$oAiSX4B|QB z9bJMLI2?mR{TL=Y3dspYH+jhh8(2Ujcz{MQOy0w*vUv;l3Py1U)?h=Z7N86hL>m_q PNZ;gaUg^!;d_mj*>(n5J diff --git a/tests/data/acpi/pc/DSDT.roothp b/tests/data/acpi/pc/DSDT.roothp index d58f4d2f0adbb86f8f6403a1cf9b13e1cabed035..578de7540f6f09c05ad81f62abd142be8cb288ee 100644 GIT binary patch delta 128 zcmZ4Qv)hNuCD^cS3y&i8 delta 80 zcmV-W0I&bMOs`A|L{mgmFD3v00g8j5O8Jz3poad`Y}v?sGz)AQGOFI0QXoKo&W#< delta 76 zcmcZ^c{!5HCDb1Op<{LrPGhtQFh_2ii zs+<|Cat0*j5pJh~4K3lWVFtS1=P1JY(M@g#k$Ffyi*Di%Hb(fH4eC`^gyFWETNEcV z@(H-IfE79hhx&1`O>R_H*&L#Lhm#i)G%Ny|(2!9P5n*!)^9x~uNH8NLHk<1zFarSE CJzC`e delta 344 zcmZoqyq>`266_KZZot66_;w>#lN=WlV@!OoQ@lV2=QZp?+L!lRqk}Y*tdf!^tkf<`U)?!n8SESAiJ-)N@Yo diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index 1983fa596b..dfb8523c8b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1,15 +1 @@ /* List of comma-separated changed AML files to ignore */ -"tests/data/acpi/pc/DSDT", -"tests/data/acpi/pc/DSDT.acpierst", -"tests/data/acpi/pc/DSDT.acpihmat", -"tests/data/acpi/pc/DSDT.bridge", -"tests/data/acpi/pc/DSDT.cphp", -"tests/data/acpi/pc/DSDT.dimmpxm", -"tests/data/acpi/pc/DSDT.hpbridge", -"tests/data/acpi/pc/DSDT.ipmikcs", -"tests/data/acpi/pc/DSDT.memhp", -"tests/data/acpi/pc/DSDT.nohpet", -"tests/data/acpi/pc/DSDT.numamem", -"tests/data/acpi/pc/DSDT.roothp", -"tests/data/acpi/q35/DSDT.bridge", -"tests/data/acpi/q35/DSDT.multi-bridge", From 9330847e6a3494a242a1180c8552ceab5d22b19d Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:07 +0100 Subject: [PATCH 45/56] tests: acpi: whitelist DSDT blobs before removing dynamic _DSM on coldplugged bridges Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-36-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test-allowed-diff.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..a83322cb08 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,5 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/pc/DSDT.bridge", +"tests/data/acpi/pc/DSDT.roothp", +"tests/data/acpi/pc/DSDT.hpbridge", +"tests/data/acpi/q35/DSDT.multi-bridge", From 64a55106e4b9f5248f096bf158a9242c2b5cc8b9 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:08 +0100 Subject: [PATCH 46/56] pcihp: acpi: ignore coldplugged bridges when composing hotpluggable slots coldplugged bridges are not unpluggable, so there is no need to describe slots where they are plugged as hotpluggable. To that effect we have a condition that marks slot as non-hotpluggable if it's populated by coldplugged bridge and prevents generation _SUN/_EJ0 objects for it. That leaves dynamic _DSM method on such slot (which also depends on BSEL and pcihp hardware). This _DSM method provides only dynamic acpi-index support so far, which is not actually used/supported by linux kernel for bridges and it's doubtful there will be need for it at all. So it's rather pointless to generate acpi-index related AML for bridges and we can simplify hotplug slots generator a bit more by completely ignoring coldplugged bridges on hotplug path. Another point in favor of dropping dynamic _DSM support, is that we can replace it with static _DSM if necessary since a slot with bridge can't change during VM runtime and without any dependency on ACPI PCI hotplug at that. Later I plan to implement bridge specific static _DSM PCI Firmware Specification 3.2 4.6.5. _DSM for Ignoring PCI Boot Configurations part of spec, to fix longstanding issue with fixed IO/MEM resource assignment that often leads to hotplugged device being in-operational within the guest due limited IO/MEM windows programmed on bridge at boot time. Expected change when coldplugged bridge is ignored by hotplug code, should look like: - Scope (S18) - { - Name (ASUN, 0x03) - Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method - { - Local0 = Package (0x02) - { - BSEL, - ASUN - } - Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0)) - } - } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-37-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 2077efbee4..a02608c215 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -409,8 +409,11 @@ static bool is_devfn_ignored_generic(const int devfn, const PCIBus *bus) static bool is_devfn_ignored_hotplug(const int devfn, const PCIBus *bus) { - if (bus->devices[devfn]) { - return is_devfn_ignored_generic(devfn, bus); + PCIDevice *pdev = bus->devices[devfn]; + if (pdev) { + return is_devfn_ignored_generic(devfn, bus) || + /* Cold plugged bridges aren't themselves hot-pluggable */ + (IS_PCI_BRIDGE(pdev) && !DEVICE(pdev)->hotplugged); } else { /* non populated slots */ /* * hotplug is supported only for non-multifunction device @@ -445,14 +448,7 @@ static void build_append_pcihp_slots(Aml *parent_scope, PCIBus *bus, } if (pdev) { - /* - * Cold plugged bridges aren't themselves hot-pluggable. - * Hotplugged bridges *are* hot-pluggable. - */ - bool cold_plugged_bridge = IS_PCI_BRIDGE(pdev) && - !DEVICE(pdev)->hotplugged; - hotpluggbale_slot = DEVICE_GET_CLASS(pdev)->hotpluggable && - !cold_plugged_bridge; + hotpluggbale_slot = DEVICE_GET_CLASS(pdev)->hotpluggable; dev = aml_scope("S%.02X", devfn); } else { dev = aml_device("S%.02X", devfn); From beb680ff28106b157f2d8372b2853753f96e15f5 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:09 +0100 Subject: [PATCH 47/56] tests: acpi: update expected blobs expected change is removal of dynamic _DSM bits from slots populated by coldplugged bridges (something like): - Scope (S18) - { - Name (ASUN, 0x03) - Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method - { - Local0 = Package (0x02) - { - BSEL, - ASUN - } - Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0)) - } - } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-38-imammedo@redhat.com> --- tests/data/acpi/pc/DSDT.bridge | Bin 12699 -> 12614 bytes tests/data/acpi/pc/DSDT.hpbridge | Bin 6459 -> 6416 bytes tests/data/acpi/pc/DSDT.roothp | Bin 9787 -> 9745 bytes tests/data/acpi/q35/DSDT.multi-bridge | Bin 12423 -> 12337 bytes tests/qtest/bios-tables-test-allowed-diff.h | 4 ---- 5 files changed, 4 deletions(-) diff --git a/tests/data/acpi/pc/DSDT.bridge b/tests/data/acpi/pc/DSDT.bridge index 4c2d77b8051de2ed21fe43c8283003d8083747f7..d65d9f053910d4ef8a77fe7f9015768dd48a53f8 100644 GIT binary patch delta 65 zcmbQ8d@PB}CDtZT5319@y_I7a%Gt; VsHQpj0zcd4Dyf6qo72=xxdF4y6yg8? delta 64 zcmV-G0KfmnVw+g7N3-|#7j(@X;4Ves+ Wyd^5L9~$xt0VMlAQdeD diff --git a/tests/data/acpi/pc/DSDT.hpbridge b/tests/data/acpi/pc/DSDT.hpbridge index ce2e1430a38b467b212573a896b94c306caa12fb..c8b388a85c8d7472a5370c9657fa2b4e1a897e38 100644 GIT binary patch delta 40 wcmdmOG{K0=CD=5C;GN diff --git a/tests/data/acpi/pc/DSDT.roothp b/tests/data/acpi/pc/DSDT.roothp index 578de7540f6f09c05ad81f62abd142be8cb288ee..657c8263f0c649abc806a67576fd74cb32af60c3 100644 GIT binary patch delta 58 zcmdn(Gtr03CDq3sF=HcFIX8FrT*jFAV5fM2rn#GYxPLG*wbyKJ<1OWxtRTd? OIg+o6bMq4A=}Z6+RuYx~ delta 66 zcmbQ}v)hNuCDpS6(Bxqf(41VLpdlc_<`U)?!o=Ye?CQhC3=!NcsH?yX0Mzgp3IG5A diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index a83322cb08..dfb8523c8b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1,5 +1 @@ /* List of comma-separated changed AML files to ignore */ -"tests/data/acpi/pc/DSDT.bridge", -"tests/data/acpi/pc/DSDT.roothp", -"tests/data/acpi/pc/DSDT.hpbridge", -"tests/data/acpi/q35/DSDT.multi-bridge", From 85ea72b96b746e2f88918e1b04495c9d329bb5c4 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:10 +0100 Subject: [PATCH 48/56] tests: acpi: whitelist DSDT before moving non-hotpluggble slots description from hotplug path Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-39-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test-allowed-diff.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..0adf61bac3 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,12 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/pc/DSDT", +"tests/data/acpi/pc/DSDT.acpierst", +"tests/data/acpi/pc/DSDT.acpihmat", +"tests/data/acpi/pc/DSDT.bridge", +"tests/data/acpi/pc/DSDT.cphp", +"tests/data/acpi/pc/DSDT.dimmpxm", +"tests/data/acpi/pc/DSDT.hpbridge", +"tests/data/acpi/pc/DSDT.ipmikcs", +"tests/data/acpi/pc/DSDT.memhp", +"tests/data/acpi/pc/DSDT.nohpet", +"tests/data/acpi/pc/DSDT.numamem", From 17f4cedba14f882a4816c5be320ca2192f04e31c Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:11 +0100 Subject: [PATCH 49/56] pcihp: generate populated non-hotpluggble slot descriptions on non-hotplug path Generating slots descriptions populated by non-hotpluggable devices is akward at best and complicates hotplug path (build_append_pcihp_slots) needlessly, and builds only dynamic _DSM for such slots which is overlkill. Clean it up and let non-hotplug path (build_append_pci_bus_devices) to handle that task. Such clean up effectively drops dynamic _DSM methods on non-hotpluggable slots (even though bus itself is hotpluggable), but in practice it affects only built-in devices (ide controllers/various bridges) that don't use acpi-index anyways so effectively it doesn't matter (NICs are hotpluggble). Follow up series will add static _DSM for non-hotpluggble devices/buses that will not depend on ACPI PCI hotplug at all, and potentially would allows us to reuse non-hotplug path elsewhere (PBX/microvm/arm-virt), including new support for acpi-index for non-hotpluggable devices. Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-40-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index a02608c215..145389aa58 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -412,6 +412,7 @@ static bool is_devfn_ignored_hotplug(const int devfn, const PCIBus *bus) PCIDevice *pdev = bus->devices[devfn]; if (pdev) { return is_devfn_ignored_generic(devfn, bus) || + !DEVICE_GET_CLASS(pdev)->hotpluggable || /* Cold plugged bridges aren't themselves hot-pluggable */ (IS_PCI_BRIDGE(pdev) && !DEVICE(pdev)->hotplugged); } else { /* non populated slots */ @@ -438,17 +439,14 @@ static void build_append_pcihp_slots(Aml *parent_scope, PCIBus *bus, notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED); for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { - PCIDevice *pdev = bus->devices[devfn]; int slot = PCI_SLOT(devfn); int adr = slot << 16 | PCI_FUNC(devfn); - bool hotpluggbale_slot = true; if (is_devfn_ignored_hotplug(devfn, bus)) { continue; } - if (pdev) { - hotpluggbale_slot = DEVICE_GET_CLASS(pdev)->hotpluggable; + if (bus->devices[devfn]) { dev = aml_scope("S%.02X", devfn); } else { dev = aml_device("S%.02X", devfn); @@ -462,17 +460,15 @@ static void build_append_pcihp_slots(Aml *parent_scope, PCIBus *bus, aml_append(dev, aml_name_decl("ASUN", aml_int(slot))); aml_append(dev, aml_pci_device_dsm()); - if (hotpluggbale_slot) { - aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); - /* add _EJ0 to make slot hotpluggable */ - method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); - aml_append(method, - aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) - ); - aml_append(dev, method); + aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); + /* add _EJ0 to make slot hotpluggable */ + method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); + aml_append(method, + aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) + ); + aml_append(dev, method); - build_append_pcihp_notify_entry(notify_method, slot); - } + build_append_pcihp_notify_entry(notify_method, slot); /* device descriptor has been composed, add it into parent context */ aml_append(parent_scope, dev); @@ -491,8 +487,9 @@ void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ int adr = PCI_SLOT(devfn) << 16 | PCI_FUNC(devfn); + PCIDevice *pdev = bus->devices[devfn]; - if (!bus->devices[devfn] || is_devfn_ignored_generic(devfn, bus)) { + if (!pdev || is_devfn_ignored_generic(devfn, bus)) { continue; } From 4d6ee555ef649d00805a0730aa2e22fe152e90f0 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 12 Jan 2023 15:03:12 +0100 Subject: [PATCH 50/56] tests: acpi: update expected blobs Expected change removal of dynamic _DSM AML for non-hotpluggable hots-bridge, storage, isa bridge devices from PC machine blobs: - Scope (S00) - { - Name (ASUN, Zero) - Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method - { - Local0 = Package (0x02) - { - BSEL, - ASUN - } - Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0)) - } - } - - Scope (S08) - { - Name (ASUN, One) - Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method - { - Local0 = Package (0x02) - { - BSEL, - ASUN - } - Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0)) - } - } - - Scope (S10) - { - Name (ASUN, 0x02) - Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method - { - Local0 = Package (0x02) - { - BSEL, - ASUN - } - Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0)) - } - } Signed-off-by: Igor Mammedov Message-Id: <20230112140312.3096331-41-imammedo@redhat.com> --- tests/data/acpi/pc/DSDT | Bin 6487 -> 6360 bytes tests/data/acpi/pc/DSDT.acpierst | Bin 6453 -> 6283 bytes tests/data/acpi/pc/DSDT.acpihmat | Bin 7812 -> 7685 bytes tests/data/acpi/pc/DSDT.bridge | Bin 12614 -> 12487 bytes tests/data/acpi/pc/DSDT.cphp | Bin 6951 -> 6824 bytes tests/data/acpi/pc/DSDT.dimmpxm | Bin 8141 -> 8014 bytes tests/data/acpi/pc/DSDT.hpbridge | Bin 6416 -> 6289 bytes tests/data/acpi/pc/DSDT.ipmikcs | Bin 6559 -> 6432 bytes tests/data/acpi/pc/DSDT.memhp | Bin 7846 -> 7719 bytes tests/data/acpi/pc/DSDT.nohpet | Bin 6345 -> 6218 bytes tests/data/acpi/pc/DSDT.numamem | Bin 6493 -> 6366 bytes tests/qtest/bios-tables-test-allowed-diff.h | 11 ----------- 12 files changed, 11 deletions(-) diff --git a/tests/data/acpi/pc/DSDT b/tests/data/acpi/pc/DSDT index 1bc656f2a4897d2932d593e8768173e0d2597d45..0b475fb5a966543fef2cd7672a0b198838a63151 100644 GIT binary patch delta 40 wcmca^bi`|Vl`n`J03N^%qW}N^ delta 92 zcmca%c-@H0CDbUcyECDt1=UQs0S`?J*8l(j delta 71 zcmX?}cr1y_CDenv?HJYVZhX1{+w! bb2tWv`Y}#Ucsu?!`@?jHo diff --git a/tests/data/acpi/pc/DSDT.cphp b/tests/data/acpi/pc/DSDT.cphp index ef487176e117acf8f271d0a54bfad8dfa33ef696..754ab854dc48fc1af2d335e7269c23a056e66eb8 100644 GIT binary patch delta 40 wcmZ2(w!)OlCDW=VIc6o#F+Y&uwND`pvkxS|o@Y036s2$^ZZW delta 92 zcmX?Sch;WECDvQZ05_H!ZvX%Q diff --git a/tests/data/acpi/pc/DSDT.hpbridge b/tests/data/acpi/pc/DSDT.hpbridge index c8b388a85c8d7472a5370c9657fa2b4e1a897e38..834c27002edbd3e2298a71c9ff1b501e3a3314f7 100644 GIT binary patch delta 40 wcmbPWG|`aDCDw>EckJ2UYJXa*Y?#B(?Xhx##0 f78H~N3s^t|7(oKO0$Ra_P!(KEAd$`Ic!RhBSb!F` diff --git a/tests/data/acpi/pc/DSDT.memhp b/tests/data/acpi/pc/DSDT.memhp index 45b434d485444750cf00ebc1b2658f2fa40f0884..2f895e9b385c1ae2f58c7ade4de02328b1be7356 100644 GIT binary patch delta 40 wcmZ2xyWED$CD=VIc6o#F+Y&uzXX@SAaSh;R@$00@>1nE(I) delta 92 zcmZ2(v&@#uCDo#F+Yu5G?0@SBlGKr`6DAfCf9IMk0} g@o#F+Yu5JFy^_!7LKr`6DAfCf9IMk0} gvLLSbkC5>CD`|Vn=gnP02yixaR2}S delta 92 zcmca-c-M%_CD|7PS7& Date: Mon, 23 Jan 2023 20:21:19 +0800 Subject: [PATCH 51/56] vhost-user: Skip unnecessary duplicated VHOST_USER_ADD/REM_MEM_REG requests The VHOST_USER_ADD/REM_MEM_REG requests should be categorized into non-vring specific messages, and should be sent only once. Signed-off-by: Minghao Yuan Message-Id: <20230123122119.194347-1-yuanmh12@chinatelecom.cn> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/vhost-user.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 6c79da953b..eca9e104ba 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -459,6 +459,8 @@ static bool vhost_user_one_time_request(VhostUserRequest request) case VHOST_USER_SET_MEM_TABLE: case VHOST_USER_GET_QUEUE_NUM: case VHOST_USER_NET_SET_MTU: + case VHOST_USER_ADD_MEM_REG: + case VHOST_USER_REM_MEM_REG: return true; default: return false; From c45e7619dbe4033dfd525a417edb828af37a892e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 17 Jan 2023 20:30:14 +0100 Subject: [PATCH 52/56] hw: Use TYPE_PCI_BUS definition where appropriate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use the proper QOM type definition instead of magic string. This also helps during eventual refactor while using git-grep. Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20230117193014.83502-1-philmd@linaro.org> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Eric Auger --- hw/arm/smmu-common.c | 3 ++- hw/virtio/virtio-iommu.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index 54186f31cb..733c964778 100644 --- a/hw/arm/smmu-common.c +++ b/hw/arm/smmu-common.c @@ -535,7 +535,8 @@ static void smmu_base_reset_hold(Object *obj) static Property smmu_dev_properties[] = { DEFINE_PROP_UINT8("bus_num", SMMUState, bus_num, 0), - DEFINE_PROP_LINK("primary-bus", SMMUState, primary_bus, "PCI", PCIBus *), + DEFINE_PROP_LINK("primary-bus", SMMUState, primary_bus, + TYPE_PCI_BUS, PCIBus *), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 23c470977e..1cd258135d 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -1366,7 +1366,8 @@ static const VMStateDescription vmstate_virtio_iommu = { }; static Property virtio_iommu_properties[] = { - DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus, "PCI", PCIBus *), + DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus, + TYPE_PCI_BUS, PCIBus *), DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true), DEFINE_PROP_END_OF_LIST(), }; From 4ffa3a1baa2678bb3c835aebdc3636e4a99c4ddf Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 18 Jan 2023 13:51:32 +0100 Subject: [PATCH 53/56] tests/qtest/bios-tables-test: Make the test less verbose by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We are facing the issues that our test logs in the gitlab CI are too big (and thus cut off). The bios-tables-test is one of the few qtests that prints many lines of output by default when running with V=1, so it contributes to this problem. Almost all other qtests are silent with V=1 and only print debug messages with V=2 and higher. Thus let's change the bios-tables-test to behave more like the other tests and only print the debug messages with V=2 (or higher). Signed-off-by: Thomas Huth Message-Id: <20230118125132.1694469-1-thuth@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Daniel P. Berrangé Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Igor Mammedov --- tests/qtest/bios-tables-test.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 22b22c403d..d8c8cda58e 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -24,7 +24,7 @@ * You will also notice that tests/qtest/bios-tables-test-allowed-diff.h lists * a bunch of files. This is your hint that you need to do the below: * 4. Run - * make check V=1 + * make check V=2 * this will produce a bunch of warnings about differences * beween actual and expected ACPI tables. If you have IASL installed, * they will also be disassembled so you can look at the disassembled @@ -108,6 +108,8 @@ static const char *iasl = CONFIG_IASL; static const char *iasl; #endif +static int verbosity_level; + static bool compare_signature(const AcpiSdtTable *sdt, const char *signature) { return !memcmp(sdt->aml, signature, 4); @@ -368,7 +370,7 @@ static GArray *load_expected_aml(test_data *data) gsize aml_len; GArray *exp_tables = g_array_new(false, true, sizeof(AcpiSdtTable)); - if (getenv("V")) { + if (verbosity_level >= 2) { fputc('\n', stderr); } for (i = 0; i < data->tables->len; ++i) { @@ -383,7 +385,7 @@ static GArray *load_expected_aml(test_data *data) try_again: aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine, sdt->aml, ext); - if (getenv("V")) { + if (verbosity_level >= 2) { fprintf(stderr, "Looking for expected file '%s'\n", aml_file); } if (g_file_test(aml_file, G_FILE_TEST_EXISTS)) { @@ -395,7 +397,7 @@ try_again: goto try_again; } g_assert(exp_sdt.aml_file); - if (getenv("V")) { + if (verbosity_level >= 2) { fprintf(stderr, "Using expected file '%s'\n", aml_file); } ret = g_file_get_contents(aml_file, (gchar **)&exp_sdt.aml, @@ -503,7 +505,7 @@ static void test_acpi_asl(test_data *data) exp_sdt->aml, sdt->asl_file, sdt->aml_file, exp_sdt->asl_file, exp_sdt->aml_file); fflush(stderr); - if (getenv("V")) { + if (verbosity_level >= 1) { const char *diff_env = getenv("DIFF"); const char *diff_cmd = diff_env ? diff_env : "diff -U 16"; char *diff = g_strdup_printf("%s %s %s", diff_cmd, @@ -2042,8 +2044,13 @@ int main(int argc, char *argv[]) const char *arch = qtest_get_arch(); const bool has_kvm = qtest_has_accel("kvm"); const bool has_tcg = qtest_has_accel("tcg"); + char *v_env = getenv("V"); int ret; + if (v_env) { + verbosity_level = atoi(v_env); + } + g_test_init(&argc, &argv, NULL); if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { From f340a59d5a852d75ae34555723694c7e8eafbd0c Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 19 Jan 2023 18:24:23 +0100 Subject: [PATCH 54/56] Revert "vhost-user: Monitor slave channel in vhost_user_read()" This reverts commit db8a3772e300c1a656331a92da0785d81667dc81. Motivation : this is breaking vhost-user with DPDK as reported in [0]. Received unexpected msg type. Expected 22 received 40 Fail to update device iotlb Received unexpected msg type. Expected 40 received 22 Received unexpected msg type. Expected 22 received 11 Fail to update device iotlb Received unexpected msg type. Expected 11 received 22 vhost VQ 1 ring restore failed: -71: Protocol error (71) Received unexpected msg type. Expected 22 received 11 Fail to update device iotlb Received unexpected msg type. Expected 11 received 22 vhost VQ 0 ring restore failed: -71: Protocol error (71) unable to start vhost net: 71: falling back on userspace virtio The failing sequence that leads to the first error is : - QEMU sends a VHOST_USER_GET_STATUS (40) request to DPDK on the master socket - QEMU starts a nested event loop in order to wait for the VHOST_USER_GET_STATUS response and to be able to process messages from the slave channel - DPDK sends a couple of legitimate IOTLB miss messages on the slave channel - QEMU processes each IOTLB request and sends VHOST_USER_IOTLB_MSG (22) updates on the master socket - QEMU assumes to receive a response for the latest VHOST_USER_IOTLB_MSG but it gets the response for the VHOST_USER_GET_STATUS instead The subsequent errors have the same root cause : the nested event loop breaks the order by design. It lures QEMU to expect responses to the latest message sent on the master socket to arrive first. Since this was only needed for DAX enablement which is still not merged upstream, just drop the code for now. A working solution will have to be merged later on. Likely protect the master socket with a mutex and service the slave channel with a separate thread, as discussed with Maxime in the mail thread below. [0] https://lore.kernel.org/qemu-devel/43145ede-89dc-280e-b953-6a2b436de395@redhat.com/ Reported-by: Yanghang Liu Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2155173 Signed-off-by: Greg Kurz Message-Id: <20230119172424.478268-2-groug@kaod.org> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Acked-by: Stefan Hajnoczi Acked-by: Maxime Coquelin --- hw/virtio/vhost-user.c | 35 +++-------------------------------- 1 file changed, 3 insertions(+), 32 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index eca9e104ba..ad8883113b 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -356,35 +356,6 @@ end: return G_SOURCE_REMOVE; } -static gboolean slave_read(QIOChannel *ioc, GIOCondition condition, - gpointer opaque); - -/* - * This updates the read handler to use a new event loop context. - * Event sources are removed from the previous context : this ensures - * that events detected in the previous context are purged. They will - * be re-detected and processed in the new context. - */ -static void slave_update_read_handler(struct vhost_dev *dev, - GMainContext *ctxt) -{ - struct vhost_user *u = dev->opaque; - - if (!u->slave_ioc) { - return; - } - - if (u->slave_src) { - g_source_destroy(u->slave_src); - g_source_unref(u->slave_src); - } - - u->slave_src = qio_channel_add_watch_source(u->slave_ioc, - G_IO_IN | G_IO_HUP, - slave_read, dev, NULL, - ctxt); -} - static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) { struct vhost_user *u = dev->opaque; @@ -406,7 +377,6 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) * be prepared for re-entrancy. So we create a new one and switch chr * to use it. */ - slave_update_read_handler(dev, ctxt); qemu_chr_be_update_read_handlers(chr->chr, ctxt); qemu_chr_fe_add_watch(chr, G_IO_IN | G_IO_HUP, vhost_user_read_cb, &data); @@ -418,7 +388,6 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) * context that have been processed by the nested loop are purged. */ qemu_chr_be_update_read_handlers(chr->chr, prev_ctxt); - slave_update_read_handler(dev, NULL); g_main_loop_unref(loop); g_main_context_unref(ctxt); @@ -1809,7 +1778,9 @@ static int vhost_setup_slave_channel(struct vhost_dev *dev) return -ECONNREFUSED; } u->slave_ioc = ioc; - slave_update_read_handler(dev, NULL); + u->slave_src = qio_channel_add_watch_source(u->slave_ioc, + G_IO_IN | G_IO_HUP, + slave_read, dev, NULL, NULL); if (reply_supported) { msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; From 4382138f642f69fdbc79ebf4e93d84be8061191f Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 19 Jan 2023 18:24:24 +0100 Subject: [PATCH 55/56] Revert "vhost-user: Introduce nested event loop in vhost_user_read()" This reverts commit a7f523c7d114d445c5d83aecdba3efc038e5a692. The nested event loop is broken by design. It's only user was removed. Drop the code as well so that nobody ever tries to use it again. I had to fix a couple of trivial conflicts around return values because of 025faa872bcf ("vhost-user: stick to -errno error return convention"). Signed-off-by: Greg Kurz Message-Id: <20230119172424.478268-3-groug@kaod.org> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Acked-by: Maxime Coquelin --- hw/virtio/vhost-user.c | 65 ++++-------------------------------------- 1 file changed, 5 insertions(+), 60 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index ad8883113b..e68daa35d4 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -305,19 +305,8 @@ static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg) return 0; } -struct vhost_user_read_cb_data { - struct vhost_dev *dev; - VhostUserMsg *msg; - GMainLoop *loop; - int ret; -}; - -static gboolean vhost_user_read_cb(void *do_not_use, GIOCondition condition, - gpointer opaque) +static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) { - struct vhost_user_read_cb_data *data = opaque; - struct vhost_dev *dev = data->dev; - VhostUserMsg *msg = data->msg; struct vhost_user *u = dev->opaque; CharBackend *chr = u->user->chr; uint8_t *p = (uint8_t *) msg; @@ -325,8 +314,7 @@ static gboolean vhost_user_read_cb(void *do_not_use, GIOCondition condition, r = vhost_user_read_header(dev, msg); if (r < 0) { - data->ret = r; - goto end; + return r; } /* validate message size is sane */ @@ -334,8 +322,7 @@ static gboolean vhost_user_read_cb(void *do_not_use, GIOCondition condition, error_report("Failed to read msg header." " Size %d exceeds the maximum %zu.", msg->hdr.size, VHOST_USER_PAYLOAD_SIZE); - data->ret = -EPROTO; - goto end; + return -EPROTO; } if (msg->hdr.size) { @@ -346,53 +333,11 @@ static gboolean vhost_user_read_cb(void *do_not_use, GIOCondition condition, int saved_errno = errno; error_report("Failed to read msg payload." " Read %d instead of %d.", r, msg->hdr.size); - data->ret = r < 0 ? -saved_errno : -EIO; - goto end; + return r < 0 ? -saved_errno : -EIO; } } -end: - g_main_loop_quit(data->loop); - return G_SOURCE_REMOVE; -} - -static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg) -{ - struct vhost_user *u = dev->opaque; - CharBackend *chr = u->user->chr; - GMainContext *prev_ctxt = chr->chr->gcontext; - GMainContext *ctxt = g_main_context_new(); - GMainLoop *loop = g_main_loop_new(ctxt, FALSE); - struct vhost_user_read_cb_data data = { - .dev = dev, - .loop = loop, - .msg = msg, - .ret = 0 - }; - - /* - * We want to be able to monitor the slave channel fd while waiting - * for chr I/O. This requires an event loop, but we can't nest the - * one to which chr is currently attached : its fd handlers might not - * be prepared for re-entrancy. So we create a new one and switch chr - * to use it. - */ - qemu_chr_be_update_read_handlers(chr->chr, ctxt); - qemu_chr_fe_add_watch(chr, G_IO_IN | G_IO_HUP, vhost_user_read_cb, &data); - - g_main_loop_run(loop); - - /* - * Restore the previous event loop context. This also destroys/recreates - * event sources : this guarantees that all pending events in the original - * context that have been processed by the nested loop are purged. - */ - qemu_chr_be_update_read_handlers(chr->chr, prev_ctxt); - - g_main_loop_unref(loop); - g_main_context_unref(ctxt); - - return data.ret; + return 0; } static int process_message_reply(struct vhost_dev *dev, From f5cb612867d3b10b86d6361ba041767e02c1b127 Mon Sep 17 00:00:00 2001 From: Marcel Apfelbaum Date: Mon, 23 Jan 2023 17:42:05 +0000 Subject: [PATCH 56/56] docs/pcie.txt: Replace ioh3420 with pcie-root-port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Do not mention ioh3420 in the "how to" doc. The device still works and can be used by already existing setups, but no need to be mentioned. Suggested-by: Andrew Jones Reviewed-by: Laszlo Ersek Signed-off-by: Marcel Apfelbaum Signed-off-by: Daniel P. Berrangé Message-Id: <20230123174205.683979-1-berrange@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- docs/pcie.txt | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/pcie.txt b/docs/pcie.txt index 89e3502075..df49178311 100644 --- a/docs/pcie.txt +++ b/docs/pcie.txt @@ -48,8 +48,8 @@ Place only the following kinds of devices directly on the Root Complex: strangely when PCI Express devices are integrated with the Root Complex. - (2) PCI Express Root Ports (ioh3420), for starting exclusively PCI Express - hierarchies. + (2) PCI Express Root Ports (pcie-root-port), for starting exclusively + PCI Express hierarchies. (3) PCI Express to PCI Bridge (pcie-pci-bridge), for starting legacy PCI hierarchies. @@ -70,7 +70,7 @@ Place only the following kinds of devices directly on the Root Complex: -device pxb-pcie,id=pcie.1,bus_nr=x[,numa_node=y][,addr=z] PCI Express Root Ports and PCI Express to PCI bridges can be connected to the pcie.1 bus: - -device ioh3420,id=root_port1[,bus=pcie.1][,chassis=x][,slot=y][,addr=z] \ + -device pcie-root-port,id=root_port1[,bus=pcie.1][,chassis=x][,slot=y][,addr=z] \ -device pcie-pci-bridge,id=pcie_pci_bridge1,bus=pcie.1 @@ -112,14 +112,14 @@ Plug only PCI Express devices into PCI Express Ports. ------------ 2.2.1 Plugging a PCI Express device into a PCI Express Root Port: - -device ioh3420,id=root_port1,chassis=x,slot=y[,bus=pcie.0][,addr=z] \ + -device pcie-root-port,id=root_port1,chassis=x,slot=y[,bus=pcie.0][,addr=z] \ -device ,bus=root_port1 2.2.2 Using multi-function PCI Express Root Ports: - -device ioh3420,id=root_port1,multifunction=on,chassis=x,addr=z.0[,slot=y][,bus=pcie.0] \ - -device ioh3420,id=root_port2,chassis=x1,addr=z.1[,slot=y1][,bus=pcie.0] \ - -device ioh3420,id=root_port3,chassis=x2,addr=z.2[,slot=y2][,bus=pcie.0] \ + -device pcie-root-port,id=root_port1,multifunction=on,chassis=x,addr=z.0[,slot=y][,bus=pcie.0] \ + -device pcie-root-port,id=root_port2,chassis=x1,addr=z.1[,slot=y1][,bus=pcie.0] \ + -device pcie-root-port,id=root_port3,chassis=x2,addr=z.2[,slot=y2][,bus=pcie.0] \ 2.2.3 Plugging a PCI Express device into a Switch: - -device ioh3420,id=root_port1,chassis=x,slot=y[,bus=pcie.0][,addr=z] \ + -device pcie-root-port,id=root_port1,chassis=x,slot=y[,bus=pcie.0][,addr=z] \ -device x3130-upstream,id=upstream_port1,bus=root_port1[,addr=x] \ -device xio3130-downstream,id=downstream_port1,bus=upstream_port1,chassis=x1,slot=y1[,addr=z1]] \ -device ,bus=downstream_port1