Re: [Qemu-devel] [PATCH v3 3/6] pc: Move acpi_data_size global to PCMachineClass

[Marcel Apfelbaum <marcel@redhat.com>]

On 12/02/2015 12:58 AM, Eduardo Habkost wrote:
> This way we don't need code in pc_compat_*() functions to set the legacy
> acpi_data_size value.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/pc.c         | 17 ++++++-----------
>   hw/i386/pc_piix.c    |  2 +-
>   hw/i386/pc_q35.c     |  2 +-
>   include/hw/i386/pc.h |  6 ++++--
>   4 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 129aa04..a79b8af 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -76,15 +76,6 @@
>   #define DPRINTF(fmt, ...)
>   #endif
>
> -/* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables
> - * (128K) and other BIOS datastructures (less than 4K reported to be used at
> - * the moment, 32K should be enough for a while).  */
> -static unsigned acpi_data_size = 0x20000 + 0x8000;
> -void pc_set_legacy_acpi_data_size(void)
> -{
> -    acpi_data_size = 0x10000;
> -}
> -
>   #define BIOS_CFG_IOPORT 0x510
>   #define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
>   #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
> @@ -840,6 +831,7 @@ static void load_linux(PCMachineState *pcms,
>       FILE *f;
>       char *vmode;
>       MachineState *machine = MACHINE(pcms);
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>       const char *kernel_filename = machine->kernel_filename;
>       const char *initrd_filename = machine->initrd_filename;
>       const char *kernel_cmdline = machine->kernel_cmdline;
> @@ -907,8 +899,8 @@ static void load_linux(PCMachineState *pcms,
>           initrd_max = 0x37ffffff;
>       }
>
> -    if (initrd_max >= pcms->below_4g_mem_size - acpi_data_size) {
> -        initrd_max = pcms->below_4g_mem_size - acpi_data_size - 1;
> +    if (initrd_max >= pcms->below_4g_mem_size - pcmc->acpi_data_size) {
> +        initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
>       }
>
>       fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
> @@ -1960,6 +1952,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>       pcmc->gigabyte_align = true;
>       pcmc->has_reserved_memory = true;
>       pcmc->kvmclock_enabled = true;
> +    /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported
> +     * to be used at the moment, 32K should be enough for a while.  */
> +    pcmc->acpi_data_size = 0x20000 + 0x8000;
>       mc->get_hotplug_handler = pc_get_hotpug_handler;
>       mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
>       mc->default_boot_order = "cad";
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 5f23e9c..3562888 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -317,7 +317,6 @@ static void pc_compat_2_1(MachineState *machine)
>   static void pc_compat_2_0(MachineState *machine)
>   {
>       pc_compat_2_1(machine);
> -    pc_set_legacy_acpi_data_size();
>   }
>
>   static void pc_compat_1_7(MachineState *machine)
> @@ -516,6 +515,7 @@ static void pc_i440fx_2_0_machine_options(MachineClass *m)
>        * QEMU 1.7 it is 6414.  For RHEL/CentOS 7.0 it is 6418.
>        */
>       pcmc->legacy_acpi_table_size = 6652;
> +    pcmc->acpi_data_size = 0x10000;
>   }
>
>   DEFINE_I440FX_MACHINE(v2_0, "pc-i440fx-2.0", pc_compat_2_0,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index f9afaa2..43e77f4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -302,7 +302,6 @@ static void pc_compat_2_1(MachineState *machine)
>   static void pc_compat_2_0(MachineState *machine)
>   {
>       pc_compat_2_1(machine);
> -    pc_set_legacy_acpi_data_size();
>   }
>
>   static void pc_compat_1_7(MachineState *machine)
> @@ -424,6 +423,7 @@ static void pc_q35_2_0_machine_options(MachineClass *m)
>       SET_MACHINE_COMPAT(m, PC_COMPAT_2_0);
>       pcmc->has_reserved_memory = false;
>       pcmc->smbios_legacy_mode = true;
> +    pcmc->acpi_data_size = 0x10000;
>   }
>
>   DEFINE_Q35_MACHINE(v2_0, "pc-q35-2.0", pc_compat_2_0,
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 7c2cc9f..e604ca5 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -77,6 +77,10 @@ struct PCMachineClass {
>       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;
>   };
>
>   #define TYPE_PC_MACHINE "generic-pc-machine"
> @@ -188,8 +192,6 @@ void pc_acpi_init(const char *default_dsdt);
>
>   PcGuestInfo *pc_guest_info_init(PCMachineState *pcms);
>
> -void pc_set_legacy_acpi_data_size(void);
> -
>   #define PCI_HOST_PROP_PCI_HOLE_START   "pci-hole-start"
>   #define PCI_HOST_PROP_PCI_HOLE_END     "pci-hole-end"
>   #define PCI_HOST_PROP_PCI_HOLE64_START "pci-hole64-start"
>

Looks OK to me.

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

Thanks,
Marcel