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

[Qemu-devel] [PATCH v2 00/14] 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

Changes v1 -> v2:
* Remove PCMachineState field from AcpiBuildState, use
  qdev_get_machine() instead
* Reorder series and squash some patches together

Eduardo Habkost (14):
  q35: Remove MCHPCIState.guest_info field
  pc: Group and document related PCMachineState/PCMachineclass fields
  pc: Move PcGuestInfo declaration to top of file
  pc: Eliminate struct PcGuestInfoState
  pc: Simplify pc_memory_init() signature
  pc: Simplify xen_load_linux() signature
  acpi: Remove guest_info parameters from functions
  acpi: Don't save PcGuestInfo on AcpiBuildState
  pc: Remove compat fields from PcGuestInfo
  pc: Remove RAM size fields from PcGuestInfo
  pc: Remove PcGuestInfo.isapc_ram_fw field
  pc: Move PcGuestInfo.fw_cfg to PCMachineState
  pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState
  pc: Eliminate PcGuestInfo struct

 hw/i386/acpi-build.c      | 77 +++++++++++++++++++++--------------------
 hw/i386/acpi-build.h      |  2 +-
 hw/i386/pc.c              | 77 +++++++++++++++++------------------------
 hw/i386/pc_piix.c         | 14 ++------
 hw/i386/pc_q35.c          | 15 ++------
 include/hw/i386/pc.h      | 88 ++++++++++++++++++++++++++++-------------------
 include/hw/pci-host/q35.h |  1 -
 7 files changed, 129 insertions(+), 145 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 01/14] 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 9da751b..43ee8bb 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -176,7 +176,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

Re: [Qemu-devel] [PATCH v2 01/14] q35: Remove MCHPCIState.guest_info field

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> 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 9da751b..43ee8bb 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -176,7 +176,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;
>

So nobody is using it.


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

[Qemu-devel] [PATCH v2 02/14] pc: Group and document related PCMachineState/PCMachineclass fields

[Eduardo Habkost]

Group related PCMachineState and PCMachineClass fields into
sections, and move existing field descriptions to doc comments.

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

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9811229..9503dbb 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -29,16 +29,22 @@ struct PCMachineState {
     MachineState parent_obj;
 
     /* <public> */
+
+    /* State for other subsystems/APIs: */
     MemoryHotplugState hotplug_memory;
 
+    /* Pointers to devices and objects: */
     HotplugHandler *acpi_dev;
     ISADevice *rtc;
+    PCIBus *bus;
 
+    /* Configuration options: */
     uint64_t max_ram_below_4g;
     OnOffAuto vmport;
     OnOffAuto smm;
+
+    /* RAM information (sizes, addresses, configuration): */
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
-    PCIBus *bus;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
@@ -49,38 +55,56 @@ struct PCMachineState {
 
 /**
  * PCMachineClass:
+ *
+ * Methods:
+ *
  * @get_hotplug_handler: pointer to parent class callback @get_hotplug_handler
+ *
+ * Compat fields:
+ *
  * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
  *                        backend's alignment value if provided
+ * @acpi_data_size: Size of the chunk of memory at the top of RAM
+ *                  for the BIOS ACPI tables and other BIOS
+ *                  datastructures.
+ * @gigabyte_align: Make sure that guest addresses aligned at
+ *                  1Gbyte boundaries get mapped to host
+ *                  addresses aligned at 1Gbyte boundaries. This
+ *                  way we can use 1GByte pages in the host.
+ *
  */
 struct PCMachineClass {
     /*< private >*/
     MachineClass parent_class;
 
     /*< public >*/
-    bool broken_reserved_end;
+
+    /* Methods: */
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
 
+    /* Device configuration: */
     bool pci_enabled;
+    bool kvmclock_enabled;
+
+    /* Compat options: */
+
+    /* ACPI compat: */
     bool has_acpi_build;
     bool rsdp_in_ram;
+    int legacy_acpi_table_size;
+    unsigned acpi_data_size;
+
+    /* SMBIOS compat: */
     bool smbios_defaults;
     bool smbios_legacy_mode;
     bool smbios_uuid_encoded;
-    /* Make sure that guest addresses aligned at 1Gbyte boundaries get
-     * mapped to host addresses aligned at 1Gbyte boundaries.  This way
-     * we can use 1GByte pages in the host.
-     */
+
+    /* RAM / address space compat: */
     bool gigabyte_align;
     bool has_reserved_memory;
-    bool kvmclock_enabled;
-    int legacy_acpi_table_size;
-    /* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables
-     * and other BIOS datastructures.
-     */
-    unsigned acpi_data_size;
     bool enforce_aligned_dimm;
+    bool broken_reserved_end;
 };
 
 #define TYPE_PC_MACHINE "generic-pc-machine"
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 02/14] pc: Group and document related PCMachineState/PCMachineclass fields

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> Group related PCMachineState and PCMachineClass fields into
> sections, and move existing field descriptions to doc comments.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   include/hw/i386/pc.h | 48 ++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 9811229..9503dbb 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -29,16 +29,22 @@ struct PCMachineState {
>       MachineState parent_obj;
>
>       /* <public> */
> +
> +    /* State for other subsystems/APIs: */
>       MemoryHotplugState hotplug_memory;
>
> +    /* Pointers to devices and objects: */
>       HotplugHandler *acpi_dev;
>       ISADevice *rtc;
> +    PCIBus *bus;
>
> +    /* Configuration options: */
>       uint64_t max_ram_below_4g;
>       OnOffAuto vmport;
>       OnOffAuto smm;
> +
> +    /* RAM information (sizes, addresses, configuration): */
>       ram_addr_t below_4g_mem_size, above_4g_mem_size;
> -    PCIBus *bus;
>   };
>
>   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> @@ -49,38 +55,56 @@ struct PCMachineState {
>
>   /**
>    * PCMachineClass:
> + *
> + * Methods:
> + *
>    * @get_hotplug_handler: pointer to parent class callback @get_hotplug_handler
> + *
> + * Compat fields:
> + *
>    * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
>    *                        backend's alignment value if provided
> + * @acpi_data_size: Size of the chunk of memory at the top of RAM
> + *                  for the BIOS ACPI tables and other BIOS
> + *                  datastructures.
> + * @gigabyte_align: Make sure that guest addresses aligned at
> + *                  1Gbyte boundaries get mapped to host
> + *                  addresses aligned at 1Gbyte boundaries. This
> + *                  way we can use 1GByte pages in the host.
> + *
>    */
>   struct PCMachineClass {
>       /*< private >*/
>       MachineClass parent_class;
>
>       /*< public >*/
> -    bool broken_reserved_end;
> +
> +    /* Methods: */
>       HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                              DeviceState *dev);
>
> +    /* Device configuration: */
>       bool pci_enabled;
> +    bool kvmclock_enabled;
> +
> +    /* Compat options: */
> +
> +    /* ACPI compat: */
>       bool has_acpi_build;
>       bool rsdp_in_ram;
> +    int legacy_acpi_table_size;
> +    unsigned acpi_data_size;
> +
> +    /* SMBIOS compat: */
>       bool smbios_defaults;
>       bool smbios_legacy_mode;
>       bool smbios_uuid_encoded;
> -    /* Make sure that guest addresses aligned at 1Gbyte boundaries get
> -     * mapped to host addresses aligned at 1Gbyte boundaries.  This way
> -     * we can use 1GByte pages in the host.
> -     */
> +
> +    /* RAM / address space compat: */
>       bool gigabyte_align;
>       bool has_reserved_memory;
> -    bool kvmclock_enabled;
> -    int legacy_acpi_table_size;
> -    /* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables
> -     * and other BIOS datastructures.
> -     */
> -    unsigned acpi_data_size;
>       bool enforce_aligned_dimm;
> +    bool broken_reserved_end;
>   };
>
>   #define TYPE_PC_MACHINE "generic-pc-machine"
>


Thanks for this! It will really help to understand the PCMachine components.


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

[Qemu-devel] [PATCH v2 03/14] 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 9503dbb..84bc88f 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
@@ -133,21 +149,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

Re: [Qemu-devel] [PATCH v2 03/14] pc: Move PcGuestInfo declaration to top of file

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> 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 9503dbb..84bc88f 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
> @@ -133,21 +149,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);
>

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

[Qemu-devel] [PATCH v2 04/14] 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 84bc88f..74c0515 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -48,6 +48,8 @@ struct PCMachineState {
 
     /* State for other subsystems/APIs: */
     MemoryHotplugState hotplug_memory;
+    PcGuestInfo acpi_guest_info;
+    Notifier machine_done;
 
     /* Pointers to devices and objects: */
     HotplugHandler *acpi_dev;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 04/14] pc: Eliminate struct PcGuestInfoState

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> Instead of allocating a new struct just for PcGuestInfo and the
> mchine_done Notifier, place them inside PCMachineState.

^^^^^^^^ "machine_done", ..... ^^ "it"

  it doesn't worth a new version, maybe the maintainer
can take care of it.

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

good! no need for the ad-hoc qdev_get_machine() query anymore.

> +    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 84bc88f..74c0515 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -48,6 +48,8 @@ struct PCMachineState {
>
>       /* State for other subsystems/APIs: */
>       MemoryHotplugState hotplug_memory;
> +    PcGuestInfo acpi_guest_info;

thanks for the clue, we know is some acpi related stuff.
you could get rid off the _guest_ and use  acpi_info,
but since is will be removed anyway there is no need for it.


> +    Notifier machine_done;
>
>       /* Pointers to devices and objects: */
>       HotplugHandler *acpi_dev;
>


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

[Qemu-devel] [PATCH v2 05/14] pc: Simplify pc_memory_init() signature

[Eduardo Habkost]

We can get the PcGuestInfo struct directly from PCMachineState,
and the return value is not needed at all.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 30cdfaf..b4c638d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1276,12 +1276,12 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
     return fw_cfg;
 }
 
-FWCfgState *pc_memory_init(PCMachineState *pcms,
-                           MemoryRegion *system_memory,
-                           MemoryRegion *rom_memory,
-                           MemoryRegion **ram_memory,
-                           PcGuestInfo *guest_info)
+void pc_memory_init(PCMachineState *pcms,
+                    MemoryRegion *system_memory,
+                    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;
@@ -1403,7 +1403,6 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
         rom_add_option(option_rom[i].name, option_rom[i].bootindex);
     }
     guest_info->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 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 43ee8bb..bb1436e 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 74c0515..3943507 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -232,11 +232,10 @@ void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
 
 FWCfgState *xen_load_linux(PCMachineState *pcms,
                            PcGuestInfo *guest_info);
-FWCfgState *pc_memory_init(PCMachineState *pcms,
-                           MemoryRegion *system_memory,
-                           MemoryRegion *rom_memory,
-                           MemoryRegion **ram_memory,
-                           PcGuestInfo *guest_info);
+void pc_memory_init(PCMachineState *pcms,
+                    MemoryRegion *system_memory,
+                    MemoryRegion *rom_memory,
+                    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

Re: [Qemu-devel] [PATCH v2 05/14] pc: Simplify pc_memory_init() signature

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> We can get the PcGuestInfo struct directly from PCMachineState,
> and the return value is not needed at all.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/pc.c         | 11 +++++------
>   hw/i386/pc_piix.c    |  2 +-
>   hw/i386/pc_q35.c     |  2 +-
>   include/hw/i386/pc.h |  9 ++++-----
>   4 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 30cdfaf..b4c638d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1276,12 +1276,12 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
>       return fw_cfg;
>   }
>
> -FWCfgState *pc_memory_init(PCMachineState *pcms,
> -                           MemoryRegion *system_memory,
> -                           MemoryRegion *rom_memory,
> -                           MemoryRegion **ram_memory,
> -                           PcGuestInfo *guest_info)
> +void pc_memory_init(PCMachineState *pcms,
> +                    MemoryRegion *system_memory,
> +                    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;
> @@ -1403,7 +1403,6 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
>           rom_add_option(option_rom[i].name, option_rom[i].bootindex);
>       }
>       guest_info->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 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 43ee8bb..bb1436e 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 74c0515..3943507 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -232,11 +232,10 @@ void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
>
>   FWCfgState *xen_load_linux(PCMachineState *pcms,
>                              PcGuestInfo *guest_info);
> -FWCfgState *pc_memory_init(PCMachineState *pcms,
> -                           MemoryRegion *system_memory,
> -                           MemoryRegion *rom_memory,
> -                           MemoryRegion **ram_memory,
> -                           PcGuestInfo *guest_info);
> +void pc_memory_init(PCMachineState *pcms,
> +                    MemoryRegion *system_memory,
> +                    MemoryRegion *rom_memory,
> +                    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,
>


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

[Qemu-devel] [PATCH v2 06/14] pc: Simplify xen_load_linux() signature

[Eduardo Habkost]

We can get the PcGuestInfo struct directly from PCMachineState,
and the return value is not needed at all.

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, 4 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b4c638d..aa12814 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1255,11 +1255,11 @@ 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;
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
 
     assert(MACHINE(pcms)->kernel_filename != NULL);
 
@@ -1273,7 +1273,6 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
         rom_add_option(option_rom[i].name, option_rom[i].bootindex);
     }
     guest_info->fw_cfg = fw_cfg;
-    return fw_cfg;
 }
 
 void pc_memory_init(PCMachineState *pcms,
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 3943507..24362ef 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -230,8 +230,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);
 void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 06/14] pc: Simplify xen_load_linux() signature

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> We can get the PcGuestInfo struct directly from PCMachineState,
> and the return value is not needed at all.
>
> 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, 4 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b4c638d..aa12814 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1255,11 +1255,11 @@ 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;
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>
>       assert(MACHINE(pcms)->kernel_filename != NULL);
>
> @@ -1273,7 +1273,6 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
>           rom_add_option(option_rom[i].name, option_rom[i].bootindex);
>       }
>       guest_info->fw_cfg = fw_cfg;
> -    return fw_cfg;
>   }
>
>   void pc_memory_init(PCMachineState *pcms,
> 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 3943507..24362ef 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -230,8 +230,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);
>   void pc_memory_init(PCMachineState *pcms,
>                       MemoryRegion *system_memory,
>                       MemoryRegion *rom_memory,
>


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

[Qemu-devel] [PATCH v2 07/14] acpi: Remove guest_info parameters from functions

[Eduardo Habkost]

We can use PC_MACHINE(qdev_get_machine())->acpi_guest_info to get
guest_info.

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index bca3f06..92d25c9 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);
@@ -365,9 +365,10 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
 }
 
 static void
-build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
-           PcGuestInfo *guest_info)
+build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu)
 {
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int madt_start = table_data->len;
 
     AcpiMultipleApicTable *madt;
@@ -928,9 +929,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)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
+    PCMachineState *pcms = PC_MACHINE(machine);
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     uint32_t nr_mem = machine->ram_slots;
     unsigned acpi_cpus = guest_info->apic_id_limit;
     Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
@@ -1453,7 +1456,7 @@ 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)
 {
     AcpiSystemResourceAffinityTable *srat;
     AcpiSratProcessorAffinity *core;
@@ -1464,6 +1467,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
     int srat_start, numa_start, slots;
     uint64_t mem_len, mem_base, next_base;
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     ram_addr_t hotplugabble_address_space_size =
         object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
                                 NULL);
@@ -1683,8 +1687,10 @@ static bool acpi_has_iommu(void)
 }
 
 static
-void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
+void acpi_build(AcpiBuildTables *tables)
 {
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     GArray *table_offsets;
     unsigned facs, ssdt, dsdt, rsdt;
     AcpiCpuInfo cpu;
@@ -1716,7 +1722,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;
@@ -1733,12 +1739,11 @@ void acpi_build(PcGuestInfo *guest_info, 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);
+    build_ssdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci);
     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);
 
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
@@ -1755,7 +1760,7 @@ void acpi_build(PcGuestInfo *guest_info, 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);
     }
     if (acpi_get_mcfg(&mcfg)) {
         acpi_add_table(table_offsets, tables_blob);
@@ -1855,7 +1860,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(&tables);
 
     acpi_ram_update(build_state->table_mr, tables.table_data);
 
@@ -1893,8 +1898,10 @@ static const VMStateDescription vmstate_acpi_build = {
     },
 };
 
-void acpi_setup(PcGuestInfo *guest_info)
+void acpi_setup(void)
 {
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
 
@@ -1920,7 +1927,7 @@ void acpi_setup(PcGuestInfo *guest_info)
     acpi_set_pci_info();
 
     acpi_build_tables_init(&tables);
-    acpi_build(build_state->guest_info, &tables);
+    acpi_build(&tables);
 
     /* Now expose it all to Guest */
     build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index e57b1aa..148c0f9 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(void);
 
 #endif
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index aa12814..14c0116 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();
 }
 
 PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 07/14] acpi: Remove guest_info parameters from functions

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> We can use PC_MACHINE(qdev_get_machine())->acpi_guest_info to get
> guest_info.

Hi Eduardo,

I like the idea of using qdev_get_machine() to get the machine instead of
keeping the reference around, however only in places we cannot get a reference
to it otherwise.

I'll follow inline.

>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/acpi-build.c | 35 +++++++++++++++++++++--------------
>   hw/i386/acpi-build.h |  2 +-
>   hw/i386/pc.c         |  2 +-
>   3 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index bca3f06..92d25c9 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)

this function does not need guest_info, no problem here.

>   {
>       AcpiFacsDescriptorRev1 *facs = acpi_data_push(table_data, sizeof *facs);
>       memcpy(&facs->signature, "FACS", 4);
> @@ -365,9 +365,10 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
>   }
>
>   static void
> -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> -           PcGuestInfo *guest_info)
> +build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu)
>   {
> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;

This function can get a reference of the machine from the caller, acpi_build, which
already queries for it. I am not talking about "speed" here, simply about keeping
the code clean, which is exactly what this series does.

>       int madt_start = table_data->len;
>
>       AcpiMultipleApicTable *madt;
> @@ -928,9 +929,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)
>   {
>       MachineState *machine = MACHINE(qdev_get_machine());
> +    PCMachineState *pcms = PC_MACHINE(machine);
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;

Same here. If you pass the machine you don't need another query here.
I see it was here before, but we can get rid of it.

>       uint32_t nr_mem = machine->ram_slots;
>       unsigned acpi_cpus = guest_info->apic_id_limit;
>       Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> @@ -1453,7 +1456,7 @@ 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)
>   {
>       AcpiSystemResourceAffinityTable *srat;
>       AcpiSratProcessorAffinity *core;
> @@ -1464,6 +1467,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
>       int srat_start, numa_start, slots;
>       uint64_t mem_len, mem_base, next_base;
>       PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;

Same here

>       ram_addr_t hotplugabble_address_space_size =
>           object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
>                                   NULL);
> @@ -1683,8 +1687,10 @@ static bool acpi_has_iommu(void)
>   }
>
>   static
> -void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
> +void acpi_build(AcpiBuildTables *tables)
>   {
> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;

Here it makes sense to query for the machine because it is called from
acpi_build_update which is a callback with no machine reference.

However the other acpi methods can use this reference. Of course
you still get rid of PcGuestInfo.


>       GArray *table_offsets;
>       unsigned facs, ssdt, dsdt, rsdt;
>       AcpiCpuInfo cpu;
> @@ -1716,7 +1722,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;
> @@ -1733,12 +1739,11 @@ void acpi_build(PcGuestInfo *guest_info, 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);
> +    build_ssdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci);
>       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);
>
>       if (misc.has_hpet) {
>           acpi_add_table(table_offsets, tables_blob);
> @@ -1755,7 +1760,7 @@ void acpi_build(PcGuestInfo *guest_info, 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);
>       }
>       if (acpi_get_mcfg(&mcfg)) {
>           acpi_add_table(table_offsets, tables_blob);
> @@ -1855,7 +1860,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(&tables);
>
>       acpi_ram_update(build_state->table_mr, tables.table_data);
>
> @@ -1893,8 +1898,10 @@ static const VMStateDescription vmstate_acpi_build = {
>       },
>   };
>
> -void acpi_setup(PcGuestInfo *guest_info)
> +void acpi_setup(void)
>   {
> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;

acpi_setup is called from pc_guest_info_machine_done that after one
of you previous patches (4/14) has a reference to machine.

>       AcpiBuildTables tables;
>       AcpiBuildState *build_state;
>
> @@ -1920,7 +1927,7 @@ void acpi_setup(PcGuestInfo *guest_info)
>       acpi_set_pci_info();
>
>       acpi_build_tables_init(&tables);
> -    acpi_build(build_state->guest_info, &tables);
> +    acpi_build(&tables);
>
>       /* Now expose it all to Guest */
>       build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index e57b1aa..148c0f9 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(void);
>
>   #endif
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index aa12814..14c0116 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();
>   }
>
>   PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>


My point is, keeping a reference to machine when it can be queried is not preferred,
querying for a pc machine when the caller has already a reference to it is also not preferred.

All this of course IMHO.

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 07/14] acpi: Remove guest_info parameters from functions

[Eduardo Habkost]

On Tue, Dec 15, 2015 at 02:36:10PM +0200, Marcel Apfelbaum wrote:
> On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> >We can use PC_MACHINE(qdev_get_machine())->acpi_guest_info to get
> >guest_info.
> 
> Hi Eduardo,
> 
> I like the idea of using qdev_get_machine() to get the machine instead of
> keeping the reference around, however only in places we cannot get a reference
> to it otherwise.

I was unsure if I should go to the extreme of removing all the
guest_info parameters, when doing that. I agree with your
suggestions, but v3 will have to wait until I get back from
vacations in Jan 18th. At least we will have less conflicts with
Igor's AML series. :)

Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 07/14] acpi: Remove guest_info parameters from functions

[Marcel Apfelbaum]

On 12/18/2015 08:08 PM, Eduardo Habkost wrote:
> On Tue, Dec 15, 2015 at 02:36:10PM +0200, Marcel Apfelbaum wrote:
>> On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
>>> We can use PC_MACHINE(qdev_get_machine())->acpi_guest_info to get
>>> guest_info.
>>
>> Hi Eduardo,
>>
>> I like the idea of using qdev_get_machine() to get the machine instead of
>> keeping the reference around, however only in places we cannot get a reference
>> to it otherwise.
>
> I was unsure if I should go to the extreme of removing all the
> guest_info parameters, when doing that. I agree with your
> suggestions, but v3 will have to wait until I get back from
> vacations in Jan 18th. At least we will have less conflicts with
> Igor's AML series. :)

Sure, and have fun in your vacation! :)
Marcel

>
> Thanks!
>

[Qemu-devel] [PATCH v2 08/14] acpi: Don't save PcGuestInfo on AcpiBuildState

[Eduardo Habkost]

We don't need to save the pointer on AcpiBuildState, as it is not
used anymore.

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 92d25c9..82d55bb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1648,7 +1648,6 @@ struct AcpiBuildState {
     MemoryRegion *table_mr;
     /* Is table patched? */
     uint8_t patched;
-    PcGuestInfo *guest_info;
     void *rsdp;
     MemoryRegion *rsdp_mr;
     MemoryRegion *linker_mr;
@@ -1922,8 +1921,6 @@ void acpi_setup(void)
 
     build_state = g_malloc0(sizeof *build_state);
 
-    build_state->guest_info = guest_info;
-
     acpi_set_pci_info();
 
     acpi_build_tables_init(&tables);
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 08/14] acpi: Don't save PcGuestInfo on AcpiBuildState

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> We don't need to save the pointer on AcpiBuildState, as it is not
> used anymore.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/acpi-build.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 92d25c9..82d55bb 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1648,7 +1648,6 @@ struct AcpiBuildState {
>       MemoryRegion *table_mr;
>       /* Is table patched? */
>       uint8_t patched;
> -    PcGuestInfo *guest_info;
>       void *rsdp;
>       MemoryRegion *rsdp_mr;
>       MemoryRegion *linker_mr;
> @@ -1922,8 +1921,6 @@ void acpi_setup(void)
>
>       build_state = g_malloc0(sizeof *build_state);
>
> -    build_state->guest_info = guest_info;
> -
>       acpi_set_pci_info();
>
>       acpi_build_tables_init(&tables);
>

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

[Qemu-devel] [PATCH v2 09/14] pc: Remove compat fields from PcGuestInfo

[Eduardo Habkost]

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

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 82d55bb..46b83ad 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1689,6 +1689,7 @@ static
 void acpi_build(AcpiBuildTables *tables)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     GArray *table_offsets;
     unsigned facs, ssdt, dsdt, rsdt;
@@ -1802,12 +1803,12 @@ void acpi_build(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,
@@ -1900,6 +1901,7 @@ static const VMStateDescription vmstate_acpi_build = {
 void acpi_setup(void)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
@@ -1909,7 +1911,7 @@ void acpi_setup(void)
         return;
     }
 
-    if (!guest_info->has_acpi_build) {
+    if (!pcmc->has_acpi_build) {
         ACPI_BUILD_DPRINTF("ACPI build disabled. Bailing out.\n");
         return;
     }
@@ -1938,7 +1940,7 @@ void acpi_setup(void)
     fw_cfg_add_file(guest_info->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 14c0116..f8d4531 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1316,7 +1316,7 @@ void 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);
@@ -1327,7 +1327,7 @@ void 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;
@@ -1382,7 +1382,7 @@ void 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 f39c086..fe00086 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -142,12 +142,7 @@ static void pc_init1(MachineState *machine,
 
     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->isapc_ram_fw = !pcmc->pci_enabled;
-    guest_info->has_reserved_memory = pcmc->has_reserved_memory;
-    guest_info->rsdp_in_ram = pcmc->rsdp_in_ram;
 
     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 bb1436e..1f29943 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -133,14 +133,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;
-
-    /* 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;
 
     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 24362ef..d24682f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -30,10 +30,6 @@ struct PcGuestInfo {
     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;
 };
 
 /**
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 09/14] pc: Remove compat fields from PcGuestInfo

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> Remove the fields: legacy_acpi_table_size, has_acpi_build,
> has_reserved_memory, and rsdp_in_ram from PcGuestInfo, and let
> the existing code use the PCMachineClass fields directly.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/acpi-build.c | 10 ++++++----
>   hw/i386/pc.c         |  6 +++---
>   hw/i386/pc_piix.c    |  5 -----
>   hw/i386/pc_q35.c     |  8 --------
>   include/hw/i386/pc.h |  4 ----
>   5 files changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 82d55bb..46b83ad 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1689,6 +1689,7 @@ static
>   void acpi_build(AcpiBuildTables *tables)
>   {
>       PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>       PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       GArray *table_offsets;
>       unsigned facs, ssdt, dsdt, rsdt;
> @@ -1802,12 +1803,12 @@ void acpi_build(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,
> @@ -1900,6 +1901,7 @@ static const VMStateDescription vmstate_acpi_build = {
>   void acpi_setup(void)
>   {
>       PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>       PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       AcpiBuildTables tables;
>       AcpiBuildState *build_state;
> @@ -1909,7 +1911,7 @@ void acpi_setup(void)
>           return;
>       }
>
> -    if (!guest_info->has_acpi_build) {
> +    if (!pcmc->has_acpi_build) {
>           ACPI_BUILD_DPRINTF("ACPI build disabled. Bailing out.\n");
>           return;
>       }
> @@ -1938,7 +1940,7 @@ void acpi_setup(void)
>       fw_cfg_add_file(guest_info->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 14c0116..f8d4531 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1316,7 +1316,7 @@ void 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);
> @@ -1327,7 +1327,7 @@ void 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;
> @@ -1382,7 +1382,7 @@ void 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 f39c086..fe00086 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -142,12 +142,7 @@ static void pc_init1(MachineState *machine,
>
>       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->isapc_ram_fw = !pcmc->pci_enabled;
> -    guest_info->has_reserved_memory = pcmc->has_reserved_memory;
> -    guest_info->rsdp_in_ram = pcmc->rsdp_in_ram;
>
>       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 bb1436e..1f29943 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -133,14 +133,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;
> -
> -    /* 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;
>
>       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 24362ef..d24682f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -30,10 +30,6 @@ struct PcGuestInfo {
>       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;
>   };
>
>   /**
>

Looks OK to me

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

[Qemu-devel] [PATCH v2 10/14] pc: Remove RAM size fields from 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 46b83ad..9598eac 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1509,17 +1509,17 @@ build_srat(GArray *table_data, GArray *linker)
         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 f8d4531..998fe4e 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 d24682f..30c7a5b 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

Re: [Qemu-devel] [PATCH v2 10/14] pc: Remove RAM size fields from PcGuestInfo

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> 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 46b83ad..9598eac 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1509,17 +1509,17 @@ build_srat(GArray *table_data, GArray *linker)
>           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 f8d4531..998fe4e 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 d24682f..30c7a5b 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;

I understand the ram_size_below_4g change.
So nobody was using PcGuestInfo->ram_size?
Since this is probably true:

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


>       unsigned apic_id_limit;
>       bool apic_xrupt_override;
>       uint64_t numa_nodes;
>

[Qemu-devel] [PATCH v2 11/14] 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    | 5 +----
 hw/i386/pc_q35.c     | 4 +---
 include/hw/i386/pc.h | 1 -
 4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 998fe4e..02d0e19 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1365,7 +1365,7 @@ void 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 fe00086..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,9 +139,7 @@ static void pc_init1(MachineState *machine,
         rom_memory = system_memory;
     }
 
-    guest_info = pc_guest_info_init(pcms);
-
-    guest_info->isapc_ram_fw = !pcmc->pci_enabled;
+    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 1f29943..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,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
         rom_memory = get_system_memory();
     }
 
-    guest_info = pc_guest_info_init(pcms);
-    guest_info->isapc_ram_fw = false;
+    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 30c7a5b..20a425c 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

Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> 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    | 5 +----
>   hw/i386/pc_q35.c     | 4 +---
>   include/hw/i386/pc.h | 1 -
>   4 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 998fe4e..02d0e19 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1365,7 +1365,7 @@ void 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 fe00086..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,9 +139,7 @@ static void pc_init1(MachineState *machine,
>           rom_memory = system_memory;
>       }
>
> -    guest_info = pc_guest_info_init(pcms);
> -
> -    guest_info->isapc_ram_fw = !pcmc->pci_enabled;
> +    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 1f29943..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,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
>           rom_memory = get_system_memory();
>       }
>
> -    guest_info = pc_guest_info_init(pcms);
> -    guest_info->isapc_ram_fw = false;

This may not be an issue, I just want be sure.
For Q35 isapc_ram_fw is always false, but now we are always querying
!pcmc->pci_enabled.

Now we have a Q35 case when !pcmc->pci_enabled *can* be true.

Do we need to care?

Thanks,
Marcel

> +    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 30c7a5b..20a425c 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;
>

Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

[Eduardo Habkost]

On Tue, Dec 15, 2015 at 04:27:51PM +0200, Marcel Apfelbaum wrote:
> On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
[...]
> >@@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
> >          rom_memory = get_system_memory();
> >      }
> >
> >-    guest_info = pc_guest_info_init(pcms);
> >-    guest_info->isapc_ram_fw = false;
> 
> This may not be an issue, I just want be sure.
> For Q35 isapc_ram_fw is always false, but now we are always querying
> !pcmc->pci_enabled.
> 
> Now we have a Q35 case when !pcmc->pci_enabled *can* be true.

Do we? pcmc->pci_enabled is always true in Q35.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

[Marcel Apfelbaum]

On 12/16/2015 09:48 PM, Eduardo Habkost wrote:
> On Tue, Dec 15, 2015 at 04:27:51PM +0200, Marcel Apfelbaum wrote:
>> On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> [...]
>>> @@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
>>>           rom_memory = get_system_memory();
>>>       }
>>>
>>> -    guest_info = pc_guest_info_init(pcms);
>>> -    guest_info->isapc_ram_fw = false;
>>
>> This may not be an issue, I just want be sure.
>> For Q35 isapc_ram_fw is always false, but now we are always querying
>> !pcmc->pci_enabled.
>>
>> Now we have a Q35 case when !pcmc->pci_enabled *can* be true.
>
> Do we? pcmc->pci_enabled is always true in Q35.

OK, thanks, so all the pcmc->pci_enabled "if" clauses in pc_q35_init are not necessary, right?
Anyway, this is no connected tot his patch.


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

Thanks,
Marcel

>

Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

[Eduardo Habkost]

On Thu, Dec 17, 2015 at 11:40:39AM +0200, Marcel Apfelbaum wrote:
> On 12/16/2015 09:48 PM, Eduardo Habkost wrote:
> >On Tue, Dec 15, 2015 at 04:27:51PM +0200, Marcel Apfelbaum wrote:
> >>On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> >[...]
> >>>@@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
> >>>          rom_memory = get_system_memory();
> >>>      }
> >>>
> >>>-    guest_info = pc_guest_info_init(pcms);
> >>>-    guest_info->isapc_ram_fw = false;
> >>
> >>This may not be an issue, I just want be sure.
> >>For Q35 isapc_ram_fw is always false, but now we are always querying
> >>!pcmc->pci_enabled.
> >>
> >>Now we have a Q35 case when !pcmc->pci_enabled *can* be true.
> >
> >Do we? pcmc->pci_enabled is always true in Q35.
> 
> OK, thanks, so all the pcmc->pci_enabled "if" clauses in
> pc_q35_init are not necessary, right?

They are not necessary, but keeping them helps us identify
duplicate code in pc_q35.c:pc_q35_init() and pc_piix.c:pc_init1()
more easily (so we can move it to common code later).

-- 
Eduardo

[Qemu-devel] [PATCH v2 12/14] pc: Move PcGuestInfo.fw_cfg to PCMachineState

[Eduardo Habkost]

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9598eac..43d8166 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1902,11 +1902,10 @@ void acpi_setup(void)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
 
-    if (!guest_info->fw_cfg) {
+    if (!pcms->fw_cfg) {
         ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
         return;
     }
@@ -1937,7 +1936,7 @@ void acpi_setup(void)
     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 (!pcmc->rsdp_in_ram) {
@@ -1949,7 +1948,7 @@ void acpi_setup(void)
         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 02d0e19..6d8ea76 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));
         }
     }
@@ -1257,7 +1257,6 @@ void xen_load_linux(PCMachineState *pcms)
 {
     int i;
     FWCfgState *fw_cfg;
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
 
     assert(MACHINE(pcms)->kernel_filename != NULL);
 
@@ -1270,7 +1269,7 @@ void 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;
 }
 
 void pc_memory_init(PCMachineState *pcms,
@@ -1278,7 +1277,6 @@ void 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;
@@ -1399,7 +1397,7 @@ void 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;
 }
 
 qemu_irq pc_allocate_cpu_irq(void)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 20a425c..a80adc6 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -27,7 +27,6 @@ struct PcGuestInfo {
     uint64_t numa_nodes;
     uint64_t *node_mem;
     uint64_t *node_cpu;
-    FWCfgState *fw_cfg;
 };
 
 /**
@@ -49,6 +48,7 @@ struct PCMachineState {
     HotplugHandler *acpi_dev;
     ISADevice *rtc;
     PCIBus *bus;
+    FWCfgState *fw_cfg;
 
     /* Configuration options: */
     uint64_t max_ram_below_4g;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 12/14] pc: Move PcGuestInfo.fw_cfg to PCMachineState

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/acpi-build.c |  7 +++----
>   hw/i386/pc.c         | 10 ++++------
>   include/hw/i386/pc.h |  2 +-
>   3 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9598eac..43d8166 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1902,11 +1902,10 @@ void acpi_setup(void)
>   {
>       PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>       PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> -    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       AcpiBuildTables tables;
>       AcpiBuildState *build_state;
>
> -    if (!guest_info->fw_cfg) {
> +    if (!pcms->fw_cfg) {
>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>           return;
>       }
> @@ -1937,7 +1936,7 @@ void acpi_setup(void)
>       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 (!pcmc->rsdp_in_ram) {
> @@ -1949,7 +1948,7 @@ void acpi_setup(void)
>           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 02d0e19..6d8ea76 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));
>           }
>       }
> @@ -1257,7 +1257,6 @@ void xen_load_linux(PCMachineState *pcms)
>   {
>       int i;
>       FWCfgState *fw_cfg;
> -    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>
>       assert(MACHINE(pcms)->kernel_filename != NULL);
>
> @@ -1270,7 +1269,7 @@ void 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;
>   }
>
>   void pc_memory_init(PCMachineState *pcms,
> @@ -1278,7 +1277,6 @@ void 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;
> @@ -1399,7 +1397,7 @@ void 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;
>   }
>
>   qemu_irq pc_allocate_cpu_irq(void)
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 20a425c..a80adc6 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -27,7 +27,6 @@ struct PcGuestInfo {
>       uint64_t numa_nodes;
>       uint64_t *node_mem;
>       uint64_t *node_cpu;
> -    FWCfgState *fw_cfg;
>   };
>
>   /**
> @@ -49,6 +48,7 @@ struct PCMachineState {
>       HotplugHandler *acpi_dev;
>       ISADevice *rtc;
>       PCIBus *bus;
> +    FWCfgState *fw_cfg;
>
>       /* Configuration options: */
>       uint64_t max_ram_below_4g;
>

Looks OK to me.

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

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

[Eduardo Habkost]

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 43d8166..a1f7ea0 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 = PC_MACHINE(qdev_get_machine());
-    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);
@@ -401,7 +400,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);
@@ -933,9 +932,8 @@ build_ssdt(GArray *table_data, GArray *linker,
 {
     MachineState *machine = MACHINE(qdev_get_machine());
     PCMachineState *pcms = PC_MACHINE(machine);
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     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);
@@ -1467,7 +1465,6 @@ build_srat(GArray *table_data, GArray *linker)
     int srat_start, numa_start, slots;
     uint64_t mem_len, mem_base, next_base;
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     ram_addr_t hotplugabble_address_space_size =
         object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
                                 NULL);
@@ -1478,12 +1475,12 @@ build_srat(GArray *table_data, GArray *linker)
     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;
@@ -1500,9 +1497,9 @@ build_srat(GArray *table_data, GArray *linker)
     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;
         }
@@ -1526,7 +1523,7 @@ build_srat(GArray *table_data, GArray *linker)
                                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);
     }
@@ -1690,7 +1687,6 @@ void acpi_build(AcpiBuildTables *tables)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     GArray *table_offsets;
     unsigned facs, ssdt, dsdt, rsdt;
     AcpiCpuInfo cpu;
@@ -1758,7 +1754,7 @@ void acpi_build(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);
     }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6d8ea76..43a25a0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1187,24 +1187,24 @@ PcGuestInfo *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);
-    guest_info->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->apic_id_limit = pc_apic_id_limit(max_cpus);
+    pcms->apic_xrupt_override = kvm_allows_irq0_override();
+    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;
             }
         }
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index a80adc6..4b7f8f9 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -22,11 +22,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;
 };
 
 /**
@@ -57,6 +52,15 @@ struct PCMachineState {
 
     /* RAM information (sizes, addresses, configuration): */
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
+
+    /* CPU and apic information: */
+    bool apic_xrupt_override;
+    unsigned apic_id_limit;
+
+    /* NUMA information: */
+    uint64_t numa_nodes;
+    uint64_t *node_mem;
+    uint64_t *node_cpu;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 13/14] pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/acpi-build.c | 22 +++++++++-------------
>   hw/i386/pc.c         | 20 ++++++++++----------
>   include/hw/i386/pc.h | 14 +++++++++-----
>   3 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 43d8166..a1f7ea0 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 = PC_MACHINE(qdev_get_machine());
> -    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);
> @@ -401,7 +400,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);
> @@ -933,9 +932,8 @@ build_ssdt(GArray *table_data, GArray *linker,
>   {
>       MachineState *machine = MACHINE(qdev_get_machine());
>       PCMachineState *pcms = PC_MACHINE(machine);
> -    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       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);
> @@ -1467,7 +1465,6 @@ build_srat(GArray *table_data, GArray *linker)
>       int srat_start, numa_start, slots;
>       uint64_t mem_len, mem_base, next_base;
>       PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> -    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       ram_addr_t hotplugabble_address_space_size =
>           object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
>                                   NULL);
> @@ -1478,12 +1475,12 @@ build_srat(GArray *table_data, GArray *linker)
>       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;
> @@ -1500,9 +1497,9 @@ build_srat(GArray *table_data, GArray *linker)
>       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;
>           }
> @@ -1526,7 +1523,7 @@ build_srat(GArray *table_data, GArray *linker)
>                                  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);
>       }
> @@ -1690,7 +1687,6 @@ void acpi_build(AcpiBuildTables *tables)
>   {
>       PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>       PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> -    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       GArray *table_offsets;
>       unsigned facs, ssdt, dsdt, rsdt;
>       AcpiCpuInfo cpu;
> @@ -1758,7 +1754,7 @@ void acpi_build(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);
>       }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6d8ea76..43a25a0 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1187,24 +1187,24 @@ PcGuestInfo *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);
> -    guest_info->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->apic_id_limit = pc_apic_id_limit(max_cpus);
> +    pcms->apic_xrupt_override = kvm_allows_irq0_override();
> +    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;
>               }
>           }
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index a80adc6..4b7f8f9 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -22,11 +22,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;
>   };
>
>   /**
> @@ -57,6 +52,15 @@ struct PCMachineState {
>
>       /* RAM information (sizes, addresses, configuration): */
>       ram_addr_t below_4g_mem_size, above_4g_mem_size;
> +
> +    /* CPU and apic information: */
> +    bool apic_xrupt_override;
> +    unsigned apic_id_limit;
> +
> +    /* NUMA information: */
> +    uint64_t numa_nodes;
> +    uint64_t *node_mem;
> +    uint64_t *node_cpu;
>   };
>
>   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
>

A clean re-factoring.

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

[Qemu-devel] [PATCH v2 14/14] pc: Eliminate PcGuestInfo struct

[Eduardo Habkost]

The struct is not used for anything, now.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 43a25a0..f78b877 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1182,9 +1182,8 @@ void pc_machine_done(Notifier *notifier, void *data)
     acpi_setup();
 }
 
-PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
+void pc_guest_info_init(PCMachineState *pcms)
 {
-    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
     int i, j;
 
     pcms->apic_id_limit = pc_apic_id_limit(max_cpus);
@@ -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 */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 4b7f8f9..7243774 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -20,10 +20,6 @@
 
 #define HPET_INTCAP "hpet-intcap"
 
-/* Machine info for ACPI build: */
-struct PcGuestInfo {
-};
-
 /**
  * PCMachineState:
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
@@ -36,7 +32,6 @@ struct PCMachineState {
 
     /* State for other subsystems/APIs: */
     MemoryHotplugState hotplug_memory;
-    PcGuestInfo acpi_guest_info;
     Notifier machine_done;
 
     /* Pointers to devices and objects: */
@@ -215,7 +210,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 v2 14/14] pc: Eliminate PcGuestInfo struct

[Marcel Apfelbaum]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> The struct is not used for anything, now.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/pc.c         | 4 +---
>   include/hw/i386/pc.h | 7 +------
>   2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 43a25a0..f78b877 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1182,9 +1182,8 @@ void pc_machine_done(Notifier *notifier, void *data)
>       acpi_setup();
>   }
>
> -PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> +void pc_guest_info_init(PCMachineState *pcms)

We don't have a PCGuestInfo anymore :) so maybe we can change the function name
to something more meaningful (it does not init the PCGuestInfo anymore...).
Not that I have an idea for the name.

Thanks for the patches!
I hope I helped,
Marcel

>   {
> -    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       int i, j;
>
>       pcms->apic_id_limit = pc_apic_id_limit(max_cpus);
> @@ -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 */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 4b7f8f9..7243774 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -20,10 +20,6 @@
>
>   #define HPET_INTCAP "hpet-intcap"
>
> -/* Machine info for ACPI build: */
> -struct PcGuestInfo {
> -};
> -
>   /**
>    * PCMachineState:
>    * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> @@ -36,7 +32,6 @@ struct PCMachineState {
>
>       /* State for other subsystems/APIs: */
>       MemoryHotplugState hotplug_memory;
> -    PcGuestInfo acpi_guest_info;
>       Notifier machine_done;
>
>       /* Pointers to devices and objects: */
> @@ -215,7 +210,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"
>