| Message ID | 1358886309-26258-7-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 22.01.2013 21:25, schrieb Eduardo Habkost: > This changes FW_CFG_MAX_CPUS and FW_CFG_NUMA to use apic_id_for_cpu(), > so the NUMA table can be based on the APIC IDs, instead of CPU index > (SeaBIOS knows nothing about CPU indexes, just APIC IDs). > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Changes v2: > - Get PC object as argument > - Add more detailed comments explaining the reason for FW_CFG_MAX_CPUS > not being simply 'max_cpus' > > Changes v3: > - Use PCInitArgs instead of PC object > > Changes v4: > - Don't use PCInitArgs, just add the necessary data for apic_id_limit() > as argument > - Rename function to pc_apic_id_limit() > - Rename max_apic_id to apic_id_limit > > Changes v5: > - Refresh after apic_id_for_cpu() -> x86_cpu_apic_id_from_index() > rename > - Refresh after original code changes to use g_new0() > --- > hw/pc.c | 45 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 38 insertions(+), 7 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 44bb1dc..9029a55 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -551,6 +551,18 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) > return 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 *bochs_bios_init(void) > { > void *fw_cfg; > @@ -558,9 +570,24 @@ static void *bochs_bios_init(void) > size_t smbios_len; > uint64_t *numa_fw_cfg; > int i, j; > + unsigned int apic_id_limit = pc_apic_id_limit(max_cpus); > > fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0); > - fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus); > + /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86: > + * > + * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug > + * QEMU<->SeaBIOS interface is not based on the "CPU index", but on the APIC > + * ID of hotplugged CPUs[1]. This means that FW_CFG_MAX_CPUS is not the > + * "maximum number of CPUs", but the "limit to the APIC ID values SeaBIOS > + * may see". > + * > + * So, this means we must not use max_cpus, here, but the maximum possible > + * APIC ID value, plus one. > + * > + * [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_i32(fw_cfg, FW_CFG_ID, 1); > fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size); > fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, > @@ -579,21 +606,25 @@ static void *bochs_bios_init(void) > * 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 + max_cpus + nb_numa_nodes); > + numa_fw_cfg = g_new0(uint64_t, 1 + 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 cpu_idx; Beep. > + for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) { > + unsigned int apic_id = x86_cpu_apic_id_from_index(cpu_idx); > + assert(apic_id < apic_id_limit); > for (j = 0; j < nb_numa_nodes; j++) { > - if (test_bit(i, node_cpumask[j])) { > - numa_fw_cfg[i + 1] = cpu_to_le64(j); > + if (test_bit(cpu_idx, node_cpumask[j])) { > + numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); > break; > } > } > } Why can't we keep using i here? That would leave the "for (..." and "test_bit" lines unchanged and let us spot the actual changes of i vs. apic_id more easily. Andreas > for (i = 0; i < nb_numa_nodes; i++) { > - numa_fw_cfg[max_cpus + 1 + i] = cpu_to_le64(node_mem[i]); > + numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(node_mem[i]); > } > fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg, > - (1 + max_cpus + nb_numa_nodes) * sizeof(*numa_fw_cfg)); > + (1 + apic_id_limit + nb_numa_nodes) * > + sizeof(*numa_fw_cfg)); > > return fw_cfg; > } >
On Wed, Jan 23, 2013 at 05:33:25PM +0100, Andreas Färber wrote: > Am 22.01.2013 21:25, schrieb Eduardo Habkost: > > This changes FW_CFG_MAX_CPUS and FW_CFG_NUMA to use apic_id_for_cpu(), > > so the NUMA table can be based on the APIC IDs, instead of CPU index > > (SeaBIOS knows nothing about CPU indexes, just APIC IDs). > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > Changes v2: > > - Get PC object as argument > > - Add more detailed comments explaining the reason for FW_CFG_MAX_CPUS > > not being simply 'max_cpus' > > > > Changes v3: > > - Use PCInitArgs instead of PC object > > > > Changes v4: > > - Don't use PCInitArgs, just add the necessary data for apic_id_limit() > > as argument > > - Rename function to pc_apic_id_limit() > > - Rename max_apic_id to apic_id_limit > > > > Changes v5: > > - Refresh after apic_id_for_cpu() -> x86_cpu_apic_id_from_index() > > rename > > - Refresh after original code changes to use g_new0() > > --- > > hw/pc.c | 45 ++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 38 insertions(+), 7 deletions(-) > > > > diff --git a/hw/pc.c b/hw/pc.c > > index 44bb1dc..9029a55 100644 > > --- a/hw/pc.c > > +++ b/hw/pc.c > > @@ -551,6 +551,18 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) > > return 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 *bochs_bios_init(void) > > { > > void *fw_cfg; > > @@ -558,9 +570,24 @@ static void *bochs_bios_init(void) > > size_t smbios_len; > > uint64_t *numa_fw_cfg; > > int i, j; > > + unsigned int apic_id_limit = pc_apic_id_limit(max_cpus); > > > > fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0); > > - fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus); > > + /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86: > > + * > > + * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug > > + * QEMU<->SeaBIOS interface is not based on the "CPU index", but on the APIC > > + * ID of hotplugged CPUs[1]. This means that FW_CFG_MAX_CPUS is not the > > + * "maximum number of CPUs", but the "limit to the APIC ID values SeaBIOS > > + * may see". > > + * > > + * So, this means we must not use max_cpus, here, but the maximum possible > > + * APIC ID value, plus one. > > + * > > + * [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_i32(fw_cfg, FW_CFG_ID, 1); > > fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size); > > fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, > > @@ -579,21 +606,25 @@ static void *bochs_bios_init(void) > > * 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 + max_cpus + nb_numa_nodes); > > + numa_fw_cfg = g_new0(uint64_t, 1 + 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 cpu_idx; > > Beep. After so many rebases, I didn't even remember this variable declaration was here. But, what prevents us from declaring variables only when they are being used, in QEMU code? I didn't find anything on CODING_STYLE or HACKING. (I will move the declaration to the top of the file, anyway) > > > + for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) { > > + unsigned int apic_id = x86_cpu_apic_id_from_index(cpu_idx); > > + assert(apic_id < apic_id_limit); > > for (j = 0; j < nb_numa_nodes; j++) { > > - if (test_bit(i, node_cpumask[j])) { > > - numa_fw_cfg[i + 1] = cpu_to_le64(j); > > + if (test_bit(cpu_idx, node_cpumask[j])) { > > + numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); > > break; > > } > > } > > } > > Why can't we keep using i here? That would leave the "for (..." and > "test_bit" lines unchanged and let us spot the actual changes of i vs. > apic_id more easily. It would make the patch simpler, but at the cost of keeping variable names opaque for people reading the code in the future. I believe readable code is more important than making patches smaller.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Wed, Jan 23, 2013 at 05:33:25PM +0100, Andreas Färber wrote: >> Am 22.01.2013 21:25, schrieb Eduardo Habkost: [...] >> > + for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) { >> > + unsigned int apic_id = x86_cpu_apic_id_from_index(cpu_idx); >> > + assert(apic_id < apic_id_limit); >> > for (j = 0; j < nb_numa_nodes; j++) { >> > - if (test_bit(i, node_cpumask[j])) { >> > - numa_fw_cfg[i + 1] = cpu_to_le64(j); >> > + if (test_bit(cpu_idx, node_cpumask[j])) { >> > + numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); >> > break; >> > } >> > } >> > } >> >> Why can't we keep using i here? That would leave the "for (..." and >> "test_bit" lines unchanged and let us spot the actual changes of i vs. >> apic_id more easily. > > It would make the patch simpler, but at the cost of keeping variable > names opaque for people reading the code in the future. I believe > readable code is more important than making patches smaller. Both are important, and you can have both at the same time: put the rename in its own patch. Whether that's justified in this case is debatable.
On Wed, Jan 23, 2013 at 06:32:29PM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Wed, Jan 23, 2013 at 05:33:25PM +0100, Andreas Färber wrote: > >> Am 22.01.2013 21:25, schrieb Eduardo Habkost: > [...] > >> > + for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) { > >> > + unsigned int apic_id = x86_cpu_apic_id_from_index(cpu_idx); > >> > + assert(apic_id < apic_id_limit); > >> > for (j = 0; j < nb_numa_nodes; j++) { > >> > - if (test_bit(i, node_cpumask[j])) { > >> > - numa_fw_cfg[i + 1] = cpu_to_le64(j); > >> > + if (test_bit(cpu_idx, node_cpumask[j])) { > >> > + numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); > >> > break; > >> > } > >> > } > >> > } > >> > >> Why can't we keep using i here? That would leave the "for (..." and > >> "test_bit" lines unchanged and let us spot the actual changes of i vs. > >> apic_id more easily. > > > > It would make the patch simpler, but at the cost of keeping variable > > names opaque for people reading the code in the future. I believe > > readable code is more important than making patches smaller. > > Both are important, and you can have both at the same time: put the > rename in its own patch. Whether that's justified in this case is > debatable. I like making small patches and do everything in very small steps. But I learned that in practice this was making my patches take forever to be reviewed, and I often got questions like "why don't you squash this with the next/previous patch?" and "why are you touching this code that is going to be changed again later?". (In other words: it's true that we can have both, but that has a price as well).
On Wed, Jan 23, 2013 at 5:09 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Jan 23, 2013 at 05:33:25PM +0100, Andreas Färber wrote: >> Am 22.01.2013 21:25, schrieb Eduardo Habkost: >> > This changes FW_CFG_MAX_CPUS and FW_CFG_NUMA to use apic_id_for_cpu(), >> > so the NUMA table can be based on the APIC IDs, instead of CPU index >> > (SeaBIOS knows nothing about CPU indexes, just APIC IDs). >> > >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> > --- >> > Changes v2: >> > - Get PC object as argument >> > - Add more detailed comments explaining the reason for FW_CFG_MAX_CPUS >> > not being simply 'max_cpus' >> > >> > Changes v3: >> > - Use PCInitArgs instead of PC object >> > >> > Changes v4: >> > - Don't use PCInitArgs, just add the necessary data for apic_id_limit() >> > as argument >> > - Rename function to pc_apic_id_limit() >> > - Rename max_apic_id to apic_id_limit >> > >> > Changes v5: >> > - Refresh after apic_id_for_cpu() -> x86_cpu_apic_id_from_index() >> > rename >> > - Refresh after original code changes to use g_new0() >> > --- >> > hw/pc.c | 45 ++++++++++++++++++++++++++++++++++++++------- >> > 1 file changed, 38 insertions(+), 7 deletions(-) >> > >> > diff --git a/hw/pc.c b/hw/pc.c >> > index 44bb1dc..9029a55 100644 >> > --- a/hw/pc.c >> > +++ b/hw/pc.c >> > @@ -551,6 +551,18 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) >> > return 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 *bochs_bios_init(void) >> > { >> > void *fw_cfg; >> > @@ -558,9 +570,24 @@ static void *bochs_bios_init(void) >> > size_t smbios_len; >> > uint64_t *numa_fw_cfg; >> > int i, j; >> > + unsigned int apic_id_limit = pc_apic_id_limit(max_cpus); >> > >> > fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0); >> > - fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus); >> > + /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86: >> > + * >> > + * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug >> > + * QEMU<->SeaBIOS interface is not based on the "CPU index", but on the APIC >> > + * ID of hotplugged CPUs[1]. This means that FW_CFG_MAX_CPUS is not the >> > + * "maximum number of CPUs", but the "limit to the APIC ID values SeaBIOS >> > + * may see". >> > + * >> > + * So, this means we must not use max_cpus, here, but the maximum possible >> > + * APIC ID value, plus one. >> > + * >> > + * [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_i32(fw_cfg, FW_CFG_ID, 1); >> > fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size); >> > fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, >> > @@ -579,21 +606,25 @@ static void *bochs_bios_init(void) >> > * 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 + max_cpus + nb_numa_nodes); >> > + numa_fw_cfg = g_new0(uint64_t, 1 + 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 cpu_idx; >> >> Beep. > > After so many rebases, I didn't even remember this variable declaration > was here. > > But, what prevents us from declaring variables only when they are being > used, in QEMU code? I didn't find anything on CODING_STYLE or HACKING. Here's one explanation: http://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg03955.html Also IIRC Anthony gave a nice explanation once too but I can't find it now. > > (I will move the declaration to the top of the file, anyway) > >> >> > + for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) { >> > + unsigned int apic_id = x86_cpu_apic_id_from_index(cpu_idx); >> > + assert(apic_id < apic_id_limit); >> > for (j = 0; j < nb_numa_nodes; j++) { >> > - if (test_bit(i, node_cpumask[j])) { >> > - numa_fw_cfg[i + 1] = cpu_to_le64(j); >> > + if (test_bit(cpu_idx, node_cpumask[j])) { >> > + numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); >> > break; >> > } >> > } >> > } >> >> Why can't we keep using i here? That would leave the "for (..." and >> "test_bit" lines unchanged and let us spot the actual changes of i vs. >> apic_id more easily. > > It would make the patch simpler, but at the cost of keeping variable > names opaque for people reading the code in the future. I believe > readable code is more important than making patches smaller. > > -- > Eduardo >
On Fri, Jan 25, 2013 at 05:22:33PM +0000, Blue Swirl wrote: > On Wed, Jan 23, 2013 at 5:09 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Wed, Jan 23, 2013 at 05:33:25PM +0100, Andreas Färber wrote: [...] > >> > - numa_fw_cfg = g_new0(uint64_t, 1 + max_cpus + nb_numa_nodes); > >> > + numa_fw_cfg = g_new0(uint64_t, 1 + 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 cpu_idx; > >> > >> Beep. > > > > After so many rebases, I didn't even remember this variable declaration > > was here. > > > > But, what prevents us from declaring variables only when they are being > > used, in QEMU code? I didn't find anything on CODING_STYLE or HACKING. > > Here's one explanation: > http://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg03955.html > > Also IIRC Anthony gave a nice explanation once too but I can't find it now. I'm not even looking for an explanation why[1]. I just expected to have this requirement documented on CODING_STYLE or HACKING. [1] Item 3 on Anthony's explanation is enough to me: https://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg04116.html "3) It's not how the rest of QEMU is written. Consistency is the most important purpose of Coding Style."
diff --git a/hw/pc.c b/hw/pc.c index 44bb1dc..9029a55 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -551,6 +551,18 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) return 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 *bochs_bios_init(void) { void *fw_cfg; @@ -558,9 +570,24 @@ static void *bochs_bios_init(void) size_t smbios_len; uint64_t *numa_fw_cfg; int i, j; + unsigned int apic_id_limit = pc_apic_id_limit(max_cpus); fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0); - fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus); + /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86: + * + * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug + * QEMU<->SeaBIOS interface is not based on the "CPU index", but on the APIC + * ID of hotplugged CPUs[1]. This means that FW_CFG_MAX_CPUS is not the + * "maximum number of CPUs", but the "limit to the APIC ID values SeaBIOS + * may see". + * + * So, this means we must not use max_cpus, here, but the maximum possible + * APIC ID value, plus one. + * + * [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_i32(fw_cfg, FW_CFG_ID, 1); fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size); fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, @@ -579,21 +606,25 @@ static void *bochs_bios_init(void) * 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 + max_cpus + nb_numa_nodes); + numa_fw_cfg = g_new0(uint64_t, 1 + 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 cpu_idx; + for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) { + unsigned int apic_id = x86_cpu_apic_id_from_index(cpu_idx); + assert(apic_id < apic_id_limit); for (j = 0; j < nb_numa_nodes; j++) { - if (test_bit(i, node_cpumask[j])) { - numa_fw_cfg[i + 1] = cpu_to_le64(j); + if (test_bit(cpu_idx, node_cpumask[j])) { + numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); break; } } } for (i = 0; i < nb_numa_nodes; i++) { - numa_fw_cfg[max_cpus + 1 + i] = cpu_to_le64(node_mem[i]); + numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(node_mem[i]); } fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg, - (1 + max_cpus + nb_numa_nodes) * sizeof(*numa_fw_cfg)); + (1 + apic_id_limit + nb_numa_nodes) * + sizeof(*numa_fw_cfg)); return fw_cfg; }
This changes FW_CFG_MAX_CPUS and FW_CFG_NUMA to use apic_id_for_cpu(), so the NUMA table can be based on the APIC IDs, instead of CPU index (SeaBIOS knows nothing about CPU indexes, just APIC IDs). Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes v2: - Get PC object as argument - Add more detailed comments explaining the reason for FW_CFG_MAX_CPUS not being simply 'max_cpus' Changes v3: - Use PCInitArgs instead of PC object Changes v4: - Don't use PCInitArgs, just add the necessary data for apic_id_limit() as argument - Rename function to pc_apic_id_limit() - Rename max_apic_id to apic_id_limit Changes v5: - Refresh after apic_id_for_cpu() -> x86_cpu_apic_id_from_index() rename - Refresh after original code changes to use g_new0() --- hw/pc.c | 45 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 7 deletions(-)