On Wed, Jan 23, 2013 at 5:09 PM, Eduardo Habkost <ehabk...@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 <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.
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
>
