From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50524) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aa7h0-0003EG-4D for qemu-devel@nongnu.org; Sun, 28 Feb 2016 15:11:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aa7gu-0003vZ-Vg for qemu-devel@nongnu.org; Sun, 28 Feb 2016 15:11:50 -0500 Received: from mail-wm0-x243.google.com ([2a00:1450:400c:c09::243]:36119) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aa7gu-0003vT-Kg for qemu-devel@nongnu.org; Sun, 28 Feb 2016 15:11:44 -0500 Received: by mail-wm0-x243.google.com with SMTP id l68so5095436wml.3 for ; Sun, 28 Feb 2016 12:11:44 -0800 (PST) References: <1456495168-144510-1-git-send-email-imammedo@redhat.com> <1456495168-144510-3-git-send-email-imammedo@redhat.com> From: Marcel Apfelbaum Message-ID: <56D3547D.3@gmail.com> Date: Sun, 28 Feb 2016 22:11:41 +0200 MIME-Version: 1.0 In-Reply-To: <1456495168-144510-3-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 02/10] pc: init pcms->apic_id_limit once and use it throughout pc.c Reply-To: marcel@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov , qemu-devel@nongnu.org Cc: marcel@redhat.com, ehabkost@redhat.com, mst@redhat.com On 02/26/2016 03:59 PM, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov > --- > hw/i386/pc.c | 45 +++++++++++++++++++-------------------------- > 1 file changed, 19 insertions(+), 26 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 0aeefd2..151a64c 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -700,18 +700,6 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index) > } > } > > -/* Calculates the limit to CPU APIC ID values > - * > - * This function returns the limit for the APIC ID value, so that all > - * CPU APIC IDs are < pc_apic_id_limit(). > - * > - * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init(). > - */ > -static unsigned int pc_apic_id_limit(unsigned int max_cpus) > -{ > - return x86_cpu_apic_id_from_index(max_cpus - 1) + 1; > -} > - > static void pc_build_smbios(FWCfgState *fw_cfg) > { > uint8_t *smbios_tables, *smbios_anchor; > @@ -749,12 +737,11 @@ static void pc_build_smbios(FWCfgState *fw_cfg) > } > } > > -static FWCfgState *bochs_bios_init(AddressSpace *as) > +static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms) > { > FWCfgState *fw_cfg; > uint64_t *numa_fw_cfg; > int i, j; > - unsigned int apic_id_limit = pc_apic_id_limit(max_cpus); > > fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as); > > @@ -772,7 +759,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as) > * [1] The only kind of "CPU identifier" used between SeaBIOS and QEMU is > * the APIC ID, not the "CPU index" > */ > - fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)apic_id_limit); > + fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)pcms->apic_id_limit); > fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size); > fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, > acpi_tables, acpi_tables_len); > @@ -790,11 +777,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as) > * of nodes, one word for each VCPU->node and one word for each node to > * hold the amount of memory. > */ > - numa_fw_cfg = g_new0(uint64_t, 1 + apic_id_limit + nb_numa_nodes); > + numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes); > numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes); > for (i = 0; i < max_cpus; i++) { > unsigned int apic_id = x86_cpu_apic_id_from_index(i); > - assert(apic_id < 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)) { > numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); > @@ -803,10 +790,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as) > } > } > for (i = 0; i < nb_numa_nodes; i++) { > - numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(numa_info[i].node_mem); > + numa_fw_cfg[pcms->apic_id_limit + 1 + i] = > + cpu_to_le64(numa_info[i].node_mem); > } > fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg, > - (1 + apic_id_limit + nb_numa_nodes) * > + (1 + pcms->apic_id_limit + nb_numa_nodes) * > sizeof(*numa_fw_cfg)); > > return fw_cfg; > @@ -1120,7 +1108,6 @@ void pc_cpus_init(PCMachineState *pcms) > int i; > X86CPU *cpu = NULL; > MachineState *machine = MACHINE(pcms); > - unsigned long apic_id_limit; > > /* init CPUs */ > if (machine->cpu_model == NULL) { > @@ -1131,10 +1118,17 @@ void pc_cpus_init(PCMachineState *pcms) > #endif > } > > - apic_id_limit = pc_apic_id_limit(max_cpus); > - if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) { > - error_report("max_cpus is too large. APIC ID of last CPU is %lu", > - apic_id_limit - 1); > + /* Calculates the limit to CPU APIC ID values > + * > + * Limit for the APIC ID value, so that all > + * CPU APIC IDs are < pcms->apic_id_limit. > + * > + * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init(). > + */ > + pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1; > + if (pcms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) { > + error_report("max_cpus is too large. APIC ID of last CPU is %u", > + pcms->apic_id_limit - 1); > exit(1); > } > > @@ -1187,7 +1181,6 @@ void pc_guest_info_init(PCMachineState *pcms) > { > int i, j; > > - 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 * > @@ -1372,7 +1365,7 @@ void pc_memory_init(PCMachineState *pcms, > option_rom_mr, > 1); > > - fw_cfg = bochs_bios_init(&address_space_memory); > + fw_cfg = bochs_bios_init(&address_space_memory, pcms); > > rom_set_fw(fw_cfg); > > Looks OK to me. Reviewed-by: Marcel Apfelbaum Thanks, Marcel