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

2015-12-11 Thread 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  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 
> > >>> ---
> > >>>   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();
> > >>>
> > >>> -acpi_build(build_state->guest_info, );
> > >>> +acpi_build(_state->pcms->acpi_guest_info, );
> > >>>
> > >>>   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

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

2015-12-09 Thread Igor Mammedov 
On Tue, 8 Dec 2015 20:44:38 +0200
Marcel Apfelbaum  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 
> >>> ---
> >>>   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();
> >>>
> >>> -acpi_build(build_state->guest_info, );
> >>> +acpi_build(_state->pcms->acpi_guest_info, );
> >>>
> >>>   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
> 
> 
> 
> >
>

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

2015-12-08 Thread Eduardo Habkost 
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 
> >---
> >  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();
> >
> >-acpi_build(build_state->guest_info, );
> >+acpi_build(_state->pcms->acpi_guest_info, );
> >
> >  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.

-- 
Eduardo

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

2015-12-08 Thread Marcel Apfelbaum 

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 
---
  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();

-acpi_build(build_state->guest_info, );
+acpi_build(_state->pcms->acpi_guest_info, );

  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

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

2015-12-07 Thread Marcel Apfelbaum 

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 
---
  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();

-acpi_build(build_state->guest_info, );
+acpi_build(_state->pcms->acpi_guest_info, );

  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();
-acpi_build(build_state->guest_info, );
+acpi_build(_state->pcms->acpi_guest_info, );

  /* Now expose it all to Guest */
  build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,

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

2015-12-01 Thread Eduardo Habkost 
PCMachineState will be used in some of the steps of ACPI table
building.

Signed-off-by: Eduardo Habkost 
---
 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();
 
-acpi_build(build_state->guest_info, );
+acpi_build(_state->pcms->acpi_guest_info, );
 
 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();
-acpi_build(build_state->guest_info, );
+acpi_build(_state->pcms->acpi_guest_info, );
 
 /* Now expose it all to Guest */
 build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
-- 
2.1.0