Patchwork [for-1.4,07/12] target-i386/cpu: Introduce apic_id_for_cpu() function

login
register
mail settings
Submitter Eduardo Habkost
Date Jan. 17, 2013, 8:59 p.m.
Message ID <1358456378-29248-8-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/213379/
State New
Headers show

Comments

Eduardo Habkost - Jan. 17, 2013, 8:59 p.m.
This function will be used by both the CPU initialization code and the
fw_cfg table initialization code.

Later this function will be updated to generate APIC IDs according to
the CPU topology.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 17 ++++++++++++++++-
 target-i386/cpu.h |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)
Andreas Färber - Jan. 21, 2013, 11:18 a.m.
Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> This function will be used by both the CPU initialization code and the
> fw_cfg table initialization code.
> 
> Later this function will be updated to generate APIC IDs according to
> the CPU topology.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 17 ++++++++++++++++-
>  target-i386/cpu.h |  2 ++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d1a14d5..d90789d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2194,6 +2194,21 @@ void x86_cpu_realize(Object *obj, Error **errp)
>      cpu_reset(CPU(cpu));
>  }
>  
> +/* Calculates initial APIC ID for a specific CPU index
> + *
> + * Currently we need to be able to calculate the APIC ID from the CPU index
> + * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
> + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
> + * all CPUs up to max_cpus.
> + */
> +uint32_t apic_id_for_cpu(unsigned int cpu_index)

Can we rather make this x86_cpu_apic_id(X86CPU *cpu) to account for
future changes to topology modelling?

Andreas

> +{
> +    /* right now APIC ID == CPU index. this will eventually change to use
> +     * the CPU topology configuration properly
> +     */
> +    return cpu_index;
> +}
> +
>  static void x86_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -2228,7 +2243,7 @@ static void x86_cpu_initfn(Object *obj)
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
>  
> -    env->cpuid_apic_id = cs->cpu_index;
> +    env->cpuid_apic_id = apic_id_for_cpu(cs->cpu_index);
>  
>      /* init various static tables used in TCG mode */
>      if (tcg_enabled() && !inited) {
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 9d4fcf9..d86c0af 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1255,4 +1255,6 @@ void disable_kvm_pv_eoi(void);
>  /* Return name of 32-bit register, from a R_* constant */
>  const char *get_register_name_32(unsigned int reg);
>  
> +uint32_t apic_id_for_cpu(unsigned int cpu_index);
> +
>  #endif /* CPU_I386_H */
Eduardo Habkost - Jan. 21, 2013, 11:31 a.m.
On Mon, Jan 21, 2013 at 12:18:55PM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > This function will be used by both the CPU initialization code and the
> > fw_cfg table initialization code.
> > 
> > Later this function will be updated to generate APIC IDs according to
> > the CPU topology.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 17 ++++++++++++++++-
> >  target-i386/cpu.h |  2 ++
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index d1a14d5..d90789d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2194,6 +2194,21 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >      cpu_reset(CPU(cpu));
> >  }
> >  
> > +/* Calculates initial APIC ID for a specific CPU index
> > + *
> > + * Currently we need to be able to calculate the APIC ID from the CPU index
> > + * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
> > + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
> > + * all CPUs up to max_cpus.
> > + */
> > +uint32_t apic_id_for_cpu(unsigned int cpu_index)
> 
> Can we rather make this x86_cpu_apic_id(X86CPU *cpu) to account for
> future changes to topology modelling?

We can't make it get a X86CPU as parameter, because the ACPI tables have
to be built up to max_cpus, before the CPUs get actually created. But it
can be renamed, yes.
Andreas Färber - Jan. 21, 2013, 3:39 p.m.
Am 21.01.2013 12:31, schrieb Eduardo Habkost:
> On Mon, Jan 21, 2013 at 12:18:55PM +0100, Andreas Färber wrote:
>> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
>>> This function will be used by both the CPU initialization code and the
>>> fw_cfg table initialization code.
>>>
>>> Later this function will be updated to generate APIC IDs according to
>>> the CPU topology.
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  target-i386/cpu.c | 17 ++++++++++++++++-
>>>  target-i386/cpu.h |  2 ++
>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index d1a14d5..d90789d 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -2194,6 +2194,21 @@ void x86_cpu_realize(Object *obj, Error **errp)
>>>      cpu_reset(CPU(cpu));
>>>  }
>>>  
>>> +/* Calculates initial APIC ID for a specific CPU index
>>> + *
>>> + * Currently we need to be able to calculate the APIC ID from the CPU index
>>> + * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
>>> + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
>>> + * all CPUs up to max_cpus.
>>> + */
>>> +uint32_t apic_id_for_cpu(unsigned int cpu_index)
>>
>> Can we rather make this x86_cpu_apic_id(X86CPU *cpu) to account for
>> future changes to topology modelling?
> 
> We can't make it get a X86CPU as parameter, because the ACPI tables have
> to be built up to max_cpus, before the CPUs get actually created. But it
> can be renamed, yes.

I see. Is this a limitation of the current code?

IIUC the cpu_index is set once during qemu_init_vcpu() by counting the
CPUArchStates in the first_cpu list. So if we hot-unplug a non-last CPU
this is gonna result in chaos. Therefore I was hoping we could do
something more clever than hardcoding cpu_index in our internal API ...
but we can refactor that as follow-up when the need comes up.

Andreas
Eduardo Habkost - Jan. 21, 2013, 4:29 p.m.
On Mon, Jan 21, 2013 at 04:39:56PM +0100, Andreas Färber wrote:
> Am 21.01.2013 12:31, schrieb Eduardo Habkost:
> > On Mon, Jan 21, 2013 at 12:18:55PM +0100, Andreas Färber wrote:
> >> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> >>> This function will be used by both the CPU initialization code and the
> >>> fw_cfg table initialization code.
> >>>
> >>> Later this function will be updated to generate APIC IDs according to
> >>> the CPU topology.
> >>>
> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >>> ---
> >>>  target-i386/cpu.c | 17 ++++++++++++++++-
> >>>  target-i386/cpu.h |  2 ++
> >>>  2 files changed, 18 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >>> index d1a14d5..d90789d 100644
> >>> --- a/target-i386/cpu.c
> >>> +++ b/target-i386/cpu.c
> >>> @@ -2194,6 +2194,21 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >>>      cpu_reset(CPU(cpu));
> >>>  }
> >>>  
> >>> +/* Calculates initial APIC ID for a specific CPU index
> >>> + *
> >>> + * Currently we need to be able to calculate the APIC ID from the CPU index
> >>> + * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
> >>> + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
> >>> + * all CPUs up to max_cpus.
> >>> + */
> >>> +uint32_t apic_id_for_cpu(unsigned int cpu_index)
> >>
> >> Can we rather make this x86_cpu_apic_id(X86CPU *cpu) to account for
> >> future changes to topology modelling?
> > 
> > We can't make it get a X86CPU as parameter, because the ACPI tables have
> > to be built up to max_cpus, before the CPUs get actually created. But it
> > can be renamed, yes.
> 
> I see. Is this a limitation of the current code?

No, it is a requirement of the BIOS: it needs to know in advance the
list of APIC IDs of all possible CPUs up to max_cpus, so it can build
the ACPI tables properly.

> 
> IIUC the cpu_index is set once during qemu_init_vcpu() by counting the
> CPUArchStates in the first_cpu list. So if we hot-unplug a non-last CPU
> this is gonna result in chaos. Therefore I was hoping we could do
> something more clever than hardcoding cpu_index in our internal API ...
> but we can refactor that as follow-up when the need comes up.

So we have to fix the way the CPU index is calculated when we
implemented hotplug/hotunplug. We need a way to identify the CPU we are
calculating the APIC ID for, and today the only "CPU identifier" we have
is the CPU index.

In the future we can consider making apic_id_for_cpu() get the
package/core/thread IDs directly. But to do that, we first need to
actually model the package/core/thread lists in the PC CPU creation
code.

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d1a14d5..d90789d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2194,6 +2194,21 @@  void x86_cpu_realize(Object *obj, Error **errp)
     cpu_reset(CPU(cpu));
 }
 
+/* Calculates initial APIC ID for a specific CPU index
+ *
+ * Currently we need to be able to calculate the APIC ID from the CPU index
+ * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
+ * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
+ * all CPUs up to max_cpus.
+ */
+uint32_t apic_id_for_cpu(unsigned int cpu_index)
+{
+    /* right now APIC ID == CPU index. this will eventually change to use
+     * the CPU topology configuration properly
+     */
+    return cpu_index;
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -2228,7 +2243,7 @@  static void x86_cpu_initfn(Object *obj)
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
 
-    env->cpuid_apic_id = cs->cpu_index;
+    env->cpuid_apic_id = apic_id_for_cpu(cs->cpu_index);
 
     /* init various static tables used in TCG mode */
     if (tcg_enabled() && !inited) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 9d4fcf9..d86c0af 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1255,4 +1255,6 @@  void disable_kvm_pv_eoi(void);
 /* Return name of 32-bit register, from a R_* constant */
 const char *get_register_name_32(unsigned int reg);
 
+uint32_t apic_id_for_cpu(unsigned int cpu_index);
+
 #endif /* CPU_I386_H */