[Qemu-devel] [PATCH 00/16] pc: Eliminate struct PcGuestInfo

[Qemu-devel] [PATCH 00/16] pc: Eliminate struct PcGuestInfo

[Eduardo Habkost]

This moves all data from PcGuestInfo to either PCMachineState or
PCMachineClass.

This series depends on other two series:
* [PATCH v3 0/6] pc: Initialization and compat function cleanup
* [PATCH V3 0/3]  hw/pcie: Multi-root support for Q35

For reference, there's a git tree containing this series plus all
the dependencies, at:
  git://github.com/ehabkost/qemu-hacks.git work/pcguestinfo-eliminate

Eduardo Habkost (16):
  pc: Move PcGuestInfo declaration to top of file
  pc: Eliminate struct PcGuestInfoState
  pc: Remove guest_info parameter from pc_memory_init()
  acpi: Make acpi_setup() get PCMachineState as argument
  acpi: Remove unused build_facs() PcGuestInfo paramter
  acpi: Save PCMachineState on AcpiBuildState
  acpi: Make acpi_build() get PCMachineState as argument
  acpi: Make build_srat() get PCMachineState as argument
  acpi: Remove ram size fields fron PcGuestInfo
  pc: Move PcGuestInfo.fw_cfg field to PCMachineState
  pc: Simplify signature of xen_load_linux()
  pc: Remove PcGuestInfo.isapc_ram_fw field
  q35: Remove MCHPCIState.guest_info field
  acpi: Use PCMachineClass fields directly
  pc: Move PcGuestInfo.apic_xrupt_override field to PCMachineState
  pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState

 hw/i386/acpi-build.c      | 75 ++++++++++++++++++++++++-----------------------
 hw/i386/acpi-build.h      |  2 +-
 hw/i386/pc.c              | 71 ++++++++++++++++++--------------------------
 hw/i386/pc_piix.c         | 14 ++-------
 hw/i386/pc_q35.c          | 15 ++--------
 include/hw/i386/pc.h      | 30 +++++++------------
 include/hw/pci-host/q35.h |  1 -
 7 files changed, 82 insertions(+), 126 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 01/16] pc: Move PcGuestInfo declaration to top of file

[Eduardo Habkost]

The struct will be used inside PCMachineState.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/i386/pc.h | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9811229..a74bded 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -20,6 +20,22 @@
 
 #define HPET_INTCAP "hpet-intcap"
 
+/* Machine info for ACPI build: */
+struct PcGuestInfo {
+    bool isapc_ram_fw;
+    hwaddr ram_size, ram_size_below_4g;
+    unsigned apic_id_limit;
+    bool apic_xrupt_override;
+    uint64_t numa_nodes;
+    uint64_t *node_mem;
+    uint64_t *node_cpu;
+    FWCfgState *fw_cfg;
+    int legacy_acpi_table_size;
+    bool has_acpi_build;
+    bool has_reserved_memory;
+    bool rsdp_in_ram;
+};
+
 /**
  * PCMachineState:
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
@@ -109,21 +125,6 @@ typedef struct PcPciInfo {
 #define ACPI_PM_PROP_GPE0_BLK_LEN "gpe0_blk_len"
 #define ACPI_PM_PROP_TCO_ENABLED "enable_tco"
 
-struct PcGuestInfo {
-    bool isapc_ram_fw;
-    hwaddr ram_size, ram_size_below_4g;
-    unsigned apic_id_limit;
-    bool apic_xrupt_override;
-    uint64_t numa_nodes;
-    uint64_t *node_mem;
-    uint64_t *node_cpu;
-    FWCfgState *fw_cfg;
-    int legacy_acpi_table_size;
-    bool has_acpi_build;
-    bool has_reserved_memory;
-    bool rsdp_in_ram;
-};
-
 /* parallel.c */
 
 void parallel_hds_isa_init(ISABus *bus, int n);
-- 
2.1.0

[Qemu-devel] [PATCH 02/16] pc: Eliminate struct PcGuestInfoState

[Eduardo Habkost]

Instead of allocating a new struct just for PcGuestInfo and the
mchine_done Notifier, place them inside PCMachineState.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c         | 27 ++++++++++-----------------
 include/hw/i386/pc.h |  2 ++
 2 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f32000a..30cdfaf 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1155,18 +1155,12 @@ typedef struct PcRomPciInfo {
     uint64_t w64_max;
 } PcRomPciInfo;
 
-typedef struct PcGuestInfoState {
-    PcGuestInfo info;
-    Notifier machine_done;
-} PcGuestInfoState;
-
 static
-void pc_guest_info_machine_done(Notifier *notifier, void *data)
+void pc_machine_done(Notifier *notifier, void *data)
 {
-    PcGuestInfoState *guest_info_state = container_of(notifier,
-                                                      PcGuestInfoState,
-                                                      machine_done);
-    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
+    PCMachineState *pcms = container_of(notifier,
+                                        PCMachineState, machine_done);
+    PCIBus *bus = pcms->bus;
 
     if (bus) {
         int extra_hosts = 0;
@@ -1177,21 +1171,20 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
                 extra_hosts++;
             }
         }
-        if (extra_hosts && guest_info_state->info.fw_cfg) {
+        if (extra_hosts && pcms->acpi_guest_info.fw_cfg) {
             uint64_t *val = g_malloc(sizeof(*val));
             *val = cpu_to_le64(extra_hosts);
-            fw_cfg_add_file(guest_info_state->info.fw_cfg,
+            fw_cfg_add_file(pcms->acpi_guest_info.fw_cfg,
                     "etc/extra-pci-roots", val, sizeof(*val));
         }
     }
 
-    acpi_setup(&guest_info_state->info);
+    acpi_setup(&pcms->acpi_guest_info);
 }
 
 PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
 {
-    PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
-    PcGuestInfo *guest_info = &guest_info_state->info;
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int i, j;
 
     guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
@@ -1219,8 +1212,8 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
         }
     }
 
-    guest_info_state->machine_done.notify = pc_guest_info_machine_done;
-    qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
+    pcms->machine_done.notify = pc_machine_done;
+    qemu_add_machine_init_done_notifier(&pcms->machine_done);
     return guest_info;
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index a74bded..61aa6ee 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -55,6 +55,8 @@ struct PCMachineState {
     OnOffAuto smm;
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
     PCIBus *bus;
+    PcGuestInfo acpi_guest_info;
+    Notifier machine_done;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
-- 
2.1.0

Re: [Qemu-devel] [PATCH 02/16] pc: Eliminate struct PcGuestInfoState

[Marcel Apfelbaum]

On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
> Instead of allocating a new struct just for PcGuestInfo and the
> mchine_done Notifier, place them inside PCMachineState.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/pc.c         | 27 ++++++++++-----------------
>   include/hw/i386/pc.h |  2 ++
>   2 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f32000a..30cdfaf 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1155,18 +1155,12 @@ typedef struct PcRomPciInfo {
>       uint64_t w64_max;
>   } PcRomPciInfo;
>
> -typedef struct PcGuestInfoState {
> -    PcGuestInfo info;
> -    Notifier machine_done;
> -} PcGuestInfoState;
> -
>   static
> -void pc_guest_info_machine_done(Notifier *notifier, void *data)
> +void pc_machine_done(Notifier *notifier, void *data)
>   {
> -    PcGuestInfoState *guest_info_state = container_of(notifier,
> -                                                      PcGuestInfoState,
> -                                                      machine_done);
> -    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
> +    PCMachineState *pcms = container_of(notifier,
> +                                        PCMachineState, machine_done);
> +    PCIBus *bus = pcms->bus;
>
>       if (bus) {
>           int extra_hosts = 0;
> @@ -1177,21 +1171,20 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
>                   extra_hosts++;
>               }
>           }
> -        if (extra_hosts && guest_info_state->info.fw_cfg) {
> +        if (extra_hosts && pcms->acpi_guest_info.fw_cfg) {
>               uint64_t *val = g_malloc(sizeof(*val));
>               *val = cpu_to_le64(extra_hosts);
> -            fw_cfg_add_file(guest_info_state->info.fw_cfg,
> +            fw_cfg_add_file(pcms->acpi_guest_info.fw_cfg,
>                       "etc/extra-pci-roots", val, sizeof(*val));
>           }
>       }
>
> -    acpi_setup(&guest_info_state->info);
> +    acpi_setup(&pcms->acpi_guest_info);
>   }
>
>   PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>   {
> -    PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> -    PcGuestInfo *guest_info = &guest_info_state->info;
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       int i, j;
>
>       guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
> @@ -1219,8 +1212,8 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>           }
>       }
>
> -    guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> -    qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> +    pcms->machine_done.notify = pc_machine_done;
> +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>       return guest_info;
>   }
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index a74bded..61aa6ee 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -55,6 +55,8 @@ struct PCMachineState {
>       OnOffAuto smm;
>       ram_addr_t below_4g_mem_size, above_4g_mem_size;
>       PCIBus *bus;
> +    PcGuestInfo acpi_guest_info;
> +    Notifier machine_done;

Hi,

Arm's virt machine also has a  machine_done notifier
and a PcGuestInfo like structure (for building the ACPI).

Maybe it worth checking moving machine_done up to Machine
and investigate also the same possibility for a common
acpi guest info ancestor. For the moment all they have in
common is fw_config.

If anybody is looking for an adventure we can make a common
base class for ARM virt and PC machine, something like
ACPI_MACHINE. :)

Just a thought anyway,
Thanks,
Marcel

>   };
>
>   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
>

[Qemu-devel] [PATCH 03/16] pc: Remove guest_info parameter from pc_memory_init()

[Eduardo Habkost]

We can get the PcGuestInfo struct directly from PCMachineState.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c         | 4 ++--
 hw/i386/pc_piix.c    | 2 +-
 hw/i386/pc_q35.c     | 2 +-
 include/hw/i386/pc.h | 3 +--
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 30cdfaf..a17e5b3e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1279,9 +1279,9 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
 FWCfgState *pc_memory_init(PCMachineState *pcms,
                            MemoryRegion *system_memory,
                            MemoryRegion *rom_memory,
-                           MemoryRegion **ram_memory,
-                           PcGuestInfo *guest_info)
+                           MemoryRegion **ram_memory)
 {
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int linux_boot, i;
     MemoryRegion *ram, *option_rom_mr;
     MemoryRegion *ram_below_4g, *ram_above_4g;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9718d7b..f7bc1c0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -161,7 +161,7 @@ static void pc_init1(MachineState *machine,
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
         pc_memory_init(pcms, system_memory,
-                       rom_memory, &ram_memory, guest_info);
+                       rom_memory, &ram_memory);
     } else if (machine->kernel_filename != NULL) {
         /* For xen HVM direct kernel boot, load linux here */
         xen_load_linux(pcms, guest_info);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 9da751b..7563bca 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -153,7 +153,7 @@ static void pc_q35_init(MachineState *machine)
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
         pc_memory_init(pcms, get_system_memory(),
-                       rom_memory, &ram_memory, guest_info);
+                       rom_memory, &ram_memory);
     }
 
     /* irq lines */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 61aa6ee..1be2641 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -211,8 +211,7 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
 FWCfgState *pc_memory_init(PCMachineState *pcms,
                            MemoryRegion *system_memory,
                            MemoryRegion *rom_memory,
-                           MemoryRegion **ram_memory,
-                           PcGuestInfo *guest_info);
+                           MemoryRegion **ram_memory);
 qemu_irq pc_allocate_cpu_irq(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
-- 
2.1.0

[Qemu-devel] [PATCH 04/16] acpi: Make acpi_setup() get PCMachineState as argument

[Eduardo Habkost]

Lots of PcGuestInfo fields are duplicates of PCMachineClass or
PCMachineState fields. Pass PCMachineState as argument to
acpi_setup(), so we can simply let the ACPI code use those fields
directly.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 3 ++-
 hw/i386/acpi-build.h | 2 +-
 hw/i386/pc.c         | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index bca3f06..74f0922 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1893,8 +1893,9 @@ static const VMStateDescription vmstate_acpi_build = {
     },
 };
 
-void acpi_setup(PcGuestInfo *guest_info)
+void acpi_setup(PCMachineState *pcms)
 {
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
 
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index e57b1aa..132aba2 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -4,6 +4,6 @@
 
 #include "qemu/typedefs.h"
 
-void acpi_setup(PcGuestInfo *);
+void acpi_setup(PCMachineState *pcms);
 
 #endif
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a17e5b3e..fc98a20 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1179,7 +1179,7 @@ void pc_machine_done(Notifier *notifier, void *data)
         }
     }
 
-    acpi_setup(&pcms->acpi_guest_info);
+    acpi_setup(pcms);
 }
 
 PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
-- 
2.1.0

Re: [Qemu-devel] [PATCH 04/16] acpi: Make acpi_setup() get PCMachineState as argument

[Marcel Apfelbaum]

On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
> Lots of PcGuestInfo fields are duplicates of PCMachineClass or
> PCMachineState fields. Pass PCMachineState as argument to
> acpi_setup(), so we can simply let the ACPI code use those fields
> directly.

I completely agree with removing duplicated fields and using PCMachine
fields directly, but this not what this patch does.
It only extracts PcGuestInfo info from the machine.

Thanks,
Marcel


>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/acpi-build.c | 3 ++-
>   hw/i386/acpi-build.h | 2 +-
>   hw/i386/pc.c         | 2 +-
>   3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index bca3f06..74f0922 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1893,8 +1893,9 @@ static const VMStateDescription vmstate_acpi_build = {
>       },
>   };
>
> -void acpi_setup(PcGuestInfo *guest_info)
> +void acpi_setup(PCMachineState *pcms)
>   {
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       AcpiBuildTables tables;
>       AcpiBuildState *build_state;
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index e57b1aa..132aba2 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -4,6 +4,6 @@
>
>   #include "qemu/typedefs.h"
>
> -void acpi_setup(PcGuestInfo *);
> +void acpi_setup(PCMachineState *pcms);
>
>   #endif
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a17e5b3e..fc98a20 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1179,7 +1179,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>           }
>       }
>
> -    acpi_setup(&pcms->acpi_guest_info);
> +    acpi_setup(pcms);
>   }
>
>   PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>

Re: [Qemu-devel] [PATCH 04/16] acpi: Make acpi_setup() get PCMachineState as argument

[Eduardo Habkost]

On Mon, Dec 07, 2015 at 05:24:27PM +0200, Marcel Apfelbaum wrote:
> On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
> >Lots of PcGuestInfo fields are duplicates of PCMachineClass or
> >PCMachineState fields. Pass PCMachineState as argument to
> >acpi_setup(), so we can simply let the ACPI code use those fields
> >directly.
> 
> I completely agree with removing duplicated fields and using PCMachine
> fields directly, but this not what this patch does.
> It only extracts PcGuestInfo info from the machine.

I should have appended "later" to the commit message. "So we are
able to simply let the ACPI code use those fields later (in
another commit)". The goal of this commit is to just change the
function signature to allow us to move the fields later. Maybe I
will squash some of those changes together in a new version of
the series.

-- 
Eduardo

[Qemu-devel] [PATCH 05/16] acpi: Remove unused build_facs() PcGuestInfo paramter

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 74f0922..85a5c53 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -295,7 +295,7 @@ static void acpi_align_size(GArray *blob, unsigned align)
 
 /* FACS */
 static void
-build_facs(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
+build_facs(GArray *table_data, GArray *linker)
 {
     AcpiFacsDescriptorRev1 *facs = acpi_data_push(table_data, sizeof *facs);
     memcpy(&facs->signature, "FACS", 4);
@@ -1716,7 +1716,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
      * requirements.
      */
     facs = tables_blob->len;
-    build_facs(tables_blob, tables->linker, guest_info);
+    build_facs(tables_blob, tables->linker);
 
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 05/16] acpi: Remove unused build_facs() PcGuestInfo paramter

[Marcel Apfelbaum]

On 12/02/2015 03:47 AM, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/acpi-build.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 74f0922..85a5c53 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -295,7 +295,7 @@ static void acpi_align_size(GArray *blob, unsigned align)
>
>   /* FACS */
>   static void
> -build_facs(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
> +build_facs(GArray *table_data, GArray *linker)
>   {
>       AcpiFacsDescriptorRev1 *facs = acpi_data_push(table_data, sizeof *facs);
>       memcpy(&facs->signature, "FACS", 4);
> @@ -1716,7 +1716,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>        * requirements.
>        */
>       facs = tables_blob->len;
> -    build_facs(tables_blob, tables->linker, guest_info);
> +    build_facs(tables_blob, tables->linker);
>
>       /* DSDT is pointed to by FADT */
>       dsdt = tables_blob->len;
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

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

[Eduardo Habkost]

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;
 
     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,
-- 
2.1.0

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

[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 <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,
>

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

[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 <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.

-- 
Eduardo

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

[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 <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



>

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

[Igor Mammedov]

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
> 
> 
> 
> >
> 

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 <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!

-- 
Eduardo

[Qemu-devel] [PATCH 07/16] acpi: Make acpi_build() get PCMachineState as argument

[Eduardo Habkost]

Some PCMachineState and PCMachineClass fields will be used by
acpi_build().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ca11c88..b1548e7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1683,8 +1683,9 @@ static bool acpi_has_iommu(void)
 }
 
 static
-void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
+void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
 {
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     GArray *table_offsets;
     unsigned facs, ssdt, dsdt, rsdt;
     AcpiCpuInfo cpu;
@@ -1855,7 +1856,7 @@ static void acpi_build_update(void *build_opaque, uint32_t offset)
 
     acpi_build_tables_init(&tables);
 
-    acpi_build(&build_state->pcms->acpi_guest_info, &tables);
+    acpi_build(build_state->pcms, &tables);
 
     acpi_ram_update(build_state->table_mr, tables.table_data);
 
@@ -1921,7 +1922,7 @@ void acpi_setup(PCMachineState *pcms)
     acpi_set_pci_info();
 
     acpi_build_tables_init(&tables);
-    acpi_build(&build_state->pcms->acpi_guest_info, &tables);
+    acpi_build(build_state->pcms, &tables);
 
     /* Now expose it all to Guest */
     build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
-- 
2.1.0

[Qemu-devel] [PATCH 08/16] acpi: Make build_srat() get PCMachineState as argument

[Eduardo Habkost]

Some PCMachineState and PCMachineClass fields will be used by
build_srat().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b1548e7..8841798 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1453,8 +1453,9 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 }
 
 static void
-build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
+build_srat(GArray *table_data, GArray *linker, PCMachineState *pcms)
 {
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     AcpiSystemResourceAffinityTable *srat;
     AcpiSratProcessorAffinity *core;
     AcpiSratMemoryAffinity *numamem;
@@ -1463,7 +1464,6 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
     uint64_t curnode;
     int srat_start, numa_start, slots;
     uint64_t mem_len, mem_base, next_base;
-    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     ram_addr_t hotplugabble_address_space_size =
         object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
                                 NULL);
@@ -1756,7 +1756,7 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
     }
     if (guest_info->numa_nodes) {
         acpi_add_table(table_offsets, tables_blob);
-        build_srat(tables_blob, tables->linker, guest_info);
+        build_srat(tables_blob, tables->linker, pcms);
     }
     if (acpi_get_mcfg(&mcfg)) {
         acpi_add_table(table_offsets, tables_blob);
-- 
2.1.0

[Qemu-devel] [PATCH 09/16] acpi: Remove ram size fields fron PcGuestInfo

[Eduardo Habkost]

The ACPI code can use the PCMachineState fields directly.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 10 +++++-----
 hw/i386/pc.c         |  2 --
 include/hw/i386/pc.h |  1 -
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 8841798..9f2129d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1505,17 +1505,17 @@ build_srat(GArray *table_data, GArray *linker, PCMachineState *pcms)
         next_base = mem_base + mem_len;
 
         /* Cut out the ACPI_PCI hole */
-        if (mem_base <= guest_info->ram_size_below_4g &&
-            next_base > guest_info->ram_size_below_4g) {
-            mem_len -= next_base - guest_info->ram_size_below_4g;
+        if (mem_base <= pcms->below_4g_mem_size &&
+            next_base > pcms->below_4g_mem_size) {
+            mem_len -= next_base - pcms->below_4g_mem_size;
             if (mem_len > 0) {
                 numamem = acpi_data_push(table_data, sizeof *numamem);
                 acpi_build_srat_memory(numamem, mem_base, mem_len, i - 1,
                                        MEM_AFFINITY_ENABLED);
             }
             mem_base = 1ULL << 32;
-            mem_len = next_base - guest_info->ram_size_below_4g;
-            next_base += (1ULL << 32) - guest_info->ram_size_below_4g;
+            mem_len = next_base - pcms->below_4g_mem_size;
+            next_base += (1ULL << 32) - pcms->below_4g_mem_size;
         }
         numamem = acpi_data_push(table_data, sizeof *numamem);
         acpi_build_srat_memory(numamem, mem_base, mem_len, i - 1,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fc98a20..8687adb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1187,8 +1187,6 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
     PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int i, j;
 
-    guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
-    guest_info->ram_size = pcms->below_4g_mem_size + pcms->above_4g_mem_size;
     guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
     guest_info->apic_xrupt_override = kvm_allows_irq0_override();
     guest_info->numa_nodes = nb_numa_nodes;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1be2641..6cb9ca8 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -23,7 +23,6 @@
 /* Machine info for ACPI build: */
 struct PcGuestInfo {
     bool isapc_ram_fw;
-    hwaddr ram_size, ram_size_below_4g;
     unsigned apic_id_limit;
     bool apic_xrupt_override;
     uint64_t numa_nodes;
-- 
2.1.0

[Qemu-devel] [PATCH 10/16] pc: Move PcGuestInfo.fw_cfg field to PCMachineState

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 6 +++---
 hw/i386/pc.c         | 8 ++++----
 include/hw/i386/pc.h | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9f2129d..efc4151 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1900,7 +1900,7 @@ void acpi_setup(PCMachineState *pcms)
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
 
-    if (!guest_info->fw_cfg) {
+    if (!pcms->fw_cfg) {
         ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
         return;
     }
@@ -1933,7 +1933,7 @@ void acpi_setup(PCMachineState *pcms)
     build_state->linker_mr =
         acpi_add_rom_blob(build_state, tables.linker, "etc/table-loader", 0);
 
-    fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
+    fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
 
     if (!guest_info->rsdp_in_ram) {
@@ -1945,7 +1945,7 @@ void acpi_setup(PCMachineState *pcms)
         uint32_t rsdp_size = acpi_data_len(tables.rsdp);
 
         build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
-        fw_cfg_add_file_callback(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
+        fw_cfg_add_file_callback(pcms->fw_cfg, ACPI_BUILD_RSDP_FILE,
                                  acpi_build_update, build_state,
                                  build_state->rsdp, rsdp_size);
         build_state->rsdp_mr = NULL;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8687adb..a219187 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1171,10 +1171,10 @@ void pc_machine_done(Notifier *notifier, void *data)
                 extra_hosts++;
             }
         }
-        if (extra_hosts && pcms->acpi_guest_info.fw_cfg) {
+        if (extra_hosts && pcms->fw_cfg) {
             uint64_t *val = g_malloc(sizeof(*val));
             *val = cpu_to_le64(extra_hosts);
-            fw_cfg_add_file(pcms->acpi_guest_info.fw_cfg,
+            fw_cfg_add_file(pcms->fw_cfg,
                     "etc/extra-pci-roots", val, sizeof(*val));
         }
     }
@@ -1270,7 +1270,7 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
                !strcmp(option_rom[i].name, "multiboot.bin"));
         rom_add_option(option_rom[i].name, option_rom[i].bootindex);
     }
-    guest_info->fw_cfg = fw_cfg;
+    pcms->fw_cfg = fw_cfg;
     return fw_cfg;
 }
 
@@ -1400,7 +1400,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
     for (i = 0; i < nb_option_roms; i++) {
         rom_add_option(option_rom[i].name, option_rom[i].bootindex);
     }
-    guest_info->fw_cfg = fw_cfg;
+    pcms->fw_cfg = fw_cfg;
     return fw_cfg;
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 6cb9ca8..03750bc 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -28,7 +28,6 @@ struct PcGuestInfo {
     uint64_t numa_nodes;
     uint64_t *node_mem;
     uint64_t *node_cpu;
-    FWCfgState *fw_cfg;
     int legacy_acpi_table_size;
     bool has_acpi_build;
     bool has_reserved_memory;
@@ -56,6 +55,7 @@ struct PCMachineState {
     PCIBus *bus;
     PcGuestInfo acpi_guest_info;
     Notifier machine_done;
+    FWCfgState *fw_cfg;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
-- 
2.1.0

[Qemu-devel] [PATCH 11/16] pc: Simplify signature of xen_load_linux()

[Eduardo Habkost]

We don't need the FWCfgState return value and the PcGuestInfo
parameter.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c         | 5 +----
 hw/i386/pc_piix.c    | 2 +-
 include/hw/i386/pc.h | 3 +--
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a219187..a9ec402 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1253,8 +1253,7 @@ void pc_acpi_init(const char *default_dsdt)
     }
 }
 
-FWCfgState *xen_load_linux(PCMachineState *pcms,
-                           PcGuestInfo *guest_info)
+void xen_load_linux(PCMachineState *pcms)
 {
     int i;
     FWCfgState *fw_cfg;
@@ -1271,7 +1270,6 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
         rom_add_option(option_rom[i].name, option_rom[i].bootindex);
     }
     pcms->fw_cfg = fw_cfg;
-    return fw_cfg;
 }
 
 FWCfgState *pc_memory_init(PCMachineState *pcms,
@@ -1401,7 +1399,6 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
         rom_add_option(option_rom[i].name, option_rom[i].bootindex);
     }
     pcms->fw_cfg = fw_cfg;
-    return fw_cfg;
 }
 
 qemu_irq pc_allocate_cpu_irq(void)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f7bc1c0..f39c086 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -164,7 +164,7 @@ static void pc_init1(MachineState *machine,
                        rom_memory, &ram_memory);
     } else if (machine->kernel_filename != NULL) {
         /* For xen HVM direct kernel boot, load linux here */
-        xen_load_linux(pcms, guest_info);
+        xen_load_linux(pcms);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 03750bc..2732a72 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -205,8 +205,7 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms);
 void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
                             MemoryRegion *pci_address_space);
 
-FWCfgState *xen_load_linux(PCMachineState *pcms,
-                           PcGuestInfo *guest_info);
+void xen_load_linux(PCMachineState *pcms);
 FWCfgState *pc_memory_init(PCMachineState *pcms,
                            MemoryRegion *system_memory,
                            MemoryRegion *rom_memory,
-- 
2.1.0

Re: [Qemu-devel] [PATCH 11/16] pc: Simplify signature of xen_load_linux()

[Stefano Stabellini]

On Tue, 1 Dec 2015, Eduardo Habkost wrote:
> We don't need the FWCfgState return value and the PcGuestInfo
> parameter.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/i386/pc.c         | 5 +----
>  hw/i386/pc_piix.c    | 2 +-
>  include/hw/i386/pc.h | 3 +--
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a219187..a9ec402 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1253,8 +1253,7 @@ void pc_acpi_init(const char *default_dsdt)
>      }
>  }
>  
> -FWCfgState *xen_load_linux(PCMachineState *pcms,
> -                           PcGuestInfo *guest_info)
> +void xen_load_linux(PCMachineState *pcms)
>  {
>      int i;
>      FWCfgState *fw_cfg;
> @@ -1271,7 +1270,6 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
>          rom_add_option(option_rom[i].name, option_rom[i].bootindex);
>      }
>      pcms->fw_cfg = fw_cfg;
> -    return fw_cfg;
>  }
>  
>  FWCfgState *pc_memory_init(PCMachineState *pcms,
> @@ -1401,7 +1399,6 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
>          rom_add_option(option_rom[i].name, option_rom[i].bootindex);
>      }
>      pcms->fw_cfg = fw_cfg;
> -    return fw_cfg;
>  }
>  
>  qemu_irq pc_allocate_cpu_irq(void)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index f7bc1c0..f39c086 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -164,7 +164,7 @@ static void pc_init1(MachineState *machine,
>                         rom_memory, &ram_memory);
>      } else if (machine->kernel_filename != NULL) {
>          /* For xen HVM direct kernel boot, load linux here */
> -        xen_load_linux(pcms, guest_info);
> +        xen_load_linux(pcms);
>      }
>  
>      gsi_state = g_malloc0(sizeof(*gsi_state));
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 03750bc..2732a72 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -205,8 +205,7 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms);
>  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
>                              MemoryRegion *pci_address_space);
>  
> -FWCfgState *xen_load_linux(PCMachineState *pcms,
> -                           PcGuestInfo *guest_info);
> +void xen_load_linux(PCMachineState *pcms);
>  FWCfgState *pc_memory_init(PCMachineState *pcms,
>                             MemoryRegion *system_memory,
>                             MemoryRegion *rom_memory,
> -- 
> 2.1.0
> 
> 

[Qemu-devel] [PATCH 12/16] pc: Remove PcGuestInfo.isapc_ram_fw field

[Eduardo Habkost]

The code can use the PCMachineClass.pci_enabled field directly.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c         | 2 +-
 hw/i386/pc_piix.c    | 1 -
 hw/i386/pc_q35.c     | 1 -
 include/hw/i386/pc.h | 1 -
 4 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a9ec402..82025e2 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1364,7 +1364,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
     }
 
     /* Initialize PC system firmware */
-    pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
+    pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);
 
     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
     memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f39c086..1828cf8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -145,7 +145,6 @@ static void pc_init1(MachineState *machine,
     guest_info->has_acpi_build = pcmc->has_acpi_build;
     guest_info->legacy_acpi_table_size = pcmc->legacy_acpi_table_size;
 
-    guest_info->isapc_ram_fw = !pcmc->pci_enabled;
     guest_info->has_reserved_memory = pcmc->has_reserved_memory;
     guest_info->rsdp_in_ram = pcmc->rsdp_in_ram;
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7563bca..f124e59 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -132,7 +132,6 @@ static void pc_q35_init(MachineState *machine)
     }
 
     guest_info = pc_guest_info_init(pcms);
-    guest_info->isapc_ram_fw = false;
     guest_info->has_acpi_build = pcmc->has_acpi_build;
     guest_info->has_reserved_memory = pcmc->has_reserved_memory;
     guest_info->rsdp_in_ram = pcmc->rsdp_in_ram;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 2732a72..64f2b4b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -22,7 +22,6 @@
 
 /* Machine info for ACPI build: */
 struct PcGuestInfo {
-    bool isapc_ram_fw;
     unsigned apic_id_limit;
     bool apic_xrupt_override;
     uint64_t numa_nodes;
-- 
2.1.0

[Qemu-devel] [PATCH 13/16] q35: Remove MCHPCIState.guest_info field

[Eduardo Habkost]

The field is not used for anything.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_q35.c          | 1 -
 include/hw/pci-host/q35.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index f124e59..ad7a51e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -175,7 +175,6 @@ static void pc_q35_init(MachineState *machine)
     q35_host->mch.address_space_io = get_system_io();
     q35_host->mch.below_4g_mem_size = pcms->below_4g_mem_size;
     q35_host->mch.above_4g_mem_size = pcms->above_4g_mem_size;
-    q35_host->mch.guest_info = guest_info;
     /* pci */
     qdev_init_nofail(DEVICE(q35_host));
     phb = PCI_HOST_BRIDGE(q35_host);
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index dbe6dc0..c5c073d 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -59,7 +59,6 @@ typedef struct MCHPCIState {
     ram_addr_t below_4g_mem_size;
     ram_addr_t above_4g_mem_size;
     uint64_t pci_hole64_size;
-    PcGuestInfo *guest_info;
     uint32_t short_root_bus;
     IntelIOMMUState *iommu;
 } MCHPCIState;
-- 
2.1.0

[Qemu-devel] [PATCH 14/16] acpi: Use PCMachineClass fields directly

[Eduardo Habkost]

Remove the fields: legacy_acpi_table_size, has_acpi_build,
has_reserved_memory, and rsdp_in_ram from PcGuestInfo, and let
the ACPI code use the PCMachineClass fields directly.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 11 ++++++-----
 hw/i386/pc.c         |  6 +++---
 hw/i386/pc_piix.c    |  9 +--------
 hw/i386/pc_q35.c     | 11 +----------
 include/hw/i386/pc.h |  4 ----
 5 files changed, 11 insertions(+), 30 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index efc4151..7771be7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1686,6 +1686,7 @@ static
 void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
 {
     PcGuestInfo *guest_info = &pcms->acpi_guest_info;
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     GArray *table_offsets;
     unsigned facs, ssdt, dsdt, rsdt;
     AcpiCpuInfo cpu;
@@ -1799,12 +1800,12 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
      *
      * All this is for PIIX4, since QEMU 2.0 didn't support Q35 migration.
      */
-    if (guest_info->legacy_acpi_table_size) {
+    if (pcmc->legacy_acpi_table_size) {
         /* Subtracting aml_len gives the size of fixed tables.  Then add the
          * size of the PIIX4 DSDT/SSDT in QEMU 2.0.
          */
         int legacy_aml_len =
-            guest_info->legacy_acpi_table_size +
+            pcmc->legacy_acpi_table_size +
             ACPI_BUILD_LEGACY_CPU_AML_SIZE * max_cpus;
         int legacy_table_size =
             ROUND_UP(tables_blob->len - aml_len + legacy_aml_len,
@@ -1896,7 +1897,7 @@ static const VMStateDescription vmstate_acpi_build = {
 
 void acpi_setup(PCMachineState *pcms)
 {
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
 
@@ -1905,7 +1906,7 @@ void acpi_setup(PCMachineState *pcms)
         return;
     }
 
-    if (!guest_info->has_acpi_build) {
+    if (!pcmc->has_acpi_build) {
         ACPI_BUILD_DPRINTF("ACPI build disabled. Bailing out.\n");
         return;
     }
@@ -1936,7 +1937,7 @@ void acpi_setup(PCMachineState *pcms)
     fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
 
-    if (!guest_info->rsdp_in_ram) {
+    if (!pcmc->rsdp_in_ram) {
         /*
          * Keep for compatibility with old machine types.
          * Though RSDP is small, its contents isn't immutable, so
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 82025e2..cdfdd52 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1313,7 +1313,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
         e820_add_entry(0x100000000ULL, pcms->above_4g_mem_size, E820_RAM);
     }
 
-    if (!guest_info->has_reserved_memory &&
+    if (!pcmc->has_reserved_memory &&
         (machine->ram_slots ||
          (machine->maxram_size > machine->ram_size))) {
         MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -1324,7 +1324,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
     }
 
     /* initialize hotplug memory address space */
-    if (guest_info->has_reserved_memory &&
+    if (pcmc->has_reserved_memory &&
         (machine->ram_size < machine->maxram_size)) {
         ram_addr_t hotplug_mem_size =
             machine->maxram_size - machine->ram_size;
@@ -1379,7 +1379,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
 
     rom_set_fw(fw_cfg);
 
-    if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
+    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
         uint64_t *val = g_malloc(sizeof(*val));
         PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
         uint64_t res_mem_end = pcms->hotplug_memory.base;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1828cf8..f0c2dc8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -83,7 +83,6 @@ static void pc_init1(MachineState *machine,
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    PcGuestInfo *guest_info;
     ram_addr_t lowmem;
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
@@ -140,13 +139,7 @@ static void pc_init1(MachineState *machine,
         rom_memory = system_memory;
     }
 
-    guest_info = pc_guest_info_init(pcms);
-
-    guest_info->has_acpi_build = pcmc->has_acpi_build;
-    guest_info->legacy_acpi_table_size = pcmc->legacy_acpi_table_size;
-
-    guest_info->has_reserved_memory = pcmc->has_reserved_memory;
-    guest_info->rsdp_in_ram = pcmc->rsdp_in_ram;
+    pc_guest_info_init(pcms);
 
     if (pcmc->smbios_defaults) {
         MachineClass *mc = MACHINE_GET_CLASS(machine);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index ad7a51e..0907746 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -70,7 +70,6 @@ static void pc_q35_init(MachineState *machine)
     int i;
     ICH9LPCState *ich9_lpc;
     PCIDevice *ahci;
-    PcGuestInfo *guest_info;
     ram_addr_t lowmem;
     DriveInfo *hd[MAX_SATA_PORTS];
     MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -131,15 +130,7 @@ static void pc_q35_init(MachineState *machine)
         rom_memory = get_system_memory();
     }
 
-    guest_info = pc_guest_info_init(pcms);
-    guest_info->has_acpi_build = pcmc->has_acpi_build;
-    guest_info->has_reserved_memory = pcmc->has_reserved_memory;
-    guest_info->rsdp_in_ram = pcmc->rsdp_in_ram;
-
-    /* Migration was not supported in 2.0 for Q35, so do not bother
-     * with this hack (see hw/i386/acpi-build.c).
-     */
-    guest_info->legacy_acpi_table_size = 0;
+    pc_guest_info_init(pcms);
 
     if (pcmc->smbios_defaults) {
         /* These values are guest ABI, do not change */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 64f2b4b..a175131 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -27,10 +27,6 @@ struct PcGuestInfo {
     uint64_t numa_nodes;
     uint64_t *node_mem;
     uint64_t *node_cpu;
-    int legacy_acpi_table_size;
-    bool has_acpi_build;
-    bool has_reserved_memory;
-    bool rsdp_in_ram;
 };
 
 /**
-- 
2.1.0

[Qemu-devel] [PATCH 15/16] pc: Move PcGuestInfo.apic_xrupt_override field to PCMachineState

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 7 ++++---
 hw/i386/pc.c         | 2 +-
 include/hw/i386/pc.h | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 7771be7..cdbe5b9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -366,8 +366,9 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
 
 static void
 build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
-           PcGuestInfo *guest_info)
+           PCMachineState *pcms)
 {
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int madt_start = table_data->len;
 
     AcpiMultipleApicTable *madt;
@@ -400,7 +401,7 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
     io_apic->address = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS);
     io_apic->interrupt = cpu_to_le32(0);
 
-    if (guest_info->apic_xrupt_override) {
+    if (pcms->apic_xrupt_override) {
         intsrcovr = acpi_data_push(table_data, sizeof *intsrcovr);
         intsrcovr->type   = ACPI_APIC_XRUPT_OVERRIDE;
         intsrcovr->length = sizeof(*intsrcovr);
@@ -1740,7 +1741,7 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
     aml_len += tables_blob->len - ssdt;
 
     acpi_add_table(table_offsets, tables_blob);
-    build_madt(tables_blob, tables->linker, &cpu, guest_info);
+    build_madt(tables_blob, tables->linker, &cpu, pcms);
 
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index cdfdd52..f399d14 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1188,7 +1188,7 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
     int i, j;
 
     guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
-    guest_info->apic_xrupt_override = kvm_allows_irq0_override();
+    pcms->apic_xrupt_override = kvm_allows_irq0_override();
     guest_info->numa_nodes = nb_numa_nodes;
     guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
                                     sizeof *guest_info->node_mem);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index a175131..5ded182 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -23,7 +23,6 @@
 /* Machine info for ACPI build: */
 struct PcGuestInfo {
     unsigned apic_id_limit;
-    bool apic_xrupt_override;
     uint64_t numa_nodes;
     uint64_t *node_mem;
     uint64_t *node_cpu;
@@ -51,6 +50,7 @@ struct PCMachineState {
     PcGuestInfo acpi_guest_info;
     Notifier machine_done;
     FWCfgState *fw_cfg;
+    bool apic_xrupt_override;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
-- 
2.1.0

[Qemu-devel] [PATCH 16/16] pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState

[Eduardo Habkost]

With this, we can eliminate struct PcGuestInfo completely.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 25 +++++++++++--------------
 hw/i386/pc.c         | 23 ++++++++++-------------
 include/hw/i386/pc.h | 15 +++++----------
 3 files changed, 26 insertions(+), 37 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index cdbe5b9..3c7af74 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -368,7 +368,6 @@ static void
 build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
            PCMachineState *pcms)
 {
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int madt_start = table_data->len;
 
     AcpiMultipleApicTable *madt;
@@ -381,7 +380,7 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
     madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
     madt->flags = cpu_to_le32(1);
 
-    for (i = 0; i < guest_info->apic_id_limit; i++) {
+    for (i = 0; i < pcms->apic_id_limit; i++) {
         AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic);
         apic->type = ACPI_APIC_PROCESSOR;
         apic->length = sizeof(*apic);
@@ -929,11 +928,11 @@ static Aml *build_crs(PCIHostState *host,
 static void
 build_ssdt(GArray *table_data, GArray *linker,
            AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
-           PcPciInfo *pci, PcGuestInfo *guest_info)
+           PcPciInfo *pci, PCMachineState *pcms)
 {
-    MachineState *machine = MACHINE(qdev_get_machine());
+    MachineState *machine = MACHINE(pcms);
     uint32_t nr_mem = machine->ram_slots;
-    unsigned acpi_cpus = guest_info->apic_id_limit;
+    unsigned acpi_cpus = pcms->apic_id_limit;
     Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
     PCIBus *bus = NULL;
     GPtrArray *io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
@@ -1456,7 +1455,6 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 static void
 build_srat(GArray *table_data, GArray *linker, PCMachineState *pcms)
 {
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     AcpiSystemResourceAffinityTable *srat;
     AcpiSratProcessorAffinity *core;
     AcpiSratMemoryAffinity *numamem;
@@ -1475,12 +1473,12 @@ build_srat(GArray *table_data, GArray *linker, PCMachineState *pcms)
     srat->reserved1 = cpu_to_le32(1);
     core = (void *)(srat + 1);
 
-    for (i = 0; i < guest_info->apic_id_limit; ++i) {
+    for (i = 0; i < pcms->apic_id_limit; ++i) {
         core = acpi_data_push(table_data, sizeof *core);
         core->type = ACPI_SRAT_PROCESSOR;
         core->length = sizeof(*core);
         core->local_apic_id = i;
-        curnode = guest_info->node_cpu[i];
+        curnode = pcms->node_cpu[i];
         core->proximity_lo = curnode;
         memset(core->proximity_hi, 0, 3);
         core->local_sapic_eid = 0;
@@ -1497,9 +1495,9 @@ build_srat(GArray *table_data, GArray *linker, PCMachineState *pcms)
     numamem = acpi_data_push(table_data, sizeof *numamem);
     acpi_build_srat_memory(numamem, 0, 640*1024, 0, MEM_AFFINITY_ENABLED);
     next_base = 1024 * 1024;
-    for (i = 1; i < guest_info->numa_nodes + 1; ++i) {
+    for (i = 1; i < pcms->numa_nodes + 1; ++i) {
         mem_base = next_base;
-        mem_len = guest_info->node_mem[i - 1];
+        mem_len = pcms->node_mem[i - 1];
         if (i == 1) {
             mem_len -= 1024 * 1024;
         }
@@ -1523,7 +1521,7 @@ build_srat(GArray *table_data, GArray *linker, PCMachineState *pcms)
                                MEM_AFFINITY_ENABLED);
     }
     slots = (table_data->len - numa_start) / sizeof *numamem;
-    for (; slots < guest_info->numa_nodes + 2; slots++) {
+    for (; slots < pcms->numa_nodes + 2; slots++) {
         numamem = acpi_data_push(table_data, sizeof *numamem);
         acpi_build_srat_memory(numamem, 0, 0, 0, MEM_AFFINITY_NOFLAGS);
     }
@@ -1686,7 +1684,6 @@ static bool acpi_has_iommu(void)
 static
 void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
 {
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     GArray *table_offsets;
     unsigned facs, ssdt, dsdt, rsdt;
@@ -1737,7 +1734,7 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
     ssdt = tables_blob->len;
     acpi_add_table(table_offsets, tables_blob);
     build_ssdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci,
-               guest_info);
+               pcms);
     aml_len += tables_blob->len - ssdt;
 
     acpi_add_table(table_offsets, tables_blob);
@@ -1756,7 +1753,7 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
             build_tpm2(tables_blob, tables->linker);
         }
     }
-    if (guest_info->numa_nodes) {
+    if (pcms->numa_nodes) {
         acpi_add_table(table_offsets, tables_blob);
         build_srat(tables_blob, tables->linker, pcms);
     }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f399d14..bcd4351 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1182,29 +1182,28 @@ void pc_machine_done(Notifier *notifier, void *data)
     acpi_setup(pcms);
 }
 
-PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
+void pc_guest_info_init(PCMachineState *pcms)
 {
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int i, j;
 
-    guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
+    pcms->apic_id_limit = pc_apic_id_limit(max_cpus);
     pcms->apic_xrupt_override = kvm_allows_irq0_override();
-    guest_info->numa_nodes = nb_numa_nodes;
-    guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
-                                    sizeof *guest_info->node_mem);
+    pcms->numa_nodes = nb_numa_nodes;
+    pcms->node_mem = g_malloc0(pcms->numa_nodes *
+                                    sizeof *pcms->node_mem);
     for (i = 0; i < nb_numa_nodes; i++) {
-        guest_info->node_mem[i] = numa_info[i].node_mem;
+        pcms->node_mem[i] = numa_info[i].node_mem;
     }
 
-    guest_info->node_cpu = g_malloc0(guest_info->apic_id_limit *
-                                     sizeof *guest_info->node_cpu);
+    pcms->node_cpu = g_malloc0(pcms->apic_id_limit *
+                                     sizeof *pcms->node_cpu);
 
     for (i = 0; i < max_cpus; i++) {
         unsigned int apic_id = x86_cpu_apic_id_from_index(i);
-        assert(apic_id < guest_info->apic_id_limit);
+        assert(apic_id < pcms->apic_id_limit);
         for (j = 0; j < nb_numa_nodes; j++) {
             if (test_bit(i, numa_info[j].node_cpu)) {
-                guest_info->node_cpu[apic_id] = j;
+                pcms->node_cpu[apic_id] = j;
                 break;
             }
         }
@@ -1212,7 +1211,6 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
 
     pcms->machine_done.notify = pc_machine_done;
     qemu_add_machine_init_done_notifier(&pcms->machine_done);
-    return guest_info;
 }
 
 /* setup pci memory address space mapping into system address space */
@@ -1277,7 +1275,6 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
                            MemoryRegion *rom_memory,
                            MemoryRegion **ram_memory)
 {
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int linux_boot, i;
     MemoryRegion *ram, *option_rom_mr;
     MemoryRegion *ram_below_4g, *ram_above_4g;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 5ded182..6ff4721 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -20,14 +20,6 @@
 
 #define HPET_INTCAP "hpet-intcap"
 
-/* Machine info for ACPI build: */
-struct PcGuestInfo {
-    unsigned apic_id_limit;
-    uint64_t numa_nodes;
-    uint64_t *node_mem;
-    uint64_t *node_cpu;
-};
-
 /**
  * PCMachineState:
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
@@ -47,10 +39,13 @@ struct PCMachineState {
     OnOffAuto smm;
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
     PCIBus *bus;
-    PcGuestInfo acpi_guest_info;
     Notifier machine_done;
     FWCfgState *fw_cfg;
     bool apic_xrupt_override;
+    unsigned apic_id_limit;
+    uint64_t numa_nodes;
+    uint64_t *node_mem;
+    uint64_t *node_cpu;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
@@ -187,7 +182,7 @@ void pc_cpus_init(PCMachineState *pcms);
 void pc_hot_add_cpu(const int64_t id, Error **errp);
 void pc_acpi_init(const char *default_dsdt);
 
-PcGuestInfo *pc_guest_info_init(PCMachineState *pcms);
+void pc_guest_info_init(PCMachineState *pcms);
 
 #define PCI_HOST_PROP_PCI_HOLE_START   "pci-hole-start"
 #define PCI_HOST_PROP_PCI_HOLE_END     "pci-hole-end"
-- 
2.1.0

Re: [Qemu-devel] [PATCH 00/16] pc: Eliminate struct PcGuestInfo

[Marcel Apfelbaum]

On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
> This moves all data from PcGuestInfo to either PCMachineState or
> PCMachineClass.
>
> This series depends on other two series:
> * [PATCH v3 0/6] pc: Initialization and compat function cleanup
> * [PATCH V3 0/3]  hw/pcie: Multi-root support for Q35
>
> For reference, there's a git tree containing this series plus all
> the dependencies, at:
>    git://github.com/ehabkost/qemu-hacks.git work/pcguestinfo-eliminate
>
> Eduardo Habkost (16):
>    pc: Move PcGuestInfo declaration to top of file
>    pc: Eliminate struct PcGuestInfoState
>    pc: Remove guest_info parameter from pc_memory_init()
>    acpi: Make acpi_setup() get PCMachineState as argument
>    acpi: Remove unused build_facs() PcGuestInfo paramter
>    acpi: Save PCMachineState on AcpiBuildState
>    acpi: Make acpi_build() get PCMachineState as argument
>    acpi: Make build_srat() get PCMachineState as argument
>    acpi: Remove ram size fields fron PcGuestInfo
>    pc: Move PcGuestInfo.fw_cfg field to PCMachineState
>    pc: Simplify signature of xen_load_linux()
>    pc: Remove PcGuestInfo.isapc_ram_fw field
>    q35: Remove MCHPCIState.guest_info field
>    acpi: Use PCMachineClass fields directly
>    pc: Move PcGuestInfo.apic_xrupt_override field to PCMachineState
>    pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState

Hi,

I mainly agree with the removal of PcGuestInfo , I commented on some patches.

I do have a minor reservation, we kind of loose some information about the fields.
Until now it was pretty clear that the fields were related to guest because
they were part of PcGuestInfo. Now this information is lost and the fields
appear as yet other machine attributes.

I suppose this can be addressed by:
- a prefix for guest fields (e.g numa_nodes-> guest_numa_nodes),
- a comment in the class /* guest fields */,
- keeping the fields in PcGuestInfo struct but make the machine field short: guest so we can call machine->guest.numa_nodes
- or not be addressed at all :)


Thanks,
Marcel



>
>   hw/i386/acpi-build.c      | 75 ++++++++++++++++++++++++-----------------------
>   hw/i386/acpi-build.h      |  2 +-
>   hw/i386/pc.c              | 71 ++++++++++++++++++--------------------------
>   hw/i386/pc_piix.c         | 14 ++-------
>   hw/i386/pc_q35.c          | 15 ++--------
>   include/hw/i386/pc.h      | 30 +++++++------------
>   include/hw/pci-host/q35.h |  1 -
>   7 files changed, 82 insertions(+), 126 deletions(-)
>

Re: [Qemu-devel] [PATCH 00/16] pc: Eliminate struct PcGuestInfo

[Eduardo Habkost]

On Mon, Dec 07, 2015 at 08:57:03PM +0200, Marcel Apfelbaum wrote:
> On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
> >This moves all data from PcGuestInfo to either PCMachineState or
> >PCMachineClass.
> >
> >This series depends on other two series:
> >* [PATCH v3 0/6] pc: Initialization and compat function cleanup
> >* [PATCH V3 0/3]  hw/pcie: Multi-root support for Q35
> >
> >For reference, there's a git tree containing this series plus all
> >the dependencies, at:
> >   git://github.com/ehabkost/qemu-hacks.git work/pcguestinfo-eliminate
> >
> >Eduardo Habkost (16):
> >   pc: Move PcGuestInfo declaration to top of file
> >   pc: Eliminate struct PcGuestInfoState
> >   pc: Remove guest_info parameter from pc_memory_init()
> >   acpi: Make acpi_setup() get PCMachineState as argument
> >   acpi: Remove unused build_facs() PcGuestInfo paramter
> >   acpi: Save PCMachineState on AcpiBuildState
> >   acpi: Make acpi_build() get PCMachineState as argument
> >   acpi: Make build_srat() get PCMachineState as argument
> >   acpi: Remove ram size fields fron PcGuestInfo
> >   pc: Move PcGuestInfo.fw_cfg field to PCMachineState
> >   pc: Simplify signature of xen_load_linux()
> >   pc: Remove PcGuestInfo.isapc_ram_fw field
> >   q35: Remove MCHPCIState.guest_info field
> >   acpi: Use PCMachineClass fields directly
> >   pc: Move PcGuestInfo.apic_xrupt_override field to PCMachineState
> >   pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState
> 
> Hi,
> 
> I mainly agree with the removal of PcGuestInfo , I commented on some patches.
> 
> I do have a minor reservation, we kind of loose some information about the fields.
> Until now it was pretty clear that the fields were related to guest because
> they were part of PcGuestInfo. Now this information is lost and the fields
> appear as yet other machine attributes.

But they really are just machine attributes, aren't they?

> 
> I suppose this can be addressed by:
> - a prefix for guest fields (e.g numa_nodes-> guest_numa_nodes),
> - a comment in the class /* guest fields */,
> - keeping the fields in PcGuestInfo struct but make the machine field short: guest so we can call machine->guest.numa_nodes
> - or not be addressed at all :)

I don't see your point. Could you explain what you mean by
"related to the guest" and "guest fields"?

They are just machine attributes, and they happen to be used as
input when building ACPI tables (just like other machine
attributes are used as input for other guest-visible data, like
CPUID, SMBIOS, and other tables). What exactly make them "related
to guest"?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 00/16] pc: Eliminate struct PcGuestInfo

[Marcel Apfelbaum]

On 12/08/2015 07:53 PM, Eduardo Habkost wrote:
> On Mon, Dec 07, 2015 at 08:57:03PM +0200, Marcel Apfelbaum wrote:
>> On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
>>> This moves all data from PcGuestInfo to either PCMachineState or
>>> PCMachineClass.
>>>
>>> This series depends on other two series:
>>> * [PATCH v3 0/6] pc: Initialization and compat function cleanup
>>> * [PATCH V3 0/3]  hw/pcie: Multi-root support for Q35
>>>
>>> For reference, there's a git tree containing this series plus all
>>> the dependencies, at:
>>>    git://github.com/ehabkost/qemu-hacks.git work/pcguestinfo-eliminate
>>>
>>> Eduardo Habkost (16):
>>>    pc: Move PcGuestInfo declaration to top of file
>>>    pc: Eliminate struct PcGuestInfoState
>>>    pc: Remove guest_info parameter from pc_memory_init()
>>>    acpi: Make acpi_setup() get PCMachineState as argument
>>>    acpi: Remove unused build_facs() PcGuestInfo paramter
>>>    acpi: Save PCMachineState on AcpiBuildState
>>>    acpi: Make acpi_build() get PCMachineState as argument
>>>    acpi: Make build_srat() get PCMachineState as argument
>>>    acpi: Remove ram size fields fron PcGuestInfo
>>>    pc: Move PcGuestInfo.fw_cfg field to PCMachineState
>>>    pc: Simplify signature of xen_load_linux()
>>>    pc: Remove PcGuestInfo.isapc_ram_fw field
>>>    q35: Remove MCHPCIState.guest_info field
>>>    acpi: Use PCMachineClass fields directly
>>>    pc: Move PcGuestInfo.apic_xrupt_override field to PCMachineState
>>>    pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState
>>
>> Hi,
>>
>> I mainly agree with the removal of PcGuestInfo , I commented on some patches.
>>
>> I do have a minor reservation, we kind of loose some information about the fields.
>> Until now it was pretty clear that the fields were related to guest because
>> they were part of PcGuestInfo. Now this information is lost and the fields
>> appear as yet other machine attributes.
>
> But they really are just machine attributes, aren't they?
>
>>
>> I suppose this can be addressed by:
>> - a prefix for guest fields (e.g numa_nodes-> guest_numa_nodes),
>> - a comment in the class /* guest fields */,
>> - keeping the fields in PcGuestInfo struct but make the machine field short: guest so we can call machine->guest.numa_nodes
>> - or not be addressed at all :)
>
> I don't see your point. Could you explain what you mean by
> "related to the guest" and "guest fields"?
>
> They are just machine attributes, and they happen to be used as
> input when building ACPI tables (just like other machine
> attributes are used as input for other guest-visible data, like
> CPUID, SMBIOS, and other tables). What exactly make them "related
> to guest"?
>

Maybe I wasn't clear indeed, let me try again please.

I (personally) don't like structures with a lot of not related fields.
The reason is, it will be very hard for someone reading the code to understand the use
of each field => a global code query will be necessary, *exactly* like a global variable.
(given that a machine is also one per system)

I do understand that sometimes, machine class included, there is a need for a lot of fields.
What I am suggesting is grouping the fields by their purpose/subsystem.
If "guest visible" does not do the trick, maybe other logical partition can be made.
For example (this is only an example): acpi fields, cpu fields, ...

In this way the code reviewer can understand with a quick look what are the "parts" of a machine
and where are used.

Since the (very good!) re-factoring you are doing makes the code less complex by removing an
unnecessary artifact (PcGuestInfo), I wouldn't want to miss the opportunity to point to
another code complexity we may get into.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 00/16] pc: Eliminate struct PcGuestInfo

[Eduardo Habkost]

On Thu, Dec 10, 2015 at 01:27:50PM +0200, Marcel Apfelbaum wrote:
> On 12/08/2015 07:53 PM, Eduardo Habkost wrote:
> >On Mon, Dec 07, 2015 at 08:57:03PM +0200, Marcel Apfelbaum wrote:
> >>On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
> >>>This moves all data from PcGuestInfo to either PCMachineState or
> >>>PCMachineClass.
> >>>
> >>>This series depends on other two series:
> >>>* [PATCH v3 0/6] pc: Initialization and compat function cleanup
> >>>* [PATCH V3 0/3]  hw/pcie: Multi-root support for Q35
> >>>
> >>>For reference, there's a git tree containing this series plus all
> >>>the dependencies, at:
> >>>   git://github.com/ehabkost/qemu-hacks.git work/pcguestinfo-eliminate
> >>>
> >>>Eduardo Habkost (16):
> >>>   pc: Move PcGuestInfo declaration to top of file
> >>>   pc: Eliminate struct PcGuestInfoState
> >>>   pc: Remove guest_info parameter from pc_memory_init()
> >>>   acpi: Make acpi_setup() get PCMachineState as argument
> >>>   acpi: Remove unused build_facs() PcGuestInfo paramter
> >>>   acpi: Save PCMachineState on AcpiBuildState
> >>>   acpi: Make acpi_build() get PCMachineState as argument
> >>>   acpi: Make build_srat() get PCMachineState as argument
> >>>   acpi: Remove ram size fields fron PcGuestInfo
> >>>   pc: Move PcGuestInfo.fw_cfg field to PCMachineState
> >>>   pc: Simplify signature of xen_load_linux()
> >>>   pc: Remove PcGuestInfo.isapc_ram_fw field
> >>>   q35: Remove MCHPCIState.guest_info field
> >>>   acpi: Use PCMachineClass fields directly
> >>>   pc: Move PcGuestInfo.apic_xrupt_override field to PCMachineState
> >>>   pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState
> >>
> >>Hi,
> >>
> >>I mainly agree with the removal of PcGuestInfo , I commented on some patches.
> >>
> >>I do have a minor reservation, we kind of loose some information about the fields.
> >>Until now it was pretty clear that the fields were related to guest because
> >>they were part of PcGuestInfo. Now this information is lost and the fields
> >>appear as yet other machine attributes.
> >
> >But they really are just machine attributes, aren't they?
> >
> >>
> >>I suppose this can be addressed by:
> >>- a prefix for guest fields (e.g numa_nodes-> guest_numa_nodes),
> >>- a comment in the class /* guest fields */,
> >>- keeping the fields in PcGuestInfo struct but make the machine field short: guest so we can call machine->guest.numa_nodes
> >>- or not be addressed at all :)
> >
> >I don't see your point. Could you explain what you mean by
> >"related to the guest" and "guest fields"?
> >
> >They are just machine attributes, and they happen to be used as
> >input when building ACPI tables (just like other machine
> >attributes are used as input for other guest-visible data, like
> >CPUID, SMBIOS, and other tables). What exactly make them "related
> >to guest"?
> >
> 
> Maybe I wasn't clear indeed, let me try again please.
> 
> I (personally) don't like structures with a lot of not related fields.
> The reason is, it will be very hard for someone reading the code to understand the use
> of each field => a global code query will be necessary, *exactly* like a global variable.
> (given that a machine is also one per system)
> 
> I do understand that sometimes, machine class included, there is a need for a lot of fields.
> What I am suggesting is grouping the fields by their purpose/subsystem.
> If "guest visible" does not do the trick, maybe other logical partition can be made.
> For example (this is only an example): acpi fields, cpu fields, ...

I see. I believe "guest fields" wouldn't be a clear partitioning,
but grouping CPU/RAM/NUMA/ACPI fields would be nice.

I will send a new version of this series to implement the
qdev_get_machine() suggestion, and try to group and document
related fields in PCMachineState/PCMachienClass.

> 
> In this way the code reviewer can understand with a quick look what are the "parts" of a machine
> and where are used.
> 
> Since the (very good!) re-factoring you are doing makes the code less complex by removing an
> unnecessary artifact (PcGuestInfo), I wouldn't want to miss the opportunity to point to
> another code complexity we may get into.

Yes, it does makes sense.

-- 
Eduardo