Re: [Qemu-devel] [PATCH 22/27] pc: set CPU APIC ID explicitly

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 1 Nov 2012 12:30:39 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Nov 01, 2012 at 03:04:05PM +0100, Igor Mammedov wrote:
> > On Wed, 24 Oct 2012 15:49:56 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > The PC code takes care of CPU topology, and CPU topology affect the CPU
> > > APIC ID. So the PC CPU initialization code needs to set the APIC ID
> > > explicitly.
> > > 
> > > By now, keep the existing behavior but create a apic_id_for_cpu()
> > > function that will be changed later to implement appropriate
> > > topology-dependent behavior.
> > > 
> > > The cpuid_apic_id field is used only at:
> > > 
> > >  - x86_cpu_apic_init(), called from x86_cpu_realize()
> > >  - kvm_init_vcpu(), that is called from the VCPU thread
> > >    created by qemu_init_vcpu(), called by x86_cpu_realize()
> > >  - helper_cpuid(), called only when the VCPU is already running
> > >  - kvm_arch_init_vcpu(), that's called by kvm_init_vcpu()
> > > 
> > > So it's safe to change it before x86_cpu_realize() is called.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > This is based on the patch that I have originally suybmitted as:
> > >  Subject: pc: create apic_id_for_cpu() function (v3)
> > > ---
> > >  hw/pc.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/hw/pc.c b/hw/pc.c
> > > index 0e9a00f..eb68851 100644
> > > --- a/hw/pc.c
> > > +++ b/hw/pc.c
> > > @@ -553,6 +553,21 @@ static void bochs_bios_write(void *opaque, uint32_t
> > > addr, uint32_t val) }
> > >  }
> > >  
> > > +/* 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.
> > > + */
> > > +static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
> > > +{
> > > +    /* right now APIC ID == CPU index. this will eventually change to
> > > use
> > > +     * the CPU topology configuration properly
> > > +     */
> > > +    return cpu_index;
> > > +}
> > > +
> > >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> > >  {
> > >      int index = le32_to_cpu(e820_table.count);
> > > @@ -870,6 +885,13 @@ static void pc_cpu_init(PCInitArgs *args, int
> > > cpu_index) exit(1);
> > >      }
> > >  
> > > +    /* Override the default APIC set by the X86CPU init function.
> > > +     * We need to do that because:
> > > +     * - The APIC ID depends on the CPU topology;
> > > +     * - The exact APIC ID used may depend on the machine-type init
> > > arguments.
> > > +     */
> > > +    cpu->env.cpuid_apic_id = apic_id_for_cpu(args, cpu_index);
> > Could you create property setter for apic_id and use it here, please.
> 
> I was considering doing that, but I thought it could be overkill. I will
> do it in the next version.
Reason for asking it is not to have any CPU internals dangling outside of CPU
object and have CPU created only with help of QOM/qdev calls.

Thanks!
> > 
> > > +
> > >      x86_cpu_realize(OBJECT(cpu), &err);
> > >      if (err) {
> > >          error_report("pc_cpu_init: %s\n", error_get_pretty(err));
> > 
>