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 <ehabk...@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
