[Qemu-devel] [RFC v2 00/18] Fix APIC-ID-based CPU topology

[Qemu-devel] [RFC v2 00/18] Fix APIC-ID-based CPU topology

[Eduardo Habkost]

Bug description:

The CPU APIC IDs generated by QEMU are broken if the number of cores-per-socket
or threads-per-core are not powers of 2, as the bits on the APIC ID do not
correspond to what's expected to reflect the CPU sockets/cores/threads
topology[1].

[1] http://software.intel.com/en-us/articles/intel-64-architecture-processor-topolog
y-enumeration/

Changes v1 -> v2:

 * Created a qemu-stdio.h file for some I/O related definitions
 * Now all the topology-based APIC ID calculation logic is inside pc.c
 * I have now created a "PC" object just to carry the machine-type compatibility
   flag that keeps the previous behavior.
   * Dropped the qemu_global_get() function, as it is not needed anymore
 * Fixed compile errors on non-x86 targets with KVM enabled (ppc, s390x)
 * Add more descriptive comments on THE FW_CFG_MAX_CPUS calculation
 * Add pointers to documentation on topology.h

This series is based on the cpu-queue branch, from:
 https://github.com/ehabkost/qemu/commits/cpu-queue
 (more exactly, it was based on commit 8ffa1dbb0f4efa3d465d66bcdfbf545c350fe3ae)
This series is available on git, at:
 https://github.com/ehabkost/qemu-hacks/commits/work/apicid-rfc-v2-2012-10-03

Eduardo Habkost (18):
  pc: create "PC" device class
  pc: create PC object on pc_init1()
  pc: add PC object argument to some init functions
  move I/O-related definitions from qemu-common.h to a new header
    (qemu-stdio.h)
  cpus.h: include cpu-stdio.h
  hw/apic.c: rename bit functions to not conflict with bitops.h (v2)
  kvm: create kvm_arch_vcpu_id() function
  target-i386: kvm: set vcpu_id to APIC ID instead of CPU index (v2)
  target-i386: cpu: move cpuid_apic_id initialization to
    cpu_x86_register()
  target-i386: cpu: add apic_id argument to cpu_x86_register()
  target-i386: cpu_x86_init: allow APIC ID to be set by caller
  fw_cfg: remove FW_CFG_MAX_CPUS from fw_cfg_init()
  pc: set explicit APIC ID for CPUs
  pc: create apic_id_for_cpu() function (v3)
  pc: set fw_cfg data based on APIC ID calculation (v2)
  tests: support target-specific unit tests
  target-i386: topology & APIC ID utility functions (v2)
  pc: generate APIC IDs according to CPU topology (v3)

 cpus.h                 |   2 +
 hw/apic.c              |  35 ++++++-------
 hw/fw_cfg.c            |   1 -
 hw/pc.c                | 125 ++++++++++++++++++++++++++++++++++++++++------
 hw/pc.h                |  11 +++-
 hw/pc_piix.c           |  11 +++-
 hw/ppc_newworld.c      |   1 +
 hw/ppc_oldworld.c      |   1 +
 hw/sun4m.c             |   3 ++
 hw/sun4u.c             |   1 +
 kvm-all.c              |   2 +-
 kvm.h                  |   3 ++
 qemu-common.h          |  57 +--------------------
 qemu-stdio.h           |  73 +++++++++++++++++++++++++++
 target-i386/cpu.c      |   6 +--
 target-i386/cpu.h      |   6 +--
 target-i386/helper.c   |  15 +++++-
 target-i386/kvm.c      |   5 ++
 target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm.c       |   5 ++
 target-s390x/kvm.c     |   5 ++
 tests/.gitignore       |   1 +
 tests/Makefile         |  19 +++++--
 tests/test-x86-cpuid.c | 102 +++++++++++++++++++++++++++++++++++++
 24 files changed, 519 insertions(+), 104 deletions(-)
 create mode 100644 qemu-stdio.h
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c

-- 
1.7.11.4

[Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Eduardo Habkost]

We can make it a child of a generic "machine" class later, but right now
a "PC" class is needed to allow global-properties to control some
details of CPU creation on the PC code.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c | 18 ++++++++++++++++++
 hw/pc.h |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/hw/pc.c b/hw/pc.c
index 7e7e0e2..9b68282 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
+typedef struct PC {
+    DeviceState parent_obj;
+} PC;
+
+static const TypeInfo pc_type_info = {
+    .name = TYPE_PC_MACHINE,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(PC),
+    .class_size = sizeof(DeviceClass),
+};
+
+static void pc_register_type(void)
+{
+    type_register_static(&pc_type_info);
+}
+
+type_init(pc_register_type);
+
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
     int index = le32_to_cpu(e820_table.count);
diff --git a/hw/pc.h b/hw/pc.h
index e4db071..77e898f 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
 /* pc.c */
 extern int fd_bootchk;
 
+#define TYPE_PC_MACHINE "PC"
+#define PC(obj) \
+    OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE)
+struct PC;
+typedef struct PC PC;
+
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
-- 
1.7.11.4

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Paolo Bonzini]

Il 03/10/2012 15:28, Eduardo Habkost ha scritto:
> We can make it a child of a generic "machine" class later, but right now
> a "PC" class is needed to allow global-properties to control some
> details of CPU creation on the PC code.

Does it need to be a Device, or can it be a normal Object (or for
clarity a derivative of TYPE_CONTAINER)?

Paolo

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/pc.c | 18 ++++++++++++++++++
>  hw/pc.h |  6 ++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 7e7e0e2..9b68282 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>      }
>  }
>  
> +typedef struct PC {
> +    DeviceState parent_obj;
> +} PC;
> +
> +static const TypeInfo pc_type_info = {
> +    .name = TYPE_PC_MACHINE,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(PC),
> +    .class_size = sizeof(DeviceClass),
> +};
> +
> +static void pc_register_type(void)
> +{
> +    type_register_static(&pc_type_info);
> +}
> +
> +type_init(pc_register_type);
> +
>  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>  {
>      int index = le32_to_cpu(e820_table.count);
> diff --git a/hw/pc.h b/hw/pc.h
> index e4db071..77e898f 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
>  /* pc.c */
>  extern int fd_bootchk;
>  
> +#define TYPE_PC_MACHINE "PC"
> +#define PC(obj) \
> +    OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE)
> +struct PC;
> +typedef struct PC PC;
> +
>  void pc_register_ferr_irq(qemu_irq irq);
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>  
> 

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Eduardo Habkost]

On Wed, Oct 03, 2012 at 04:38:10PM +0200, Paolo Bonzini wrote:
> Il 03/10/2012 15:28, Eduardo Habkost ha scritto:
> > We can make it a child of a generic "machine" class later, but right now
> > a "PC" class is needed to allow global-properties to control some
> > details of CPU creation on the PC code.
> 
> Does it need to be a Device, or can it be a normal Object (or for
> clarity a derivative of TYPE_CONTAINER)?

It needs to be a Device because that's the only way to use global
properties, today.

I have some experimental patches that allow normal objects to get global
properties set (as long as an explicit call to a
object_set_global_props() function is made). Would that be preferrable?

> 
> Paolo
> 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/pc.c | 18 ++++++++++++++++++
> >  hw/pc.h |  6 ++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 7e7e0e2..9b68282 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
> >      }
> >  }
> >  
> > +typedef struct PC {
> > +    DeviceState parent_obj;
> > +} PC;
> > +
> > +static const TypeInfo pc_type_info = {
> > +    .name = TYPE_PC_MACHINE,
> > +    .parent = TYPE_DEVICE,
> > +    .instance_size = sizeof(PC),
> > +    .class_size = sizeof(DeviceClass),
> > +};
> > +
> > +static void pc_register_type(void)
> > +{
> > +    type_register_static(&pc_type_info);
> > +}
> > +
> > +type_init(pc_register_type);
> > +
> >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> >  {
> >      int index = le32_to_cpu(e820_table.count);
> > diff --git a/hw/pc.h b/hw/pc.h
> > index e4db071..77e898f 100644
> > --- a/hw/pc.h
> > +++ b/hw/pc.h
> > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
> >  /* pc.c */
> >  extern int fd_bootchk;
> >  
> > +#define TYPE_PC_MACHINE "PC"
> > +#define PC(obj) \
> > +    OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE)
> > +struct PC;
> > +typedef struct PC PC;
> > +
> >  void pc_register_ferr_irq(qemu_irq irq);
> >  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
> >  
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Paolo Bonzini]

Il 03/10/2012 16:48, Eduardo Habkost ha scritto:
>> > 
>> > Does it need to be a Device, or can it be a normal Object (or for
>> > clarity a derivative of TYPE_CONTAINER)?
> It needs to be a Device because that's the only way to use global
> properties, today.
> 
> I have some experimental patches that allow normal objects to get global
> properties set (as long as an explicit call to a
> object_set_global_props() function is made). Would that be preferrable?

That's nice, though there would be the question of when to call the
function...  I suppose instance_init would do.

Paolo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Anthony Liguori]

Eduardo Habkost <ehabkost@redhat.com> writes:

> We can make it a child of a generic "machine" class later, but right now
> a "PC" class is needed to allow global-properties to control some
> details of CPU creation on the PC code.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/pc.c | 18 ++++++++++++++++++
>  hw/pc.h |  6 ++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 7e7e0e2..9b68282 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>      }
>  }
>  
> +typedef struct PC {
> +    DeviceState parent_obj;
> +} PC;

So the general problem with this approach is that it strays from
modeling hardware.

I guess I'm confused why we're not just adding an apic_id property to
the CPU objects and setting that via the normal QOM accessors.

Wouldn't that solve the problem?

Regards,

Anthony Liguori

> +
> +static const TypeInfo pc_type_info = {
> +    .name = TYPE_PC_MACHINE,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(PC),
> +    .class_size = sizeof(DeviceClass),
> +};
> +
> +static void pc_register_type(void)
> +{
> +    type_register_static(&pc_type_info);
> +}
> +
> +type_init(pc_register_type);
> +
>  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>  {
>      int index = le32_to_cpu(e820_table.count);
> diff --git a/hw/pc.h b/hw/pc.h
> index e4db071..77e898f 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
>  /* pc.c */
>  extern int fd_bootchk;
>  
> +#define TYPE_PC_MACHINE "PC"
> +#define PC(obj) \
> +    OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE)
> +struct PC;
> +typedef struct PC PC;
> +
>  void pc_register_ferr_irq(qemu_irq irq);
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>  
> -- 
> 1.7.11.4

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Paolo Bonzini]

Il 04/10/2012 15:46, Anthony Liguori ha scritto:
>> > +typedef struct PC {
>> > +    DeviceState parent_obj;
>> > +} PC;
> So the general problem with this approach is that it strays from
> modeling hardware.

It doesn't really; it's a motherboard object, there's no reason why
/machine shouldn't be a Device itself, with a few objects (CPUs, the
i440FX, the IOAPIC, and of course the peripherals) hanging off it.

Paolo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 04/10/2012 15:46, Anthony Liguori ha scritto:
>>> > +typedef struct PC {
>>> > +    DeviceState parent_obj;
>>> > +} PC;
>> So the general problem with this approach is that it strays from
>> modeling hardware.
>
> It doesn't really; it's a motherboard object, there's no reason why
> /machine shouldn't be a Device itself, with a few objects (CPUs, the
> i440FX, the IOAPIC, and of course the peripherals) hanging off it.

Okay, but modeling a motherboard is different than creating a "PC"
object and throwing in the kitchen skink.

And I'm not sure that going top-down is the best strategy.  I think
going bottom up makes more sense (starting with modeling Super IO chip).

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 04/10/2012 15:46, Anthony Liguori ha scritto:
> >>> > +typedef struct PC {
> >>> > +    DeviceState parent_obj;
> >>> > +} PC;
> >> So the general problem with this approach is that it strays from
> >> modeling hardware.
> >
> > It doesn't really; it's a motherboard object, there's no reason why
> > /machine shouldn't be a Device itself, with a few objects (CPUs, the
> > i440FX, the IOAPIC, and of course the peripherals) hanging off it.
> 
> Okay, but modeling a motherboard is different than creating a "PC"
> object and throwing in the kitchen skink.
> 
> And I'm not sure that going top-down is the best strategy.  I think
> going bottom up makes more sense (starting with modeling Super IO chip).
> 

So, would you be OK with this implementation if the class were named
"Motherboard", "set-of-CPU-sockets", or something like that?

(The change on the APIC ID generation logic is not something that
affects only individual CPUs or APICs, but the fw_cfg NUMA & CPU hotplug
code as well, that's why I don't think "contiguous_apic_ids" should be a
property of CPU or APIC objects).


> Regards,
> 
> Anthony Liguori
> 
> >
> > Paolo

-- 
Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Anthony Liguori]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > Il 04/10/2012 15:46, Anthony Liguori ha scritto:
>> >>> > +typedef struct PC {
>> >>> > +    DeviceState parent_obj;
>> >>> > +} PC;
>> >> So the general problem with this approach is that it strays from
>> >> modeling hardware.
>> >
>> > It doesn't really; it's a motherboard object, there's no reason why
>> > /machine shouldn't be a Device itself, with a few objects (CPUs, the
>> > i440FX, the IOAPIC, and of course the peripherals) hanging off it.
>> 
>> Okay, but modeling a motherboard is different than creating a "PC"
>> object and throwing in the kitchen skink.
>> 
>> And I'm not sure that going top-down is the best strategy.  I think
>> going bottom up makes more sense (starting with modeling Super IO chip).
>> 
>
> So, would you be OK with this implementation if the class were named
> "Motherboard", "set-of-CPU-sockets", or something like that?

I would, but you're mixing up modeling with bug fixing.

There's a very easy way to achieve your goal without dramatic
remodeling.

Just assign APIC ids during CPU creation and make contiguous_apic_ids a
parameter of pc_init1.

You don't need to worry about CPU hotplug.  It doesn't exist in qemu.git
and is broken in qemu-kvm.git.

Regards,

Anthony Liguori

>
> (The change on the APIC ID generation logic is not something that
> affects only individual CPUs or APICs, but the fw_cfg NUMA & CPU hotplug
> code as well, that's why I don't think "contiguous_apic_ids" should be a
> property of CPU or APIC objects).
>
>
>> Regards,
>> 
>> Anthony Liguori
>> 
>> >
>> > Paolo
>
> -- 
> Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 10:10:16AM -0500, Anthony Liguori wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >> > Il 04/10/2012 15:46, Anthony Liguori ha scritto:
> >> >>> > +typedef struct PC {
> >> >>> > +    DeviceState parent_obj;
> >> >>> > +} PC;
> >> >> So the general problem with this approach is that it strays from
> >> >> modeling hardware.
> >> >
> >> > It doesn't really; it's a motherboard object, there's no reason why
> >> > /machine shouldn't be a Device itself, with a few objects (CPUs, the
> >> > i440FX, the IOAPIC, and of course the peripherals) hanging off it.
> >> 
> >> Okay, but modeling a motherboard is different than creating a "PC"
> >> object and throwing in the kitchen skink.
> >> 
> >> And I'm not sure that going top-down is the best strategy.  I think
> >> going bottom up makes more sense (starting with modeling Super IO chip).
> >> 
> >
> > So, would you be OK with this implementation if the class were named
> > "Motherboard", "set-of-CPU-sockets", or something like that?
> 
> I would, but you're mixing up modeling with bug fixing.
> 
> There's a very easy way to achieve your goal without dramatic
> remodeling.
> 
> Just assign APIC ids during CPU creation and make contiguous_apic_ids a
> parameter of pc_init1.
> 
> You don't need to worry about CPU hotplug.  It doesn't exist in qemu.git
> and is broken in qemu-kvm.git.

With or without CPU hotplug, the max_cpus variable already exists, and I
want to avoid breaking code that's already using it, and adding Yet
Another problem to be fixed by whoever is going to make CPU hotplug
work.

-- 
Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Anthony Liguori]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Oct 04, 2012 at 10:10:16AM -0500, Anthony Liguori wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote:
>> >> Paolo Bonzini <pbonzini@redhat.com> writes:
>> >> 
>> >> > Il 04/10/2012 15:46, Anthony Liguori ha scritto:
>> >> >>> > +typedef struct PC {
>> >> >>> > +    DeviceState parent_obj;
>> >> >>> > +} PC;
>> >> >> So the general problem with this approach is that it strays from
>> >> >> modeling hardware.
>> >> >
>> >> > It doesn't really; it's a motherboard object, there's no reason why
>> >> > /machine shouldn't be a Device itself, with a few objects (CPUs, the
>> >> > i440FX, the IOAPIC, and of course the peripherals) hanging off it.
>> >> 
>> >> Okay, but modeling a motherboard is different than creating a "PC"
>> >> object and throwing in the kitchen skink.
>> >> 
>> >> And I'm not sure that going top-down is the best strategy.  I think
>> >> going bottom up makes more sense (starting with modeling Super IO chip).
>> >> 
>> >
>> > So, would you be OK with this implementation if the class were named
>> > "Motherboard", "set-of-CPU-sockets", or something like that?
>> 
>> I would, but you're mixing up modeling with bug fixing.
>> 
>> There's a very easy way to achieve your goal without dramatic
>> remodeling.
>> 
>> Just assign APIC ids during CPU creation and make contiguous_apic_ids a
>> parameter of pc_init1.
>> 
>> You don't need to worry about CPU hotplug.  It doesn't exist in qemu.git
>> and is broken in qemu-kvm.git.
>
> With or without CPU hotplug, the max_cpus variable already exists, and I
> want to avoid breaking code that's already using it, and adding Yet
> Another problem to be fixed by whoever is going to make CPU hotplug
> work.

Sorry, what does max_cpus have to do with apic ids??

Regards,

Anthony Liguori

>
> -- 
> Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 12:42:23PM -0500, Anthony Liguori wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Oct 04, 2012 at 10:10:16AM -0500, Anthony Liguori wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> 
> >> > On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote:
> >> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> >> 
> >> >> > Il 04/10/2012 15:46, Anthony Liguori ha scritto:
> >> >> >>> > +typedef struct PC {
> >> >> >>> > +    DeviceState parent_obj;
> >> >> >>> > +} PC;
> >> >> >> So the general problem with this approach is that it strays from
> >> >> >> modeling hardware.
> >> >> >
> >> >> > It doesn't really; it's a motherboard object, there's no reason why
> >> >> > /machine shouldn't be a Device itself, with a few objects (CPUs, the
> >> >> > i440FX, the IOAPIC, and of course the peripherals) hanging off it.
> >> >> 
> >> >> Okay, but modeling a motherboard is different than creating a "PC"
> >> >> object and throwing in the kitchen skink.
> >> >> 
> >> >> And I'm not sure that going top-down is the best strategy.  I think
> >> >> going bottom up makes more sense (starting with modeling Super IO chip).
> >> >> 
> >> >
> >> > So, would you be OK with this implementation if the class were named
> >> > "Motherboard", "set-of-CPU-sockets", or something like that?
> >> 
> >> I would, but you're mixing up modeling with bug fixing.
> >> 
> >> There's a very easy way to achieve your goal without dramatic
> >> remodeling.
> >> 
> >> Just assign APIC ids during CPU creation and make contiguous_apic_ids a
> >> parameter of pc_init1.
> >> 
> >> You don't need to worry about CPU hotplug.  It doesn't exist in qemu.git
> >> and is broken in qemu-kvm.git.
> >
> > With or without CPU hotplug, the max_cpus variable already exists, and I
> > want to avoid breaking code that's already using it, and adding Yet
> > Another problem to be fixed by whoever is going to make CPU hotplug
> > work.
> 
> Sorry, what does max_cpus have to do with apic ids??

See patch 15/18.

-- 
Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Andreas Färber]

Am 04.10.2012 15:50, schrieb Paolo Bonzini:
> Il 04/10/2012 15:46, Anthony Liguori ha scritto:
>>>> +typedef struct PC {
>>>> +    DeviceState parent_obj;
>>>> +} PC;
>> So the general problem with this approach is that it strays from
>> modeling hardware.
> 
> It doesn't really; it's a motherboard object, there's no reason why
> /machine shouldn't be a Device itself, with a few objects (CPUs, the
> i440FX, the IOAPIC, and of course the peripherals) hanging off it.

On the other hand, why should it? We'd have QEMUMachine::reset vs.
DeviceState::reset then and I don't see an immanent need for gpio / irqs
on the mainboard either (on evaluation boards there are, but they are
usually properties of the SoC modelling-wise).

If we want to bundle objects, we have a dedicated Container type;
otherwise we could just derive from TYPE_OBJECT directly.

In the end it just goes back to our attempt to generalize the properties
mechanisms and/or to a lack of APIs to do exactly what we want... ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 08:46:46AM -0500, Anthony Liguori wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > We can make it a child of a generic "machine" class later, but right now
> > a "PC" class is needed to allow global-properties to control some
> > details of CPU creation on the PC code.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/pc.c | 18 ++++++++++++++++++
> >  hw/pc.h |  6 ++++++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 7e7e0e2..9b68282 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
> >      }
> >  }
> >  
> > +typedef struct PC {
> > +    DeviceState parent_obj;
> > +} PC;
> 
> So the general problem with this approach is that it strays from
> modeling hardware.

True, it's not modelling hardware. It's controlling the behavior of the
QEMU code that set APIC IDs, because we need to keep the old behavior on
old machine-types.

> 
> I guess I'm confused why we're not just adding an apic_id property to
> the CPU objects and setting that via the normal QOM accessors.
> 
> Wouldn't that solve the problem?
> 

It wouldn't solve the problem (although it can make the code look
better).

The problem is not setting the APIC ID, is controlling the code that
generates the APIC IDs. I don't care too much where that code would live
(it could be inside cpu.c or helper.c), but it still needs a flag where
old machine-types tell it "please keep the old behavior for
compatibility".


> Regards,
> 
> Anthony Liguori
> 
> > +
> > +static const TypeInfo pc_type_info = {
> > +    .name = TYPE_PC_MACHINE,
> > +    .parent = TYPE_DEVICE,
> > +    .instance_size = sizeof(PC),
> > +    .class_size = sizeof(DeviceClass),
> > +};
> > +
> > +static void pc_register_type(void)
> > +{
> > +    type_register_static(&pc_type_info);
> > +}
> > +
> > +type_init(pc_register_type);
> > +
> >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> >  {
> >      int index = le32_to_cpu(e820_table.count);
> > diff --git a/hw/pc.h b/hw/pc.h
> > index e4db071..77e898f 100644
> > --- a/hw/pc.h
> > +++ b/hw/pc.h
> > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
> >  /* pc.c */
> >  extern int fd_bootchk;
> >  
> > +#define TYPE_PC_MACHINE "PC"
> > +#define PC(obj) \
> > +    OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE)
> > +struct PC;
> > +typedef struct PC PC;
> > +
> >  void pc_register_ferr_irq(qemu_irq irq);
> >  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
> >  
> > -- 
> > 1.7.11.4

-- 
Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Anthony Liguori]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Oct 04, 2012 at 08:46:46AM -0500, Anthony Liguori wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > We can make it a child of a generic "machine" class later, but right now
>> > a "PC" class is needed to allow global-properties to control some
>> > details of CPU creation on the PC code.
>> >
>> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> > ---
>> >  hw/pc.c | 18 ++++++++++++++++++
>> >  hw/pc.h |  6 ++++++
>> >  2 files changed, 24 insertions(+)
>> >
>> > diff --git a/hw/pc.c b/hw/pc.c
>> > index 7e7e0e2..9b68282 100644
>> > --- a/hw/pc.c
>> > +++ b/hw/pc.c
>> > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>> >      }
>> >  }
>> >  
>> > +typedef struct PC {
>> > +    DeviceState parent_obj;
>> > +} PC;
>> 
>> So the general problem with this approach is that it strays from
>> modeling hardware.
>
> True, it's not modelling hardware. It's controlling the behavior of the
> QEMU code that set APIC IDs, because we need to keep the old behavior on
> old machine-types.
>
>> 
>> I guess I'm confused why we're not just adding an apic_id property to
>> the CPU objects and setting that via the normal QOM accessors.
>> 
>> Wouldn't that solve the problem?
>> 
>
> It wouldn't solve the problem (although it can make the code look
> better).
>
> The problem is not setting the APIC ID, is controlling the code that
> generates the APIC IDs. I don't care too much where that code would live
> (it could be inside cpu.c or helper.c), but it still needs a flag where
> old machine-types tell it "please keep the old behavior for
> compatibility".

Can you just add a flag to pc_init1 and set the apic_id property
according to that flag?

Then you simply add a pc_init_post_1_3 and pc_init_pre_1_3 that calls
pc_init1 with the appropriate flag value.

Regards,

Anthony Liguori

>
>
>> Regards,
>> 
>> Anthony Liguori
>> 
>> > +
>> > +static const TypeInfo pc_type_info = {
>> > +    .name = TYPE_PC_MACHINE,
>> > +    .parent = TYPE_DEVICE,
>> > +    .instance_size = sizeof(PC),
>> > +    .class_size = sizeof(DeviceClass),
>> > +};
>> > +
>> > +static void pc_register_type(void)
>> > +{
>> > +    type_register_static(&pc_type_info);
>> > +}
>> > +
>> > +type_init(pc_register_type);
>> > +
>> >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>> >  {
>> >      int index = le32_to_cpu(e820_table.count);
>> > diff --git a/hw/pc.h b/hw/pc.h
>> > index e4db071..77e898f 100644
>> > --- a/hw/pc.h
>> > +++ b/hw/pc.h
>> > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
>> >  /* pc.c */
>> >  extern int fd_bootchk;
>> >  
>> > +#define TYPE_PC_MACHINE "PC"
>> > +#define PC(obj) \
>> > +    OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE)
>> > +struct PC;
>> > +typedef struct PC PC;
>> > +
>> >  void pc_register_ferr_irq(qemu_irq irq);
>> >  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>> >  
>> > -- 
>> > 1.7.11.4
>
> -- 
> Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 09:29:57AM -0500, Anthony Liguori wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Oct 04, 2012 at 08:46:46AM -0500, Anthony Liguori wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> 
> >> > We can make it a child of a generic "machine" class later, but right now
> >> > a "PC" class is needed to allow global-properties to control some
> >> > details of CPU creation on the PC code.
> >> >
> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > ---
> >> >  hw/pc.c | 18 ++++++++++++++++++
> >> >  hw/pc.h |  6 ++++++
> >> >  2 files changed, 24 insertions(+)
> >> >
> >> > diff --git a/hw/pc.c b/hw/pc.c
> >> > index 7e7e0e2..9b68282 100644
> >> > --- a/hw/pc.c
> >> > +++ b/hw/pc.c
> >> > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
> >> >      }
> >> >  }
> >> >  
> >> > +typedef struct PC {
> >> > +    DeviceState parent_obj;
> >> > +} PC;
> >> 
> >> So the general problem with this approach is that it strays from
> >> modeling hardware.
> >
> > True, it's not modelling hardware. It's controlling the behavior of the
> > QEMU code that set APIC IDs, because we need to keep the old behavior on
> > old machine-types.
> >
> >> 
> >> I guess I'm confused why we're not just adding an apic_id property to
> >> the CPU objects and setting that via the normal QOM accessors.
> >> 
> >> Wouldn't that solve the problem?
> >> 
> >
> > It wouldn't solve the problem (although it can make the code look
> > better).
> >
> > The problem is not setting the APIC ID, is controlling the code that
> > generates the APIC IDs. I don't care too much where that code would live
> > (it could be inside cpu.c or helper.c), but it still needs a flag where
> > old machine-types tell it "please keep the old behavior for
> > compatibility".
> 
> Can you just add a flag to pc_init1 and set the apic_id property
> according to that flag?
> 
> Then you simply add a pc_init_post_1_3 and pc_init_pre_1_3 that calls
> pc_init1 with the appropriate flag value.

I wish I was told this 6 months ago! I thought we wanted to avoid that
and wanted to start using global properties instead of making the list
of pc_init1() parameters grow.

if that's acceptable then, yes, we could fix the bug without having to
deal with class and object modelling.


> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >
> >> Regards,
> >> 
> >> Anthony Liguori
> >> 
> >> > +
> >> > +static const TypeInfo pc_type_info = {
> >> > +    .name = TYPE_PC_MACHINE,
> >> > +    .parent = TYPE_DEVICE,
> >> > +    .instance_size = sizeof(PC),
> >> > +    .class_size = sizeof(DeviceClass),
> >> > +};
> >> > +
> >> > +static void pc_register_type(void)
> >> > +{
> >> > +    type_register_static(&pc_type_info);
> >> > +}
> >> > +
> >> > +type_init(pc_register_type);
> >> > +
> >> >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> >> >  {
> >> >      int index = le32_to_cpu(e820_table.count);
> >> > diff --git a/hw/pc.h b/hw/pc.h
> >> > index e4db071..77e898f 100644
> >> > --- a/hw/pc.h
> >> > +++ b/hw/pc.h
> >> > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
> >> >  /* pc.c */
> >> >  extern int fd_bootchk;
> >> >  
> >> > +#define TYPE_PC_MACHINE "PC"
> >> > +#define PC(obj) \
> >> > +    OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE)
> >> > +struct PC;
> >> > +typedef struct PC PC;
> >> > +
> >> >  void pc_register_ferr_irq(qemu_irq irq);
> >> >  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
> >> >  
> >> > -- 
> >> > 1.7.11.4
> >
> > -- 
> > Eduardo

-- 
Eduardo

Re: [Qemu-devel] [RFC 01/18] pc: create "PC" device class

[Anthony Liguori]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Oct 04, 2012 at 09:29:57AM -0500, Anthony Liguori wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Thu, Oct 04, 2012 at 08:46:46AM -0500, Anthony Liguori wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> 
>> >> > We can make it a child of a generic "machine" class later, but right now
>> >> > a "PC" class is needed to allow global-properties to control some
>> >> > details of CPU creation on the PC code.
>> >> >
>> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> >> > ---
>> >> >  hw/pc.c | 18 ++++++++++++++++++
>> >> >  hw/pc.h |  6 ++++++
>> >> >  2 files changed, 24 insertions(+)
>> >> >
>> >> > diff --git a/hw/pc.c b/hw/pc.c
>> >> > index 7e7e0e2..9b68282 100644
>> >> > --- a/hw/pc.c
>> >> > +++ b/hw/pc.c
>> >> > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>> >> >      }
>> >> >  }
>> >> >  
>> >> > +typedef struct PC {
>> >> > +    DeviceState parent_obj;
>> >> > +} PC;
>> >> 
>> >> So the general problem with this approach is that it strays from
>> >> modeling hardware.
>> >
>> > True, it's not modelling hardware. It's controlling the behavior of the
>> > QEMU code that set APIC IDs, because we need to keep the old behavior on
>> > old machine-types.
>> >
>> >> 
>> >> I guess I'm confused why we're not just adding an apic_id property to
>> >> the CPU objects and setting that via the normal QOM accessors.
>> >> 
>> >> Wouldn't that solve the problem?
>> >> 
>> >
>> > It wouldn't solve the problem (although it can make the code look
>> > better).
>> >
>> > The problem is not setting the APIC ID, is controlling the code that
>> > generates the APIC IDs. I don't care too much where that code would live
>> > (it could be inside cpu.c or helper.c), but it still needs a flag where
>> > old machine-types tell it "please keep the old behavior for
>> > compatibility".
>> 
>> Can you just add a flag to pc_init1 and set the apic_id property
>> according to that flag?
>> 
>> Then you simply add a pc_init_post_1_3 and pc_init_pre_1_3 that calls
>> pc_init1 with the appropriate flag value.
>
> I wish I was told this 6 months ago! I thought we wanted to avoid that
> and wanted to start using global properties instead of making the list
> of pc_init1() parameters grow.
>
> if that's acceptable then, yes, we could fix the bug without having to
> deal with class and object modelling.

Yes, this is absolutely acceptable.  This is how we handle this kind of
stuff today (like for kvmclock).

Regards,

Anthony Liguori

>
>
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>> >
>> >
>> >> Regards,
>> >> 
>> >> Anthony Liguori
>> >> 
>> >> > +
>> >> > +static const TypeInfo pc_type_info = {
>> >> > +    .name = TYPE_PC_MACHINE,
>> >> > +    .parent = TYPE_DEVICE,
>> >> > +    .instance_size = sizeof(PC),
>> >> > +    .class_size = sizeof(DeviceClass),
>> >> > +};
>> >> > +
>> >> > +static void pc_register_type(void)
>> >> > +{
>> >> > +    type_register_static(&pc_type_info);
>> >> > +}
>> >> > +
>> >> > +type_init(pc_register_type);
>> >> > +
>> >> >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>> >> >  {
>> >> >      int index = le32_to_cpu(e820_table.count);
>> >> > diff --git a/hw/pc.h b/hw/pc.h
>> >> > index e4db071..77e898f 100644
>> >> > --- a/hw/pc.h
>> >> > +++ b/hw/pc.h
>> >> > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
>> >> >  /* pc.c */
>> >> >  extern int fd_bootchk;
>> >> >  
>> >> > +#define TYPE_PC_MACHINE "PC"
>> >> > +#define PC(obj) \
>> >> > +    OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE)
>> >> > +struct PC;
>> >> > +typedef struct PC PC;
>> >> > +
>> >> >  void pc_register_ferr_irq(qemu_irq irq);
>> >> >  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>> >> >  
>> >> > -- 
>> >> > 1.7.11.4
>> >
>> > -- 
>> > Eduardo
>
> -- 
> Eduardo

[Qemu-devel] [RFC 02/18] pc: create PC object on pc_init1()

[Eduardo Habkost]

It would be interesting to make the generic machine intialization code
create a machine object instead, but changing the machine initialization
function signature is a nightmare, so by now I am creating the object
inside pc_init1().

The object is not used for anything by now, but it will be used during
some steps of the initialization, later.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc_piix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index fd5898f..28b5f8a 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -148,6 +148,7 @@ static void pc_init1(MemoryRegion *system_memory,
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
     void *fw_cfg = NULL;
+    PC *pc = PC(object_new(TYPE_PC_MACHINE));
 
     pc_cpus_init(cpu_model);
 
@@ -285,6 +286,8 @@ static void pc_init1(MemoryRegion *system_memory,
     if (pci_enabled) {
         pc_pci_device_init(pci_bus);
     }
+
+    qdev_init_nofail(DEVICE(pc));
 }
 
 static void pc_init_pci(ram_addr_t ram_size,
-- 
1.7.11.4

Re: [Qemu-devel] [RFC 02/18] pc: create PC object on pc_init1()

[Paolo Bonzini]

Il 03/10/2012 15:28, Eduardo Habkost ha scritto:
> It would be interesting to make the generic machine intialization code
> create a machine object instead, but changing the machine initialization
> function signature is a nightmare, so by now I am creating the object
> inside pc_init1().
> 
> The object is not used for anything by now, but it will be used during
> some steps of the initialization, later.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/pc_piix.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index fd5898f..28b5f8a 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -148,6 +148,7 @@ static void pc_init1(MemoryRegion *system_memory,
>      MemoryRegion *pci_memory;
>      MemoryRegion *rom_memory;
>      void *fw_cfg = NULL;
> +    PC *pc = PC(object_new(TYPE_PC_MACHINE));
>  
>      pc_cpus_init(cpu_model);
>  
> @@ -285,6 +286,8 @@ static void pc_init1(MemoryRegion *system_memory,
>      if (pci_enabled) {
>          pc_pci_device_init(pci_bus);
>      }
> +
> +    qdev_init_nofail(DEVICE(pc));
>  }
>  
>  static void pc_init_pci(ram_addr_t ram_size,
> 

Can you add a hook to QEMUMachine so that this object is created by
qdev_get_machine() and ends up at /machine?

Paolo

Re: [Qemu-devel] [RFC 02/18] pc: create PC object on pc_init1()

[Eduardo Habkost]

On Wed, Oct 03, 2012 at 04:40:06PM +0200, Paolo Bonzini wrote:
> Il 03/10/2012 15:28, Eduardo Habkost ha scritto:
> > It would be interesting to make the generic machine intialization code
> > create a machine object instead, but changing the machine initialization
> > function signature is a nightmare, so by now I am creating the object
> > inside pc_init1().
> > 
> > The object is not used for anything by now, but it will be used during
> > some steps of the initialization, later.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/pc_piix.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index fd5898f..28b5f8a 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -148,6 +148,7 @@ static void pc_init1(MemoryRegion *system_memory,
> >      MemoryRegion *pci_memory;
> >      MemoryRegion *rom_memory;
> >      void *fw_cfg = NULL;
> > +    PC *pc = PC(object_new(TYPE_PC_MACHINE));
> >  
> >      pc_cpus_init(cpu_model);
> >  
> > @@ -285,6 +286,8 @@ static void pc_init1(MemoryRegion *system_memory,
> >      if (pci_enabled) {
> >          pc_pci_device_init(pci_bus);
> >      }
> > +
> > +    qdev_init_nofail(DEVICE(pc));
> >  }
> >  
> >  static void pc_init_pci(ram_addr_t ram_size,
> > 
> 
> Can you add a hook to QEMUMachine so that this object is created by
> qdev_get_machine() and ends up at /machine?

Oh, I didn't know there was an existing "machine" object already, I
didn't know qdev_get_machine().

Shouldn't /machine be a child of the "container" class? That leads to
the other question you asked in another message: in this case, PC
wouldn't be a child of DeviceState, and we would need an additional
mechanism to allow non-DeviceState objects to use global properties.

(The sole reason I introduced the PC class was to allow the PC code to
use the compatibility "contiguous_apic_ids" global property.)

-- 
Eduardo

Re: [Qemu-devel] [RFC 02/18] pc: create PC object on pc_init1()

[Paolo Bonzini]

Il 03/10/2012 16:53, Eduardo Habkost ha scritto:
>> > 
>> > Can you add a hook to QEMUMachine so that this object is created by
>> > qdev_get_machine() and ends up at /machine?
> Oh, I didn't know there was an existing "machine" object already, I
> didn't know qdev_get_machine().
> 
> Shouldn't /machine be a child of the "container" class?

It should, but note that "container" is really just the same as
TYPE_OBJECT.  It is only for clarity that a difference class is used.
So using TYPE_DEVICE for now is not too bad.

Paolo

> That leads to
> the other question you asked in another message: in this case, PC
> wouldn't be a child of DeviceState, and we would need an additional
> mechanism to allow non-DeviceState objects to use global properties.
> 
> (The sole reason I introduced the PC class was to allow the PC code to
> use the compatibility "contiguous_apic_ids" global property.)

[Qemu-devel] [RFC 03/18] pc: add PC object argument to some init functions

[Eduardo Habkost]

Add an extra argument to:
 - pc_memory_init()
 - bochs_bios_init()
 - pc_cpus_init()
 - pc_new_cpu()

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c      | 13 +++++++------
 hw/pc.h      |  5 +++--
 hw/pc_piix.c |  4 ++--
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 9b68282..3cf56de 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -585,7 +585,7 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
-static void *bochs_bios_init(void)
+static void *bochs_bios_init(PC *pc)
 {
     void *fw_cfg;
     uint8_t *smbios_table;
@@ -903,7 +903,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model)
+static X86CPU *pc_new_cpu(PC *pc, const char *cpu_model)
 {
     X86CPU *cpu;
     CPUX86State *env;
@@ -921,7 +921,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
     return cpu;
 }
 
-void pc_cpus_init(const char *cpu_model)
+void pc_cpus_init(PC *pc, const char *cpu_model)
 {
     int i;
 
@@ -935,11 +935,12 @@ void pc_cpus_init(const char *cpu_model)
     }
 
     for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model);
+        pc_new_cpu(pc, cpu_model);
     }
 }
 
-void *pc_memory_init(MemoryRegion *system_memory,
+void *pc_memory_init(PC *pc,
+                    MemoryRegion *system_memory,
                     const char *kernel_filename,
                     const char *kernel_cmdline,
                     const char *initrd_filename,
@@ -988,7 +989,7 @@ void *pc_memory_init(MemoryRegion *system_memory,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init();
+    fw_cfg = bochs_bios_init(pc);
     rom_set_fw(fw_cfg);
 
     if (linux_boot) {
diff --git a/hw/pc.h b/hw/pc.h
index 77e898f..f1ca54e 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -111,8 +111,9 @@ typedef struct PC PC;
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
-void pc_cpus_init(const char *cpu_model);
-void *pc_memory_init(MemoryRegion *system_memory,
+void pc_cpus_init(PC *pc, const char *cpu_model);
+void *pc_memory_init(PC *pc,
+                    MemoryRegion *system_memory,
                     const char *kernel_filename,
                     const char *kernel_cmdline,
                     const char *initrd_filename,
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 28b5f8a..f861907 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -150,7 +150,7 @@ static void pc_init1(MemoryRegion *system_memory,
     void *fw_cfg = NULL;
     PC *pc = PC(object_new(TYPE_PC_MACHINE));
 
-    pc_cpus_init(cpu_model);
+    pc_cpus_init(pc, cpu_model);
 
     if (kvmclock_enabled) {
         kvmclock_create();
@@ -175,7 +175,7 @@ static void pc_init1(MemoryRegion *system_memory,
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        fw_cfg = pc_memory_init(system_memory,
+        fw_cfg = pc_memory_init(pc, system_memory,
                        kernel_filename, kernel_cmdline, initrd_filename,
                        below_4g_mem_size, above_4g_mem_size,
                        pci_enabled ? rom_memory : system_memory, &ram_memory);
-- 
1.7.11.4

Re: [Qemu-devel] [RFC 03/18] pc: add PC object argument to some init functions

[Igor Mammedov]

On Wed,  3 Oct 2012 10:28:59 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Add an extra argument to:
>  - pc_memory_init()
>  - bochs_bios_init()
>  - pc_cpus_init()
>  - pc_new_cpu()
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/pc.c      | 13 +++++++------
>  hw/pc.h      |  5 +++--
>  hw/pc_piix.c |  4 ++--
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 9b68282..3cf56de 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -585,7 +585,7 @@ int e820_add_entry(uint64_t address, uint64_t length,
> uint32_t type) return index;
>  }
>  
> -static void *bochs_bios_init(void)
> +static void *bochs_bios_init(PC *pc)
>  {
>      void *fw_cfg;
>      uint8_t *smbios_table;
> @@ -903,7 +903,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> level) }
>  }
>  
> -static X86CPU *pc_new_cpu(const char *cpu_model)
> +static X86CPU *pc_new_cpu(PC *pc, const char *cpu_model)
this function probably will go away.
https://github.com/imammedo/qemu/commit/ebdd53b8519d1071131855f5408a96f96c7df75a

I'll post latest version of that patch today.

>  {
>      X86CPU *cpu;
>      CPUX86State *env;
> @@ -921,7 +921,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>      return cpu;
>  }>  

> -void pc_cpus_init(const char *cpu_model)
> +void pc_cpus_init(PC *pc, const char *cpu_model)
>  {
>      int i;
>  
> @@ -935,11 +935,12 @@ void pc_cpus_init(const char *cpu_model)
>      }
>  
>      for(i = 0; i < smp_cpus; i++) {
> -        pc_new_cpu(cpu_model);
> +        pc_new_cpu(pc, cpu_model);
>      }
>  }
>  
> -void *pc_memory_init(MemoryRegion *system_memory,
> +void *pc_memory_init(PC *pc,
> +                    MemoryRegion *system_memory,
>                      const char *kernel_filename,
>                      const char *kernel_cmdline,
>                      const char *initrd_filename,
> @@ -988,7 +989,7 @@ void *pc_memory_init(MemoryRegion *system_memory,
>                                          option_rom_mr,
>                                          1);
>  
> -    fw_cfg = bochs_bios_init();
> +    fw_cfg = bochs_bios_init(pc);
>      rom_set_fw(fw_cfg);
>  
>      if (linux_boot) {
> diff --git a/hw/pc.h b/hw/pc.h
> index 77e898f..f1ca54e 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -111,8 +111,9 @@ typedef struct PC PC;
>  void pc_register_ferr_irq(qemu_irq irq);
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>  
> -void pc_cpus_init(const char *cpu_model);
> -void *pc_memory_init(MemoryRegion *system_memory,
> +void pc_cpus_init(PC *pc, const char *cpu_model);
> +void *pc_memory_init(PC *pc,
> +                    MemoryRegion *system_memory,
>                      const char *kernel_filename,
>                      const char *kernel_cmdline,
>                      const char *initrd_filename,
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 28b5f8a..f861907 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -150,7 +150,7 @@ static void pc_init1(MemoryRegion *system_memory,
>      void *fw_cfg = NULL;
>      PC *pc = PC(object_new(TYPE_PC_MACHINE));
>  
> -    pc_cpus_init(cpu_model);
> +    pc_cpus_init(pc, cpu_model);
>  
>      if (kvmclock_enabled) {
>          kvmclock_create();
> @@ -175,7 +175,7 @@ static void pc_init1(MemoryRegion *system_memory,
>  
>      /* allocate ram and load rom/bios */
>      if (!xen_enabled()) {
> -        fw_cfg = pc_memory_init(system_memory,
> +        fw_cfg = pc_memory_init(pc, system_memory,
>                         kernel_filename, kernel_cmdline, initrd_filename,
>                         below_4g_mem_size, above_4g_mem_size,
>                         pci_enabled ? rom_memory : system_memory,
> &ram_memory);

[Qemu-devel] [RFC 04/18] move I/O-related definitions from qemu-common.h to a new header (qemu-stdio.h)

[Eduardo Habkost]

This will help reduce the qemu-common.h dependency hell.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-common.h | 57 ++--------------------------------------------
 qemu-stdio.h  | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 55 deletions(-)
 create mode 100644 qemu-stdio.h

diff --git a/qemu-common.h b/qemu-common.h
index 15d9e4e..ed7ee81 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -6,6 +6,8 @@
 #include "compiler.h"
 #include "config-host.h"
 
+#include "qemu-stdio.h"
+
 #if defined(__arm__) || defined(__sparc__) || defined(__mips__) || defined(__hppa__) || defined(__ia64__)
 #define WORDS_ALIGNED
 #endif
@@ -48,28 +50,6 @@ typedef struct MigrationParams MigrationParams;
 #include "qemu-os-posix.h"
 #endif
 
-#ifndef O_LARGEFILE
-#define O_LARGEFILE 0
-#endif
-#ifndef O_BINARY
-#define O_BINARY 0
-#endif
-#ifndef MAP_ANONYMOUS
-#define MAP_ANONYMOUS MAP_ANON
-#endif
-#ifndef ENOMEDIUM
-#define ENOMEDIUM ENODEV
-#endif
-#if !defined(ENOTSUP)
-#define ENOTSUP 4096
-#endif
-#if !defined(ECANCELED)
-#define ECANCELED 4097
-#endif
-#ifndef TIME_MAX
-#define TIME_MAX LONG_MAX
-#endif
-
 /* HOST_LONG_BITS is the size of a native pointer in bits. */
 #if UINTPTR_MAX == UINT32_MAX
 # define HOST_LONG_BITS 32
@@ -79,39 +59,6 @@ typedef struct MigrationParams MigrationParams;
 # error Unknown pointer size
 #endif
 
-#ifndef CONFIG_IOVEC
-#define CONFIG_IOVEC
-struct iovec {
-    void *iov_base;
-    size_t iov_len;
-};
-/*
- * Use the same value as Linux for now.
- */
-#define IOV_MAX		1024
-#else
-#include <sys/uio.h>
-#endif
-
-typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
-    GCC_FMT_ATTR(2, 3);
-
-#ifdef _WIN32
-#define fsync _commit
-#if !defined(lseek)
-# define lseek _lseeki64
-#endif
-int qemu_ftruncate64(int, int64_t);
-#if !defined(ftruncate)
-# define ftruncate qemu_ftruncate64
-#endif
-
-static inline char *realpath(const char *path, char *resolved_path)
-{
-    _fullpath(resolved_path, path, _MAX_PATH);
-    return resolved_path;
-}
-#endif
 
 /* icount */
 void configure_icount(const char *option);
diff --git a/qemu-stdio.h b/qemu-stdio.h
new file mode 100644
index 0000000..73d9f91
--- /dev/null
+++ b/qemu-stdio.h
@@ -0,0 +1,73 @@
+/* Some basic definitions related to stdio.h or other I/O interfaces
+ */
+#ifndef QEMU_STDIO_H
+#define QEMU_STDIO_H
+
+#include "compiler.h"
+#include "config-host.h"
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include <sys/mman.h>
+
+#ifndef O_LARGEFILE
+#define O_LARGEFILE 0
+#endif
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+#ifndef MAP_ANONYMOUS
+#define MAP_ANONYMOUS MAP_ANON
+#endif
+#ifndef ENOMEDIUM
+#define ENOMEDIUM ENODEV
+#endif
+#if !defined(ENOTSUP)
+#define ENOTSUP 4096
+#endif
+#if !defined(ECANCELED)
+#define ECANCELED 4097
+#endif
+#ifndef TIME_MAX
+#define TIME_MAX LONG_MAX
+#endif
+
+#ifndef CONFIG_IOVEC
+#define CONFIG_IOVEC
+struct iovec {
+    void *iov_base;
+    size_t iov_len;
+};
+/*
+ * Use the same value as Linux for now.
+ */
+#define IOV_MAX     1024
+#else
+#include <sys/uio.h>
+#endif
+
+typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
+
+#ifdef _WIN32
+#define fsync _commit
+#if !defined(lseek)
+# define lseek _lseeki64
+#endif
+int qemu_ftruncate64(int, int64_t);
+#if !defined(ftruncate)
+# define ftruncate qemu_ftruncate64
+#endif
+
+static inline char *realpath(const char *path, char *resolved_path)
+{
+    _fullpath(resolved_path, path, _MAX_PATH);
+    return resolved_path;
+}
+#endif
+
+#endif /* QEMU_STDIO_H */
-- 
1.7.11.4

[Qemu-devel] [RFC 05/18] cpus.h: include qemu-stdio.h

[Eduardo Habkost]

Needed for the definition of fprint_function.

This is not necessary right now, but it will be necessary if code that
doesn't include cpu-common.h includes cpus.h.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 cpus.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cpus.h b/cpus.h
index 81bd817..b7c3708 100644
--- a/cpus.h
+++ b/cpus.h
@@ -1,6 +1,8 @@
 #ifndef QEMU_CPUS_H
 #define QEMU_CPUS_H
 
+#include "qemu-stdio.h"
+
 /* cpus.c */
 void qemu_init_cpu_loop(void);
 void resume_all_vcpus(void);
-- 
1.7.11.4

Re: [Qemu-devel] [RFC 05/18] cpus.h: include qemu-stdio.h

[Igor Mammedov]

On Wed,  3 Oct 2012 10:29:01 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Needed for the definition of fprint_function.
> 
> This is not necessary right now, but it will be necessary if code that
> doesn't include cpu-common.h includes cpus.h.
Could be it cut to a separate standard hearders, and include only that on
which it depends?

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  cpus.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/cpus.h b/cpus.h
> index 81bd817..b7c3708 100644
> --- a/cpus.h
> +++ b/cpus.h
> @@ -1,6 +1,8 @@
>  #ifndef QEMU_CPUS_H
>  #define QEMU_CPUS_H
>  
> +#include "qemu-stdio.h"
> +
>  /* cpus.c */
>  void qemu_init_cpu_loop(void);
>  void resume_all_vcpus(void);

Re: [Qemu-devel] [RFC 05/18] cpus.h: include qemu-stdio.h

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 02:00:57PM +0200, Igor Mammedov wrote:
> On Wed,  3 Oct 2012 10:29:01 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Needed for the definition of fprint_function.
> > 
> > This is not necessary right now, but it will be necessary if code that
> > doesn't include cpu-common.h includes cpus.h.
> Could be it cut to a separate standard hearders, and include only that on
> which it depends?

Are you talking about cpus.h, or qemu-stdio.h?

qemu-stdio.h is a new header, and is very small. I don't see how we
could split it further.

cpus.h is tiny, too, I don't see why we would split it.

> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  cpus.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/cpus.h b/cpus.h
> > index 81bd817..b7c3708 100644
> > --- a/cpus.h
> > +++ b/cpus.h
> > @@ -1,6 +1,8 @@
> >  #ifndef QEMU_CPUS_H
> >  #define QEMU_CPUS_H
> >  
> > +#include "qemu-stdio.h"
> > +
> >  /* cpus.c */
> >  void qemu_init_cpu_loop(void);
> >  void resume_all_vcpus(void);
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 05/18] cpus.h: include qemu-stdio.h

[Igor Mammedov]

On Thu, 4 Oct 2012 10:14:48 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 04, 2012 at 02:00:57PM +0200, Igor Mammedov wrote:
> > On Wed,  3 Oct 2012 10:29:01 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > Needed for the definition of fprint_function.
> > > 
> > > This is not necessary right now, but it will be necessary if code that
> > > doesn't include cpu-common.h includes cpus.h.
> > Could be it cut to a separate standard hearders, and include only that on
> > which it depends?
> 
> Are you talking about cpus.h, or qemu-stdio.h?
Ah, never mind, I see it is included for fprint_function().

> 
> qemu-stdio.h is a new header, and is very small. I don't see how we
> could split it further.
> 
> cpus.h is tiny, too, I don't see why we would split it.
> 
> > 
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  cpus.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/cpus.h b/cpus.h
> > > index 81bd817..b7c3708 100644
> > > --- a/cpus.h
> > > +++ b/cpus.h
> > > @@ -1,6 +1,8 @@
> > >  #ifndef QEMU_CPUS_H
> > >  #define QEMU_CPUS_H
> > >  
> > > +#include "qemu-stdio.h"
> > > +
> > >  /* cpus.c */
> > >  void qemu_init_cpu_loop(void);
> > >  void resume_all_vcpus(void);
> > 
> > 
> 

[Qemu-devel] [RFC 06/18] hw/apic.c: rename bit functions to not conflict with bitops.h (v2)

[Eduardo Habkost]

Changes v1 -> v2:
 - Coding style change: break too-long line

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/apic.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 385555e..e1f633a 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -51,7 +51,7 @@ static int ffs_bit(uint32_t value)
     return ctz32(value);
 }
 
-static inline void set_bit(uint32_t *tab, int index)
+static inline void apic_set_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -59,7 +59,7 @@ static inline void set_bit(uint32_t *tab, int index)
     tab[i] |= mask;
 }
 
-static inline void reset_bit(uint32_t *tab, int index)
+static inline void apic_reset_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -67,7 +67,7 @@ static inline void reset_bit(uint32_t *tab, int index)
     tab[i] &= ~mask;
 }
 
-static inline int get_bit(uint32_t *tab, int index)
+static inline int apic_get_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -184,7 +184,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
         case APIC_DM_FIXED:
             if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
                 break;
-            reset_bit(s->irr, lvt & 0xff);
+            apic_reset_bit(s->irr, lvt & 0xff);
             /* fall through */
         case APIC_DM_EXTINT:
             cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
@@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
 
 static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
 {
-    apic_report_irq_delivered(!get_bit(s->irr, vector_num));
+    apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
 
-    set_bit(s->irr, vector_num);
+    apic_set_bit(s->irr, vector_num);
     if (trigger_mode)
-        set_bit(s->tmr, vector_num);
+        apic_set_bit(s->tmr, vector_num);
     else
-        reset_bit(s->tmr, vector_num);
+        apic_reset_bit(s->tmr, vector_num);
     if (s->vapic_paddr) {
         apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
         /*
@@ -405,8 +405,9 @@ static void apic_eoi(APICCommonState *s)
     isrv = get_highest_priority_int(s->isr);
     if (isrv < 0)
         return;
-    reset_bit(s->isr, isrv);
-    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
+    apic_reset_bit(s->isr, isrv);
+    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) &&
+            apic_get_bit(s->tmr, isrv)) {
         ioapic_eoi_broadcast(isrv);
     }
     apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
@@ -445,7 +446,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
             int idx = apic_find_dest(dest);
             memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
             if (idx >= 0)
-                set_bit(deliver_bitmask, idx);
+                apic_set_bit(deliver_bitmask, idx);
         }
     } else {
         /* XXX: cluster mode */
@@ -455,11 +456,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
             if (apic_iter) {
                 if (apic_iter->dest_mode == 0xf) {
                     if (dest & apic_iter->log_dest)
-                        set_bit(deliver_bitmask, i);
+                        apic_set_bit(deliver_bitmask, i);
                 } else if (apic_iter->dest_mode == 0x0) {
                     if ((dest & 0xf0) == (apic_iter->log_dest & 0xf0) &&
                         (dest & apic_iter->log_dest & 0x0f)) {
-                        set_bit(deliver_bitmask, i);
+                        apic_set_bit(deliver_bitmask, i);
                     }
                 }
             } else {
@@ -502,14 +503,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
         break;
     case 1:
         memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
-        set_bit(deliver_bitmask, s->idx);
+        apic_set_bit(deliver_bitmask, s->idx);
         break;
     case 2:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
         break;
     case 3:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
-        reset_bit(deliver_bitmask, s->idx);
+        apic_reset_bit(deliver_bitmask, s->idx);
         break;
     }
 
@@ -566,8 +567,8 @@ int apic_get_interrupt(DeviceState *d)
         apic_sync_vapic(s, SYNC_TO_VAPIC);
         return s->spurious_vec & 0xff;
     }
-    reset_bit(s->irr, intno);
-    set_bit(s->isr, intno);
+    apic_reset_bit(s->irr, intno);
+    apic_set_bit(s->isr, intno);
     apic_sync_vapic(s, SYNC_TO_VAPIC);
 
     /* re-inject if there is still a pending PIC interrupt */
-- 
1.7.11.4

[Qemu-devel] [RFC 07/18] kvm: create kvm_arch_vcpu_id() function

[Eduardo Habkost]

This will allow each architecture to define how the VCPU ID is set on
the KVM_CREATE_VCPU ioctl call.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 kvm-all.c          | 2 +-
 kvm.h              | 3 +++
 target-i386/kvm.c  | 5 +++++
 target-ppc/kvm.c   | 5 +++++
 target-s390x/kvm.c | 5 +++++
 5 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/kvm-all.c b/kvm-all.c
index 92a7137..2fb2596 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUArchState *env)
 
     DPRINTF("kvm_init_vcpu\n");
 
-    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
+    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(env));
     if (ret < 0) {
         DPRINTF("kvm_create_vcpu failed\n");
         goto err;
diff --git a/kvm.h b/kvm.h
index dea2998..e88058f 100644
--- a/kvm.h
+++ b/kvm.h
@@ -181,6 +181,9 @@ int kvm_arch_init(KVMState *s);
 
 int kvm_arch_init_vcpu(CPUArchState *env);
 
+/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
+unsigned long kvm_arch_vcpu_id(CPUArchState *env);
+
 void kvm_arch_reset_vcpu(CPUArchState *env);
 
 int kvm_arch_on_sigbus_vcpu(CPUArchState *env, int code, void *addr);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ce8678d..bb972d8 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -353,6 +353,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
     }
 }
 
+unsigned long kvm_arch_vcpu_id(CPUArchState *env)
+{
+    return env->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUX86State *env)
 {
     struct {
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index a31d278..e74a189 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -371,6 +371,11 @@ static inline void kvm_fixup_page_sizes(CPUPPCState *env)
 
 #endif /* !defined (TARGET_PPC64) */
 
+unsigned long kvm_arch_vcpu_id(CPUArchState *env)
+{
+    return env->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUPPCState *cenv)
 {
     int ret;
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 07edf93..9e4c6c6 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
     return 0;
 }
 
+unsigned long kvm_arch_vcpu_id(CPUArchState *env)
+{
+    return env->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUS390XState *env)
 {
     int ret = 0;
-- 
1.7.11.4

[Qemu-devel] [RFC 08/18] target-i386: kvm: set vcpu_id to APIC ID instead of CPU index (v2)

[Eduardo Habkost]

The CPU ID in KVM is supposed to be the APIC ID, so change the
KVM_CREATE_VCPU call to match it. The current behavior didn't break
anything yet because today the APIC ID is assumed to be == the CPU
index, but this won't be true in the future.

Chagnes v1 -> v2:
 - Change only i386 code (kvm_arch_vcpu_id())

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index bb972d8..45ebddc 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -355,7 +355,7 @@ static void cpu_update_state(void *opaque, int running, RunState state)
 
 unsigned long kvm_arch_vcpu_id(CPUArchState *env)
 {
-    return env->cpu_index;
+    return env->cpuid_apic_id;
 }
 
 int kvm_arch_init_vcpu(CPUX86State *env)
-- 
1.7.11.4

[Qemu-devel] [RFC 09/18] target-i386: cpu: move cpuid_apic_id initialization to cpu_x86_register()

[Eduardo Habkost]

The problem here is:

 - The CPU object can't define its APIC ID itself, it has to be
   defined by the creator of the CPU object
 - The object instance_init() function can't get extra arguments
 - I don't want to add a APIC ID argument to x86_cpu_realize(), as it
   should eventually become a ObjectClass::realize() method

So, the only place where the CPU object can get an additional
x86-specific initialization argument (the APIC ID) is
cpu_x86_register(). This patch just moves the initialization code there,
the apic_id argument to cpu_x86_register() will be added later.

The cpuid_apic_id field is used only at:

 - pc_new_cpu(), after the cpu_x86_register() call
 - kvm_init_vcpu(), that is called from the VCPU thread
   created by qemu_init_vcpu(), called by x86_cpu_realize()
   (called after cpu_x86_register())
 - 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 initialize it later.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1628b0e..c0db73e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1733,6 +1733,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     QDict *features = NULL;
     char *name = NULL;
 
+    cpu->env.cpuid_apic_id = env->cpu_index;
+
     /* for CPU subclasses should go into cpu_x86_init() before object_new() */
     compat_normalize_cpu_model(cpu_model, &name, &features, &error);
     if (error_is_set(&error)) {
@@ -2361,8 +2363,6 @@ static void x86_cpu_initfn(Object *obj)
     x86_register_cpuid_properties(obj, svm_feature_name);
     x86_register_cpuid_properties(obj, cpuid_7_0_ebx_feature_name);
 
-    env->cpuid_apic_id = env->cpu_index;
-
     /* init various static tables used in TCG mode */
     if (tcg_enabled() && !inited) {
         inited = 1;
-- 
1.7.11.4

[Qemu-devel] [RFC 10/18] target-i386: cpu: add apic_id argument to cpu_x86_register()

[Eduardo Habkost]

It's not up to the CPU object to decide its APIC ID, but to the CPU
object creator (that knows about the CPU sockets, cores, and threads
topology).

This keeps the current APIC ID == CPU index behavior, by now.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c    | 4 ++--
 target-i386/cpu.h    | 2 +-
 target-i386/helper.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c0db73e..063f5a6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1726,14 +1726,14 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+int cpu_x86_register(X86CPU *cpu, const char *cpu_model, uint32_t apic_id)
 {
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
     QDict *features = NULL;
     char *name = NULL;
 
-    cpu->env.cpuid_apic_id = env->cpu_index;
+    cpu->env.cpuid_apic_id = apic_id;
 
     /* for CPU subclasses should go into cpu_x86_init() before object_new() */
     compat_normalize_cpu_model(cpu_model, &name, &features, &error);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 5edadd1..f37e80b 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -990,7 +990,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
+int cpu_x86_register(X86CPU *cpu, const char *cpu_model, uint32_t apic_id);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 1d39ba9..70a9f72 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1249,7 +1249,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
-    if (cpu_x86_register(cpu, cpu_model) < 0) {
+    if (cpu_x86_register(cpu, cpu_model, env->cpu_index) < 0) {
         object_delete(OBJECT(cpu));
         return NULL;
     }
-- 
1.7.11.4

[Qemu-devel] [RFC 11/18] target-i386: cpu_x86_init: allow APIC ID to be set by caller

[Eduardo Habkost]

This allows callers of cpu_x86_init() to override the APIC ID, in case
it needs to build a specific cores/threads topology.

Because *-user doesn't have any concept of CPU topology, we do not
require every caller to specify an APIC ID. So a negative value will
indicate that the CPU index can be used as APIC ID, and *-user will use
that mode.

By now, all the callers use the default behavior, that's using the CPU
index as APIC ID, but later the PC code will be changed to calculate the
APIC IDs according to CPU topology.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c              |  2 +-
 target-i386/cpu.h    |  4 ++--
 target-i386/helper.c | 15 +++++++++++++--
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 3cf56de..0915a9b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -908,7 +908,7 @@ static X86CPU *pc_new_cpu(PC *pc, const char *cpu_model)
     X86CPU *cpu;
     CPUX86State *env;
 
-    cpu = cpu_x86_init(cpu_model);
+    cpu = cpu_x86_init(cpu_model, -1);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to find x86 CPU definition\n");
         exit(1);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f37e80b..3b6445c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -874,7 +874,7 @@ typedef struct CPUX86State {
 
 #include "cpu-qom.h"
 
-X86CPU *cpu_x86_init(const char *cpu_model);
+X86CPU *cpu_x86_init(const char *cpu_model, long apic_id);
 int cpu_x86_exec(CPUX86State *s);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 void x86_cpudef_setup(void);
@@ -1050,7 +1050,7 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 
 static inline CPUX86State *cpu_init(const char *cpu_model)
 {
-    X86CPU *cpu = cpu_x86_init(cpu_model);
+    X86CPU *cpu = cpu_x86_init(cpu_model, -1);
     if (cpu == NULL) {
         return NULL;
     }
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 70a9f72..423d8da 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1239,7 +1239,15 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
     return 1;
 }
 
-X86CPU *cpu_x86_init(const char *cpu_model)
+/**
+ * cpu_x86_init:
+ *
+ * Creates and initializes a X86CPU object.
+ *
+ * @apic_id: sets a specific APIC ID for the CPU. If negative, the CPU index
+ *           will be used as APIC ID.
+ */
+X86CPU *cpu_x86_init(const char *cpu_model, long apic_id)
 {
     X86CPU *cpu;
     CPUX86State *env;
@@ -1249,7 +1257,10 @@ X86CPU *cpu_x86_init(const char *cpu_model)
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
-    if (cpu_x86_register(cpu, cpu_model, env->cpu_index) < 0) {
+    if (apic_id < 0) {
+        apic_id = env->cpu_index;
+    }
+    if (cpu_x86_register(cpu, cpu_model, apic_id) < 0) {
         object_delete(OBJECT(cpu));
         return NULL;
     }
-- 
1.7.11.4

Re: [Qemu-devel] [RFC 11/18] target-i386: cpu_x86_init: allow APIC ID to be set by caller

[Igor Mammedov]

On Wed,  3 Oct 2012 10:29:07 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This allows callers of cpu_x86_init() to override the APIC ID, in case
> it needs to build a specific cores/threads topology.
> 
> Because *-user doesn't have any concept of CPU topology, we do not
> require every caller to specify an APIC ID. So a negative value will
> indicate that the CPU index can be used as APIC ID, and *-user will use
> that mode.
> 
> By now, all the callers use the default behavior, that's using the CPU
> index as APIC ID, but later the PC code will be changed to calculate the
> APIC IDs according to CPU topology.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/pc.c              |  2 +-
>  target-i386/cpu.h    |  4 ++--
>  target-i386/helper.c | 15 +++++++++++++--
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 3cf56de..0915a9b 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -908,7 +908,7 @@ static X86CPU *pc_new_cpu(PC *pc, const char *cpu_model)
>      X86CPU *cpu;
>      CPUX86State *env;
>  
> -    cpu = cpu_x86_init(cpu_model);
> +    cpu = cpu_x86_init(cpu_model, -1);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          exit(1);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index f37e80b..3b6445c 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -874,7 +874,7 @@ typedef struct CPUX86State {
>  
>  #include "cpu-qom.h"
>  
> -X86CPU *cpu_x86_init(const char *cpu_model);
> +X86CPU *cpu_x86_init(const char *cpu_model, long apic_id);
>  int cpu_x86_exec(CPUX86State *s);
>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>  void x86_cpudef_setup(void);
> @@ -1050,7 +1050,7 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>  
>  static inline CPUX86State *cpu_init(const char *cpu_model)
>  {
> -    X86CPU *cpu = cpu_x86_init(cpu_model);
> +    X86CPU *cpu = cpu_x86_init(cpu_model, -1);
>      if (cpu == NULL) {
>          return NULL;
>      }
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 70a9f72..423d8da 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1239,7 +1239,15 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> unsigned int selector, return 1;
>  }
>  
> -X86CPU *cpu_x86_init(const char *cpu_model)
> +/**
> + * cpu_x86_init:
> + *
> + * Creates and initializes a X86CPU object.
> + *
> + * @apic_id: sets a specific APIC ID for the CPU. If negative, the CPU
> index
> + *           will be used as APIC ID.
> + */
> +X86CPU *cpu_x86_init(const char *cpu_model, long apic_id)
Risking to being slapped again:

We are trying to move from cpu_model and probably cpu_init itself,
once CPU could be created & initialized just as any QOM object. So
maybe cpu_x86_init() could be left alone just as it is now for
using in *-linux-user and softmmu part could done in pc_cpus_init().

That will allow to play with machine specific properties only for softmmu
instead of additional ifdef-enery in cpu_x86_init() since apic is not part
of *-linux-user.
Additionally it would allow to set parent of CPU at board level right
after CPU is created and at realize time set back-link<> from apic to
CPU.

>  {
>      X86CPU *cpu;
>      CPUX86State *env;
> @@ -1249,7 +1257,10 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>      env = &cpu->env;
>      env->cpu_model_str = cpu_model;
>  
> -    if (cpu_x86_register(cpu, cpu_model, env->cpu_index) < 0) {
> +    if (apic_id < 0) {
> +        apic_id = env->cpu_index;
> +    }
> +    if (cpu_x86_register(cpu, cpu_model, apic_id) < 0) {
>          object_delete(OBJECT(cpu));
>          return NULL;
>      }

[Qemu-devel] [RFC 12/18] fw_cfg: remove FW_CFG_MAX_CPUS from fw_cfg_init()

[Eduardo Habkost]

PC will not use max_cpus for that field, so move it outside the common
code so it can use a different value on PC.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/fw_cfg.c       | 1 -
 hw/pc.c           | 2 +-
 hw/ppc_newworld.c | 1 +
 hw/ppc_oldworld.c | 1 +
 hw/sun4m.c        | 3 +++
 hw/sun4u.c        | 1 +
 6 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index dcde1a9..80132c2 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -518,7 +518,6 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
-    fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
     fw_cfg_bootsplash(s);
     fw_cfg_reboot(s);
diff --git a/hw/pc.c b/hw/pc.c
index 0915a9b..a8788de 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -600,7 +600,7 @@ static void *bochs_bios_init(PC *pc)
     register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL);
 
     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_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, (uint8_t *)acpi_tables,
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index e95cfe8..a25187d 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -381,6 +381,7 @@ static void ppc_core99_init (ram_addr_t ram_size,
     /* No PCI init: the BIOS will do it */
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     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_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
index 1dcd8a6..3596ad2 100644
--- a/hw/ppc_oldworld.c
+++ b/hw/ppc_oldworld.c
@@ -296,6 +296,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
     /* No PCI init: the BIOS will do it */
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     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_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
diff --git a/hw/sun4m.c b/hw/sun4m.c
index c98cd5e..2c4b3ff 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -1018,6 +1018,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
                  hwdef->ecc_version);
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     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_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
@@ -1629,6 +1630,7 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
                "Sun4d");
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     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_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
@@ -1823,6 +1825,7 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
                "Sun4c");
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     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_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
diff --git a/hw/sun4u.c b/hw/sun4u.c
index 07cd042..f9acd68 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -874,6 +874,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
                            (uint8_t *)&nd_table[0].macaddr);
 
     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_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_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
-- 
1.7.11.4

[Qemu-devel] [RFC 13/18] pc: set explicit APIC ID for CPUs

[Eduardo Habkost]

The current behavior (setting APIC ID = CPU index) is kept, but this
will allow the PC code to set the proper CPU topology, later.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index a8788de..c64c218 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -903,19 +903,19 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(PC *pc, const char *cpu_model)
+static X86CPU *pc_new_cpu(PC *pc, const char *cpu_model, uint32_t apic_id)
 {
     X86CPU *cpu;
     CPUX86State *env;
 
-    cpu = cpu_x86_init(cpu_model, -1);
+    cpu = cpu_x86_init(cpu_model, apic_id);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to find x86 CPU definition\n");
         exit(1);
     }
     env = &cpu->env;
     if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
-        env->apic_state = apic_init(env, env->cpuid_apic_id);
+        env->apic_state = apic_init(env, apic_id);
     }
     cpu_reset(CPU(cpu));
     return cpu;
@@ -935,7 +935,7 @@ void pc_cpus_init(PC *pc, const char *cpu_model)
     }
 
     for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(pc, cpu_model);
+        pc_new_cpu(pc, cpu_model, i);
     }
 }
 
-- 
1.7.11.4

[Qemu-devel] [RFC 14/18] pc: create apic_id_for_cpu() function (v3)

[Eduardo Habkost]

Currently we need a way to calculate the Initial APIC ID using only the
CPU index (without needing a CPU object), as the NUMA fw_cfg data is
APIC-ID-based, and may include data for hotplug CPUs (that don't exist
yet), up to max_cpus.

Changes v2 -> v3:
  - Move the whole code to hw/pc.c, now only the PC code will need
    to handle topology-based APIC IDs

Changes v1 -> v2:
  - make function return value 'unsigned int' (it's not specific for the 8-bit
    xAPIC ID)
  - move implementation to cpu.c

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/pc.c b/hw/pc.c
index c64c218..4b30f38 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -568,6 +568,20 @@ static void pc_register_type(void)
 
 type_init(pc_register_type);
 
+/* 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, as the QEMU<->Seabios interfaces have no concept of "CPU index",
+ * and the NUMA tables need the APIC ID of all CPUs up to max_cpus.
+ */
+static uint32_t apic_id_for_cpu(PC *pc, 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);
@@ -935,7 +949,7 @@ void pc_cpus_init(PC *pc, const char *cpu_model)
     }
 
     for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(pc, cpu_model, i);
+        pc_new_cpu(pc, cpu_model, apic_id_for_cpu(pc, i));
     }
 }
 
-- 
1.7.11.4

[Qemu-devel] [RFC 15/18] pc: set fw_cfg data based on APIC ID calculation (v2)

[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).

Changes v1 -> v2:
 - Get PC object as argument
 - Add more detailed comments explaining the reason for FW_CFG_MAX_CPUS
   not being simply 'max_cpus'

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 4b30f38..4c9b2f6 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -599,6 +599,15 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
+/* Returns the limit to APIC ID values
+ *
+ * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
+ */
+static unsigned int apic_id_limit(PC *pc)
+{
+    return apic_id_for_cpu(pc, max_cpus - 1) + 1;
+}
+
 static void *bochs_bios_init(PC *pc)
 {
     void *fw_cfg;
@@ -606,6 +615,7 @@ static void *bochs_bios_init(PC *pc)
     size_t smbios_len;
     uint64_t *numa_fw_cfg;
     int i, j;
+    unsigned int max_apic_id = apic_id_limit(pc);
 
     register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
 
@@ -614,7 +624,21 @@ static void *bochs_bios_init(PC *pc)
     register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL);
 
     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)max_apic_id);
     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, (uint8_t *)acpi_tables,
@@ -634,21 +658,24 @@ static void *bochs_bios_init(PC *pc)
      * of nodes, one word for each VCPU->node and one word for each node to
      * hold the amount of memory.
      */
-    numa_fw_cfg = g_malloc0((1 + max_cpus + nb_numa_nodes) * 8);
+    numa_fw_cfg = g_malloc0((1 + max_apic_id + nb_numa_nodes) * 8);
     numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
-    for (i = 0; i < max_cpus; i++) {
+    unsigned int cpu_idx;
+    for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) {
+        unsigned int apic_id = apic_id_for_cpu(pc, cpu_idx);
+        assert(apic_id < max_apic_id);
         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;
             }
         }
     }
     for (i = 0; i < nb_numa_nodes; i++) {
-        numa_fw_cfg[max_cpus + 1 + i] = cpu_to_le64(node_mem[i]);
+        numa_fw_cfg[max_apic_id + 1 + i] = cpu_to_le64(node_mem[i]);
     }
     fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, (uint8_t *)numa_fw_cfg,
-                     (1 + max_cpus + nb_numa_nodes) * 8);
+                     (1 + max_apic_id + nb_numa_nodes) * 8);
 
     return fw_cfg;
 }
-- 
1.7.11.4

[Qemu-devel] [RFC 16/18] tests: support target-specific unit tests

[Eduardo Habkost]

To make unit tests that depend on target-specific files, use
check-unit-<arch>-y and test-obj-<arch>-y.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/Makefile | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 26a67ce..ce4f6f2 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -40,8 +40,6 @@ test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
 test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
 test-qapi-obj-y += module.o
 
-$(test-obj-y): QEMU_INCLUDES += -Itests
-
 tests/check-qint$(EXESUF): tests/check-qint.o qint.o $(tools-obj-y)
 tests/check-qstring$(EXESUF): tests/check-qstring.o qstring.o $(tools-obj-y)
 tests/check-qdict$(EXESUF): tests/check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qlist.o $(tools-obj-y)
@@ -75,9 +73,21 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
 tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
 
-# QTest rules
+# unit test rules:
 
 TARGETS=$(patsubst %-softmmu,%, $(filter %-softmmu,$(TARGET_DIRS)))
+
+# target-specific tests/objs:
+test-obj-y += $(foreach TARGET,$(TARGETS), $(test-obj-$(TARGET)-y))
+check-unit-y += $(foreach TARGET,$(TARGETS), $(check-unit-$(TARGET)-y))
+
+$(foreach TARGET,$(TARGETS),$(eval $(test-obj-$(TARGET)-y): QEMU_INCLUDES += -Itarget-$(TARGET)))
+
+
+$(test-obj-y): QEMU_INCLUDES += -Itests
+
+# QTest rules
+
 QTEST_TARGETS=$(foreach TARGET,$(TARGETS), $(if $(check-qtest-$(TARGET)-y), $(TARGET),))
 check-qtest-$(CONFIG_POSIX)=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y))
 
-- 
1.7.11.4

[Qemu-devel] [RFC 17/18] target-i386: topology & APIC ID utility functions (v2)

[Eduardo Habkost]

Changes v2 -> v3:
 - Add documentation pointers to the code
 - Rename bits_for_count() to bitwidth_for_count()
 - Remove unused apicid_*_id() functions

Changes v1 -> v2:
 - Support 32-bit APIC IDs (in case x2APIC is going to be used)
 - Coding style changes
 - Use TARGET_I386_TOPOLOGY_H instead of __QEMU_X86_TOPOLOGY_H__
 - Rename topo_make_apic_id() to topo_apicid_for_cpu()
 - Rename __make_apicid() to topo_make_apicid()
 - Spaces around operators on test-x86-cpuid.c, as requested by
   Blue Swirl
 - Make test-x86-cpuid a target-specific test

Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/.gitignore       |   1 +
 tests/Makefile         |   3 ++
 tests/test-x86-cpuid.c | 102 +++++++++++++++++++++++++++++++++++++
 4 files changed, 239 insertions(+)
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c

diff --git a/target-i386/topology.h b/target-i386/topology.h
new file mode 100644
index 0000000..6f4f2ff
--- /dev/null
+++ b/target-i386/topology.h
@@ -0,0 +1,133 @@
+/*
+ *  x86 CPU topology data structures and functions
+ *
+ *  Copyright (c) 2012 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef TARGET_I386_TOPOLOGY_H
+#define TARGET_I386_TOPOLOGY_H
+
+/* This file implements the APIC-ID-based CPU topology enumeration logic,
+ * documented at the following document:
+ *   Intel® 64 Architecture Processor Topology Enumeration
+ *   http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
+ *
+ * This code should be compatible with AMD's "Extended Method" described at:
+ *   AMD CPUID Specification (Publication #25481)
+ *   Section 3: Multiple Core Calcuation
+ * as long as:
+ *  nr_threads is set to 1;
+ *  OFFSET_IDX is assumed to be 0;
+ *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
+ */
+
+#include <stdint.h>
+#include <string.h>
+
+#include "bitops.h"
+
+/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
+ */
+typedef uint32_t apic_id_t;
+
+/* Return the bit width needed for 'count' IDs
+ */
+static unsigned bitwidth_for_count(unsigned count)
+{
+    g_assert(count >= 1);
+    if (count == 1) {
+        return 0;
+    }
+    return bitops_flsl(count - 1) + 1;
+}
+
+/* Bit width of the SMT_ID (thread ID) field on the APIC ID
+ */
+static inline unsigned apicid_smt_width(unsigned nr_cores, unsigned nr_threads)
+{
+    return bitwidth_for_count(nr_threads);
+}
+
+/* Bit width of the Core_ID field
+ */
+static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
+{
+    return bitwidth_for_count(nr_cores);
+}
+
+/* Bit offset of the Core_ID field
+ */
+static inline unsigned apicid_core_offset(unsigned nr_cores,
+                                          unsigned nr_threads)
+{
+    return apicid_smt_width(nr_cores, nr_threads);
+}
+
+/* Bit offset of the Pkg_ID (socket ID) field
+ */
+static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
+{
+    return apicid_core_offset(nr_cores, nr_threads) + \
+           apicid_core_width(nr_cores, nr_threads);
+}
+
+/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
+ *
+ * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
+ */
+static inline apic_id_t topo_make_apicid(unsigned nr_cores,
+                                         unsigned nr_threads,
+                                         unsigned pkg_id, unsigned core_id,
+                                         unsigned smt_id)
+{
+    return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) | \
+           (core_id << apicid_core_offset(nr_cores, nr_threads)) | \
+           smt_id;
+}
+
+/* Calculate thread/core/package IDs for a specific topology,
+ * based on (contiguous) CPU index
+ */
+static inline void topo_ids_from_idx(unsigned nr_cores, unsigned nr_threads,
+                                     unsigned cpu_index,
+                                     unsigned *pkg_id, unsigned *core_id,
+                                     unsigned *smt_id)
+{
+    unsigned core_index = cpu_index / nr_threads;
+    *smt_id = cpu_index % nr_threads;
+    *core_id = core_index % nr_cores;
+    *pkg_id = core_index / nr_cores;
+}
+
+/* Make APIC ID for the CPU 'cpu_index'
+ *
+ * 'cpu_index' is a sequential, contiguous ID for the CPU.
+ */
+static inline apic_id_t topo_apicid_for_cpu(unsigned nr_cores,
+                                            unsigned nr_threads,
+                                            unsigned cpu_index)
+{
+    unsigned pkg_id, core_id, smt_id;
+    topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
+                      &pkg_id, &core_id, &smt_id);
+    return topo_make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id);
+}
+
+#endif /* TARGET_I386_TOPOLOGY_H */
diff --git a/tests/.gitignore b/tests/.gitignore
index f9041f3..38c94ef 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -10,4 +10,5 @@ test-qmp-commands.h
 test-qmp-commands
 test-qmp-input-strict
 test-qmp-marshal.c
+test-x86-cpuid
 *-test
diff --git a/tests/Makefile b/tests/Makefile
index ce4f6f2..83f622c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
 check-unit-y += tests/test-coroutine$(EXESUF)
 check-unit-y += tests/test-visitor-serialization$(EXESUF)
 check-unit-y += tests/test-iov$(EXESUF)
+check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -35,6 +36,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
 	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
 	tests/test-qmp-commands.o tests/test-visitor-serialization.o
+test-obj-i386-y = tests/test-x86-cpuid.o
 
 test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
 test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
@@ -48,6 +50,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
 tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o iov.o
+tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
 
 tests/test-qapi-types.c tests/test-qapi-types.h :\
 $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
new file mode 100644
index 0000000..e76f73a
--- /dev/null
+++ b/tests/test-x86-cpuid.c
@@ -0,0 +1,102 @@
+/*
+ *  Test code for x86 CPUID and Topology functions
+ *
+ *  Copyright (c) 2012 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <glib.h>
+
+/*FIXME: this should be built inside the i386 target directory instead */
+#include "topology.h"
+
+static void test_topo_bits(void)
+{
+    /* simple tests for 1 thread per core, 1 core per socket */
+    g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0);
+    g_assert_cmpuint(apicid_core_width(1, 1), ==, 0);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 0), ==, 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 1), ==, 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 2), ==, 2);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 3), ==, 3);
+
+
+    /* Test field width calculation for multiple values
+     */
+    g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1);
+    g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2);
+    g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2);
+
+    g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5);
+
+
+    g_assert_cmpuint(apicid_core_width(30, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(31, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(32, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(33, 2), ==, 6);
+
+
+    /* build a weird topology and see if IDs are calculated correctly
+     */
+
+    /* This will use 2 bits for thread ID and 3 bits for core ID
+     */
+    g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2);
+    g_assert_cmpuint(apicid_core_width(6, 3), ==, 3);
+    g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 0), ==, 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1), ==, 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2), ==, 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 0), ==, (1 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 1), ==, (1 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 2), ==, (1 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 0), ==, (2 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 1), ==, (2 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 2), ==, (2 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 0 * 3 + 0), ==,
+                                         (1 << 5));
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 1 * 3 + 1), ==,
+                                         (1 << 5) | (1 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 3 * 6 * 3 + 5 * 3 + 2), ==,
+                                         (3 << 5) | (5 << 2) | 2);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/cpuid/topology/basic", test_topo_bits);
+
+    g_test_run();
+
+    return 0;
+}
-- 
1.7.11.4

Re: [Qemu-devel] [RFC 17/18] target-i386: topology & APIC ID utility functions (v2)

[Blue Swirl]

On Wed, Oct 3, 2012 at 1:29 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Changes v2 -> v3:
>  - Add documentation pointers to the code
>  - Rename bits_for_count() to bitwidth_for_count()
>  - Remove unused apicid_*_id() functions
>
> Changes v1 -> v2:
>  - Support 32-bit APIC IDs (in case x2APIC is going to be used)
>  - Coding style changes
>  - Use TARGET_I386_TOPOLOGY_H instead of __QEMU_X86_TOPOLOGY_H__
>  - Rename topo_make_apic_id() to topo_apicid_for_cpu()
>  - Rename __make_apicid() to topo_make_apicid()
>  - Spaces around operators on test-x86-cpuid.c, as requested by
>    Blue Swirl
>  - Make test-x86-cpuid a target-specific test
>
> Cc: Blue Swirl <blauwirbel@gmail.com>

Acked-by: Blue Swirl <blauwirbel@gmail.com>

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |   3 ++
>  tests/test-x86-cpuid.c | 102 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 239 insertions(+)
>  create mode 100644 target-i386/topology.h
>  create mode 100644 tests/test-x86-cpuid.c
>
> diff --git a/target-i386/topology.h b/target-i386/topology.h
> new file mode 100644
> index 0000000..6f4f2ff
> --- /dev/null
> +++ b/target-i386/topology.h
> @@ -0,0 +1,133 @@
> +/*
> + *  x86 CPU topology data structures and functions
> + *
> + *  Copyright (c) 2012 Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#ifndef TARGET_I386_TOPOLOGY_H
> +#define TARGET_I386_TOPOLOGY_H
> +
> +/* This file implements the APIC-ID-based CPU topology enumeration logic,
> + * documented at the following document:
> + *   Intel® 64 Architecture Processor Topology Enumeration
> + *   http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
> + *
> + * This code should be compatible with AMD's "Extended Method" described at:
> + *   AMD CPUID Specification (Publication #25481)
> + *   Section 3: Multiple Core Calcuation
> + * as long as:
> + *  nr_threads is set to 1;
> + *  OFFSET_IDX is assumed to be 0;
> + *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
> + */
> +
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "bitops.h"
> +
> +/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
> + */
> +typedef uint32_t apic_id_t;
> +
> +/* Return the bit width needed for 'count' IDs
> + */
> +static unsigned bitwidth_for_count(unsigned count)
> +{
> +    g_assert(count >= 1);
> +    if (count == 1) {
> +        return 0;
> +    }
> +    return bitops_flsl(count - 1) + 1;
> +}
> +
> +/* Bit width of the SMT_ID (thread ID) field on the APIC ID
> + */
> +static inline unsigned apicid_smt_width(unsigned nr_cores, unsigned nr_threads)
> +{
> +    return bitwidth_for_count(nr_threads);
> +}
> +
> +/* Bit width of the Core_ID field
> + */
> +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
> +{
> +    return bitwidth_for_count(nr_cores);
> +}
> +
> +/* Bit offset of the Core_ID field
> + */
> +static inline unsigned apicid_core_offset(unsigned nr_cores,
> +                                          unsigned nr_threads)
> +{
> +    return apicid_smt_width(nr_cores, nr_threads);
> +}
> +
> +/* Bit offset of the Pkg_ID (socket ID) field
> + */
> +static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
> +{
> +    return apicid_core_offset(nr_cores, nr_threads) + \
> +           apicid_core_width(nr_cores, nr_threads);
> +}
> +
> +/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
> + *
> + * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
> + */
> +static inline apic_id_t topo_make_apicid(unsigned nr_cores,
> +                                         unsigned nr_threads,
> +                                         unsigned pkg_id, unsigned core_id,
> +                                         unsigned smt_id)
> +{
> +    return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) | \
> +           (core_id << apicid_core_offset(nr_cores, nr_threads)) | \
> +           smt_id;
> +}
> +
> +/* Calculate thread/core/package IDs for a specific topology,
> + * based on (contiguous) CPU index
> + */
> +static inline void topo_ids_from_idx(unsigned nr_cores, unsigned nr_threads,
> +                                     unsigned cpu_index,
> +                                     unsigned *pkg_id, unsigned *core_id,
> +                                     unsigned *smt_id)
> +{
> +    unsigned core_index = cpu_index / nr_threads;
> +    *smt_id = cpu_index % nr_threads;
> +    *core_id = core_index % nr_cores;
> +    *pkg_id = core_index / nr_cores;
> +}
> +
> +/* Make APIC ID for the CPU 'cpu_index'
> + *
> + * 'cpu_index' is a sequential, contiguous ID for the CPU.
> + */
> +static inline apic_id_t topo_apicid_for_cpu(unsigned nr_cores,
> +                                            unsigned nr_threads,
> +                                            unsigned cpu_index)
> +{
> +    unsigned pkg_id, core_id, smt_id;
> +    topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
> +                      &pkg_id, &core_id, &smt_id);
> +    return topo_make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id);
> +}
> +
> +#endif /* TARGET_I386_TOPOLOGY_H */
> diff --git a/tests/.gitignore b/tests/.gitignore
> index f9041f3..38c94ef 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -10,4 +10,5 @@ test-qmp-commands.h
>  test-qmp-commands
>  test-qmp-input-strict
>  test-qmp-marshal.c
> +test-x86-cpuid
>  *-test
> diff --git a/tests/Makefile b/tests/Makefile
> index ce4f6f2..83f622c 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
>  check-unit-y += tests/test-coroutine$(EXESUF)
>  check-unit-y += tests/test-visitor-serialization$(EXESUF)
>  check-unit-y += tests/test-iov$(EXESUF)
> +check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
>
>  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>
> @@ -35,6 +36,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>         tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
>         tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
>         tests/test-qmp-commands.o tests/test-visitor-serialization.o
> +test-obj-i386-y = tests/test-x86-cpuid.o
>
>  test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
>  test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
> @@ -48,6 +50,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
>  tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
>  tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
>  tests/test-iov$(EXESUF): tests/test-iov.o iov.o
> +tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
>
>  tests/test-qapi-types.c tests/test-qapi-types.h :\
>  $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
> diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
> new file mode 100644
> index 0000000..e76f73a
> --- /dev/null
> +++ b/tests/test-x86-cpuid.c
> @@ -0,0 +1,102 @@
> +/*
> + *  Test code for x86 CPUID and Topology functions
> + *
> + *  Copyright (c) 2012 Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include <glib.h>
> +
> +/*FIXME: this should be built inside the i386 target directory instead */
> +#include "topology.h"
> +
> +static void test_topo_bits(void)
> +{
> +    /* simple tests for 1 thread per core, 1 core per socket */
> +    g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0);
> +    g_assert_cmpuint(apicid_core_width(1, 1), ==, 0);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 0), ==, 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 1), ==, 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 2), ==, 2);
> +    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 3), ==, 3);
> +
> +
> +    /* Test field width calculation for multiple values
> +     */
> +    g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1);
> +    g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2);
> +    g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2);
> +
> +    g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4);
> +    g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4);
> +    g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4);
> +    g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5);
> +
> +
> +    g_assert_cmpuint(apicid_core_width(30, 2), ==, 5);
> +    g_assert_cmpuint(apicid_core_width(31, 2), ==, 5);
> +    g_assert_cmpuint(apicid_core_width(32, 2), ==, 5);
> +    g_assert_cmpuint(apicid_core_width(33, 2), ==, 6);
> +
> +
> +    /* build a weird topology and see if IDs are calculated correctly
> +     */
> +
> +    /* This will use 2 bits for thread ID and 3 bits for core ID
> +     */
> +    g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2);
> +    g_assert_cmpuint(apicid_core_width(6, 3), ==, 3);
> +    g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 0), ==, 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1), ==, 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2), ==, 2);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 0), ==, (1 << 2) | 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 1), ==, (1 << 2) | 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 2), ==, (1 << 2) | 2);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 0), ==, (2 << 2) | 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 1), ==, (2 << 2) | 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 2), ==, (2 << 2) | 2);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 0 * 3 + 0), ==,
> +                                         (1 << 5));
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 1 * 3 + 1), ==,
> +                                         (1 << 5) | (1 << 2) | 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 3 * 6 * 3 + 5 * 3 + 2), ==,
> +                                         (3 << 5) | (5 << 2) | 2);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    g_test_add_func("/cpuid/topology/basic", test_topo_bits);
> +
> +    g_test_run();
> +
> +    return 0;
> +}
> --
> 1.7.11.4
>

[Qemu-devel] [RFC 18/18] pc: generate APIC IDs according to CPU topology (v3)

[Eduardo Habkost]

This keeps compatibility on machine-types pc-1.2 and older, and prints a
warning in case the requested configuration won't get the correct
topology.

At first I was going to use object_property_add() on PC instance_init
function, but this wouldn't allow us to use global properties to set
it[1]. So I had to use a static DEFINE_PROP_BIT for the
contiguous_apic_ids property.

[1] See my qdev question at:
    http://article.gmane.org/gmane.comp.emulators.qemu/173625

Changes v2 -> v3:
 - Now all code is inside hw/pc.c
 - Use a real "PC" class and a "contiguous_apic_ids" property

Changes v1 -> v2:
 - Move code to cpu.c
 - keep using cpu_index on *-user
 - Use SMP.contiguous_apic_ids global property
 - Prints warning in case the compatibility mode will expose incorrect
   topology

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c      | 45 ++++++++++++++++++++++++++++++++++++++++-----
 hw/pc_piix.c |  4 ++++
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 4c9b2f6..c33768b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -52,6 +52,8 @@
 #include "arch_init.h"
 #include "bitmap.h"
 #include "vga-pci.h"
+#include "topology.h"
+#include "cpus.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -552,13 +554,34 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
 
 typedef struct PC {
     DeviceState parent_obj;
+
+    /* Compatibility mode that force APIC IDs to be contiguous
+     *
+     * Setting it to true may break some CPU topologies (e.g. if core or thread
+     * count is not a power of 2).
+     */
+#define PC_FLAG_CONTIGUOUS_APIC_IDS 0
+
+    uint32_t flags;
 } PC;
 
+static Property pc_properties[] = {
+    DEFINE_PROP_BIT("contiguous_apic_ids", PC, flags,
+                    PC_FLAG_CONTIGUOUS_APIC_IDS, false),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void pc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    dc->props = pc_properties;
+}
+
 static const TypeInfo pc_type_info = {
     .name = TYPE_PC_MACHINE,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(PC),
-    .class_size = sizeof(DeviceClass),
+    .class_init = pc_class_init,
 };
 
 static void pc_register_type(void)
@@ -576,10 +599,22 @@ type_init(pc_register_type);
  */
 static uint32_t apic_id_for_cpu(PC *pc, int cpu_index)
 {
-    /* right now APIC ID == CPU index. this will eventually change to use
-     * the CPU topology configuration properly
-     */
-    return cpu_index;
+    unsigned int correct_id;
+    static bool warned;
+    bool contig;
+
+    correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index);
+    contig = object_property_get_bool(OBJECT(pc), "contiguous_apic_ids", NULL);
+    if (contig) {
+        if (cpu_index != correct_id && !warned) {
+            fprintf(stderr, "warning: CPU topology in compat mode, "
+                            "results won't match the requested topology\n");
+            warned = true;
+        }
+        return cpu_index;
+    } else {
+        return correct_id;
+    }
 }
 
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index f861907..8a4a5f6 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -374,6 +374,10 @@ static QEMUMachine pc_machine_v1_3 = {
             .driver   = "ivshmem",\
             .property = "use64",\
             .value    = "0",\
+        },{\
+            .driver   = "PC",\
+            .property = "contiguous_apic_ids",\
+            .value    = "true",\
         }
 
 static QEMUMachine pc_machine_v1_2 = {
-- 
1.7.11.4