From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48238) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDpJF-0003g1-9A for qemu-devel@nongnu.org; Fri, 17 Jun 2016 04:39:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDpJ8-0002Yp-AF for qemu-devel@nongnu.org; Fri, 17 Jun 2016 04:39:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57343) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDpJ8-0002Yj-1o for qemu-devel@nongnu.org; Fri, 17 Jun 2016 04:39:18 -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 9214D63E12 for ; Fri, 17 Jun 2016 08:39:17 +0000 (UTC) Date: Fri, 17 Jun 2016 10:39:14 +0200 From: Igor Mammedov Message-ID: <20160617103914.73f10319@nial.brq.redhat.com> In-Reply-To: <1464716918-29689-3-git-send-email-marcel@redhat.com> References: <1464716918-29689-1-git-send-email-marcel@redhat.com> <1464716918-29689-3-git-send-email-marcel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 2/7] hw/acpi: simplify dsdt building code 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, 31 May 2016 20:48:33 +0300 Marcel Apfelbaum wrote: > Get the pci root-bus once and pass it around. > > Signed-off-by: Marcel Apfelbaum > --- > hw/i386/acpi-build.c | 58 +++++++++++++++++++++++----------------------------- > 1 file changed, 26 insertions(+), 32 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 9aef25d..c6f4afe 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -1990,7 +1990,7 @@ build_dsdt(GArray *table_data, GArray *linker, > PCMachineState *pcms = PC_MACHINE(machine); > uint32_t nr_mem = machine->ram_slots; > int root_bus_limit = 0xFF; > - PCIBus *bus = NULL; > + PCIBus *root_bus = NULL; > int i; > > dsdt = init_aml_allocator(); > @@ -2033,6 +2033,9 @@ build_dsdt(GArray *table_data, GArray *linker, > build_q35_pci0_int(dsdt); > } > > + root_bus = PC_MACHINE(machine)->bus; > + assert(root_bus); nit: I'd put it in place of removed bus = PC_MACHINE(machine)->bus; in the next hunk > + > build_cpu_hotplug_aml(dsdt); > build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base, > pm->mem_hp_io_len); > @@ -2077,8 +2080,8 @@ build_dsdt(GArray *table_data, GArray *linker, > } > aml_append(dsdt, scope); > > - bus = PC_MACHINE(machine)->bus; > - if (bus) { > + { > + PCIBus *bus = root_bus; > QLIST_FOREACH(bus, &bus->child, sibling) { > uint8_t bus_num = pci_bus_num(bus); > uint8_t numa_node = pci_bus_numa_node(bus); > @@ -2318,38 +2321,29 @@ build_dsdt(GArray *table_data, GArray *linker, > pm->mem_hp_io_len); > > { > - Object *pci_host; > - PCIBus *bus = NULL; > + Aml *scope = aml_scope("PCI0"); > > - pci_host = acpi_get_i386_pci_host(); perhaps it's worth to replace acpi_get_i386_pci_host() usage in other places with PC_MACHINE(machine)->bus as well and remove acpi_get_i386_pci_host() > - if (pci_host) { > - bus = PCI_HOST_BRIDGE(pci_host)->bus; > - } > - > - if (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); > - > - if (misc->tpm_version != TPM_VERSION_UNSPEC) { > - dev = aml_device("ISA.TPM"); > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); > - aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > - crs = aml_resource_template(); > - aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, > - TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); > - /* > - FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, > - Rewrite to take IRQ from TPM device model and > - fix default IRQ value there to use some unused IRQ > - */ > - /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > - aml_append(dev, aml_name_decl("_CRS", crs)); > - aml_append(scope, dev); > - } > + /* Scan all PCI buses. Generate tables to support hotplug. */ > + build_append_pci_bus_devices(scope, root_bus, > + pm->pcihp_bridge_en); no need for line break here, it all fits in 80 column limit > > - aml_append(sb_scope, scope); > + if (misc->tpm_version != TPM_VERSION_UNSPEC) { > + dev = aml_device("ISA.TPM"); > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); > + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > + crs = aml_resource_template(); > + aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, > + TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); > + /* > + FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, > + Rewrite to take IRQ from TPM device model and > + fix default IRQ value there to use some unused IRQ > + */ > + /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > + aml_append(dev, aml_name_decl("_CRS", crs)); > + aml_append(scope, dev); > } > + aml_append(sb_scope, scope); > } > aml_append(dsdt, sb_scope); > } Otherwise looks good, Reviewed-by: Igor Mammedov