From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47371) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFK9U-0008Uk-Jr for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:47:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFK9Q-0008QU-Ac for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:47:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49687) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFK9Q-0008QQ-2D for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:47:28 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 6FF6264461 for ; Tue, 21 Jun 2016 11:47:27 +0000 (UTC) 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> <20160621132826.64937a7a@igors-macbook-pro.local> From: Marcel Apfelbaum Message-ID: <5769294C.7040704@redhat.com> Date: Tue, 21 Jun 2016 14:47:24 +0300 MIME-Version: 1.0 In-Reply-To: <20160621132826.64937a7a@igors-macbook-pro.local> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Igor Mammedov Cc: qemu-devel@nongnu.org, mst@redhat.com, pbonzini@redhat.com, ehabkost@redhat.com On 06/21/2016 02:28 PM, Igor Mammedov wrote: > 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. > Not in Q35. pcie.0 is not hot-pluggable. Run Q35 without a dmi-pci bridge or pci-bridge and you'll have an empty scope. Thanks, Marcel >> >> >> 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)); >>> >> >