Re: [Qemu-devel] [PATCH v3 1/6] pc: Move compat boolean globals to PCMachineClass

[Marcel Apfelbaum <marcel@redhat.com>]

On 12/02/2015 11:57 AM, Marcel Apfelbaum wrote:
> On 12/02/2015 12:58 AM, Eduardo Habkost wrote:
>> This way the compat flags can be initialized in the machine_options()
>> function. This will help us to eventually eliminate the pc_compat_*()
>> functions.
>
> Hi, I have only a minor comment here,
>
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>   hw/i386/pc.c         |  8 +++++
>>   hw/i386/pc_piix.c    | 84 +++++++++++++++++++++++++---------------------------
>>   hw/i386/pc_q35.c     | 54 +++++++++++++++------------------
>>   include/hw/i386/pc.h | 14 +++++++++
>>   4 files changed, 86 insertions(+), 74 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 5e20e07..129aa04 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1952,6 +1952,14 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>       HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>>
>>       pcmc->get_hotplug_handler = mc->get_hotplug_handler;
>> +    pcmc->pci_enabled = true;
>> +    pcmc->has_acpi_build = true;
>> +    pcmc->rsdp_in_ram = true;
>> +    pcmc->smbios_defaults = true;
>> +    pcmc->smbios_uuid_encoded = true;
>> +    pcmc->gigabyte_align = true;
>> +    pcmc->has_reserved_memory = true;
>> +    pcmc->kvmclock_enabled = true;
>>       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 2e41efe..7a7f748 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -60,26 +60,14 @@ static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
>>   static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
>>   static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
>>
>> -static bool pci_enabled = true;
>> -static bool has_acpi_build = true;
>> -static bool rsdp_in_ram = true;
>>   static int legacy_acpi_table_size;
>> -static bool smbios_defaults = true;
>> -static bool smbios_legacy_mode;
>> -static bool smbios_uuid_encoded = true;
>> -/* 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.
>> - */
>> -static bool gigabyte_align = true;
>> -static bool has_reserved_memory = true;
>> -static bool kvmclock_enabled = true;
>>
>>   /* PC hardware initialisation */
>>   static void pc_init1(MachineState *machine,
>>                        const char *host_type, const char *pci_type)
>>   {
>>       PCMachineState *pcms = PC_MACHINE(machine);
>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>>       MemoryRegion *system_memory = get_system_memory();
>>       MemoryRegion *system_io = get_system_io();
>>       int i;
>> @@ -108,7 +96,7 @@ static void pc_init1(MachineState *machine,
>>        * breaking migration.
>>        */
>>       if (machine->ram_size >= 0xe0000000) {
>> -        lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000;
>> +        lowmem = pcmc->gigabyte_align ? 0xc0000000 : 0xe0000000;
>>       } else {
>>           lowmem = 0xe0000000;
>>       }
>> @@ -141,11 +129,11 @@ static void pc_init1(MachineState *machine,
>>
>>       pc_cpus_init(pcms);
>>
>> -    if (kvm_enabled() && kvmclock_enabled) {
>> +    if (kvm_enabled() && pcmc->kvmclock_enabled) {
>>           kvmclock_create();
>>       }
>>
>> -    if (pci_enabled) {
>> +    if (pcmc->pci_enabled) {
>>           pci_memory = g_new(MemoryRegion, 1);
>>           memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>>           rom_memory = pci_memory;
>> @@ -156,18 +144,19 @@ static void pc_init1(MachineState *machine,
>>
>>       guest_info = pc_guest_info_init(pcms);
>>
>> -    guest_info->has_acpi_build = has_acpi_build;
>> +    guest_info->has_acpi_build = pcmc->has_acpi_build;
>>       guest_info->legacy_acpi_table_size = legacy_acpi_table_size;
>
> Why is legacy_acpi_table_size left behind? Maybe it is a new field.

Forget about it, the answer is the next patch :)

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

Thanks,
Marcel