| Message ID | 1351101001-14589-23-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > + > x86_cpu_realize(OBJECT(cpu), &err); > if (err) { > error_report("pc_cpu_init: %s\n", error_get_pretty(err));
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. > > > + > > x86_cpu_realize(OBJECT(cpu), &err); > > if (err) { > > error_report("pc_cpu_init: %s\n", error_get_pretty(err)); >
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)); > > >
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); + x86_cpu_realize(OBJECT(cpu), &err); if (err) { error_report("pc_cpu_init: %s\n", error_get_pretty(err));
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(+)