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

[Eduardo Habkost]

On Wed, Dec 09, 2015 at 08:37:16PM +0100, Igor Mammedov wrote:
> 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.

If both of you think it's better, I will change it in v2. Thanks!

-- 
Eduardo