Re: [Qemu-devel] [PATCH 06/16] acpi: Save PCMachineState on AcpiBuildState

[Igor Mammedov]

On Tue, 8 Dec 2015 20:44:38 +0200
Marcel Apfelbaum <address@hidden> wrote:

> On 12/08/2015 07:59 PM, Eduardo Habkost wrote:
> > On Mon, Dec 07, 2015 at 05:39:29PM +0200, Marcel Apfelbaum wrote:
> >> On 12/02/2015 03:47 AM, Eduardo Habkost wrote:
> >>> PCMachineState will be used in some of the steps of ACPI table
> >>> building.
> >>>
> >>> Signed-off-by: Eduardo Habkost <address@hidden>
> >>> ---
> >>>   hw/i386/acpi-build.c | 8 ++++----
> >>>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>> index 85a5c53..ca11c88 100644
> >>> --- a/hw/i386/acpi-build.c
> >>> +++ b/hw/i386/acpi-build.c
> >>> @@ -1644,7 +1644,7 @@ struct AcpiBuildState {
> >>>       MemoryRegion *table_mr;
> >>>       /* Is table patched? */
> >>>       uint8_t patched;
> >>> -    PcGuestInfo *guest_info;
> >>> +    PCMachineState *pcms;
> >>>       void *rsdp;
> >>>       MemoryRegion *rsdp_mr;
> >>>       MemoryRegion *linker_mr;
> >>> @@ -1855,7 +1855,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(&build_state->pcms->acpi_guest_info, &tables);
> >>>
> >>>       acpi_ram_update(build_state->table_mr, tables.table_data);
> >>>
> >>> @@ -1916,12 +1916,12 @@ void acpi_setup(PCMachineState *pcms)
> >>>
> >>>       build_state = g_malloc0(sizeof *build_state);
> >>>
> >>> -    build_state->guest_info = guest_info;
> >>> +    build_state->pcms = pcms;
> >>
> >> I am not "sold" on keeping a reference to machine in the
> >> build_state. We can always query current machine using
> >> qdev_machine() or something.
> >>
> >> Keeping the "guest info" made sense since is used especially for
> >> ACPI, however the machine has a wider scope. (And not having to
> >> keep it around is a very good thing!)
> >
> > I wouldn't mind using qdev_get_machine() if preferred by the
> > maintainer of that code, but I like to avoid it when possible. To
> > me, qdev_get_machine() is just a global variable disguised as a
> > harder-to-understand API.
> 
> Really? Hmm, for me is looking like the other way around :)
> I see it as "query the QOM tree", instead of "keep the reference
> around" everywhere. But it may be just a personal preference.

+1,
It's not performance critical path so I'd prefer qdev_get_machine()
instead of keeping extra reference/state.


> 
> Thanks,
> Marcel
> 
> 
> 
> >
>