[Qemu-devel] [PATCH 00/15 v6 for 1.5] target-i386: CPU hot-add with cpu-add QMP command

[Qemu-devel] [PATCH 00/15 v6 for 1.5] target-i386: CPU hot-add with cpu-add QMP command

[Igor Mammedov]

Implements alternative way for hot-adding CPU using cpu-add QMP command,
which could be useful until it would be possible to add CPUs via device_add.

To hot-add CPU use following command from qmp-shell:
 cpu-add id=[0..max-cpus - 1)

git tree for testing: https://github.com/imammedo/qemu/tree/cpu_add.v6

based on qom-cpu tree

v6->v5
  * override hot_add_cpu hook statically
  * extend and use memory_region_find() in IOAPIC
  * s/signal_cpu_creation/tcg_signal_cpu_creation/
  * add "since 1.5 to cpu-addQAPI schema description

v5->v4:
  * style fixes
  * new helper qemu_for_each_cpu()
  * switch to qemu_for_each_cpu() in cpu_exists()
  * "pc: update rtc ..." patch make depend it on "mc146818rtc: QOM'ify"
    and use QOM cast style
  * call CPU added notifier right before CPU becomes runable
  * s/resume_vcpu/cpu_resume/
  * acpi/piix4: add spec documentation for QEMU<->Seabios CPU hotplug
    interface
  * use error_propagate() in pc_new_cpu()
  * skip cpu_exists() check in apic-id property setter if new value is
    the same as current
  * embed icc-bus inside icc-bridge and use qbus_create_inplace()
  * move include/hw/i386/icc_bus.h into include/hw/cpu/
  * make missing icc-bus fatal error for softmmu target
  * split "move APIC to ICC bus" and "move IOAPIC to ICC bus" on smaller
    patches
  * use qdev_get_parent_bus() to get parent bus
  * split "add cpu-add command..." on smaller patches

v4->v3:
  * 'id' in cpu-add command will be a thread number instead of APIC ID
  * split off resume_vcpu() into separate patch
  * move notifier from rtc code into pc.c

v2->v3:
  * use local error & propagate_error() instead of operating on
    passed in errp in several places
  * replace CPUClass.get_firmware_id() with CPUClass.get_arch_id()
  * leave IOAPIC creation to board and just set bus to icc-bus
  * include kvm-stub.o in cpu libary if no KVM is configured
  * create resume_vcpu() stub and include it in libqemustub,
    and use it directly instead of CPU method
  * acpi_piix4: s/cpu_add_notifier/cpu_added_notifier/

v1->v2:
  * generalize cpu sync to KVM, resume and hot-plug notification and
    invoke them form CPUClass, to make available to all targets.
  * introduce cpu_exists() and CPUClass.get_firmware_id() and use
    the last one in acpi_piix to make code target independent.
  * move IOAPIC to ICC bus, it was suggested and easy to convert.
  * leave kvmvapic as SysBusDevice, it doesn't affect hot-plug and
    created only once for all APIC instances. I haven't found yet
    good/clean enough way to convert it to ICCDevice. May be follow-up
    though.
  * split one big ICC patch into several, one per converted device
  * add cpu_hot_add hook to machine and implement it for target-i386,
    instead of adding stabs. Could be used by other targets to
    implement cpu-add.
  * pre-allocate links<CPU> for all possible CPUs and make them available
    at /machine/icc-bridge/cpu[0..N] QOM path, so users could find out
    possible/free CPU IDs to use in cpu-add command.

Igor Mammedov (14):
  exec: add qemu_for_each_cpu
  cpu: add helper cpu_exists(), to check if CPU with specified id
    exists
  acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest
  target-i386: introduce apic-id property
  target-i386: introduce ICC bus/device/bridge
  target-i386: cpu: attach ICC bus to CPU on its creation
  target-i386: replace MSI_SPACE_SIZE with APIC_SPACE_SIZE
  target-i386: kvmvapic: make expilict dependency on sysbus.h
  target-i386: move APIC to ICC bus
  target-i386: move IOAPIC to ICC bus
  pc: pass QEMUMachineInitArgs down to pc_cpus_init()
  add hot_add_cpu hook to QEMUMachine and export machine_args
  target-i386: implement machine->hot_add_cpu hook
  QMP: add cpu-add command

Paolo Bonzini (1):
  extend memory_region_find() and use it in kvm/ioapic

 MAINTAINERS                        |    6 ++
 cpus.c                             |   13 ++--
 default-configs/i386-softmmu.mak   |    1 +
 default-configs/x86_64-softmmu.mak |    1 +
 docs/specs/acpi_cpu_hotplug.txt    |   22 +++++++
 exec.c                             |   10 +++
 hw/acpi/piix4.c                    |   90 ++++++++++++++++++++++++++-
 hw/cpu/Makefile.objs               |    1 +
 hw/cpu/icc_bus.c                   |  120 ++++++++++++++++++++++++++++++++++++
 hw/i386/kvm/apic.c                 |    2 +-
 hw/i386/kvm/ioapic.c               |    9 +++-
 hw/i386/kvmvapic.c                 |    1 +
 hw/i386/pc.c                       |   80 +++++++++++++++++++++---
 hw/i386/pc_piix.c                  |   74 +++++++---------------
 hw/i386/pc_q35.c                   |   31 +++++----
 hw/intc/apic.c                     |    2 +-
 hw/intc/apic_common.c              |   18 ++++--
 hw/intc/ioapic_common.c            |   14 +++-
 hw/xen/xen_apic.c                  |    2 +-
 include/exec/memory.h              |   13 ++--
 include/hw/boards.h                |    3 +
 include/hw/cpu/icc_bus.h           |   63 +++++++++++++++++++
 include/hw/i386/apic_internal.h    |    8 +--
 include/hw/i386/ioapic_internal.h  |    6 +-
 include/hw/i386/pc.h               |    4 +-
 include/qom/cpu.h                  |   18 ++++++
 memory.c                           |   20 +++++--
 qapi-schema.json                   |   13 ++++
 qmp-commands.hx                    |   23 +++++++
 qmp.c                              |   10 +++
 qom/cpu.c                          |   26 ++++++++
 target-i386/cpu.c                  |   73 ++++++++++++++++++----
 target-i386/cpu.h                  |    1 +
 vl.c                               |    8 ++-
 34 files changed, 656 insertions(+), 130 deletions(-)
 create mode 100644 docs/specs/acpi_cpu_hotplug.txt
 create mode 100644 hw/cpu/icc_bus.c
 create mode 100644 include/hw/cpu/icc_bus.h

[Qemu-devel] [PATCH 01/15] exec: add qemu_for_each_cpu

[Igor Mammedov]

wrapper will help to remove open-coded loops.

pathc icludes random example of how it could be used.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Note:
  Will be used by ACPI table generation and cpu_exists()

v2:
 * s/signal_cpu_creation()/tcg_signal_cpu_creation()/
---
 cpus.c            |   13 +++++++------
 exec.c            |   10 ++++++++++
 include/qom/cpu.h |    8 ++++++++
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index 1d88761..a2d92c7 100644
--- a/cpus.c
+++ b/cpus.c
@@ -812,6 +812,12 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
 
 static void tcg_exec_all(void);
 
+static void tcg_signal_cpu_creation(CPUState *cpu, void *data)
+{
+    cpu->thread_id = qemu_get_thread_id();
+    cpu->created = true;
+}
+
 static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
     CPUState *cpu = arg;
@@ -820,13 +826,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
     qemu_tcg_init_cpu_signals();
     qemu_thread_get_self(cpu->thread);
 
-    /* signal CPU creation */
     qemu_mutex_lock(&qemu_global_mutex);
-    for (env = first_cpu; env != NULL; env = env->next_cpu) {
-        cpu = ENV_GET_CPU(env);
-        cpu->thread_id = qemu_get_thread_id();
-        cpu->created = true;
-    }
+    qemu_for_each_cpu(tcg_signal_cpu_creation, NULL);
     qemu_cond_signal(&qemu_cpu_cond);
 
     /* wait for initial kick-off after machine start */
diff --git a/exec.c b/exec.c
index fa1e0c3..19725db 100644
--- a/exec.c
+++ b/exec.c
@@ -265,6 +265,16 @@ CPUState *qemu_get_cpu(int index)
     return env ? cpu : NULL;
 }
 
+void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data)
+{
+    CPUArchState *env = first_cpu;
+
+    while (env) {
+        func(ENV_GET_CPU(env), data);
+        env = env->next_cpu;
+    }
+}
+
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 1b4de17..f64727e 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -215,6 +215,14 @@ bool cpu_is_stopped(CPUState *cpu);
  */
 void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data);
 
+/** qemu_for_each_cpu:
+ * @func: The function to be executed.
+ * @data: Data to pass to the function.
+ *
+ * Executes @func on all CPUs
+ */
+void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data);
+
 /**
  * qemu_get_cpu:
  * @index: The CPUState@cpu_index value of the CPU to obtain.
-- 
1.7.1

Re: [Qemu-devel] [PATCH 01/15] exec: add qemu_for_each_cpu

[Andreas Färber]

Am 25.04.2013 16:05, schrieb Igor Mammedov:
> wrapper will help to remove open-coded loops.
> 
> pathc icludes random example of how it could be used.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Note:
>   Will be used by ACPI table generation and cpu_exists()
> 
> v2:
>  * s/signal_cpu_creation()/tcg_signal_cpu_creation()/

Updated queued patch with
s/signal_tcg_cpu_creation/tcg_signal_cpu_creation/g

https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

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

[Qemu-devel] [PATCH 02/15] cpu: add helper cpu_exists(), to check if CPU with specified id exists

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  * use qemu_for_each_cpu() instead of recursion
v2:
  * s/get_firmware_id()/get_arch_id()/ rebase fixup
  * remove check for get_arch_id being NULL, since it's always defined
---
 include/qom/cpu.h |   10 ++++++++++
 qom/cpu.c         |   26 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index f64727e..4c44f06 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -233,6 +233,16 @@ void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data);
  */
 CPUState *qemu_get_cpu(int index);
 
+/**
+ * cpu_exists:
+ * @id - guest exposed CPU ID to lookup
+ *
+ * Search for CPU with specified ID.
+ *
+ * Returns: true - CPU is found, false - CPU isn't found
+ */
+bool cpu_exists(int64_t id);
+
 #ifndef CONFIG_USER_ONLY
 
 typedef void (*CPUInterruptHandler)(CPUState *, int);
diff --git a/qom/cpu.c b/qom/cpu.c
index 9a4457b..3f79398 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -24,6 +24,32 @@
 #include "qemu/notify.h"
 #include "sysemu/sysemu.h"
 
+typedef struct CPU_exists_args {
+    int64_t id;
+    bool found;
+} CPU_exists_args;
+
+static void cpu_exist_cb(CPUState *cpu, void *data)
+{
+    CPUClass *klass = CPU_GET_CLASS(cpu);
+    CPU_exists_args *arg = data;
+
+    if (klass->get_arch_id(cpu) == arg->id) {
+        arg->found = true;
+    }
+}
+
+bool cpu_exists(int64_t id)
+{
+    CPU_exists_args data = {
+        .id = id,
+        .found = false,
+    };
+
+    qemu_for_each_cpu(cpu_exist_cb, &data);
+    return data.found;
+}
+
 /* CPU hot-plug notifiers */
 static NotifierList cpu_added_notifiers =
     NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 02/15] cpu: add helper cpu_exists(), to check if CPU with specified id exists

[Andreas Färber]

Am 25.04.2013 16:05, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
>   * use qemu_for_each_cpu() instead of recursion
> v2:
>   * s/get_firmware_id()/get_arch_id()/ rebase fixup
>   * remove check for get_arch_id being NULL, since it's always defined
> ---
>  include/qom/cpu.h |   10 ++++++++++
>  qom/cpu.c         |   26 ++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 0 deletions(-)

Thanks, applied to qom-cpu (with gtk-doc and Coding Style fixes below):
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index d7746dd..e54579b 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -236,11 +236,11 @@ CPUState *qemu_get_cpu(int index);

 /**
  * cpu_exists:
- * @id - guest exposed CPU ID to lookup
+ * @id: Guest-exposed CPU ID to lookup.
  *
  * Search for CPU with specified ID.
  *
- * Returns: true - CPU is found, false - CPU isn't found
+ * Returns: %true - CPU is found, %false - CPU isn't found.
  */
 bool cpu_exists(int64_t id);

diff --git a/qom/cpu.c b/qom/cpu.c
index 3f79398..3dc8208 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -24,15 +24,15 @@
 #include "qemu/notify.h"
 #include "sysemu/sysemu.h"

-typedef struct CPU_exists_args {
+typedef struct CPUExistsArgs {
     int64_t id;
     bool found;
-} CPU_exists_args;
+} CPUExistsArgs;

 static void cpu_exist_cb(CPUState *cpu, void *data)
 {
     CPUClass *klass = CPU_GET_CLASS(cpu);
-    CPU_exists_args *arg = data;
+    CPUExistsArgs *arg = data;

     if (klass->get_arch_id(cpu) == arg->id) {
         arg->found = true;
@@ -41,7 +41,7 @@ static void cpu_exist_cb(CPUState *cpu, void *data)

 bool cpu_exists(int64_t id)
 {
-    CPU_exists_args data = {
+    CPUExistsArgs data = {
         .id = id,
         .found = false,
     };

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

[Qemu-devel] [PATCH 03/15] acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest

[Igor Mammedov]

* introduce processor status bitmask visible to guest at 0xaf00 addr,
  where ACPI asl code expects it
* set bit corresponding to APIC ID in processor status bitmask on
  receiving CPU hot-plug notification
* trigger CPU hot-plug SCI, to notify guest about CPU hot-plug event

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Note:
  gpe_cpu.sts isn't need to be migrated, since CPU hotpluging during
  migration just doesn't work, since destination QEMU has to be started
  with all present in guest CPUs (including hotplugged).
  i.e. src-qemu -smp 2,max-cpus=4; cpu-add id=2; dst-qemu -smp 3,max-cpus=4
  Destination QEMU will recreate the same gpe_cpu.sts=t'111' bitmap as
  on source by calling qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu);
  since it has been started with 3 CPUs on command line.

v7:
  * s/struct cpu_status/struct CPUStatus/
v6:
  * drop gpe_cpu.sts migration hunks
v5:
  * add optional vmstate subsection if there was CPU hotplug event
  * remove unused Error*
  * use qemu_for_each_cpu() instead of recursion over QOM tree
v4:
  * added spec for QEMU-Seabios interface
  * added PIIX4_ prefix to PROC_ defines
v3:
  * s/get_firmware_id()/get_arch_id()/ due rebase
  * s/cpu_add_notifier/cpu_added_notifier/
v2:
  * use CPUClass.get_firmware_id() to make code target independent
  * bump up vmstate_acpi version
---
 docs/specs/acpi_cpu_hotplug.txt |   22 +++++++++
 hw/acpi/piix4.c                 |   90 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 110 insertions(+), 2 deletions(-)
 create mode 100644 docs/specs/acpi_cpu_hotplug.txt

diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
new file mode 100644
index 0000000..5dec0c5
--- /dev/null
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -0,0 +1,22 @@
+QEMU<->ACPI BIOS CPU hotplug interface
+--------------------------------------
+
+QEMU supports CPU hotplug via ACPI. This document
+describes the interface between QEMU and the ACPI BIOS.
+
+ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
+-----------------------------------------
+
+Generic ACPI GPE block. Bit 2 (GPE.2) used to notify CPU
+hot-add/remove event to ACPI BIOS, via SCI interrupt.
+
+CPU present bitmap (IO port 0xaf00-0xae1f, 1-byte access):
+---------------------------------------------------------------
+One bit per CPU. Bit position reflects corresponding CPU APIC ID.
+Read-only.
+
+CPU hot-add/remove notification:
+-----------------------------------------------------
+QEMU sets/clears corresponding CPU bit on hot-add/remove event.
+CPU present map read by ACPI BIOS GPE.2 handler to notify OS of CPU
+hot-(un)plug events.
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 88386d7..ebd1af9 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -48,19 +48,28 @@
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
 
+#define PIIX4_PROC_BASE 0xaf00
+#define PIIX4_PROC_LEN 32
+
 #define PIIX4_PCI_HOTPLUG_STATUS 2
+#define PIIX4_CPU_HOTPLUG_STATUS 4
 
 struct pci_status {
     uint32_t up; /* deprecated, maintained for migration compatibility */
     uint32_t down;
 };
 
+struct CPUStatus {
+    uint8_t sts[PIIX4_PROC_LEN];
+};
+
 typedef struct PIIX4PMState {
     PCIDevice dev;
 
     MemoryRegion io;
     MemoryRegion io_gpe;
     MemoryRegion io_pci;
+    MemoryRegion io_cpu;
     ACPIREGS ar;
 
     APMState apm;
@@ -82,6 +91,9 @@ typedef struct PIIX4PMState {
     uint8_t disable_s3;
     uint8_t disable_s4;
     uint8_t s4_val;
+
+    struct CPUStatus gpe_cpu;
+    Notifier cpu_added_notifier;
 } PIIX4PMState;
 
 static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
@@ -100,8 +112,8 @@ static void pm_update_sci(PIIX4PMState *s)
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                    ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
-        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0])
-          & PIIX4_PCI_HOTPLUG_STATUS) != 0);
+        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) &
+          (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);
 
     qemu_set_irq(s->irq, sci_level);
     /* schedule a timer interruption if needed */
@@ -585,6 +597,73 @@ static const MemoryRegionOps piix4_pci_ops = {
     },
 };
 
+static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned width)
+{
+    PIIX4PMState *s = opaque;
+    struct CPUStatus *cpus = &s->gpe_cpu;
+    uint64_t val = cpus->sts[addr];
+
+    return val;
+}
+
+static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
+                             unsigned int size)
+{
+    /* TODO: implement VCPU removal on guest signal that CPU can be removed */
+}
+
+static const MemoryRegionOps cpu_hotplug_ops = {
+    .read = cpu_status_read,
+    .write = cpu_status_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+typedef enum {
+    PLUG,
+    UNPLUG,
+} HotplugEventType;
+
+static void piix4_cpu_hotplug_req(PIIX4PMState *s, CPUState *cpu,
+                                  HotplugEventType action)
+{
+    struct CPUStatus *g = &s->gpe_cpu;
+    ACPIGPE *gpe = &s->ar.gpe;
+    CPUClass *k = CPU_GET_CLASS(cpu);
+    int64_t cpu_id;
+
+    assert(s != NULL);
+
+    *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
+    cpu_id = k->get_arch_id(CPU(cpu));
+    if (action == PLUG) {
+        g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
+    } else {
+        g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
+    }
+    pm_update_sci(s);
+}
+
+static void piix4_cpu_added_req(Notifier *n, void *opaque)
+{
+    PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
+
+    piix4_cpu_hotplug_req(s, CPU(opaque), PLUG);
+}
+
+static void piix4_init_cpu_status(CPUState *cpu, void *data)
+{
+    struct CPUStatus *g = (struct CPUStatus *)data;
+    CPUClass *k = CPU_GET_CLASS(cpu);
+    int64_t id = k->get_arch_id(cpu);
+
+    g_assert((id / 8) < PIIX4_PROC_LEN);
+    g->sts[id / 8] |= (1 << (id % 8));
+}
+
 static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
                                 PCIHotplugState state);
 
@@ -600,6 +679,13 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
     memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
                                 &s->io_pci);
     pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
+
+    qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu);
+    memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s, "apci-cpu-hotplug",
+                          PIIX4_PROC_LEN);
+    memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu);
+    s->cpu_added_notifier.notify = piix4_cpu_added_req;
+    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
 }
 
 static void enable_device(PIIX4PMState *s, int slot)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 03/15] acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest

[Andreas Färber]

Am 25.04.2013 16:05, schrieb Igor Mammedov:
> * introduce processor status bitmask visible to guest at 0xaf00 addr,
>   where ACPI asl code expects it
> * set bit corresponding to APIC ID in processor status bitmask on
>   receiving CPU hot-plug notification
> * trigger CPU hot-plug SCI, to notify guest about CPU hot-plug event
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Note:
>   gpe_cpu.sts isn't need to be migrated, since CPU hotpluging during
>   migration just doesn't work, since destination QEMU has to be started
>   with all present in guest CPUs (including hotplugged).
>   i.e. src-qemu -smp 2,max-cpus=4; cpu-add id=2; dst-qemu -smp 3,max-cpus=4
>   Destination QEMU will recreate the same gpe_cpu.sts=t'111' bitmap as
>   on source by calling qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu);
>   since it has been started with 3 CPUs on command line.
> 
> v7:
>   * s/struct cpu_status/struct CPUStatus/
> v6:
>   * drop gpe_cpu.sts migration hunks
> v5:
>   * add optional vmstate subsection if there was CPU hotplug event
>   * remove unused Error*
>   * use qemu_for_each_cpu() instead of recursion over QOM tree
> v4:
>   * added spec for QEMU-Seabios interface
>   * added PIIX4_ prefix to PROC_ defines
> v3:
>   * s/get_firmware_id()/get_arch_id()/ due rebase
>   * s/cpu_add_notifier/cpu_added_notifier/
> v2:
>   * use CPUClass.get_firmware_id() to make code target independent
>   * bump up vmstate_acpi version
> ---
>  docs/specs/acpi_cpu_hotplug.txt |   22 +++++++++
>  hw/acpi/piix4.c                 |   90 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 110 insertions(+), 2 deletions(-)
>  create mode 100644 docs/specs/acpi_cpu_hotplug.txt

Thanks, since it no longer depends on migration discussions, applied to
qom-cpu (with changes below: struct typedef and symmetric read/write):

https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index ebd1af9..c4af1cc 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -59,9 +59,9 @@ struct pci_status {
     uint32_t down;
 };

-struct CPUStatus {
+typedef struct CPUStatus {
     uint8_t sts[PIIX4_PROC_LEN];
-};
+} CPUStatus;

 typedef struct PIIX4PMState {
     PCIDevice dev;
@@ -92,7 +92,7 @@ typedef struct PIIX4PMState {
     uint8_t disable_s4;
     uint8_t s4_val;

-    struct CPUStatus gpe_cpu;
+    CPUStatus gpe_cpu;
     Notifier cpu_added_notifier;
 } PIIX4PMState;

@@ -597,10 +597,10 @@ static const MemoryRegionOps piix4_pci_ops = {
     },
 };

-static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned width)
+static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int
size)
 {
     PIIX4PMState *s = opaque;
-    struct CPUStatus *cpus = &s->gpe_cpu;
+    CPUStatus *cpus = &s->gpe_cpu;
     uint64_t val = cpus->sts[addr];

     return val;
@@ -630,7 +630,7 @@ typedef enum {
 static void piix4_cpu_hotplug_req(PIIX4PMState *s, CPUState *cpu,
                                   HotplugEventType action)
 {
-    struct CPUStatus *g = &s->gpe_cpu;
+    CPUStatus *g = &s->gpe_cpu;
     ACPIGPE *gpe = &s->ar.gpe;
     CPUClass *k = CPU_GET_CLASS(cpu);
     int64_t cpu_id;
@@ -656,7 +656,7 @@ static void piix4_cpu_added_req(Notifier *n, void
*opaque)

 static void piix4_init_cpu_status(CPUState *cpu, void *data)
 {
-    struct CPUStatus *g = (struct CPUStatus *)data;
+    CPUStatus *g = (CPUStatus *)data;
     CPUClass *k = CPU_GET_CLASS(cpu);
     int64_t id = k->get_arch_id(cpu);



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

[Qemu-devel] [PATCH 04/15] target-i386: introduce apic-id property

[Igor Mammedov]

The property is used from board level to set APIC ID for CPUs it
creates. Do so in a new pc_new_cpu() helper, to be reused for hot-plug.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Note:
  * pc_new_cpu() function will be reused later in CPU hot-plug hook.

v4:
  * after switching to qemu_for_each_cpu() in cpu_exists(), first CPU
    becomes visible to cpu_exists() early and setting property fails,
    skip APIC ID check if value to be set is the same as the current.
  * use error_propagate() in pc_new_cpu()
  * return CPU from pc_new_cpu(). Moved from "move APIC to ICC bus"
    to reduce its size.
v3:
  * user error_propagate() in property setter
v2:
  * use generic cpu_exists() instead of custom one
  * make apic-id dynamic property, so it won't be possible to use it
    as global property, since it's instance specific
---
 hw/i386/pc.c      |   29 ++++++++++++++++++++++++++++-
 target-i386/cpu.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 1 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 28fce0e..7c7dd86 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -890,9 +890,33 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
+static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
+{
+    X86CPU *cpu;
+    Error *local_err = NULL;
+
+    cpu = cpu_x86_create(cpu_model, errp);
+    if (!cpu) {
+        return cpu;
+    }
+
+    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
+    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
+
+    if (local_err) {
+        if (cpu != NULL) {
+            object_unref(OBJECT(cpu));
+            cpu = NULL;
+        }
+        error_propagate(errp, local_err);
+    }
+    return cpu;
+}
+
 void pc_cpus_init(const char *cpu_model)
 {
     int i;
+    Error *error = NULL;
 
     /* init CPUs */
     if (cpu_model == NULL) {
@@ -904,7 +928,10 @@ void pc_cpus_init(const char *cpu_model)
     }
 
     for (i = 0; i < smp_cpus; i++) {
-        if (!cpu_x86_init(cpu_model)) {
+        pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
+        if (error) {
+            fprintf(stderr, "%s\n", error_get_pretty(error));
+            error_free(error);
             exit(1);
         }
     }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f34ba23..b0eb6ca 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1271,6 +1271,45 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
+                                  const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    int64_t value = cpu->env.cpuid_apic_id;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
+                                  const char *name, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    const int64_t min = 0;
+    const int64_t max = UINT32_MAX;
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    if (value < min || value > max) {
+        error_setg(&error, "Property %s.%s doesn't take value %" PRId64
+                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
+                   object_get_typename(obj), name, value, min, max);
+        error_propagate(errp, error);
+        return;
+    }
+
+    if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
+        error_setg(&error, "CPU with APIC ID %" PRIi64 " exists", value);
+        error_propagate(errp, error);
+        return;
+    }
+    cpu->env.cpuid_apic_id = value;
+}
+
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
 {
     x86_def_t *def;
@@ -2259,6 +2298,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+    object_property_add(obj, "apic-id", "int",
+                        x86_cpuid_get_apic_id,
+                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
 
     env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 04/15] target-i386: introduce apic-id property

[Eduardo Habkost]

On Thu, Apr 25, 2013 at 04:05:26PM +0200, Igor Mammedov wrote:
[...]
> +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
> +                                  const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    const int64_t min = 0;
> +    const int64_t max = UINT32_MAX;
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +    if (value < min || value > max) {
> +        error_setg(&error, "Property %s.%s doesn't take value %" PRId64
> +                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> +                   object_get_typename(obj), name, value, min, max);
> +        error_propagate(errp, error);
> +        return;
> +    }

Why you copied and pasted the string from
QERR_PROPERTY_VALUE_OUT_OF_RANGE, instead of simply using the define
like in the other property setters?

The rest of the patch looks good to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 04/15] target-i386: introduce apic-id property

[Igor Mammedov]

On Thu, 25 Apr 2013 17:36:45 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Apr 25, 2013 at 04:05:26PM +0200, Igor Mammedov wrote:
> [...]
> > +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
> > +                                  const char *name, Error **errp)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    const int64_t min = 0;
> > +    const int64_t max = UINT32_MAX;
> > +    Error *error = NULL;
> > +    int64_t value;
> > +
> > +    visit_type_int(v, &value, name, &error);
> > +    if (error) {
> > +        error_propagate(errp, error);
> > +        return;
> > +    }
> > +    if (value < min || value > max) {
> > +        error_setg(&error, "Property %s.%s doesn't take value %" PRId64
> > +                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> > +                   object_get_typename(obj), name, value, min, max);
> > +        error_propagate(errp, error);
> > +        return;
> > +    }
> 
> Why you copied and pasted the string from
> QERR_PROPERTY_VALUE_OUT_OF_RANGE, instead of simply using the define
> like in the other property setters?
it's designed to work with error_set(), not with error_setg().

> 
> The rest of the patch looks good to me.
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 04/15] target-i386: introduce apic-id property

[Eduardo Habkost]

On Fri, Apr 26, 2013 at 09:32:59AM +0200, Igor Mammedov wrote:
> On Thu, 25 Apr 2013 17:36:45 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Apr 25, 2013 at 04:05:26PM +0200, Igor Mammedov wrote:
> > [...]
> > > +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
> > > +                                  const char *name, Error **errp)
> > > +{
> > > +    X86CPU *cpu = X86_CPU(obj);
> > > +    const int64_t min = 0;
> > > +    const int64_t max = UINT32_MAX;
> > > +    Error *error = NULL;
> > > +    int64_t value;
> > > +
> > > +    visit_type_int(v, &value, name, &error);
> > > +    if (error) {
> > > +        error_propagate(errp, error);
> > > +        return;
> > > +    }
> > > +    if (value < min || value > max) {
> > > +        error_setg(&error, "Property %s.%s doesn't take value %" PRId64
> > > +                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> > > +                   object_get_typename(obj), name, value, min, max);
> > > +        error_propagate(errp, error);
> > > +        return;
> > > +    }
> > 
> > Why you copied and pasted the string from
> > QERR_PROPERTY_VALUE_OUT_OF_RANGE, instead of simply using the define
> > like in the other property setters?
> it's designed to work with error_set(), not with error_setg().

So why you didn't use error_set()?  :-)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 04/15] target-i386: introduce apic-id property

[Igor Mammedov]

On Fri, 26 Apr 2013 05:53:55 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Apr 26, 2013 at 09:32:59AM +0200, Igor Mammedov wrote:
> > On Thu, 25 Apr 2013 17:36:45 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, Apr 25, 2013 at 04:05:26PM +0200, Igor Mammedov wrote:
> > > [...]
> > > > +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
> > > > +                                  const char *name, Error **errp)
> > > > +{
> > > > +    X86CPU *cpu = X86_CPU(obj);
> > > > +    const int64_t min = 0;
> > > > +    const int64_t max = UINT32_MAX;
> > > > +    Error *error = NULL;
> > > > +    int64_t value;
> > > > +
> > > > +    visit_type_int(v, &value, name, &error);
> > > > +    if (error) {
> > > > +        error_propagate(errp, error);
> > > > +        return;
> > > > +    }
> > > > +    if (value < min || value > max) {
> > > > +        error_setg(&error, "Property %s.%s doesn't take value %" PRId64
> > > > +                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> > > > +                   object_get_typename(obj), name, value, min, max);
> > > > +        error_propagate(errp, error);
> > > > +        return;
> > > > +    }
> > > 
> > > Why you copied and pasted the string from
> > > QERR_PROPERTY_VALUE_OUT_OF_RANGE, instead of simply using the define
> > > like in the other property setters?
> > it's designed to work with error_set(), not with error_setg().
> 
> So why you didn't use error_set()?  :-)
in short, QERR_* is depricated and error_set() shouldn't be used anymore.
http://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02520.html
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 04/15] target-i386: introduce apic-id property

[Eduardo Habkost]

On Thu, Apr 25, 2013 at 04:05:26PM +0200, Igor Mammedov wrote:
> The property is used from board level to set APIC ID for CPUs it
> creates. Do so in a new pc_new_cpu() helper, to be reused for hot-plug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
> Note:
>   * pc_new_cpu() function will be reused later in CPU hot-plug hook.
> 
> v4:
>   * after switching to qemu_for_each_cpu() in cpu_exists(), first CPU
>     becomes visible to cpu_exists() early and setting property fails,
>     skip APIC ID check if value to be set is the same as the current.
>   * use error_propagate() in pc_new_cpu()
>   * return CPU from pc_new_cpu(). Moved from "move APIC to ICC bus"
>     to reduce its size.
> v3:
>   * user error_propagate() in property setter
> v2:
>   * use generic cpu_exists() instead of custom one
>   * make apic-id dynamic property, so it won't be possible to use it
>     as global property, since it's instance specific
> ---
>  hw/i386/pc.c      |   29 ++++++++++++++++++++++++++++-
>  target-i386/cpu.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 28fce0e..7c7dd86 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -890,9 +890,33 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>      }
>  }
>  
> +static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
> +{
> +    X86CPU *cpu;
> +    Error *local_err = NULL;
> +
> +    cpu = cpu_x86_create(cpu_model, errp);
> +    if (!cpu) {
> +        return cpu;
> +    }
> +
> +    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> +
> +    if (local_err) {
> +        if (cpu != NULL) {
> +            object_unref(OBJECT(cpu));
> +            cpu = NULL;
> +        }
> +        error_propagate(errp, local_err);
> +    }
> +    return cpu;
> +}
> +
>  void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
> +    Error *error = NULL;
>  
>      /* init CPUs */
>      if (cpu_model == NULL) {
> @@ -904,7 +928,10 @@ void pc_cpus_init(const char *cpu_model)
>      }
>  
>      for (i = 0; i < smp_cpus; i++) {
> -        if (!cpu_x86_init(cpu_model)) {
> +        pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> +        if (error) {
> +            fprintf(stderr, "%s\n", error_get_pretty(error));
> +            error_free(error);
>              exit(1);
>          }
>      }
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index f34ba23..b0eb6ca 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1271,6 +1271,45 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> +static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
> +                                  const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    int64_t value = cpu->env.cpuid_apic_id;
> +
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
> +                                  const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    const int64_t min = 0;
> +    const int64_t max = UINT32_MAX;
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +    if (value < min || value > max) {
> +        error_setg(&error, "Property %s.%s doesn't take value %" PRId64
> +                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> +                   object_get_typename(obj), name, value, min, max);
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
> +        error_setg(&error, "CPU with APIC ID %" PRIi64 " exists", value);
> +        error_propagate(errp, error);
> +        return;
> +    }
> +    cpu->env.cpuid_apic_id = value;
> +}
> +
>  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  {
>      x86_def_t *def;
> @@ -2259,6 +2298,9 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> +    object_property_add(obj, "apic-id", "int",
> +                        x86_cpuid_get_apic_id,
> +                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
>  
>      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
>  
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 04/15] target-i386: introduce apic-id property

[Andreas Färber]

Am 25.04.2013 16:05, schrieb Igor Mammedov:
> The property is used from board level to set APIC ID for CPUs it
> creates. Do so in a new pc_new_cpu() helper, to be reused for hot-plug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Note:
>   * pc_new_cpu() function will be reused later in CPU hot-plug hook.
> 
> v4:
>   * after switching to qemu_for_each_cpu() in cpu_exists(), first CPU
>     becomes visible to cpu_exists() early and setting property fails,
>     skip APIC ID check if value to be set is the same as the current.
>   * use error_propagate() in pc_new_cpu()
>   * return CPU from pc_new_cpu(). Moved from "move APIC to ICC bus"
>     to reduce its size.
> v3:
>   * user error_propagate() in property setter
> v2:
>   * use generic cpu_exists() instead of custom one
>   * make apic-id dynamic property, so it won't be possible to use it
>     as global property, since it's instance specific
> ---
>  hw/i386/pc.c      |   29 ++++++++++++++++++++++++++++-
>  target-i386/cpu.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 1 deletions(-)

Thanks, applied to qom-cpu (with change suggested on v4 below):
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Question: This being a dynamic QOM property, QMP qom-set apic-id allows
to change the value during runtime. Should we suppress this with a
dev->realized check? If so, please supply a follow-up patch.

Andreas

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0be0138..f1cecc0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1295,16 +1295,14 @@ static void x86_cpuid_set_apic_id(Object *obj,
Visitor *v, void *opaque,
         return;
     }
     if (value < min || value > max) {
-        error_setg(&error, "Property %s.%s doesn't take value %" PRId64
+        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
                    " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
                    object_get_typename(obj), name, value, min, max);
-        error_propagate(errp, error);
         return;
     }

     if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
-        error_setg(&error, "CPU with APIC ID %" PRIi64 " exists", value);
-        error_propagate(errp, error);
+        error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
         return;
     }
     cpu->env.cpuid_apic_id = value;

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

[Qemu-devel] [PATCH] target-i386: Do not allow to set apic-id one CPU is realized

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f1cecc0..3152ad5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1289,6 +1289,12 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
     Error *error = NULL;
     int64_t value;
 
+    if (DEVICE(obj)->realized) {
+        error_setg(errp, "Attempt to set property '%s' on '%s' after "
+                   "it was realized", name, object_get_typename(obj));
+        return;
+    }
+
     visit_type_int(v, &value, name, &error);
     if (error) {
         error_propagate(errp, error);
-- 
1.8.2.1

Re: [Qemu-devel] [PATCH] target-i386: Do not allow to set apic-id one CPU is realized

[Eduardo Habkost]

On Fri, Apr 26, 2013 at 07:29:56PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I believe you meant "once CPU is realized" on subject line?

Except for that:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  target-i386/cpu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index f1cecc0..3152ad5 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1289,6 +1289,12 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
>      Error *error = NULL;
>      int64_t value;
>  
> +    if (DEVICE(obj)->realized) {
> +        error_setg(errp, "Attempt to set property '%s' on '%s' after "
> +                   "it was realized", name, object_get_typename(obj));
> +        return;
> +    }
> +
>      visit_type_int(v, &value, name, &error);
>      if (error) {
>          error_propagate(errp, error);
> -- 
> 1.8.2.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: Do not allow to set apic-id one CPU is realized

[Andreas Färber]

Am 26.04.2013 19:29, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index f1cecc0..3152ad5 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1289,6 +1289,12 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
>      Error *error = NULL;
>      int64_t value;
>  
> +    if (DEVICE(obj)->realized) {

http://wiki.qemu.org/QOMConventions

Anthony asked to not do FOO(x)->bar. Please add a DeviceState *dev
variable above.

> +        error_setg(errp, "Attempt to set property '%s' on '%s' after "
> +                   "it was realized", name, object_get_typename(obj));

Peter had introduced a helper for that that you had pointed me to
before, but I believe it's static in qdev-properties.c, so copying seems
the easiest here. Fine with me if you could address the above.

Andreas

> +        return;
> +    }
> +
>      visit_type_int(v, &value, name, &error);
>      if (error) {
>          error_propagate(errp, error);
> 


-- 
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] [PATCH] target-i386: Do not allow to set apic-id one CPU is realized

[Igor Mammedov]

On Fri, 26 Apr 2013 19:39:01 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 26.04.2013 19:29, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index f1cecc0..3152ad5 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1289,6 +1289,12 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
> >      Error *error = NULL;
> >      int64_t value;
> >  
> > +    if (DEVICE(obj)->realized) {
> 
> http://wiki.qemu.org/QOMConventions
> 
> Anthony asked to not do FOO(x)->bar. Please add a DeviceState *dev
> variable above.
Ok, I wanted to save a line. I'll repost in a minute.

> 
> > +        error_setg(errp, "Attempt to set property '%s' on '%s' after "
> > +                   "it was realized", name, object_get_typename(obj));
> 
> Peter had introduced a helper for that that you had pointed me to
> before, but I believe it's static in qdev-properties.c, so copying seems
> the easiest here. Fine with me if you could address the above.
> 
> Andreas
> 
> > +        return;
> > +    }
> > +
> >      visit_type_int(v, &value, name, &error);
> >      if (error) {
> >          error_propagate(errp, error);
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 


-- 
Regards,
  Igor

[Qemu-devel] [PATCH] target-i386: Do not allow to set apic-id once CPU is realized

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f1cecc0..0d9493d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1284,11 +1284,18 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
                                   const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
+    DeviceState *dev = DEVICE(obj);
     const int64_t min = 0;
     const int64_t max = UINT32_MAX;
     Error *error = NULL;
     int64_t value;
 
+    if (dev->realized) {
+        error_setg(errp, "Attempt to set property '%s' on '%s' after "
+                   "it was realized", name, object_get_typename(obj));
+        return;
+    }
+
     visit_type_int(v, &value, name, &error);
     if (error) {
         error_propagate(errp, error);
-- 
1.8.2.1

Re: [Qemu-devel] [PATCH] target-i386: Do not allow to set apic-id once CPU is realized

[Andreas Färber]

Am 26.04.2013 19:51, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Thanks, applied to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

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

[Qemu-devel] [PATCH 05/15] target-i386: introduce ICC bus/device/bridge

[Igor Mammedov]

Provides hotplug-able bus for APIC and CPU.

* icc-bridge will serve as a parent for icc-bus and provide
  mmio mapping services to child icc-devices.
* icc-device will replace SysBusDevice as a parent of APIC
  and IOAPIC devices.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  * mv include/hw/i386/icc_bus.h into include/hw/cpu/
  * various style fixes
  * embed icc-bus inside icc-bridge and use qbus_create_inplace()
  * update MAINTAINERS with new files
v2:
  * Rebase on top the last HW reorganization series.
  * Move hw/icc_bus.c into hw/cpu/ and hw/icc_bus.h include/hw/i386/
---
 MAINTAINERS                        |    6 ++
 default-configs/i386-softmmu.mak   |    1 +
 default-configs/x86_64-softmmu.mak |    1 +
 hw/cpu/Makefile.objs               |    1 +
 hw/cpu/icc_bus.c                   |  104 ++++++++++++++++++++++++++++++++++++
 hw/i386/pc_piix.c                  |    7 +++
 hw/i386/pc_q35.c                   |    7 +++
 include/hw/cpu/icc_bus.h           |   58 ++++++++++++++++++++
 8 files changed, 185 insertions(+), 0 deletions(-)
 create mode 100644 hw/cpu/icc_bus.c
 create mode 100644 include/hw/cpu/icc_bus.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 4dfd8bf..be02724 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -644,6 +644,12 @@ F: qom/cpu.c
 F: include/qemu/cpu.h
 F: target-i386/cpu.c
 
+ICC Bus
+M: Igor Mammedov <imammedo@redhat.com>
+S: Supported
+F: include/hw/cpu/icc_bus.h
+F: hw/cpu/icc_bus.c
+
 Device Tree
 M: Peter Crosthwaite <peter.crosthwaite@petalogix.com>
 M: Alexander Graf <agraf@suse.de>
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 368a776..833e722 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -44,3 +44,4 @@ CONFIG_LPC_ICH9=y
 CONFIG_PCI_Q35=y
 CONFIG_APIC=y
 CONFIG_IOAPIC=y
+CONFIG_ICC_BUS=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 2711b83..b749eb7 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -44,3 +44,4 @@ CONFIG_LPC_ICH9=y
 CONFIG_PCI_Q35=y
 CONFIG_APIC=y
 CONFIG_IOAPIC=y
+CONFIG_ICC_BUS=y
diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
index a49ca04..4461ece 100644
--- a/hw/cpu/Makefile.objs
+++ b/hw/cpu/Makefile.objs
@@ -1,4 +1,5 @@
 obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
 obj-$(CONFIG_ARM9MPCORE) += a9mpcore.o
 obj-$(CONFIG_ARM15MPCORE) += a15mpcore.o
+obj-$(CONFIG_ICC_BUS) += icc_bus.o
 
diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
new file mode 100644
index 0000000..f6091b9
--- /dev/null
+++ b/hw/cpu/icc_bus.c
@@ -0,0 +1,104 @@
+/* icc_bus.c
+ * emulate x86 ICC(Interrupt Controller Communications) bus
+ *
+ * Copyright (c) 2013 Red Hat, Inc
+ *
+ * Authors:
+ *     Igor Mammedov <imammedo@redhat.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#include "hw/cpu/icc_bus.h"
+#include "hw/sysbus.h"
+
+/* icc-bridge implementation */
+static void icc_bus_init(Object *obj)
+{
+    BusState *b = BUS(obj);
+
+    b->allow_hotplug = true;
+}
+
+static const TypeInfo icc_bus_info = {
+    .name = TYPE_ICC_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(ICCBus),
+    .instance_init = icc_bus_init,
+};
+
+/* icc-device implementation */
+static void icc_device_realizefn(DeviceState *dev, Error **err)
+{
+    ICCDevice *id = ICC_DEVICE(dev);
+    ICCDeviceClass *k = ICC_DEVICE_GET_CLASS(id);
+    Error *local_err = NULL;
+
+    if (k->init) {
+        if (k->init(id) < 0) {
+            error_setg(&local_err, "%s initialization failed.",
+                       object_get_typename(OBJECT(dev)));
+            error_propagate(err, local_err);
+        }
+    }
+}
+
+static void icc_device_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *k = DEVICE_CLASS(klass);
+
+    k->realize = icc_device_realizefn;
+    k->bus_type = TYPE_ICC_BUS;
+}
+
+static const TypeInfo icc_device_info = {
+    .name = TYPE_ICC_DEVICE,
+    .parent = TYPE_DEVICE,
+    .abstract = true,
+    .instance_size = sizeof(ICCDevice),
+    .class_size = sizeof(ICCDeviceClass),
+    .class_init = icc_device_class_init,
+};
+
+/*  icc-bridge implementation */
+typedef struct ICCBridgeState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    ICCBus icc_bus;
+} ICCBridgeState;
+
+#define ICC_BRIGDE(obj) OBJECT_CHECK(ICCBridgeState, (obj), TYPE_ICC_BRIDGE)
+
+static void icc_bridge_init(Object *obj)
+{
+    ICCBridgeState *s = ICC_BRIGDE(obj);
+
+    qbus_create_inplace(&s->icc_bus, TYPE_ICC_BUS, DEVICE(s), "icc-bus");
+}
+
+static const TypeInfo icc_bridge_info = {
+    .name  = TYPE_ICC_BRIDGE,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_init  = icc_bridge_init,
+    .instance_size  = sizeof(ICCBridgeState),
+};
+
+
+static void icc_bus_register_types(void)
+{
+    type_register_static(&icc_bus_info);
+    type_register_static(&icc_device_info);
+    type_register_static(&icc_bridge_info);
+}
+
+type_init(icc_bus_register_types)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 89b4cb4..51b738a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -37,6 +37,7 @@
 #include "hw/kvm/clock.h"
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
+#include "hw/cpu/icc_bus.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/blockdev.h"
 #include "hw/i2c/smbus.h"
@@ -85,8 +86,13 @@ static void pc_init1(MemoryRegion *system_memory,
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
+    DeviceState *icc_bridge;
     void *fw_cfg = NULL;
 
+    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
+    object_property_add_child(qdev_get_machine(), "icc-bridge",
+                              OBJECT(icc_bridge), NULL);
+
     pc_cpus_init(cpu_model);
     pc_acpi_init("acpi-dsdt.aml");
 
@@ -161,6 +167,7 @@ static void pc_init1(MemoryRegion *system_memory,
     if (pci_enabled) {
         ioapic_init_gsi(gsi_state, "i440fx");
     }
+    qdev_init_nofail(icc_bridge);
 
     pc_register_ferr_irq(gsi[13]);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index f160893..317dd0c 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -41,6 +41,7 @@
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/usb.h"
+#include "hw/cpu/icc_bus.h"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
@@ -73,6 +74,11 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     int i;
     ICH9LPCState *ich9_lpc;
     PCIDevice *ahci;
+    DeviceState *icc_bridge;
+
+    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
+    object_property_add_child(qdev_get_machine(), "icc-bridge",
+                              OBJECT(icc_bridge), NULL);
 
     pc_cpus_init(cpu_model);
     pc_acpi_init("q35-acpi-dsdt.aml");
@@ -156,6 +162,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     if (pci_enabled) {
         ioapic_init_gsi(gsi_state, NULL);
     }
+    qdev_init_nofail(icc_bridge);
 
     pc_register_ferr_irq(gsi[13]);
 
diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h
new file mode 100644
index 0000000..a0abc21
--- /dev/null
+++ b/include/hw/cpu/icc_bus.h
@@ -0,0 +1,58 @@
+/* icc_bus.h
+ * emulate x86 ICC(Interrupt Controller Communications) bus
+ *
+ * Copyright (c) 2013 Red Hat, Inc
+ *
+ * Authors:
+ *     Igor Mammedov <imammedo@redhat.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#ifndef ICC_BUS_H
+#define ICC_BUS_H
+
+#include "hw/qdev-core.h"
+
+#define TYPE_ICC_BUS "icc-bus"
+
+#ifndef CONFIG_USER_ONLY
+
+typedef struct ICCBus {
+    /*< private >*/
+    BusState parent_obj;
+} ICCBus;
+
+#define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
+
+typedef struct ICCDevice {
+    /*< public >*/
+    DeviceState qdev;
+} ICCDevice;
+
+typedef struct ICCDeviceClass {
+    DeviceClass parent_class;
+    int (*init)(ICCDevice *dev);
+} ICCDeviceClass;
+
+#define TYPE_ICC_DEVICE "icc-device"
+#define ICC_DEVICE(obj) OBJECT_CHECK(ICCDevice, (obj), TYPE_ICC_DEVICE)
+#define ICC_DEVICE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ICCDeviceClass, (klass), TYPE_ICC_DEVICE)
+#define ICC_DEVICE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ICCDeviceClass, (obj), TYPE_ICC_DEVICE)
+
+#define TYPE_ICC_BRIDGE "icc-bridge"
+
+#endif /* CONFIG_USER_ONLY */
+#endif
-- 
1.7.1

Re: [Qemu-devel] [PATCH 05/15] target-i386: introduce ICC bus/device/bridge

[Andreas Färber]

Am 25.04.2013 16:05, schrieb Igor Mammedov:
> Provides hotplug-able bus for APIC and CPU.
> 
> * icc-bridge will serve as a parent for icc-bus and provide
>   mmio mapping services to child icc-devices.
> * icc-device will replace SysBusDevice as a parent of APIC
>   and IOAPIC devices.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
>   * mv include/hw/i386/icc_bus.h into include/hw/cpu/
>   * various style fixes
>   * embed icc-bus inside icc-bridge and use qbus_create_inplace()
>   * update MAINTAINERS with new files
> v2:
>   * Rebase on top the last HW reorganization series.
>   * Move hw/icc_bus.c into hw/cpu/ and hw/icc_bus.h include/hw/i386/
> ---
>  MAINTAINERS                        |    6 ++
>  default-configs/i386-softmmu.mak   |    1 +
>  default-configs/x86_64-softmmu.mak |    1 +
>  hw/cpu/Makefile.objs               |    1 +
>  hw/cpu/icc_bus.c                   |  104 ++++++++++++++++++++++++++++++++++++
>  hw/i386/pc_piix.c                  |    7 +++
>  hw/i386/pc_q35.c                   |    7 +++
>  include/hw/cpu/icc_bus.h           |   58 ++++++++++++++++++++
>  8 files changed, 185 insertions(+), 0 deletions(-)
>  create mode 100644 hw/cpu/icc_bus.c
>  create mode 100644 include/hw/cpu/icc_bus.h

Thanks, queued on qom-cpu-next (with changes below):
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next

In particular since this is a new file I've taken the liberty to tidy
the header a bit; and since you had changed initfn -> init I renamed
realizefn alongside and normalized Error** argument.
When I converted the CPUs to QOM there were protests against the use of
k / klass and the compromise was to use oc, dc, etc., thus idc here.
Doing this temporarily on -next in case I run into non-trivial rebase
problems later on.

Andreas

diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index f6091b9..2db42b1 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -1,5 +1,5 @@
 /* icc_bus.c
- * emulate x86 ICC(Interrupt Controller Communications) bus
+ * emulate x86 ICC (Interrupt Controller Communications) bus
  *
  * Copyright (c) 2013 Red Hat, Inc
  *
@@ -23,6 +23,7 @@
 #include "hw/sysbus.h"

 /* icc-bridge implementation */
+
 static void icc_bus_init(Object *obj)
 {
     BusState *b = BUS(obj);
@@ -37,18 +38,18 @@ static const TypeInfo icc_bus_info = {
     .instance_init = icc_bus_init,
 };

+
 /* icc-device implementation */
-static void icc_device_realizefn(DeviceState *dev, Error **err)
+
+static void icc_device_realize(DeviceState *dev, Error **errp)
 {
     ICCDevice *id = ICC_DEVICE(dev);
     ICCDeviceClass *k = ICC_DEVICE_GET_CLASS(id);
-    Error *local_err = NULL;

     if (k->init) {
         if (k->init(id) < 0) {
-            error_setg(&local_err, "%s initialization failed.",
+            error_setg(errp, "%s initialization failed.",
                        object_get_typename(OBJECT(dev)));
-            error_propagate(err, local_err);
         }
     }
 }
@@ -57,7 +58,7 @@ static void icc_device_class_init(ObjectClass *klass,
void *da
ta)
 {
     DeviceClass *k = DEVICE_CLASS(klass);

-    k->realize = icc_device_realizefn;
+    k->realize = icc_device_realize;
     k->bus_type = TYPE_ICC_BUS;
 }

@@ -70,10 +71,14 @@ static const TypeInfo icc_device_info = {
     .class_init = icc_device_class_init,
 };

+
 /*  icc-bridge implementation */
+
 typedef struct ICCBridgeState {
     /*< private >*/
     SysBusDevice parent_obj;
+    /*< public >*/
+
     ICCBus icc_bus;
 } ICCBridgeState;

diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h
index a0abc21..db252c7 100644
--- a/include/hw/cpu/icc_bus.h
+++ b/include/hw/cpu/icc_bus.h
@@ -1,5 +1,5 @@
 /* icc_bus.h
- * emulate x86 ICC(Interrupt Controller Communications) bus
+ * emulate x86 ICC (Interrupt Controller Communications) bus
  *
  * Copyright (c) 2013 Red Hat, Inc
  *
@@ -28,21 +28,42 @@

 #ifndef CONFIG_USER_ONLY

+/**
+ * ICCBus:
+ *
+ * ICC bus
+ */
 typedef struct ICCBus {
     /*< private >*/
     BusState parent_obj;
+    /*< public >*/
 } ICCBus;

 #define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)

+/**
+ * ICCDevice:
+ *
+ * ICC device
+ */
 typedef struct ICCDevice {
+    /*< private >*/
+    DeviceState parent_obj;
     /*< public >*/
-    DeviceState qdev;
 } ICCDevice;

+/**
+ * ICCDeviceClass:
+ * @init: Initialization callback for derived classes.
+ *
+ * ICC device class
+ */
 typedef struct ICCDeviceClass {
+    /*< private >*/
     DeviceClass parent_class;
-    int (*init)(ICCDevice *dev);
+    /*< public >*/
+
+    int (*init)(ICCDevice *dev); /* TODO replace with QOM realize */
 } ICCDeviceClass;

 #define TYPE_ICC_DEVICE "icc-device"



diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 2db42b1..a89bbb3 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -56,10 +56,10 @@ static void icc_device_realize(DeviceState *dev,
Error **errp)

 static void icc_device_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *k = DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);

-    k->realize = icc_device_realize;
-    k->bus_type = TYPE_ICC_BUS;
+    dc->realize = icc_device_realize;
+    dc->bus_type = TYPE_ICC_BUS;
 }

 static const TypeInfo icc_device_info = {



diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index a89bbb3..1d5e646 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -44,19 +44,19 @@ static const TypeInfo icc_bus_info = {
 static void icc_device_realize(DeviceState *dev, Error **errp)
 {
     ICCDevice *id = ICC_DEVICE(dev);
-    ICCDeviceClass *k = ICC_DEVICE_GET_CLASS(id);
+    ICCDeviceClass *idc = ICC_DEVICE_GET_CLASS(id);

-    if (k->init) {
-        if (k->init(id) < 0) {
+    if (idc->init) {
+        if (idc->init(id) < 0) {
             error_setg(errp, "%s initialization failed.",
                        object_get_typename(OBJECT(dev)));
         }
     }
 }

-static void icc_device_class_init(ObjectClass *klass, void *data)
+static void icc_device_class_init(ObjectClass *oc, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(oc);

     dc->realize = icc_device_realize;
     dc->bus_type = TYPE_ICC_BUS;

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

[Qemu-devel] [PATCH 06/15] target-i386: cpu: attach ICC bus to CPU on its creation

[Igor Mammedov]

X86CPU should have parent bus so it would be possible to unplug
it later. Set bus_type to TYPE_ICC_BUS for X86CPU type to make
device_add attach hotplugged CPU to ICC bus.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * make sure that missing or ambiguous icc-bus will cause error
    on softmmu target.
---
 target-i386/cpu.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b0eb6ca..25eb158 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -41,6 +41,7 @@
 #endif
 
 #include "sysemu/sysemu.h"
+#include "hw/cpu/icc_bus.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/xen/xen.h"
 #include "hw/sysbus.h"
@@ -1619,6 +1620,19 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
     features = model_pieces[1];
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
+#ifndef CONFIG_USER_ONLY
+    do {
+        bool ambiguous = false;
+        Object *icc_bus = object_resolve_path_type("icc-bus", TYPE_ICC_BUS,
+                                                   &ambiguous);
+        if ((icc_bus == NULL) || ambiguous) {
+            error_setg(&error, "Invalid icc-bus value");
+            goto out;
+        }
+        qdev_set_parent_bus(DEVICE(cpu), BUS(icc_bus));
+        object_unref(OBJECT(cpu));
+   } while (0);
+#endif
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
@@ -2330,6 +2344,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_realize = dc->realize;
     dc->realize = x86_cpu_realizefn;
+    dc->bus_type = TYPE_ICC_BUS;
 
     xcc->parent_reset = cc->reset;
     cc->reset = x86_cpu_reset;
-- 
1.7.1

[Qemu-devel] [PATCH 07/15] target-i386: replace MSI_SPACE_SIZE with APIC_SPACE_SIZE

[Igor Mammedov]

... and put APIC_SPACE_SIZE in public header so that it could be
reused later elsewhere.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/i386/kvm/apic.c              |    2 +-
 hw/intc/apic.c                  |    2 +-
 hw/xen/xen_apic.c               |    2 +-
 include/hw/i386/apic_internal.h |    2 --
 target-i386/cpu.h               |    1 +
 5 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index c6ff982..8f80425 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -174,7 +174,7 @@ static const MemoryRegionOps kvm_apic_io_ops = {
 static void kvm_apic_init(APICCommonState *s)
 {
     memory_region_init_io(&s->io_memory, &kvm_apic_io_ops, s, "kvm-apic-msi",
-                          MSI_SPACE_SIZE);
+                          APIC_SPACE_SIZE);
 
     if (kvm_has_gsi_routing()) {
         msi_supported = true;
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 2d79a9e..756dff0 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -874,7 +874,7 @@ static const MemoryRegionOps apic_io_ops = {
 static void apic_init(APICCommonState *s)
 {
     memory_region_init_io(&s->io_memory, &apic_io_ops, s, "apic-msi",
-                          MSI_SPACE_SIZE);
+                          APIC_SPACE_SIZE);
 
     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
     local_apics[s->idx] = s;
diff --git a/hw/xen/xen_apic.c b/hw/xen/xen_apic.c
index a2eb8a1..a958782 100644
--- a/hw/xen/xen_apic.c
+++ b/hw/xen/xen_apic.c
@@ -39,7 +39,7 @@ static const MemoryRegionOps xen_apic_io_ops = {
 static void xen_apic_init(APICCommonState *s)
 {
     memory_region_init_io(&s->io_memory, &xen_apic_io_ops, s, "xen-apic-msi",
-                          MSI_SPACE_SIZE);
+                          APIC_SPACE_SIZE);
 
 #if defined(CONFIG_XEN_CTRL_INTERFACE_VERSION) \
     && CONFIG_XEN_CTRL_INTERFACE_VERSION >= 420
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 578241f..aac6290 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -66,8 +66,6 @@
 
 #define MAX_APICS 255
 
-#define MSI_SPACE_SIZE                  0x100000
-
 typedef struct APICCommonState APICCommonState;
 
 #define TYPE_APIC_COMMON "apic-common"
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index a1614e8..ab151d5 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1270,5 +1270,6 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
 void enable_compat_apic_id_mode(void);
 
 #define APIC_DEFAULT_ADDRESS 0xfee00000
+#define APIC_SPACE_SIZE      0x100000
 
 #endif /* CPU_I386_H */
-- 
1.7.1

Re: [Qemu-devel] [PATCH 07/15] target-i386: replace MSI_SPACE_SIZE with APIC_SPACE_SIZE

[Andreas Färber]

Am 25.04.2013 16:05, schrieb Igor Mammedov:
> ... and put APIC_SPACE_SIZE in public header so that it could be
> reused later elsewhere.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/i386/kvm/apic.c              |    2 +-
>  hw/intc/apic.c                  |    2 +-
>  hw/xen/xen_apic.c               |    2 +-
>  include/hw/i386/apic_internal.h |    2 --
>  target-i386/cpu.h               |    1 +
>  5 files changed, 4 insertions(+), 5 deletions(-)

Thanks, applied to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

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

[Qemu-devel] [PATCH 08/15] target-i386: kvmvapic: make expilict dependency on sysbus.h

[Igor Mammedov]

Allows kvmvapic to compile if sysbus.h is removed from apic_internal.h,
from which it is indirectly included.
sysbus.h will be removed from apic_internal.h after converting
APICs to ICCDevice.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Note:
  split it in separate patch from "move APIC to ICC bus" patch to
  simplify review. Feel free to squash it back.
---
 hw/i386/kvmvapic.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 3a10c07..5b558aa 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -12,6 +12,7 @@
 #include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "hw/i386/apic_internal.h"
+#include "hw/sysbus.h"
 
 #define VAPIC_IO_PORT           0x7e
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 08/15] target-i386: kvmvapic: make expilict dependency on sysbus.h

[Andreas Färber]

Am 25.04.2013 16:05, schrieb Igor Mammedov:
> Allows kvmvapic to compile if sysbus.h is removed from apic_internal.h,
> from which it is indirectly included.
> sysbus.h will be removed from apic_internal.h after converting
> APICs to ICCDevice.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Note:
>   split it in separate patch from "move APIC to ICC bus" patch to
>   simplify review. Feel free to squash it back.
> ---
>  hw/i386/kvmvapic.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

Thanks, applied to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Making header dependencies less accidental is a good deed, and APIC
conversion seems still blocked by MemoryRegion discussions ATM.

Andreas

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

[Qemu-devel] [PATCH 09/15] target-i386: move APIC to ICC bus

[Igor Mammedov]

It allows APIC to be hotplugged

 * map APIC's mmio at board level if it is present
 * do not register mmio region for each APIC, since
   only one is used/mapped

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * various style fixes
  * split out sysbus.h inclusion in kvmvapic.c to simplify review
  * squash pc_new_cpu() hunk into "introduce apic-id property" patch
    to simplify review
  * use qdev_get_parent_bus() to get icc-bus for apic
---
 hw/cpu/icc_bus.c                |   10 ++++++++++
 hw/i386/pc.c                    |   15 ++++++++++++++-
 hw/intc/apic_common.c           |   18 ++++++++++++------
 include/hw/cpu/icc_bus.h        |    4 ++++
 include/hw/i386/apic_internal.h |    6 +++---
 target-i386/cpu.c               |   16 +++-------------
 6 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index f6091b9..9715e07 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -75,6 +75,7 @@ typedef struct ICCBridgeState {
     /*< private >*/
     SysBusDevice parent_obj;
     ICCBus icc_bus;
+    MemoryRegion apic_container;
 } ICCBridgeState;
 
 #define ICC_BRIGDE(obj) OBJECT_CHECK(ICCBridgeState, (obj), TYPE_ICC_BRIDGE)
@@ -82,8 +83,17 @@ typedef struct ICCBridgeState {
 static void icc_bridge_init(Object *obj)
 {
     ICCBridgeState *s = ICC_BRIGDE(obj);
+    SysBusDevice *sb = SYS_BUS_DEVICE(obj);
 
     qbus_create_inplace(&s->icc_bus, TYPE_ICC_BUS, DEVICE(s), "icc-bus");
+
+    /* Do not change order of registering regions,
+     * APIC must be first registered region, board maps it by 0 index
+     */
+    memory_region_init(&s->apic_container, "icc-apic-container",
+                       APIC_SPACE_SIZE);
+    sysbus_init_mmio(sb, &s->apic_container);
+    s->icc_bus.apic_address_space = &s->apic_container;
 }
 
 static const TypeInfo icc_bridge_info = {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7c7dd86..71a0200 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -53,6 +53,7 @@
 #include "qemu/bitmap.h"
 #include "qemu/config-file.h"
 #include "hw/acpi/acpi.h"
+#include "hw/cpu/icc_bus.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -916,7 +917,9 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
 void pc_cpus_init(const char *cpu_model)
 {
     int i;
+    X86CPU *cpu = NULL;
     Error *error = NULL;
+    SysBusDevice *icc_bridge;
 
     /* init CPUs */
     if (cpu_model == NULL) {
@@ -927,14 +930,24 @@ void pc_cpus_init(const char *cpu_model)
 #endif
     }
 
+    icc_bridge = SYS_BUS_DEVICE(object_resolve_path_type("icc-bridge",
+                                                         TYPE_ICC_BRIDGE,
+                                                         NULL));
+
     for (i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
+        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
         if (error) {
             fprintf(stderr, "%s\n", error_get_pretty(error));
             error_free(error);
             exit(1);
         }
     }
+
+    /* map APIC MMIO area if CPU has APIC */
+    if (cpu && cpu->env.apic_state) {
+        /* XXX: what if the base changes? */
+        sysbus_mmio_map_overlap(icc_bridge, 0, APIC_DEFAULT_ADDRESS, 0x1000);
+    }
 }
 
 void pc_acpi_init(const char *default_dsdt)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index e0ae07a..b03e904 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -21,6 +21,8 @@
 #include "hw/i386/apic_internal.h"
 #include "trace.h"
 #include "sysemu/kvm.h"
+#include "hw/qdev.h"
+#include "hw/sysbus.h"
 
 static int apic_irq_delivered;
 bool apic_report_tpr_access;
@@ -282,12 +284,13 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static int apic_init_common(SysBusDevice *dev)
+static int apic_init_common(ICCDevice *dev)
 {
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
     static int apic_no;
+    static bool mmio_registered;
 
     if (apic_no >= MAX_APICS) {
         return -1;
@@ -296,8 +299,11 @@ static int apic_init_common(SysBusDevice *dev)
 
     info = APIC_COMMON_GET_CLASS(s);
     info->init(s);
-
-    sysbus_init_mmio(dev, &s->io_memory);
+    if (!mmio_registered) {
+        ICCBus *b = ICC_BUS(qdev_get_parent_bus(DEVICE(dev)));
+        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
+        mmio_registered = true;
+    }
 
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
@@ -375,19 +381,19 @@ static Property apic_properties_common[] = {
 
 static void apic_common_class_init(ObjectClass *klass, void *data)
 {
-    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
+    ICCDeviceClass *idc = ICC_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_apic_common;
     dc->reset = apic_reset_common;
     dc->no_user = 1;
     dc->props = apic_properties_common;
-    sc->init = apic_init_common;
+    idc->init = apic_init_common;
 }
 
 static const TypeInfo apic_common_type = {
     .name = TYPE_APIC_COMMON,
-    .parent = TYPE_SYS_BUS_DEVICE,
+    .parent = TYPE_ICC_DEVICE,
     .instance_size = sizeof(APICCommonState),
     .class_size = sizeof(APICCommonClass),
     .class_init = apic_common_class_init,
diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h
index a0abc21..53ef223 100644
--- a/include/hw/cpu/icc_bus.h
+++ b/include/hw/cpu/icc_bus.h
@@ -22,6 +22,7 @@
 #ifndef ICC_BUS_H
 #define ICC_BUS_H
 
+#include "exec/memory.h"
 #include "hw/qdev-core.h"
 
 #define TYPE_ICC_BUS "icc-bus"
@@ -31,6 +32,9 @@
 typedef struct ICCBus {
     /*< private >*/
     BusState parent_obj;
+
+    /*< public >*/
+    MemoryRegion *apic_address_space;
 } ICCBus;
 
 #define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index aac6290..1b0a7fb 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -21,7 +21,7 @@
 #define QEMU_APIC_INTERNAL_H
 
 #include "exec/memory.h"
-#include "hw/sysbus.h"
+#include "hw/cpu/icc_bus.h"
 #include "qemu/timer.h"
 
 /* APIC Local Vector Table */
@@ -78,7 +78,7 @@ typedef struct APICCommonState APICCommonState;
 
 typedef struct APICCommonClass
 {
-    SysBusDeviceClass parent_class;
+    ICCDeviceClass parent_class;
 
     void (*init)(APICCommonState *s);
     void (*set_base)(APICCommonState *s, uint64_t val);
@@ -92,7 +92,7 @@ typedef struct APICCommonClass
 } APICCommonClass;
 
 struct APICCommonState {
-    SysBusDevice busdev;
+    ICCDevice busdev;
 
     MemoryRegion io_memory;
     X86CPU *cpu;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 25eb158..708fb74 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -41,10 +41,10 @@
 #endif
 
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
 #include "hw/cpu/icc_bus.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/xen/xen.h"
-#include "hw/sysbus.h"
 #include "hw/i386/apic_internal.h"
 #endif
 
@@ -2119,6 +2119,7 @@ static void mce_init(X86CPU *cpu)
 static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
     CPUX86State *env = &cpu->env;
+    DeviceState *dev = DEVICE(cpu);
     APICCommonState *apic;
     const char *apic_type = "apic";
 
@@ -2128,7 +2129,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
         apic_type = "xen-apic";
     }
 
-    env->apic_state = qdev_try_create(NULL, apic_type);
+    env->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
     if (env->apic_state == NULL) {
         error_setg(errp, "APIC device '%s' could not be created", apic_type);
         return;
@@ -2145,7 +2146,6 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 {
     CPUX86State *env = &cpu->env;
-    static int apic_mapped;
 
     if (env->apic_state == NULL) {
         return;
@@ -2156,16 +2156,6 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
                    object_get_typename(OBJECT(env->apic_state)));
         return;
     }
-
-    /* XXX: mapping more APICs at the same memory location */
-    if (apic_mapped == 0) {
-        /* NOTE: the APIC is directly connected to the CPU - it is not
-           on the global memory bus. */
-        /* XXX: what if the base changes? */
-        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(env->apic_state), 0,
-                                APIC_DEFAULT_ADDRESS, 0x1000);
-        apic_mapped = 1;
-    }
 }
 #else
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
-- 
1.7.1

[Qemu-devel] [PATCH 10/15] extend memory_region_find() and use it in kvm/ioapic

[Igor Mammedov]

From: Paolo Bonzini <pbonzini@redhat.com>

kvm/ioapic is relying on the fact that SysBus device
maps mmio regions with offset counted from start of system memory.
But if ioapic's region is moved to another sub-region which doesn't
start at the beginning of system memory then using offset isn't correct.

To fix kvm/ioapic, extend memory_region_find() so that it can help
retrieving the absolute region address and the respective address space.

The patch is a no-op in case mr is parentless, i.e. mr->addr == 0
and mr->parent == NULL.

In addition fill in MemoryRegionSection.as which was missing in original
memory_region_find().

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/kvm/ioapic.c  |    9 ++++++++-
 include/exec/memory.h |   13 +++++++------
 memory.c              |   20 +++++++++++++++-----
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index a3bd519..dc6ddab 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -89,14 +89,21 @@ static void kvm_ioapic_put(IOAPICCommonState *s)
 {
     struct kvm_irqchip chip;
     struct kvm_ioapic_state *kioapic;
+    MemoryRegionSection mrs;
     int ret, i;
 
+    mrs = memory_region_find(&s->io_memory, 0, 0x1000);
+    if (mrs.mr != &s->io_memory || mrs.offset_within_region != 0) {
+        fprintf(stderr, "cannot find IOAPIC base\n");
+        abort();
+    }
+
     chip.chip_id = KVM_IRQCHIP_IOAPIC;
     kioapic = &chip.chip.ioapic;
 
     kioapic->id = s->id;
     kioapic->ioregsel = s->ioregsel;
-    kioapic->base_address = s->busdev.mmio[0].addr;
+    kioapic->base_address = mrs.offset_within_address_space;
     kioapic->irr = s->irr;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         kioapic->redirtbl[i].bits = s->ioredtbl[i];
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9e88320..efe210b 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -725,17 +725,18 @@ void memory_region_set_alias_offset(MemoryRegion *mr,
  *
  * Returns a #MemoryRegionSection that describes a contiguous overlap.
  * It will have the following characteristics:
- *    .@offset_within_address_space >= @addr
- *    .@offset_within_address_space + .@size <= @addr + @size
  *    .@size = 0 iff no overlap was found
  *    .@mr is non-%NULL iff an overlap was found
  *
- * @address_space: a top-level (i.e. parentless) region that contains
- *       the region to be found
- * @addr: start of the area within @address_space to be searched
+ * If @mr is parent-less,
+ *    .@offset_within_address_space >= @addr
+ *    .@offset_within_address_space + .@size <= @addr + @size
+ *
+ * @mr: a (possibly indirect) parent that contains the region to be found
+ * @addr: start of the area within @as to be searched
  * @size: size of the area to be searched
  */
-MemoryRegionSection memory_region_find(MemoryRegion *address_space,
+MemoryRegionSection memory_region_find(MemoryRegion *mr,
                                        hwaddr addr, uint64_t size);
 
 /**
diff --git a/memory.c b/memory.c
index 75ca281..34bfb13 100644
--- a/memory.c
+++ b/memory.c
@@ -1451,15 +1451,24 @@ static FlatRange *address_space_lookup(AddressSpace *as, AddrRange addr)
                    sizeof(FlatRange), cmp_flatrange_addr);
 }
 
-MemoryRegionSection memory_region_find(MemoryRegion *address_space,
+MemoryRegionSection memory_region_find(MemoryRegion *mr,
                                        hwaddr addr, uint64_t size)
 {
-    AddressSpace *as = memory_region_to_address_space(address_space);
-    AddrRange range = addrrange_make(int128_make64(addr),
-                                     int128_make64(size));
-    FlatRange *fr = address_space_lookup(as, range);
     MemoryRegionSection ret = { .mr = NULL, .size = 0 };
+    MemoryRegion *root;
+    AddressSpace *as;
+    AddrRange range;
+    FlatRange *fr;
+
+    addr += mr->addr;
+    for (root = mr; root->parent; ) {
+        root = root->parent;
+        addr += root->addr;
+    }
 
+    as = memory_region_to_address_space(root);
+    range = addrrange_make(int128_make64(addr), int128_make64(size));
+    fr = address_space_lookup(as, range);
     if (!fr) {
         return ret;
     }
@@ -1470,6 +1479,7 @@ MemoryRegionSection memory_region_find(MemoryRegion *address_space,
     }
 
     ret.mr = fr->mr;
+    ret.address_space = as;
     range = addrrange_intersection(range, fr->addr);
     ret.offset_within_region = fr->offset_in_region;
     ret.offset_within_region += int128_get64(int128_sub(range.start,
-- 
1.7.1

[Qemu-devel] [PATCH 11/15] target-i386: move IOAPIC to ICC bus

[Igor Mammedov]

 * inherit IOAPICs from ICCDevice and attach them to ICC bus
 * map IOAPIC's mmio at board level via indirect icc-bridge
   mmio region that provides address space to IOAPIC via
   icc-bus

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
 * split out introduction of memory_region_get_address() helper
 * use qdev_get_parent_bus() to get parent bus
v2:
 * do not create IOAPIC in icc-bridge and do not make it QOM child
   of icc-bridge, just attach it to icc-bus.
---
 hw/cpu/icc_bus.c                  |    6 ++++++
 hw/i386/pc.c                      |   10 ++++++----
 hw/intc/ioapic_common.c           |   14 ++++++++++----
 include/exec/memory.h             |    2 +-
 include/hw/cpu/icc_bus.h          |    1 +
 include/hw/i386/ioapic_internal.h |    6 +++---
 6 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 9715e07..b9e4f23 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -76,6 +76,7 @@ typedef struct ICCBridgeState {
     SysBusDevice parent_obj;
     ICCBus icc_bus;
     MemoryRegion apic_container;
+    MemoryRegion ioapic_container;
 } ICCBridgeState;
 
 #define ICC_BRIGDE(obj) OBJECT_CHECK(ICCBridgeState, (obj), TYPE_ICC_BRIDGE)
@@ -94,6 +95,11 @@ static void icc_bridge_init(Object *obj)
                        APIC_SPACE_SIZE);
     sysbus_init_mmio(sb, &s->apic_container);
     s->icc_bus.apic_address_space = &s->apic_container;
+
+    /* must be second registered region, board maps it by 1 index */
+    memory_region_init(&s->ioapic_container, "icc-ioapic-container", 0x1000);
+    sysbus_init_mmio(sb, &s->ioapic_container);
+    s->icc_bus.ioapic_address_space = &s->ioapic_container;
 }
 
 static const TypeInfo icc_bridge_info = {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 71a0200..4c2413b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1221,20 +1221,22 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
     DeviceState *dev;
     SysBusDevice *d;
     unsigned int i;
+    BusState *b = BUS(object_resolve_path_type("icc-bus", TYPE_ICC_BUS, NULL));
 
     if (kvm_irqchip_in_kernel()) {
-        dev = qdev_create(NULL, "kvm-ioapic");
+        dev = qdev_create(b, "kvm-ioapic");
     } else {
-        dev = qdev_create(NULL, "ioapic");
+        dev = qdev_create(b, "ioapic");
     }
     if (parent_name) {
         object_property_add_child(object_resolve_path(parent_name, NULL),
                                   "ioapic", OBJECT(dev), NULL);
     }
     qdev_init_nofail(dev);
-    d = SYS_BUS_DEVICE(dev);
-    sysbus_mmio_map(d, 0, IO_APIC_DEFAULT_ADDRESS);
 
+    d = SYS_BUS_DEVICE(object_resolve_path_type("icc-bridge", TYPE_ICC_BRIDGE,
+                                                NULL));
+    sysbus_mmio_map(d, 1, IO_APIC_DEFAULT_ADDRESS);
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
     }
diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index 5c5bb3c..a70d8a2 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -57,11 +57,12 @@ static int ioapic_dispatch_post_load(void *opaque, int version_id)
     return 0;
 }
 
-static int ioapic_init_common(SysBusDevice *dev)
+static int ioapic_init_common(ICCDevice *dev)
 {
     IOAPICCommonState *s = IOAPIC_COMMON(dev);
     IOAPICCommonClass *info;
     static int ioapic_no;
+    static bool mmio_registered;
 
     if (ioapic_no >= MAX_IOAPICS) {
         return -1;
@@ -70,7 +71,12 @@ static int ioapic_init_common(SysBusDevice *dev)
     info = IOAPIC_COMMON_GET_CLASS(s);
     info->init(s, ioapic_no);
 
-    sysbus_init_mmio(&s->busdev, &s->io_memory);
+    if (!mmio_registered) {
+        ICCBus *b = ICC_BUS(qdev_get_parent_bus(DEVICE(dev)));
+        memory_region_add_subregion(b->ioapic_address_space, 0, &s->io_memory);
+        mmio_registered = true;
+    }
+
     ioapic_no++;
 
     return 0;
@@ -95,7 +101,7 @@ static const VMStateDescription vmstate_ioapic_common = {
 
 static void ioapic_common_class_init(ObjectClass *klass, void *data)
 {
-    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
+    ICCDeviceClass *sc = ICC_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     sc->init = ioapic_init_common;
@@ -105,7 +111,7 @@ static void ioapic_common_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo ioapic_common_type = {
     .name = TYPE_IOAPIC_COMMON,
-    .parent = TYPE_SYS_BUS_DEVICE,
+    .parent = TYPE_ICC_DEVICE,
     .instance_size = sizeof(IOAPICCommonState),
     .class_size = sizeof(IOAPICCommonClass),
     .class_init = ioapic_common_class_init,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index efe210b..b840bee 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -733,7 +733,7 @@ void memory_region_set_alias_offset(MemoryRegion *mr,
  *    .@offset_within_address_space + .@size <= @addr + @size
  *
  * @mr: a (possibly indirect) parent that contains the region to be found
- * @addr: start of the area within @as to be searched
+ * @addr: start of the area within @mr to be searched
  * @size: size of the area to be searched
  */
 MemoryRegionSection memory_region_find(MemoryRegion *mr,
diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h
index 53ef223..947125d 100644
--- a/include/hw/cpu/icc_bus.h
+++ b/include/hw/cpu/icc_bus.h
@@ -35,6 +35,7 @@ typedef struct ICCBus {
 
     /*< public >*/
     MemoryRegion *apic_address_space;
+    MemoryRegion *ioapic_address_space;
 } ICCBus;
 
 #define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
index 25576c8..8d5fe3d 100644
--- a/include/hw/i386/ioapic_internal.h
+++ b/include/hw/i386/ioapic_internal.h
@@ -24,7 +24,7 @@
 
 #include "hw/hw.h"
 #include "exec/memory.h"
-#include "hw/sysbus.h"
+#include "hw/cpu/icc_bus.h"
 
 #define MAX_IOAPICS                     1
 
@@ -82,14 +82,14 @@ typedef struct IOAPICCommonState IOAPICCommonState;
      OBJECT_GET_CLASS(IOAPICCommonClass, (obj), TYPE_IOAPIC_COMMON)
 
 typedef struct IOAPICCommonClass {
-    SysBusDeviceClass parent_class;
+    ICCDeviceClass parent_class;
     void (*init)(IOAPICCommonState *s, int instance_no);
     void (*pre_save)(IOAPICCommonState *s);
     void (*post_load)(IOAPICCommonState *s);
 } IOAPICCommonClass;
 
 struct IOAPICCommonState {
-    SysBusDevice busdev;
+    ICCDevice busdev;
     MemoryRegion io_memory;
     uint8_t id;
     uint8_t ioregsel;
-- 
1.7.1

[Qemu-devel] [PATCH 12/15] pc: pass QEMUMachineInitArgs down to pc_cpus_init()

[Igor Mammedov]

It allows to store default cpu_model if none was specified.

As side effect reduces size of the pc_piix.c code.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c         |   11 ++++----
 hw/i386/pc_piix.c    |   66 ++++++++++++-------------------------------------
 hw/i386/pc_q35.c     |   23 +++++++----------
 include/hw/i386/pc.h |    3 +-
 4 files changed, 33 insertions(+), 70 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4c2413b..b768b66 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -914,7 +914,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
     return cpu;
 }
 
-void pc_cpus_init(const char *cpu_model)
+void pc_cpus_init(QEMUMachineInitArgs *args)
 {
     int i;
     X86CPU *cpu = NULL;
@@ -922,11 +922,11 @@ void pc_cpus_init(const char *cpu_model)
     SysBusDevice *icc_bridge;
 
     /* init CPUs */
-    if (cpu_model == NULL) {
+    if (args->cpu_model == NULL) {
 #ifdef TARGET_X86_64
-        cpu_model = "qemu64";
+        args->cpu_model = "qemu64";
 #else
-        cpu_model = "qemu32";
+        args->cpu_model = "qemu32";
 #endif
     }
 
@@ -935,7 +935,8 @@ void pc_cpus_init(const char *cpu_model)
                                                          NULL));
 
     for (i = 0; i < smp_cpus; i++) {
-        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
+        cpu = pc_new_cpu(args->cpu_model, x86_cpu_apic_id_from_index(i),
+                         &error);
         if (error) {
             fprintf(stderr, "%s\n", error_get_pretty(error));
             error_free(error);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 51b738a..4cc6682 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -59,12 +59,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 /* PC hardware initialisation */
 static void pc_init1(MemoryRegion *system_memory,
                      MemoryRegion *system_io,
-                     ram_addr_t ram_size,
-                     const char *boot_device,
-                     const char *kernel_filename,
-                     const char *kernel_cmdline,
-                     const char *initrd_filename,
-                     const char *cpu_model,
+                     QEMUMachineInitArgs *args,
                      int pci_enabled,
                      int kvmclock_enabled)
 {
@@ -93,19 +88,19 @@ static void pc_init1(MemoryRegion *system_memory,
     object_property_add_child(qdev_get_machine(), "icc-bridge",
                               OBJECT(icc_bridge), NULL);
 
-    pc_cpus_init(cpu_model);
+    pc_cpus_init(args);
     pc_acpi_init("acpi-dsdt.aml");
 
     if (kvmclock_enabled) {
         kvmclock_create();
     }
 
-    if (ram_size >= 0xe0000000 ) {
-        above_4g_mem_size = ram_size - 0xe0000000;
+    if (args->ram_size >= 0xe0000000 ) {
+        above_4g_mem_size = args->ram_size - 0xe0000000;
         below_4g_mem_size = 0xe0000000;
     } else {
         above_4g_mem_size = 0;
-        below_4g_mem_size = ram_size;
+        below_4g_mem_size = args->ram_size;
     }
 
     if (pci_enabled) {
@@ -120,9 +115,9 @@ static void pc_init1(MemoryRegion *system_memory,
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
         fw_cfg = pc_memory_init(system_memory,
-                       kernel_filename, kernel_cmdline, initrd_filename,
-                       below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory);
+                                args->kernel_filename, args->kernel_cmdline,
+                                args->initrd_filename, below_4g_mem_size,
+                                above_4g_mem_size, rom_memory, &ram_memory);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
@@ -136,7 +131,7 @@ static void pc_init1(MemoryRegion *system_memory,
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
-                              system_memory, system_io, ram_size,
+                              system_memory, system_io, args->ram_size,
                               below_4g_mem_size,
                               0x100000000ULL - below_4g_mem_size,
                               0x100000000ULL + above_4g_mem_size,
@@ -203,7 +198,7 @@ static void pc_init1(MemoryRegion *system_memory,
 
     audio_init(isa_bus, pci_enabled ? pci_bus : NULL);
 
-    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
+    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, args->boot_device,
                  floppy, idebus[0], idebus[1], rtc_state);
 
     if (pci_enabled && usb_enabled(false)) {
@@ -229,17 +224,7 @@ static void pc_init1(MemoryRegion *system_memory,
 
 static void pc_init_pci(QEMUMachineInitArgs *args)
 {
-    ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
-    const char *boot_device = args->boot_device;
-    pc_init1(get_system_memory(),
-             get_system_io(),
-             ram_size, boot_device,
-             kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 1, 1);
+    pc_init1(get_system_memory(), get_system_io(), args, 1, 1);
 }
 
 static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
@@ -275,38 +260,19 @@ static void pc_init_pci_1_0(QEMUMachineInitArgs *args)
 /* PC init function for pc-0.10 to pc-0.13, and reused by xenfv */
 static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 {
-    ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
-    const char *boot_device = args->boot_device;
     disable_kvm_pv_eoi();
     enable_compat_apic_id_mode();
-    pc_init1(get_system_memory(),
-             get_system_io(),
-             ram_size, boot_device,
-             kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 1, 0);
+    pc_init1(get_system_memory(), get_system_io(), args, 1, 0);
 }
 
 static void pc_init_isa(QEMUMachineInitArgs *args)
 {
-    ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
-    const char *boot_device = args->boot_device;
-    if (cpu_model == NULL)
-        cpu_model = "486";
+    if (args->cpu_model == NULL) {
+        args->cpu_model = "486";
+    }
     disable_kvm_pv_eoi();
     enable_compat_apic_id_mode();
-    pc_init1(get_system_memory(),
-             get_system_io(),
-             ram_size, boot_device,
-             kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 0, 1);
+    pc_init1(get_system_memory(), get_system_io(), args, 0, 1);
 }
 
 #ifdef CONFIG_XEN
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 317dd0c..98b9bab 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -49,12 +49,6 @@
 /* PC hardware initialisation */
 static void pc_q35_init(QEMUMachineInitArgs *args)
 {
-    ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
-    const char *boot_device = args->boot_device;
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
     Q35PCIHost *q35_host;
     PCIBus *host_bus;
@@ -80,17 +74,17 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     object_property_add_child(qdev_get_machine(), "icc-bridge",
                               OBJECT(icc_bridge), NULL);
 
-    pc_cpus_init(cpu_model);
+    pc_cpus_init(args);
     pc_acpi_init("q35-acpi-dsdt.aml");
 
     kvmclock_create();
 
-    if (ram_size >= 0xb0000000) {
-        above_4g_mem_size = ram_size - 0xb0000000;
+    if (args->ram_size >= 0xb0000000) {
+        above_4g_mem_size = args->ram_size - 0xb0000000;
         below_4g_mem_size = 0xb0000000;
     } else {
         above_4g_mem_size = 0;
-        below_4g_mem_size = ram_size;
+        below_4g_mem_size = args->ram_size;
     }
 
     /* pci enabled */
@@ -105,9 +99,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
-                       initrd_filename, below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory);
+        pc_memory_init(get_system_memory(),
+                       args->kernel_filename, args->kernel_cmdline,
+                       args->initrd_filename, below_4g_mem_size,
+                       above_4g_mem_size, rom_memory, &ram_memory);
     }
 
     /* irq lines */
@@ -191,7 +186,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
                                     0xb100),
                       8, NULL, 0);
 
-    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
+    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, args->boot_device,
                  floppy, idebus[0], idebus[1], rtc_state);
 
     /* the rest devices to which pci devfn is automatically assigned */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 14b504c..d0815c5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -9,6 +9,7 @@
 #include "net/net.h"
 #include "exec/memory.h"
 #include "hw/i386/ioapic.h"
+#include "hw/boards.h"
 
 /* PC-style peripherals (also used by other machines).  */
 
@@ -78,7 +79,7 @@ extern int fd_bootchk;
 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_cpus_init(QEMUMachineInitArgs *args);
 void pc_acpi_init(const char *default_dsdt);
 void *pc_memory_init(MemoryRegion *system_memory,
                     const char *kernel_filename,
-- 
1.7.1

[Qemu-devel] [PATCH 13/15] add hot_add_cpu hook to QEMUMachine and export machine_args

[Igor Mammedov]

hot_add_cpu hook should be overriden by target that implements
cpu hot-add via cpu-add QMP command.

Make machine_args available globaly, it allows to reuse
machine_args->cpu_model during hotplug, instead of adding target
specific globals to keep a copy of cpu_model.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * convert stack veriable args to global machine_args
  * don't move current_machine before machine->init(), override hot_add_cpu
    staticaly instead.
---
 include/hw/boards.h |    3 +++
 vl.c                |    8 ++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 425bdc7..657f379 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -18,6 +18,8 @@ typedef struct QEMUMachineInitArgs {
     const char *cpu_model;
 } QEMUMachineInitArgs;
 
+extern QEMUMachineInitArgs machine_args;
+
 typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
 
 typedef void QEMUMachineResetFunc(void);
@@ -43,6 +45,7 @@ typedef struct QEMUMachine {
     GlobalProperty *compat_props;
     struct QEMUMachine *next;
     const char *hw_version;
+    void (*hot_add_cpu)(const int64_t id, Error **errp);
 } QEMUMachine;
 
 int qemu_register_machine(QEMUMachine *m);
diff --git a/vl.c b/vl.c
index 5981db0..faf5e4b 100644
--- a/vl.c
+++ b/vl.c
@@ -179,6 +179,8 @@ int main(int argc, char **argv)
 #define MAX_VIRTIO_CONSOLES 1
 #define MAX_SCLP_CONSOLES 1
 
+QEMUMachineInitArgs machine_args;
+
 static const char *data_dir[16];
 static int data_dir_idx;
 const char *bios_name = NULL;
@@ -4273,7 +4275,8 @@ int main(int argc, char **argv, char **envp)
 
     qdev_machine_init();
 
-    QEMUMachineInitArgs args = { .ram_size = ram_size,
+    machine_args = (QEMUMachineInitArgs){
+                                 .ram_size = ram_size,
                                  .boot_device = (boot_devices[0] == '\0') ?
                                                 machine->boot_order :
                                                 boot_devices,
@@ -4281,7 +4284,8 @@ int main(int argc, char **argv, char **envp)
                                  .kernel_cmdline = kernel_cmdline,
                                  .initrd_filename = initrd_filename,
                                  .cpu_model = cpu_model };
-    machine->init(&args);
+
+    machine->init(&machine_args);
 
     cpu_synchronize_all_post_init();
 
-- 
1.7.1

[Qemu-devel] [PATCH 14/15] target-i386: implement machine->hot_add_cpu hook

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * override .hot_add_cpu staticaly starting with 1.5 machine
---
 hw/i386/pc.c         |   19 +++++++++++++++++++
 hw/i386/pc_piix.c    |    1 +
 hw/i386/pc_q35.c     |    1 +
 include/hw/i386/pc.h |    1 +
 4 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b768b66..bc668a6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -914,6 +914,25 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
     return cpu;
 }
 
+void pc_hot_add_cpu(const int64_t id, Error **errp)
+{
+    int64_t apic_id = x86_cpu_apic_id_from_index(id);
+
+    if (cpu_exists(apic_id)) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", it already exists", id);
+        return;
+    }
+
+    if (id >= max_cpus) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", max allowed: %d", id, max_cpus - 1);
+        return;
+    }
+
+    pc_new_cpu(machine_args.cpu_model, apic_id, errp);
+}
+
 void pc_cpus_init(QEMUMachineInitArgs *args)
 {
     int i;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4cc6682..e8308df 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -291,6 +291,7 @@ static QEMUMachine pc_i440fx_machine_v1_5 = {
     .alias = "pc",
     .desc = "Standard PC (i440FX + PIIX, 1996)",
     .init = pc_init_pci,
+    .hot_add_cpu = pc_hot_add_cpu,
     .max_cpus = 255,
     .is_default = 1,
     DEFAULT_MACHINE_OPTIONS,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 98b9bab..03f0cb4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -209,6 +209,7 @@ static QEMUMachine pc_q35_machine_v1_5 = {
     .alias = "q35",
     .desc = "Standard PC (Q35 + ICH9, 2009)",
     .init = pc_q35_init,
+    .hot_add_cpu = pc_hot_add_cpu,
     .max_cpus = 255,
     DEFAULT_MACHINE_OPTIONS,
 };
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d0815c5..16629fb 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -79,6 +79,7 @@ extern int fd_bootchk;
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
+void pc_hot_add_cpu(const int64_t id, Error **errp);
 void pc_cpus_init(QEMUMachineInitArgs *args);
 void pc_acpi_init(const char *default_dsdt);
 void *pc_memory_init(MemoryRegion *system_memory,
-- 
1.7.1

[Qemu-devel] [PATCH 15/15] QMP: add cpu-add command

[Igor Mammedov]

Adds "cpu-add id=xxx" QMP command.

cpu-add's "id" argument is a CPU number in a range [0..max-cpus)

Example QMP command:
 -> { "execute": "cpu-add", "arguments": { "id": 2 } }
 <- { "return": {} }

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v7:
  * added "Since 1.5" to cpu-add qapi schema definition
v6:
  * added valid values description to qapi schema
  * split out cpu_hot_add hooks introduction into separate patch
  * split out implementation of cpu_hot_add for target-i386
v5:
  * accept id=[0..max_cpus) range in cpu-add command
v4:
  * merge "qmp: add cpu-add qmp command" & "target-i386: implement CPU hot-add" patches
  * move notifier call to CPUCLass.realize()
  * add hook cpu_hot_add to QEMUMachine
  * make QEMUMachineInitArgs global and keep default cpu_model there

v3:
  * it appears that 'online/offline' in cpu-set are confusing people
    with what command actually does and users might have to distinguish
    if 'offline' is not implemented by parsing error message. To simplify
    things replace cpu-set with cpu-add command to show more clear what
    command does and just add cpu-del when CPU remove is implemented.

v2:
  * s/cpu_set/cpu-set/
  * qmp doc style fix
  * use bool type instead of opencodding online/offline string
     suggested-by: Eric Blake <eblake@redhat.com>
---
 qapi-schema.json |   13 +++++++++++++
 qmp-commands.hx  |   23 +++++++++++++++++++++++
 qmp.c            |   10 ++++++++++
 3 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 751d3c2..bb90377 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1387,6 +1387,19 @@
 { 'command': 'cpu', 'data': {'index': 'int'} }
 
 ##
+# @cpu-add
+#
+# Adds CPU with specified ID
+#
+# @id: ID of CPU to be created, valid values [0..max_cpus)
+#
+# Returns: Nothing on success
+#
+# Since 1.5
+##
+{ 'command': 'cpu-add', 'data': {'id': 'int'} }
+
+##
 # @memsave:
 #
 # Save a portion of guest memory to a file.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4d65422..1e5d299 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -385,6 +385,29 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
 EQMP
 
     {
+        .name       = "cpu-add",
+        .args_type  = "id:i",
+        .mhandler.cmd_new = qmp_marshal_input_cpu_add,
+    },
+
+SQMP
+cpu-add
+-------
+
+Adds virtual cpu
+
+Arguments:
+
+- "id": cpu id (json-int)
+
+Example:
+
+-> { "execute": "cpu-add", "arguments": { "id": 2 } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "memsave",
         .args_type  = "val:l,size:i,filename:s,cpu:i?",
         .mhandler.cmd_new = qmp_marshal_input_memsave,
diff --git a/qmp.c b/qmp.c
index ed6c7ef..dd34be6 100644
--- a/qmp.c
+++ b/qmp.c
@@ -24,6 +24,7 @@
 #include "hw/qdev.h"
 #include "sysemu/blockdev.h"
 #include "qom/qom-qobject.h"
+#include "hw/boards.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -108,6 +109,15 @@ void qmp_cpu(int64_t index, Error **errp)
     /* Just do nothing */
 }
 
+void qmp_cpu_add(int64_t id, Error **errp)
+{
+    if (current_machine->hot_add_cpu) {
+        current_machine->hot_add_cpu(id, errp);
+    } else {
+        error_setg(errp, "Not supported");
+    }
+}
+
 #ifndef CONFIG_VNC
 /* If VNC support is enabled, the "true" query-vnc command is
    defined in the VNC subsystem */
-- 
1.7.1