From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFJrE-00025y-RQ for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:28:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFJr8-0004QG-Qt for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:28:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59217) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFJr8-0004QB-Io for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:28:34 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 100D246213 for ; Tue, 21 Jun 2016 11:28:34 +0000 (UTC) Date: Tue, 21 Jun 2016 13:28:26 +0200 From: Igor Mammedov Message-ID: <20160621132826.64937a7a@igors-macbook-pro.local> In-Reply-To: <57690348.5090808@redhat.com> References: <1464716918-29689-1-git-send-email-marcel@redhat.com> <1464716918-29689-7-git-send-email-marcel@redhat.com> <20160620161136.7ef0bcf3@igors-macbook-pro.local> <57690348.5090808@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: qemu-devel@nongnu.org, mst@redhat.com, pbonzini@redhat.com, ehabkost@redhat.com On Tue, 21 Jun 2016 12:05:12 +0300 Marcel Apfelbaum wrote: > On 06/20/2016 05:11 PM, Igor Mammedov wrote: > > On Tue, 31 May 2016 20:48:37 +0300 > > Marcel Apfelbaum wrote: > > > > subj doesn't match patch, it does opposite i.e. makes sure that > > pcie isn't counted in when building acpi hotplug aml > > > > Maybe I wasn't clear enough. Until now q35 had no acpi hotplug code. > This patch adds acpi hotplug code for all Q35 PCI buses, but no for > pcie buses That means for pci-pci bridge and dmi-pci bridge. > > > Maybe I should rename it to hw/acpi: extend acpi pci hotplug support > for Q35 pci buses. this looks better to me > > >> Emit acpi code for pci hotplug on all PC machines: > >> - if legacy pci hotpug is enabled (pcihp_bridge_en) > >> - if there is at least one legacy pci bridge in current system > >> - for PCI buses only (skip PCIe buses) > >> > >> Signed-off-by: Marcel Apfelbaum > >> --- > >> hw/i386/acpi-build.c | 49 > >> ++++++++++++++++++++++++++++++++----------------- 1 file changed, > >> 32 insertions(+), 17 deletions(-) > >> > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > >> index 8ae3e53..cc51f9e 100644 > >> --- a/hw/i386/acpi-build.c > >> +++ b/hw/i386/acpi-build.c > >> @@ -250,6 +250,11 @@ static void acpi_get_pci_info(PcPciInfo *info) > >> NULL); > >> } > >> > >> +static void acpi_test_pci_bus(PCIBus *bus, void *opaque) > >> +{ > >> + *(bool *)opaque |= !pci_bus_is_express(bus); > >> +} > >> + > >> #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is > >> APM_CNT_IOPORT */ > >> static void acpi_align_size(GArray *blob, unsigned align) > >> @@ -422,7 +427,7 @@ static void *acpi_set_bsel(PCIBus *bus, void > >> *opaque) unsigned *bsel_alloc = opaque; > >> unsigned *bus_bsel; > >> > >> - if (qbus_is_hotpluggable(BUS(bus))) { > >> + if (qbus_is_hotpluggable(BUS(bus)) > >> && !pci_bus_is_express(bus)) { bus_bsel = g_malloc(sizeof > >> *bus_bsel); > >> > >> *bus_bsel = (*bsel_alloc)++; > >> @@ -433,15 +438,12 @@ static void *acpi_set_bsel(PCIBus *bus, void > >> *opaque) return bsel_alloc; > >> } > >> > >> -static void acpi_set_pci_info(void) > >> +static void acpi_set_pci_info(PCIBus *bus) > >> { > >> - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > >> unsigned bsel_alloc = 0; > >> > >> - 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); > >> - } > >> + /* Scan all PCI buses. Set property to enable acpi based > >> hotplug. */ > >> + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, > >> &bsel_alloc); } > >> > >> static void build_append_pcihp_notify_entry(Aml *method, int > >> slot) @@ -503,7 +505,7 @@ static void > >> build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > >> * In this case they aren't themselves hot-pluggable. > >> * Hotplugged bridges *are* hot-pluggable. > >> */ > >> - bridge_in_acpi = pc->is_bridge && pcihp_bridge_en && > >> + bridge_in_acpi = pc->is_bridge && !pc->is_express && > >> pcihp_bridge_en && !DEVICE(pdev)->hotplugged; > >> > >> hotplug_enabled_dev = bsel && dc->hotpluggable > >> && !bridge_in_acpi; @@ -1892,7 +1894,7 @@ static void > >> build_piix4_isa_bridge(Aml *table) aml_append(table, scope); > >> } > >> > >> -static void build_piix4_pci_hotplug(Aml *table) > >> +static void build_acpi_pci_hotplug(Aml *table) > >> { > >> Aml *scope; > >> Aml *field; > >> @@ -1995,6 +1997,7 @@ build_dsdt(GArray *table_data, GArray > >> *linker, uint32_t nr_mem = machine->ram_slots; > >> int root_bus_limit = 0xFF; > >> PCIBus *root_bus = NULL; > >> + bool has_pci_bus = false; > >> int i; > >> > >> dsdt = init_aml_allocator(); > >> @@ -2016,7 +2019,6 @@ build_dsdt(GArray *table_data, GArray > >> *linker, build_piix4_pm(dsdt); > >> build_piix4_isa_bridge(dsdt); > >> build_isa_devices_aml(dsdt); > >> - build_piix4_pci_hotplug(dsdt); > >> build_piix4_pci0_int(dsdt); > >> } else { > >> sb_scope = aml_scope("_SB"); > >> @@ -2040,6 +2042,11 @@ build_dsdt(GArray *table_data, GArray > >> *linker, root_bus = PC_MACHINE(machine)->bus; > >> assert(root_bus); > >> > >> + pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus); > >> + if (pm->pcihp_bridge_en && has_pci_bus) { > > ^^^^^ wouldn't that forbid acpi hotplug on PCI0 is it's > > not set? > > Yes, and this is by design. 'pcihp_bridge_en' flag matches 'acpi > hotplug'. I followed the initialization phase, am I wrong? Can you > have acpi hotplug if the bit is off? my understanding was that pcihp_bridge_en governs only hotplug AML for bridges, while with it disabled hotplug AML would be generated only for PCI0. > > > >> + build_acpi_pci_hotplug(dsdt); > >> + } > >> + > >> build_cpu_hotplug_aml(dsdt); > >> build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base, > >> pm->mem_hp_io_len); > >> @@ -2050,7 +2057,7 @@ build_dsdt(GArray *table_data, GArray > >> *linker, > >> > >> aml_append(scope, aml_method("_L00", 0, > >> AML_NOTSERIALIZED)); > >> > >> - if (misc->is_piix4) { > >> + if (pm->pcihp_bridge_en && has_pci_bus) { > > same question as above > > > >> method = aml_method("_E01", 0, AML_NOTSERIALIZED); > >> aml_append(method, > >> aml_acquire(aml_name("\\_SB.PCI0.BLCK"), > >> 0xFFFF)); @@ -2188,7 +2195,7 @@ build_dsdt(GArray *table_data, > >> GArray *linker, g_ptr_array_free(mem_ranges, true); > >> > >> /* reserve PCIHP resources */ > >> - if (pm->pcihp_io_len) { > >> + if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) { > > same question as above > > > >> dev = aml_device("PHPR"); > >> aml_append(dev, aml_name_decl("_HID", > >> aml_string("PNP0A06"))); aml_append(dev, > >> @@ -2326,10 +2333,14 @@ build_dsdt(GArray *table_data, GArray > >> *linker, > >> > >> { > >> Aml *scope = aml_scope("PCI0"); > >> + bool scope_used = false; > > Could you explain why ^^^ is needed? > > > > Sure. If none ACPI code will be generated by the below 'if' statements > we will have an empty PCI0 scope. I wanted to get rid of it. considering that hotplug AML for PCI0 is always generated, that seems useless. > > > Thanks, > Marcel > > >> > >> - /* Scan all PCI buses. Generate tables to support > >> hotplug. */ > >> - build_append_pci_bus_devices(scope, root_bus, > >> - pm->pcihp_bridge_en); > >> + if (pm->pcihp_bridge_en && has_pci_bus) { > >> + /* Scan all PCI buses. Generate tables to support > >> hotplug. */ > >> + build_append_pci_bus_devices(scope, root_bus, > >> + pm->pcihp_bridge_en); > >> + scope_used = true; > >> + } > >> > >> if (misc->tpm_version != TPM_VERSION_UNSPEC) { > >> dev = aml_device("ISA.TPM"); > >> @@ -2346,8 +2357,12 @@ build_dsdt(GArray *table_data, GArray > >> *linker, /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > >> aml_append(dev, aml_name_decl("_CRS", crs)); > >> aml_append(scope, dev); > >> + scope_used = true; > >> + } > >> + > >> + if (scope_used) { > >> + aml_append(sb_scope, scope); > >> } > >> - aml_append(sb_scope, scope); > >> } > >> aml_append(dsdt, sb_scope); > >> } > >> @@ -2870,7 +2885,7 @@ void acpi_setup(void) > >> > >> build_state = g_malloc0(sizeof *build_state); > >> > >> - acpi_set_pci_info(); > >> + acpi_set_pci_info(pcms->bus); > >> > >> acpi_build_tables_init(&tables); > >> acpi_build(&tables, MACHINE(pcms)); > > >