On Wed, Dec 09, 2015 at 08:37:16PM +0100, Igor Mammedov wrote:
> On Tue, 8 Dec 2015 20:44:38 +0200
> Marcel Apfelbaum <marcel.apfelb...@gmail.com> 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 <ehabk...@redhat.com>
> > >>> ---
> > >>> 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
