Message ID | 1449859353-1574-8-git-send-email-ehabkost@redhat.com |
---|---|
State | New |
Headers | show |
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 <ehabkost@redhat.com> > --- > 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
On Tue, Dec 15, 2015 at 02:36:10PM +0200, Marcel Apfelbaum wrote: > 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 was unsure if I should go to the extreme of removing all the guest_info parameters, when doing that. I agree with your suggestions, but v3 will have to wait until I get back from vacations in Jan 18th. At least we will have less conflicts with Igor's AML series. :) Thanks!
On 12/18/2015 08:08 PM, Eduardo Habkost wrote: > On Tue, Dec 15, 2015 at 02:36:10PM +0200, Marcel Apfelbaum wrote: >> 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 was unsure if I should go to the extreme of removing all the > guest_info parameters, when doing that. I agree with your > suggestions, but v3 will have to wait until I get back from > vacations in Jan 18th. At least we will have less conflicts with > Igor's AML series. :) Sure, and have fun in your vacation! :) Marcel > > Thanks! >
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) { 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; 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; 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; 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; 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; 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)
We can use PC_MACHINE(qdev_get_machine())->acpi_guest_info to get guest_info. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/i386/acpi-build.c | 35 +++++++++++++++++++++-------------- hw/i386/acpi-build.h | 2 +- hw/i386/pc.c | 2 +- 3 files changed, 23 insertions(+), 16 deletions(-)