Re: [Qemu-devel] [PATCH for-1.4 qom-cpu 6/9] pc: Set fw_cfg data based on APIC ID calculation

[Blue Swirl]

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