From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60591) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8oq2-0006lw-Mi for qemu-devel@nongnu.org; Tue, 15 Dec 2015 07:36:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8opy-0002VW-Hj for qemu-devel@nongnu.org; Tue, 15 Dec 2015 07:36:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58988) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8opy-0002VQ-7M for qemu-devel@nongnu.org; Tue, 15 Dec 2015 07:36:14 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id A81C814CAA5 for ; Tue, 15 Dec 2015 12:36:13 +0000 (UTC) References: <1449859353-1574-1-git-send-email-ehabkost@redhat.com> <1449859353-1574-8-git-send-email-ehabkost@redhat.com> From: Marcel Apfelbaum Message-ID: <5670093A.1010600@redhat.com> Date: Tue, 15 Dec 2015 14:36:10 +0200 MIME-Version: 1.0 In-Reply-To: <1449859353-1574-8-git-send-email-ehabkost@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 07/14] acpi: Remove guest_info parameters from functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost , qemu-devel@nongnu.org Cc: Igor Mammedov , Marcel Apfelbaum , "Michael S. Tsirkin" On 12/11/2015 08:42 PM, Eduardo Habkost wrote: > We can use PC_MACHINE(qdev_get_machine())->acpi_guest_info to get > guest_info. Hi Eduardo, I like the idea of using qdev_get_machine() to get the machine instead of keeping the reference around, however only in places we cannot get a reference to it otherwise. I'll follow inline. > > Signed-off-by: Eduardo Habkost > --- > hw/i386/acpi-build.c | 35 +++++++++++++++++++++-------------- > hw/i386/acpi-build.h | 2 +- > hw/i386/pc.c | 2 +- > 3 files changed, 23 insertions(+), 16 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index bca3f06..92d25c9 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -295,7 +295,7 @@ static void acpi_align_size(GArray *blob, unsigned align) > > /* FACS */ > static void > -build_facs(GArray *table_data, GArray *linker, PcGuestInfo *guest_info) > +build_facs(GArray *table_data, GArray *linker) this function does not need guest_info, no problem here. > { > AcpiFacsDescriptorRev1 *facs = acpi_data_push(table_data, sizeof *facs); > memcpy(&facs->signature, "FACS", 4); > @@ -365,9 +365,10 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm, > } > > static void > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > - PcGuestInfo *guest_info) > +build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu) > { > + PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); > + PcGuestInfo *guest_info = &pcms->acpi_guest_info; This function can get a reference of the machine from the caller, acpi_build, which already queries for it. I am not talking about "speed" here, simply about keeping the code clean, which is exactly what this series does. > int madt_start = table_data->len; > > AcpiMultipleApicTable *madt; > @@ -928,9 +929,11 @@ static Aml *build_crs(PCIHostState *host, > static void > build_ssdt(GArray *table_data, GArray *linker, > AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, > - PcPciInfo *pci, PcGuestInfo *guest_info) > + PcPciInfo *pci) > { > MachineState *machine = MACHINE(qdev_get_machine()); > + PCMachineState *pcms = PC_MACHINE(machine); > + PcGuestInfo *guest_info = &pcms->acpi_guest_info; Same here. If you pass the machine you don't need another query here. I see it was here before, but we can get rid of it. > uint32_t nr_mem = machine->ram_slots; > unsigned acpi_cpus = guest_info->apic_id_limit; > Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx; > @@ -1453,7 +1456,7 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base, > } > > static void > -build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info) > +build_srat(GArray *table_data, GArray *linker) > { > AcpiSystemResourceAffinityTable *srat; > AcpiSratProcessorAffinity *core; > @@ -1464,6 +1467,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info) > int srat_start, numa_start, slots; > uint64_t mem_len, mem_base, next_base; > PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); > + PcGuestInfo *guest_info = &pcms->acpi_guest_info; Same here > ram_addr_t hotplugabble_address_space_size = > object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE, > NULL); > @@ -1683,8 +1687,10 @@ static bool acpi_has_iommu(void) > } > > static > -void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) > +void acpi_build(AcpiBuildTables *tables) > { > + PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); > + PcGuestInfo *guest_info = &pcms->acpi_guest_info; Here it makes sense to query for the machine because it is called from acpi_build_update which is a callback with no machine reference. However the other acpi methods can use this reference. Of course you still get rid of PcGuestInfo. > GArray *table_offsets; > unsigned facs, ssdt, dsdt, rsdt; > AcpiCpuInfo cpu; > @@ -1716,7 +1722,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) > * requirements. > */ > facs = tables_blob->len; > - build_facs(tables_blob, tables->linker, guest_info); > + build_facs(tables_blob, tables->linker); > > /* DSDT is pointed to by FADT */ > dsdt = tables_blob->len; > @@ -1733,12 +1739,11 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) > > ssdt = tables_blob->len; > acpi_add_table(table_offsets, tables_blob); > - build_ssdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci, > - guest_info); > + build_ssdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci); > aml_len += tables_blob->len - ssdt; > > acpi_add_table(table_offsets, tables_blob); > - build_madt(tables_blob, tables->linker, &cpu, guest_info); > + build_madt(tables_blob, tables->linker, &cpu); > > if (misc.has_hpet) { > acpi_add_table(table_offsets, tables_blob); > @@ -1755,7 +1760,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) > } > if (guest_info->numa_nodes) { > acpi_add_table(table_offsets, tables_blob); > - build_srat(tables_blob, tables->linker, guest_info); > + build_srat(tables_blob, tables->linker); > } > if (acpi_get_mcfg(&mcfg)) { > acpi_add_table(table_offsets, tables_blob); > @@ -1855,7 +1860,7 @@ static void acpi_build_update(void *build_opaque, uint32_t offset) > > acpi_build_tables_init(&tables); > > - acpi_build(build_state->guest_info, &tables); > + acpi_build(&tables); > > acpi_ram_update(build_state->table_mr, tables.table_data); > > @@ -1893,8 +1898,10 @@ static const VMStateDescription vmstate_acpi_build = { > }, > }; > > -void acpi_setup(PcGuestInfo *guest_info) > +void acpi_setup(void) > { > + PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); > + PcGuestInfo *guest_info = &pcms->acpi_guest_info; acpi_setup is called from pc_guest_info_machine_done that after one of you previous patches (4/14) has a reference to machine. > AcpiBuildTables tables; > AcpiBuildState *build_state; > > @@ -1920,7 +1927,7 @@ void acpi_setup(PcGuestInfo *guest_info) > acpi_set_pci_info(); > > acpi_build_tables_init(&tables); > - acpi_build(build_state->guest_info, &tables); > + acpi_build(&tables); > > /* Now expose it all to Guest */ > build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data, > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h > index e57b1aa..148c0f9 100644 > --- a/hw/i386/acpi-build.h > +++ b/hw/i386/acpi-build.h > @@ -4,6 +4,6 @@ > > #include "qemu/typedefs.h" > > -void acpi_setup(PcGuestInfo *); > +void acpi_setup(void); > > #endif > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index aa12814..14c0116 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1179,7 +1179,7 @@ void pc_machine_done(Notifier *notifier, void *data) > } > } > > - acpi_setup(&pcms->acpi_guest_info); > + acpi_setup(); > } > > PcGuestInfo *pc_guest_info_init(PCMachineState *pcms) > My point is, keeping a reference to machine when it can be queried is not preferred, querying for a pc machine when the caller has already a reference to it is also not preferred. All this of course IMHO. Thanks, Marcel