| Message ID | 1449020831-8414-7-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <ehabkost@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!) Thanks, Marcel > > acpi_set_pci_info(); > > acpi_build_tables_init(&tables); > - acpi_build(build_state->guest_info, &tables); > + acpi_build(&build_state->pcms->acpi_guest_info, &tables); > > /* Now expose it all to Guest */ > build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data, >
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 <ehabkost@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.
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 <ehabkost@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. Thanks, Marcel >
On Tue, 8 Dec 2015 20:44:38 +0200 Marcel Apfelbaum <marcel.apfelbaum@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 <ehabkost@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. > > Thanks, > Marcel > > > > > >
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.apfelbaum@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 <ehabkost@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!
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; acpi_set_pci_info(); acpi_build_tables_init(&tables); - acpi_build(build_state->guest_info, &tables); + acpi_build(&build_state->pcms->acpi_guest_info, &tables); /* Now expose it all to Guest */ build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
PCMachineState will be used in some of the steps of ACPI table building. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/i386/acpi-build.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)