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

[Eduardo Habkost]

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.

(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