[Qemu-devel] [PATCH v4 0/8] sPAPR CPU hotplug pre-requisites

[Qemu-devel] [PATCH v4 0/8] sPAPR CPU hotplug pre-requisites

[Bharata B Rao]

Hi,

These are the patches that are required to support CPU hotplug for sPAPR
guests. Until now these patches were carried as part of the combined
patchset for sPAPR CPU and Memory hotplug. Since some of these patches
are well reviewed and can be included before the core hotplug changes,
I am posting them as a separate series.

The last post of these patches were with the combined series version 3.

https://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg02910.html

This split-out series starts with v4 which has the following minor changes:

- Using maxram_size from MachineState instead of sPAPREnvironment. (2/8)
- Added a patch to walk CPUs list in reverse order. (3/8)
- Walking CPUs list in reverse order to ensure CPU device tree entries
  are filled up in proper order in device tree. (4/8)
- Use of MSR_EP define instead of using direct number. (5/8)
- Don't set irq for those CPUs that are already removed during XICS reset. (8/8)
- Removed David Gibson's SoB from some patches as there were some
  minor changes in them.

This series applies against spapr-next branch of David Gibson's tree.

Bharata B Rao (8):
  spapr: Consider max_cpus during xics initialization
  spapr: Support ibm,lrdr-capacity device tree property
  cpus: Add a macro to walk CPUs in reverse
  spapr: Reorganize CPU dt generation code
  spapr: Consolidate cpu init code into a routine
  ppc: Update cpu_model in MachineState
  xics_kvm: Don't enable KVM_CAP_IRQ_XICS if already enabled
  xics_kvm: Add cpu_destroy method to XICS

 docs/specs/ppc-spapr-hotplug.txt |  18 ++
 hw/intc/xics.c                   |  14 ++
 hw/intc/xics_kvm.c               |  25 ++-
 hw/ppc/mac_newworld.c            |  10 +-
 hw/ppc/mac_oldworld.c            |   7 +-
 hw/ppc/ppc440_bamboo.c           |   7 +-
 hw/ppc/prep.c                    |   7 +-
 hw/ppc/spapr.c                   | 352 +++++++++++++++++++++------------------
 hw/ppc/spapr_rtas.c              |  16 ++
 hw/ppc/virtex_ml507.c            |   7 +-
 include/hw/ppc/spapr.h           |   2 +
 include/hw/ppc/xics.h            |   3 +
 include/qom/cpu.h                |   2 +
 13 files changed, 289 insertions(+), 181 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v4 1/8] spapr: Consider max_cpus during xics initialization

[Bharata B Rao]

Use max_cpus instead of smp_cpus when intializating xics system. Also
report max_cpus in ibm,interrupt-server-ranges device tree property of
interrupt controller node.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index acc7233..9270234 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -308,7 +308,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
     GString *hypertas = g_string_sized_new(256);
     GString *qemu_hypertas = g_string_sized_new(256);
     uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
-    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(smp_cpus)};
+    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(max_cpus)};
     int smt = kvmppc_smt_threads();
     unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
     QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
@@ -1454,9 +1454,8 @@ static void ppc_spapr_init(MachineState *machine)
 
     /* Set up Interrupt Controller before we create the VCPUs */
     spapr->icp = xics_system_init(machine,
-                                  DIV_ROUND_UP(smp_cpus * kvmppc_smt_threads(),
-                                               smp_threads),
-                                  XICS_IRQS);
+                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
+                                               smp_threads), XICS_IRQS);
 
     /* init CPUs */
     if (cpu_model == NULL) {
-- 
2.1.0

[Qemu-devel] [PATCH v4 2/8] spapr: Support ibm, lrdr-capacity device tree property

[Bharata B Rao]

Add support for ibm,lrdr-capacity since this is needed by the guest
kernel to know about the possible hot-pluggable CPUs and Memory. With
this, pseries kernels will start reporting correct maxcpus in
/sys/devices/system/cpu/possible.

Also define the minimum hotpluggable memory size as 256MB.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 docs/specs/ppc-spapr-hotplug.txt | 18 ++++++++++++++++++
 hw/ppc/spapr_rtas.c              | 16 ++++++++++++++++
 include/hw/ppc/spapr.h           |  2 ++
 3 files changed, 36 insertions(+)

diff --git a/docs/specs/ppc-spapr-hotplug.txt b/docs/specs/ppc-spapr-hotplug.txt
index d35771c..46e0719 100644
--- a/docs/specs/ppc-spapr-hotplug.txt
+++ b/docs/specs/ppc-spapr-hotplug.txt
@@ -284,4 +284,22 @@ struct rtas_event_log_v6_hp {
     } drc;
 } QEMU_PACKED;
 
+== ibm,lrdr-capacity ==
+
+ibm,lrdr-capacity is a property in the /rtas device tree node that identifies
+the dynamic reconfiguration capabilities of the guest. It consists of a triple
+consisting of <phys>, <size> and <maxcpus>.
+
+  <phys>, encoded in BE format represents the maximum address in bytes and
+  hence the maximum memory that can be allocated to the guest.
+
+  <size>, encoded in BE format represents the size increments in which
+  memory can be hot-plugged to the guest.
+
+  <maxcpus>, a BE-encoded integer, represents the maximum number of
+  processors that the guest can have.
+
+pseries guests use this property to note the maximum allowed CPUs for the
+guest.
+
 [1] http://thread.gmane.org/gmane.linux.ports.ppc.embedded/75350/focus=106867
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 3b95dfc..592e504 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -29,6 +29,7 @@
 #include "sysemu/char.h"
 #include "hw/qdev.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/cpus.h"
 
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
@@ -651,6 +652,8 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
 {
     int ret;
     int i;
+    uint32_t lrdr_capacity[5];
+    MachineState *machine = MACHINE(qdev_get_machine());
 
     ret = fdt_add_mem_rsv(fdt, rtas_addr, rtas_size);
     if (ret < 0) {
@@ -699,6 +702,19 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
         }
 
     }
+
+    lrdr_capacity[0] = cpu_to_be32(machine->maxram_size >> 32);
+    lrdr_capacity[1] = cpu_to_be32(machine->maxram_size & 0xffffffff);
+    lrdr_capacity[2] = 0;
+    lrdr_capacity[3] = cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE);
+    lrdr_capacity[4] = cpu_to_be32(max_cpus/smp_threads);
+    ret = qemu_fdt_setprop(fdt, "/rtas", "ibm,lrdr-capacity", lrdr_capacity,
+                     sizeof(lrdr_capacity));
+    if (ret < 0) {
+        fprintf(stderr, "Couldn't add ibm,lrdr-capacity rtas property\n");
+        return ret;
+    }
+
     return 0;
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 0aeac50..91a61ab 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -607,4 +607,6 @@ void spapr_ccs_reset_hook(void *opaque);
 void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
 int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
 
+#define SPAPR_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
+
 #endif /* !defined (__HW_SPAPR_H__) */
-- 
2.1.0

[Qemu-devel] [PATCH v4 3/8] cpus: Add a macro to walk CPUs in reverse

[Bharata B Rao]

Add CPU_FOREACH_REVERSE that walks CPUs in reverse.

Needed for PowerPC CPU device tree reorganization.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Cc: Andreas Färber <afaerber@suse.de>
---
 include/qom/cpu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 39f0f19..42f42f5 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -323,6 +323,8 @@ extern struct CPUTailQ cpus;
 #define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
 #define CPU_FOREACH_SAFE(cpu, next_cpu) \
     QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
+#define CPU_FOREACH_REVERSE(cpu) \
+    QTAILQ_FOREACH_REVERSE(cpu, &cpus, CPUTailQ, node)
 #define first_cpu QTAILQ_FIRST(&cpus)
 
 DECLARE_TLS(CPUState *, current_cpu);
-- 
2.1.0

[Qemu-devel] [PATCH v4 4/8] spapr: Reorganize CPU dt generation code

[Bharata B Rao]

Reorganize CPU device tree generation code so that it be reused from
hotplug path. CPU dt entries are now generated from spapr_finalize_fdt()
instead of spapr_create_fdt_skel().

Note: This is how the split-up looks like now:

Boot path
---------
spapr_finalize_fdt
 spapr_populate_cpus_dt_node
  spapr_populate_cpu_dt
   spapr_fixup_cpu_numa_dt
   spapr_fixup_cpu_smt_dt

ibm,cas path
------------
spapr_h_cas_compose_response
 spapr_fixup_cpu_dt
  spapr_fixup_cpu_numa_dt
  spapr_fixup_cpu_smt_dt

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 284 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 159 insertions(+), 125 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9270234..65a86eb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -165,6 +165,27 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
     return ret;
 }
 
+static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, CPUState *cs)
+{
+    int ret = 0;
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    int index = ppc_get_vcpu_dt_id(cpu);
+    uint32_t associativity[] = {cpu_to_be32(0x5),
+                                cpu_to_be32(0x0),
+                                cpu_to_be32(0x0),
+                                cpu_to_be32(0x0),
+                                cpu_to_be32(cs->numa_node),
+                                cpu_to_be32(index)};
+
+    /* Advertise NUMA via ibm,associativity */
+    if (nb_numa_nodes > 1) {
+        ret = fdt_setprop(fdt, offset, "ibm,associativity", associativity,
+                          sizeof(associativity));
+    }
+
+    return ret;
+}
+
 static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
 {
     int ret = 0, offset, cpus_offset;
@@ -177,12 +198,6 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
         PowerPCCPU *cpu = POWERPC_CPU(cs);
         DeviceClass *dc = DEVICE_GET_CLASS(cs);
         int index = ppc_get_vcpu_dt_id(cpu);
-        uint32_t associativity[] = {cpu_to_be32(0x5),
-                                    cpu_to_be32(0x0),
-                                    cpu_to_be32(0x0),
-                                    cpu_to_be32(0x0),
-                                    cpu_to_be32(cs->numa_node),
-                                    cpu_to_be32(index)};
 
         if ((index % smt) != 0) {
             continue;
@@ -206,16 +221,13 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
             }
         }
 
-        if (nb_numa_nodes > 1) {
-            ret = fdt_setprop(fdt, offset, "ibm,associativity", associativity,
-                              sizeof(associativity));
-            if (ret < 0) {
-                return ret;
-            }
+        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
+                      pft_size_prop, sizeof(pft_size_prop));
+        if (ret < 0) {
+            return ret;
         }
 
-        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
-                          pft_size_prop, sizeof(pft_size_prop));
+        ret = spapr_fixup_cpu_numa_dt(fdt, offset, cs);
         if (ret < 0) {
             return ret;
         }
@@ -302,18 +314,13 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
                                    uint32_t epow_irq)
 {
     void *fdt;
-    CPUState *cs;
     uint32_t start_prop = cpu_to_be32(initrd_base);
     uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
     GString *hypertas = g_string_sized_new(256);
     GString *qemu_hypertas = g_string_sized_new(256);
     uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
     uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(max_cpus)};
-    int smt = kvmppc_smt_threads();
     unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
-    QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
-    unsigned sockets = opts ? qemu_opt_get_number(opts, "sockets", 0) : 0;
-    uint32_t cpus_per_socket = sockets ? (smp_cpus / sockets) : 1;
     char *buf;
 
     add_str(hypertas, "hcall-pft");
@@ -399,107 +406,6 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
 
     _FDT((fdt_end_node(fdt)));
 
-    /* cpus */
-    _FDT((fdt_begin_node(fdt, "cpus")));
-
-    _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
-    _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
-
-    CPU_FOREACH(cs) {
-        PowerPCCPU *cpu = POWERPC_CPU(cs);
-        CPUPPCState *env = &cpu->env;
-        DeviceClass *dc = DEVICE_GET_CLASS(cs);
-        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
-        int index = ppc_get_vcpu_dt_id(cpu);
-        char *nodename;
-        uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
-                           0xffffffff, 0xffffffff};
-        uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ;
-        uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
-        uint32_t page_sizes_prop[64];
-        size_t page_sizes_prop_size;
-
-        if ((index % smt) != 0) {
-            continue;
-        }
-
-        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
-
-        _FDT((fdt_begin_node(fdt, nodename)));
-
-        g_free(nodename);
-
-        _FDT((fdt_property_cell(fdt, "reg", index)));
-        _FDT((fdt_property_string(fdt, "device_type", "cpu")));
-
-        _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
-        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
-                                env->dcache_line_size)));
-        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
-                                env->dcache_line_size)));
-        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
-                                env->icache_line_size)));
-        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
-                                env->icache_line_size)));
-
-        if (pcc->l1_dcache_size) {
-            _FDT((fdt_property_cell(fdt, "d-cache-size", pcc->l1_dcache_size)));
-        } else {
-            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
-        }
-        if (pcc->l1_icache_size) {
-            _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_size)));
-        } else {
-            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
-        }
-
-        _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq)));
-        _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq)));
-        _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr)));
-        _FDT((fdt_property_string(fdt, "status", "okay")));
-        _FDT((fdt_property(fdt, "64-bit", NULL, 0)));
-
-        if (env->spr_cb[SPR_PURR].oea_read) {
-            _FDT((fdt_property(fdt, "ibm,purr", NULL, 0)));
-        }
-
-        if (env->mmu_model & POWERPC_MMU_1TSEG) {
-            _FDT((fdt_property(fdt, "ibm,processor-segment-sizes",
-                               segs, sizeof(segs))));
-        }
-
-        /* Advertise VMX/VSX (vector extensions) if available
-         *   0 / no property == no vector extensions
-         *   1               == VMX / Altivec available
-         *   2               == VSX available */
-        if (env->insns_flags & PPC_ALTIVEC) {
-            uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
-
-            _FDT((fdt_property_cell(fdt, "ibm,vmx", vmx)));
-        }
-
-        /* Advertise DFP (Decimal Floating Point) if available
-         *   0 / no property == no DFP
-         *   1               == DFP available */
-        if (env->insns_flags2 & PPC2_DFP) {
-            _FDT((fdt_property_cell(fdt, "ibm,dfp", 1)));
-        }
-
-        page_sizes_prop_size = create_page_sizes_prop(env, page_sizes_prop,
-                                                      sizeof(page_sizes_prop));
-        if (page_sizes_prop_size) {
-            _FDT((fdt_property(fdt, "ibm,segment-page-sizes",
-                               page_sizes_prop, page_sizes_prop_size)));
-        }
-
-        _FDT((fdt_property_cell(fdt, "ibm,chip-id",
-                                cs->cpu_index / cpus_per_socket)));
-
-        _FDT((fdt_end_node(fdt)));
-    }
-
-    _FDT((fdt_end_node(fdt)));
-
     /* RTAS */
     _FDT((fdt_begin_node(fdt, "rtas")));
 
@@ -700,6 +606,137 @@ static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
     return 0;
 }
 
+static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
+                                  sPAPRMachineState *spapr)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
+    int index = ppc_get_vcpu_dt_id(cpu);
+    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
+                       0xffffffff, 0xffffffff};
+    uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ;
+    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
+    uint32_t page_sizes_prop[64];
+    size_t page_sizes_prop_size;
+    QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
+    unsigned sockets = opts ? qemu_opt_get_number(opts, "sockets", 0) : 0;
+    uint32_t cpus_per_socket = sockets ? (smp_cpus / sockets) : 1;
+    uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
+
+    _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
+    _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
+
+    _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", env->spr[SPR_PVR])));
+    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
+                            env->dcache_line_size)));
+    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-line-size",
+                            env->dcache_line_size)));
+    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-block-size",
+                            env->icache_line_size)));
+    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-line-size",
+                            env->icache_line_size)));
+
+    if (pcc->l1_dcache_size) {
+        _FDT((fdt_setprop_cell(fdt, offset, "d-cache-size",
+                             pcc->l1_dcache_size)));
+    } else {
+        fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
+    }
+    if (pcc->l1_icache_size) {
+        _FDT((fdt_setprop_cell(fdt, offset, "i-cache-size",
+                             pcc->l1_icache_size)));
+    } else {
+        fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
+    }
+
+    _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
+    _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
+    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", env->slb_nr)));
+    _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
+    _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
+
+    if (env->spr_cb[SPR_PURR].oea_read) {
+        _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
+    }
+
+    if (env->mmu_model & POWERPC_MMU_1TSEG) {
+        _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
+                           segs, sizeof(segs))));
+    }
+
+    /* Advertise VMX/VSX (vector extensions) if available
+     *   0 / no property == no vector extensions
+     *   1               == VMX / Altivec available
+     *   2               == VSX available */
+    if (env->insns_flags & PPC_ALTIVEC) {
+        uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
+
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", vmx)));
+    }
+
+    /* Advertise DFP (Decimal Floating Point) if available
+     *   0 / no property == no DFP
+     *   1               == DFP available */
+    if (env->insns_flags2 & PPC2_DFP) {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
+    }
+
+    page_sizes_prop_size = create_page_sizes_prop(env, page_sizes_prop,
+                                                  sizeof(page_sizes_prop));
+    if (page_sizes_prop_size) {
+        _FDT((fdt_setprop(fdt, offset, "ibm,segment-page-sizes",
+                           page_sizes_prop, page_sizes_prop_size)));
+    }
+
+    _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
+                            cs->cpu_index / cpus_per_socket)));
+
+    _FDT((fdt_setprop(fdt, offset, "ibm,pft-size",
+                      pft_size_prop, sizeof(pft_size_prop))));
+
+    _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cs));
+
+    _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu,
+                                 ppc_get_compat_smt_threads(cpu)));
+}
+
+static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
+{
+    CPUState *cs;
+    int cpus_offset;
+    char *nodename;
+    int smt = kvmppc_smt_threads();
+
+    cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
+    _FDT(cpus_offset);
+    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
+    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
+
+    /*
+     * We walk the CPUs in reverse order to ensure that CPU DT nodes
+     * created by fdt_add_subnode() end up in the right order in FDT
+     * for the guest kernel the enumerate the CPUs correctly.
+     */
+    CPU_FOREACH_REVERSE(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        int index = ppc_get_vcpu_dt_id(cpu);
+        DeviceClass *dc = DEVICE_GET_CLASS(cs);
+        int offset;
+
+        if ((index % smt) != 0) {
+            continue;
+        }
+
+        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
+        offset = fdt_add_subnode(fdt, cpus_offset, nodename);
+        g_free(nodename);
+        _FDT(offset);
+        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
+    }
+
+}
+
 static void spapr_finalize_fdt(sPAPRMachineState *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
@@ -745,11 +782,8 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
         fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
     }
 
-    /* Advertise NUMA via ibm,associativity */
-    ret = spapr_fixup_cpu_dt(fdt, spapr);
-    if (ret < 0) {
-        fprintf(stderr, "Couldn't finalize CPU device tree properties\n");
-    }
+    /* cpus */
+    spapr_populate_cpus_dt_node(fdt, spapr);
 
     bootlist = get_boot_devices_list(&cb, true);
     if (cb && bootlist) {
-- 
2.1.0

[Qemu-devel] [PATCH v4 5/8] spapr: Consolidate cpu init code into a routine

[Bharata B Rao]

Factor out bits of sPAPR specific CPU initialization code into
a separate routine so that it can be called from CPU hotplug
path too.

While at this, use MSR_EP define instead of using 6 directly.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 65a86eb..a0ecfd1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1408,6 +1408,34 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
     machine->boot_order = g_strdup(boot_device);
 }
 
+static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
+{
+    CPUPPCState *env = &cpu->env;
+
+    /* Set time-base frequency to 512 MHz */
+    cpu_ppc_tb_init(env, TIMEBASE_FREQ);
+
+    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
+     * MSR[IP] should never be set.
+     */
+    env->msr_mask &= ~(1 << MSR_EP);
+
+    /* Tell KVM that we're in PAPR mode */
+    if (kvm_enabled()) {
+        kvmppc_set_papr(cpu);
+    }
+
+    if (cpu->max_compat) {
+        if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
+            exit(1);
+        }
+    }
+
+    xics_cpu_setup(spapr->icp, cpu);
+
+    qemu_register_reset(spapr_cpu_reset, cpu);
+}
+
 /* pSeries LPAR / sPAPR hardware init */
 static void ppc_spapr_init(MachineState *machine)
 {
@@ -1417,7 +1445,6 @@ static void ppc_spapr_init(MachineState *machine)
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
     PowerPCCPU *cpu;
-    CPUPPCState *env;
     PCIHostState *phb;
     int i;
     MemoryRegion *sysmem = get_system_memory();
@@ -1501,30 +1528,7 @@ static void ppc_spapr_init(MachineState *machine)
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
         }
-        env = &cpu->env;
-
-        /* Set time-base frequency to 512 MHz */
-        cpu_ppc_tb_init(env, TIMEBASE_FREQ);
-
-        /* PAPR always has exception vectors in RAM not ROM. To ensure this,
-         * MSR[IP] should never be set.
-         */
-        env->msr_mask &= ~(1 << 6);
-
-        /* Tell KVM that we're in PAPR mode */
-        if (kvm_enabled()) {
-            kvmppc_set_papr(cpu);
-        }
-
-        if (cpu->max_compat) {
-            if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
-                exit(1);
-            }
-        }
-
-        xics_cpu_setup(spapr->icp, cpu);
-
-        qemu_register_reset(spapr_cpu_reset, cpu);
+        spapr_cpu_init(spapr, cpu);
     }
 
     if (kvm_enabled()) {
-- 
2.1.0

[Qemu-devel] [PATCH v4 6/8] ppc: Update cpu_model in MachineState

[Bharata B Rao]

Keep cpu_model field in MachineState uptodate so that it can be used
from the CPU hotplug path.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/mac_newworld.c  | 10 +++++-----
 hw/ppc/mac_oldworld.c  |  7 +++----
 hw/ppc/ppc440_bamboo.c |  7 +++----
 hw/ppc/prep.c          |  7 +++----
 hw/ppc/spapr.c         |  7 +++----
 hw/ppc/virtex_ml507.c  |  7 +++----
 6 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index a365bf9..c35cb14 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -145,7 +145,6 @@ static void ppc_core99_reset(void *opaque)
 static void ppc_core99_init(MachineState *machine)
 {
     ram_addr_t ram_size = machine->ram_size;
-    const char *cpu_model = machine->cpu_model;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
@@ -182,14 +181,15 @@ static void ppc_core99_init(MachineState *machine)
     linux_boot = (kernel_filename != NULL);
 
     /* init CPUs */
-    if (cpu_model == NULL)
+    if (machine->cpu_model == NULL) {
 #ifdef TARGET_PPC64
-        cpu_model = "970fx";
+        machine->cpu_model = "970fx";
 #else
-        cpu_model = "G4";
+        machine->cpu_model = "G4";
 #endif
+    }
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(cpu_model);
+        cpu = cpu_ppc_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index f26133d..1f7841e 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -75,7 +75,6 @@ static void ppc_heathrow_reset(void *opaque)
 static void ppc_heathrow_init(MachineState *machine)
 {
     ram_addr_t ram_size = machine->ram_size;
-    const char *cpu_model = machine->cpu_model;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
@@ -107,10 +106,10 @@ static void ppc_heathrow_init(MachineState *machine)
     linux_boot = (kernel_filename != NULL);
 
     /* init CPUs */
-    if (cpu_model == NULL)
-        cpu_model = "G3";
+    if (machine->cpu_model == NULL)
+        machine->cpu_model = "G3";
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(cpu_model);
+        cpu = cpu_ppc_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 778970a..032fa80 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -159,7 +159,6 @@ static void main_cpu_reset(void *opaque)
 static void bamboo_init(MachineState *machine)
 {
     ram_addr_t ram_size = machine->ram_size;
-    const char *cpu_model = machine->cpu_model;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
@@ -184,10 +183,10 @@ static void bamboo_init(MachineState *machine)
     int i;
 
     /* Setup CPU. */
-    if (cpu_model == NULL) {
-        cpu_model = "440EP";
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = "440EP";
     }
-    cpu = cpu_ppc_init(cpu_model);
+    cpu = cpu_ppc_init(machine->cpu_model);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
         exit(1);
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 7f52662..ea3c67d 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -506,7 +506,6 @@ static int PPC_NVRAM_set_params (Nvram *nvram, uint16_t NVRAM_size,
 static void ppc_prep_init(MachineState *machine)
 {
     ram_addr_t ram_size = machine->ram_size;
-    const char *cpu_model = machine->cpu_model;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
@@ -537,10 +536,10 @@ static void ppc_prep_init(MachineState *machine)
     linux_boot = (kernel_filename != NULL);
 
     /* init CPUs */
-    if (cpu_model == NULL)
-        cpu_model = "602";
+    if (machine->cpu_model == NULL)
+        machine->cpu_model = "602";
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(cpu_model);
+        cpu = cpu_ppc_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a0ecfd1..231e616 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1440,7 +1440,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
 static void ppc_spapr_init(MachineState *machine)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
-    const char *cpu_model = machine->cpu_model;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
@@ -1519,11 +1518,11 @@ static void ppc_spapr_init(MachineState *machine)
                                                smp_threads), XICS_IRQS);
 
     /* init CPUs */
-    if (cpu_model == NULL) {
-        cpu_model = kvm_enabled() ? "host" : "POWER7";
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
     }
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(cpu_model);
+        cpu = cpu_ppc_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index 6ebd5be..f33d398 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -197,7 +197,6 @@ static int xilinx_load_device_tree(hwaddr addr,
 static void virtex_init(MachineState *machine)
 {
     ram_addr_t ram_size = machine->ram_size;
-    const char *cpu_model = machine->cpu_model;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     hwaddr initrd_base = 0;
@@ -214,11 +213,11 @@ static void virtex_init(MachineState *machine)
     int i;
 
     /* init CPUs */
-    if (cpu_model == NULL) {
-        cpu_model = "440-Xilinx";
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = "440-Xilinx";
     }
 
-    cpu = ppc440_init_xilinx(&ram_size, 1, cpu_model, 400000000);
+    cpu = ppc440_init_xilinx(&ram_size, 1, machine->cpu_model, 400000000);
     env = &cpu->env;
     qemu_register_reset(main_cpu_reset, cpu);
 
-- 
2.1.0

[Qemu-devel] [PATCH v4 7/8] xics_kvm: Don't enable KVM_CAP_IRQ_XICS if already enabled

[Bharata B Rao]

When supporting CPU hot removal by parking the vCPU fd and reusing
it during hotplug again, there can be cases where we try to reenable
KVM_CAP_IRQ_XICS CAP for the vCPU for which it was already enabled.
Introduce a boolean member in ICPState to track this and don't
reenable the CAP if it was already enabled earlier.

Re-enabling this CAP should ideally work, but currently it results in
kernel trying to create and associate ICP with this vCPU and that
fails since there is already an ICP associated with it. Hence this
patch is needed to work around this problem in the kernel.

This change allows CPU hot removal to work for sPAPR.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/intc/xics_kvm.c    | 10 ++++++++++
 include/hw/ppc/xics.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index ea886da..d58729c 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -331,6 +331,15 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
         abort();
     }
 
+    /*
+     * If we are reusing a parked vCPU fd corresponding to the CPU
+     * which was hot-removed earlier we don't have to renable
+     * KVM_CAP_IRQ_XICS capability again.
+     */
+    if (ss->cap_irq_xics_enabled) {
+        return;
+    }
+
     if (icpkvm->kernel_xics_fd != -1) {
         int ret;
 
@@ -343,6 +352,7 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
                     kvm_arch_vcpu_id(cs), strerror(errno));
             exit(1);
         }
+        ss->cap_irq_xics_enabled = true;
     }
 }
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index a214dd7..355a966 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -109,6 +109,7 @@ struct ICPState {
     uint8_t pending_priority;
     uint8_t mfrr;
     qemu_irq output;
+    bool cap_irq_xics_enabled;
 };
 
 #define TYPE_ICS "ics"
-- 
2.1.0

[Qemu-devel] [PATCH v4 8/8] xics_kvm: Add cpu_destroy method to XICS

[Bharata B Rao]

XICS is setup for each CPU during initialization. Provide a routine
to undo the same when CPU is unplugged. Also ensure xics reset doesn't set
irq for CPUs that are already unplugged.

This allows reboot of a VM that has undergone CPU hotplug and unplug
to work correctly.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/intc/xics.c        | 14 ++++++++++++++
 hw/intc/xics_kvm.c    | 15 +++++++++++++--
 include/hw/ppc/xics.h |  2 ++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 924b1ae..3f87f82 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -44,6 +44,20 @@ static int get_cpu_index_by_dt_id(int cpu_dt_id)
     return -1;
 }
 
+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
+    ICPState *ss = &icp->ss[cs->cpu_index];
+
+    assert(cs->cpu_index < icp->nr_servers);
+
+    ss->output = NULL;
+    if (info->cpu_destroy) {
+        info->cpu_destroy(icp, cpu);
+    }
+}
+
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index d58729c..e35f319 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -109,8 +109,10 @@ static void icp_kvm_reset(DeviceState *dev)
     icp->pending_priority = 0xff;
     icp->mfrr = 0xff;
 
-    /* Make all outputs are deasserted */
-    qemu_set_irq(icp->output, 0);
+    /* Make all outputs are deasserted only if the CPU thread is in use */
+    if (icp->output) {
+        qemu_set_irq(icp->output, 0);
+    }
 
     icp_set_kvm_state(icp, 1);
 }
@@ -356,6 +358,14 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
     }
 }
 
+static void xics_kvm_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+    ICPState *ss = &icp->ss[cs->cpu_index];
+
+    ss->cs = NULL;
+}
+
 static void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs, Error **errp)
 {
     icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
@@ -486,6 +496,7 @@ static void xics_kvm_class_init(ObjectClass *oc, void *data)
 
     dc->realize = xics_kvm_realize;
     xsc->cpu_setup = xics_kvm_cpu_setup;
+    xsc->cpu_destroy = xics_kvm_cpu_destroy;
     xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
     xsc->set_nr_servers = xics_kvm_set_nr_servers;
 }
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 355a966..2faad48 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -68,6 +68,7 @@ struct XICSStateClass {
     DeviceClass parent_class;
 
     void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
+    void (*cpu_destroy)(XICSState *icp, PowerPCCPU *cpu);
     void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
     void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers, Error **errp);
 };
@@ -166,5 +167,6 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align);
 void xics_free(XICSState *icp, int irq, int num);
 
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu);
 
 #endif /* __XICS_H__ */
-- 
2.1.0

Re: [Qemu-devel] [PATCH v4 1/8] spapr: Consider max_cpus during xics initialization

[Alexey Kardashevskiy]

On 06/05/2015 02:25 PM, Bharata B Rao wrote:
> Use max_cpus instead of smp_cpus when intializating xics system. Also
> report max_cpus in ibm,interrupt-server-ranges device tree property of
> interrupt controller node.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>   hw/ppc/spapr.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index acc7233..9270234 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -308,7 +308,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>       GString *hypertas = g_string_sized_new(256);
>       GString *qemu_hypertas = g_string_sized_new(256);
>       uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
> -    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(smp_cpus)};
> +    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(max_cpus)};
>       int smt = kvmppc_smt_threads();
>       unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
>       QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
> @@ -1454,9 +1454,8 @@ static void ppc_spapr_init(MachineState *machine)
>
>       /* Set up Interrupt Controller before we create the VCPUs */
>       spapr->icp = xics_system_init(machine,
> -                                  DIV_ROUND_UP(smp_cpus * kvmppc_smt_threads(),
> -                                               smp_threads),
> -                                  XICS_IRQS);
> +                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> +                                               smp_threads), XICS_IRQS);


Please do not change the formatting of "XICS_IRQS);".


>
>       /* init CPUs */
>       if (cpu_model == NULL) {
>


-- 
Alexey

Re: [Qemu-devel] [PATCH v4 4/8] spapr: Reorganize CPU dt generation code

[Alexey Kardashevskiy]

On 06/05/2015 02:25 PM, Bharata B Rao wrote:
> Reorganize CPU device tree generation code so that it be reused from
> hotplug path. CPU dt entries are now generated from spapr_finalize_fdt()
> instead of spapr_create_fdt_skel().
>
> Note: This is how the split-up looks like now:
>
> Boot path
> ---------
> spapr_finalize_fdt
>   spapr_populate_cpus_dt_node
>    spapr_populate_cpu_dt
>     spapr_fixup_cpu_numa_dt
>     spapr_fixup_cpu_smt_dt
>
> ibm,cas path
> ------------
> spapr_h_cas_compose_response
>   spapr_fixup_cpu_dt
>    spapr_fixup_cpu_numa_dt
>    spapr_fixup_cpu_smt_dt
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>   hw/ppc/spapr.c | 284 ++++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 159 insertions(+), 125 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9270234..65a86eb 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -165,6 +165,27 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
>       return ret;
>   }
>
> +static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, CPUState *cs)
> +{
> +    int ret = 0;
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    int index = ppc_get_vcpu_dt_id(cpu);
> +    uint32_t associativity[] = {cpu_to_be32(0x5),
> +                                cpu_to_be32(0x0),
> +                                cpu_to_be32(0x0),
> +                                cpu_to_be32(0x0),
> +                                cpu_to_be32(cs->numa_node),
> +                                cpu_to_be32(index)};
> +
> +    /* Advertise NUMA via ibm,associativity */
> +    if (nb_numa_nodes > 1) {
> +        ret = fdt_setprop(fdt, offset, "ibm,associativity", associativity,
> +                          sizeof(associativity));
> +    }
> +
> +    return ret;
> +}
> +
>   static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
>   {
>       int ret = 0, offset, cpus_offset;
> @@ -177,12 +198,6 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
>           PowerPCCPU *cpu = POWERPC_CPU(cs);
>           DeviceClass *dc = DEVICE_GET_CLASS(cs);
>           int index = ppc_get_vcpu_dt_id(cpu);
> -        uint32_t associativity[] = {cpu_to_be32(0x5),
> -                                    cpu_to_be32(0x0),
> -                                    cpu_to_be32(0x0),
> -                                    cpu_to_be32(0x0),
> -                                    cpu_to_be32(cs->numa_node),
> -                                    cpu_to_be32(index)};
>
>           if ((index % smt) != 0) {
>               continue;
> @@ -206,16 +221,13 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
>               }
>           }
>
> -        if (nb_numa_nodes > 1) {
> -            ret = fdt_setprop(fdt, offset, "ibm,associativity", associativity,
> -                              sizeof(associativity));
> -            if (ret < 0) {
> -                return ret;
> -            }
> +        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
> +                      pft_size_prop, sizeof(pft_size_prop));
> +        if (ret < 0) {
> +            return ret;
>           }
>
> -        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
> -                          pft_size_prop, sizeof(pft_size_prop));

You broke formatting here, above 8 lines should not be in the patch.


> +        ret = spapr_fixup_cpu_numa_dt(fdt, offset, cs);
>           if (ret < 0) {
>               return ret;
>           }
> @@ -302,18 +314,13 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>                                      uint32_t epow_irq)
>   {
>       void *fdt;
> -    CPUState *cs;
>       uint32_t start_prop = cpu_to_be32(initrd_base);
>       uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
>       GString *hypertas = g_string_sized_new(256);
>       GString *qemu_hypertas = g_string_sized_new(256);
>       uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
>       uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(max_cpus)};
> -    int smt = kvmppc_smt_threads();
>       unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
> -    QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
> -    unsigned sockets = opts ? qemu_opt_get_number(opts, "sockets", 0) : 0;
> -    uint32_t cpus_per_socket = sockets ? (smp_cpus / sockets) : 1;
>       char *buf;
>
>       add_str(hypertas, "hcall-pft");
> @@ -399,107 +406,6 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>
>       _FDT((fdt_end_node(fdt)));
>
> -    /* cpus */
> -    _FDT((fdt_begin_node(fdt, "cpus")));
> -
> -    _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
> -    _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
> -
> -    CPU_FOREACH(cs) {
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        CPUPPCState *env = &cpu->env;
> -        DeviceClass *dc = DEVICE_GET_CLASS(cs);
> -        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> -        int index = ppc_get_vcpu_dt_id(cpu);
> -        char *nodename;
> -        uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
> -                           0xffffffff, 0xffffffff};
> -        uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ;
> -        uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
> -        uint32_t page_sizes_prop[64];
> -        size_t page_sizes_prop_size;
> -
> -        if ((index % smt) != 0) {
> -            continue;
> -        }
> -
> -        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
> -
> -        _FDT((fdt_begin_node(fdt, nodename)));
> -
> -        g_free(nodename);
> -
> -        _FDT((fdt_property_cell(fdt, "reg", index)));
> -        _FDT((fdt_property_string(fdt, "device_type", "cpu")));
> -
> -        _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
> -        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
> -                                env->dcache_line_size)));
> -        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
> -                                env->dcache_line_size)));
> -        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
> -                                env->icache_line_size)));
> -        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
> -                                env->icache_line_size)));
> -
> -        if (pcc->l1_dcache_size) {
> -            _FDT((fdt_property_cell(fdt, "d-cache-size", pcc->l1_dcache_size)));
> -        } else {
> -            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
> -        }
> -        if (pcc->l1_icache_size) {
> -            _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_size)));
> -        } else {
> -            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
> -        }
> -
> -        _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq)));
> -        _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq)));
> -        _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr)));
> -        _FDT((fdt_property_string(fdt, "status", "okay")));
> -        _FDT((fdt_property(fdt, "64-bit", NULL, 0)));
> -
> -        if (env->spr_cb[SPR_PURR].oea_read) {
> -            _FDT((fdt_property(fdt, "ibm,purr", NULL, 0)));
> -        }
> -
> -        if (env->mmu_model & POWERPC_MMU_1TSEG) {
> -            _FDT((fdt_property(fdt, "ibm,processor-segment-sizes",
> -                               segs, sizeof(segs))));
> -        }
> -
> -        /* Advertise VMX/VSX (vector extensions) if available
> -         *   0 / no property == no vector extensions
> -         *   1               == VMX / Altivec available
> -         *   2               == VSX available */
> -        if (env->insns_flags & PPC_ALTIVEC) {
> -            uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
> -
> -            _FDT((fdt_property_cell(fdt, "ibm,vmx", vmx)));
> -        }
> -
> -        /* Advertise DFP (Decimal Floating Point) if available
> -         *   0 / no property == no DFP
> -         *   1               == DFP available */
> -        if (env->insns_flags2 & PPC2_DFP) {
> -            _FDT((fdt_property_cell(fdt, "ibm,dfp", 1)));
> -        }
> -
> -        page_sizes_prop_size = create_page_sizes_prop(env, page_sizes_prop,
> -                                                      sizeof(page_sizes_prop));
> -        if (page_sizes_prop_size) {
> -            _FDT((fdt_property(fdt, "ibm,segment-page-sizes",
> -                               page_sizes_prop, page_sizes_prop_size)));
> -        }
> -
> -        _FDT((fdt_property_cell(fdt, "ibm,chip-id",
> -                                cs->cpu_index / cpus_per_socket)));
> -
> -        _FDT((fdt_end_node(fdt)));
> -    }
> -
> -    _FDT((fdt_end_node(fdt)));
> -
>       /* RTAS */
>       _FDT((fdt_begin_node(fdt, "rtas")));
>
> @@ -700,6 +606,137 @@ static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
>       return 0;
>   }
>
> +static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> +                                  sPAPRMachineState *spapr)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> +    int index = ppc_get_vcpu_dt_id(cpu);
> +    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
> +                       0xffffffff, 0xffffffff};
> +    uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ;
> +    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
> +    uint32_t page_sizes_prop[64];
> +    size_t page_sizes_prop_size;
> +    QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
> +    unsigned sockets = opts ? qemu_opt_get_number(opts, "sockets", 0) : 0;
> +    uint32_t cpus_per_socket = sockets ? (smp_cpus / sockets) : 1;
> +    uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> +
> +    _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
> +    _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
> +
> +    _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", env->spr[SPR_PVR])));
> +    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
> +                            env->dcache_line_size)));

Wrong indent.


> +    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-line-size",
> +                            env->dcache_line_size)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-block-size",
> +                            env->icache_line_size)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-line-size",
> +                            env->icache_line_size)));
> +
> +    if (pcc->l1_dcache_size) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "d-cache-size",
> +                             pcc->l1_dcache_size)));
> +    } else {
> +        fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
> +    }
> +    if (pcc->l1_icache_size) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "i-cache-size",
> +                             pcc->l1_icache_size)));
> +    } else {
> +        fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
> +    }
> +
> +    _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", env->slb_nr)));
> +    _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
> +    _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
> +
> +    if (env->spr_cb[SPR_PURR].oea_read) {
> +        _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
> +    }
> +
> +    if (env->mmu_model & POWERPC_MMU_1TSEG) {
> +        _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
> +                           segs, sizeof(segs))));
> +    }
> +
> +    /* Advertise VMX/VSX (vector extensions) if available
> +     *   0 / no property == no vector extensions
> +     *   1               == VMX / Altivec available
> +     *   2               == VSX available */
> +    if (env->insns_flags & PPC_ALTIVEC) {
> +        uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
> +
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", vmx)));
> +    }
> +
> +    /* Advertise DFP (Decimal Floating Point) if available
> +     *   0 / no property == no DFP
> +     *   1               == DFP available */
> +    if (env->insns_flags2 & PPC2_DFP) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
> +    }
> +
> +    page_sizes_prop_size = create_page_sizes_prop(env, page_sizes_prop,
> +                                                  sizeof(page_sizes_prop));
> +    if (page_sizes_prop_size) {
> +        _FDT((fdt_setprop(fdt, offset, "ibm,segment-page-sizes",
> +                           page_sizes_prop, page_sizes_prop_size)));
> +    }
> +
> +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
> +                            cs->cpu_index / cpus_per_socket)));
> +
> +    _FDT((fdt_setprop(fdt, offset, "ibm,pft-size",
> +                      pft_size_prop, sizeof(pft_size_prop))));
> +
> +    _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cs));
> +
> +    _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu,
> +                                 ppc_get_compat_smt_threads(cpu)));
> +}
> +
> +static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
> +{
> +    CPUState *cs;
> +    int cpus_offset;
> +    char *nodename;
> +    int smt = kvmppc_smt_threads();
> +
> +    cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> +    _FDT(cpus_offset);
> +    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
> +    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
> +
> +    /*
> +     * We walk the CPUs in reverse order to ensure that CPU DT nodes
> +     * created by fdt_add_subnode() end up in the right order in FDT
> +     * for the guest kernel the enumerate the CPUs correctly.
> +     */
> +    CPU_FOREACH_REVERSE(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +        int index = ppc_get_vcpu_dt_id(cpu);
> +        DeviceClass *dc = DEVICE_GET_CLASS(cs);
> +        int offset;
> +
> +        if ((index % smt) != 0) {
> +            continue;
> +        }
> +
> +        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
> +        offset = fdt_add_subnode(fdt, cpus_offset, nodename);
> +        g_free(nodename);
> +        _FDT(offset);
> +        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> +    }
> +
> +}
> +
>   static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>                                  hwaddr fdt_addr,
>                                  hwaddr rtas_addr,
> @@ -745,11 +782,8 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>           fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
>       }
>
> -    /* Advertise NUMA via ibm,associativity */
> -    ret = spapr_fixup_cpu_dt(fdt, spapr);
> -    if (ret < 0) {
> -        fprintf(stderr, "Couldn't finalize CPU device tree properties\n");
> -    }
> +    /* cpus */
> +    spapr_populate_cpus_dt_node(fdt, spapr);
>
>       bootlist = get_boot_devices_list(&cb, true);
>       if (cb && bootlist) {
>


-- 
Alexey

Re: [Qemu-devel] [PATCH v4 4/8] spapr: Reorganize CPU dt generation code

[Bharata B Rao]

On Fri, Jun 05, 2015 at 04:09:48PM +1000, Alexey Kardashevskiy wrote:
> >
> >-        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
> >-                          pft_size_prop, sizeof(pft_size_prop));
> 
> You broke formatting here, above 8 lines should not be in the patch.

You mean above 80 chars ? It is not above 80 chars.

> >+    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
> >+                            env->dcache_line_size)));
> 
> Wrong indent.

checkpatch.pl doesn't complain, but should we start the second line
below ( of the first line ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v4 1/8] spapr: Consider max_cpus during xics initialization

[Bharata B Rao]

On Fri, Jun 05, 2015 at 03:30:24PM +1000, Alexey Kardashevskiy wrote:
> On 06/05/2015 02:25 PM, Bharata B Rao wrote:
> >Use max_cpus instead of smp_cpus when intializating xics system. Also
> >report max_cpus in ibm,interrupt-server-ranges device tree property of
> >interrupt controller node.
> >
> >Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >---
> >  hw/ppc/spapr.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> >diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >index acc7233..9270234 100644
> >--- a/hw/ppc/spapr.c
> >+++ b/hw/ppc/spapr.c
> >@@ -308,7 +308,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
> >      GString *hypertas = g_string_sized_new(256);
> >      GString *qemu_hypertas = g_string_sized_new(256);
> >      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
> >-    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(smp_cpus)};
> >+    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(max_cpus)};
> >      int smt = kvmppc_smt_threads();
> >      unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
> >      QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
> >@@ -1454,9 +1454,8 @@ static void ppc_spapr_init(MachineState *machine)
> >
> >      /* Set up Interrupt Controller before we create the VCPUs */
> >      spapr->icp = xics_system_init(machine,
> >-                                  DIV_ROUND_UP(smp_cpus * kvmppc_smt_threads(),
> >-                                               smp_threads),
> >-                                  XICS_IRQS);
> >+                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> >+                                               smp_threads), XICS_IRQS);
> 
> 
> Please do not change the formatting of "XICS_IRQS);".

Hmmm why ? I thought I saved a line!

Again checkpatch.pl doesn't complain.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v4 4/8] spapr: Reorganize CPU dt generation code

[Alexey Kardashevskiy]

On 06/05/2015 05:06 PM, Bharata B Rao wrote:
> On Fri, Jun 05, 2015 at 04:09:48PM +1000, Alexey Kardashevskiy wrote:
>>>
>>> -        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
>>> -                          pft_size_prop, sizeof(pft_size_prop));
>>
>> You broke formatting here, above 8 lines should not be in the patch.
>
> You mean above 80 chars ? It is not above 80 chars.

The chunk is:

+        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
+                      pft_size_prop, sizeof(pft_size_prop));
+        if (ret < 0) {
+            return ret;
          }

-        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
-                          pft_size_prop, sizeof(pft_size_prop));



pft_size_prop was under "fdt", now it is not, and this is the only change.



>
>>> +    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
>>> +                            env->dcache_line_size)));
>>
>> Wrong indent.
>
> checkpatch.pl doesn't complain, but should we start the second line
> below ( of the first line ?

Below "(" plus 1 char right.

set expandtab, set tabstop=4, set shiftwidth=4, set cino=:0,(0  - this is 
what I use for QEMU.


-- 
Alexey

Re: [Qemu-devel] [PATCH v4 1/8] spapr: Consider max_cpus during xics initialization

[Alexey Kardashevskiy]

On 06/05/2015 05:07 PM, Bharata B Rao wrote:
> On Fri, Jun 05, 2015 at 03:30:24PM +1000, Alexey Kardashevskiy wrote:
>> On 06/05/2015 02:25 PM, Bharata B Rao wrote:
>>> Use max_cpus instead of smp_cpus when intializating xics system. Also
>>> report max_cpus in ibm,interrupt-server-ranges device tree property of
>>> interrupt controller node.
>>>
>>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>> ---
>>>   hw/ppc/spapr.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index acc7233..9270234 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -308,7 +308,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>       GString *hypertas = g_string_sized_new(256);
>>>       GString *qemu_hypertas = g_string_sized_new(256);
>>>       uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
>>> -    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(smp_cpus)};
>>> +    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(max_cpus)};
>>>       int smt = kvmppc_smt_threads();
>>>       unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
>>>       QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
>>> @@ -1454,9 +1454,8 @@ static void ppc_spapr_init(MachineState *machine)
>>>
>>>       /* Set up Interrupt Controller before we create the VCPUs */
>>>       spapr->icp = xics_system_init(machine,
>>> -                                  DIV_ROUND_UP(smp_cpus * kvmppc_smt_threads(),
>>> -                                               smp_threads),
>>> -                                  XICS_IRQS);
>>> +                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
>>> +                                               smp_threads), XICS_IRQS);
>>
>>
>> Please do not change the formatting of "XICS_IRQS);".
>
> Hmmm why ? I thought I saved a line!


Looks weird. There were 3 parameters, aligned. Now there are two and third 
one hides behind DIV_ROUND_UP. And we can afford an extra line ;)

And this change is not related to what the patch does, the patch does
s/smp_cpus/max_cpus/ and when I see another unrelated change - this 
confuses me.


> Again checkpatch.pl doesn't complain.

Well, you can ignore me - after all I am not the one to takes these patches 
further :)



-- 
Alexey

Re: [Qemu-devel] [PATCH v4 8/8] xics_kvm: Add cpu_destroy method to XICS

[Alexey Kardashevskiy]

On 06/05/2015 02:25 PM, Bharata B Rao wrote:
> XICS is setup for each CPU during initialization. Provide a routine
> to undo the same when CPU is unplugged. Also ensure xics reset doesn't set
> irq for CPUs that are already unplugged.
>
> This allows reboot of a VM that has undergone CPU hotplug and unplug
> to work correctly.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>   hw/intc/xics.c        | 14 ++++++++++++++
>   hw/intc/xics_kvm.c    | 15 +++++++++++++--
>   include/hw/ppc/xics.h |  2 ++
>   3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 924b1ae..3f87f82 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -44,6 +44,20 @@ static int get_cpu_index_by_dt_id(int cpu_dt_id)
>       return -1;
>   }
>
> +void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)


xics_cpu_destroy() is not used by anything, may be push it later with the 
stuff which needs it?


> +{
> +    CPUState *cs = CPU(cpu);
> +    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
> +    ICPState *ss = &icp->ss[cs->cpu_index];
> +
> +    assert(cs->cpu_index < icp->nr_servers);
> +
> +    ss->output = NULL;
> +    if (info->cpu_destroy) {
> +        info->cpu_destroy(icp, cpu);
> +    }
> +}
> +
>   void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>   {
>       CPUState *cs = CPU(cpu);
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index d58729c..e35f319 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -109,8 +109,10 @@ static void icp_kvm_reset(DeviceState *dev)
>       icp->pending_priority = 0xff;
>       icp->mfrr = 0xff;
>
> -    /* Make all outputs are deasserted */
> -    qemu_set_irq(icp->output, 0);
> +    /* Make all outputs are deasserted only if the CPU thread is in use */
> +    if (icp->output) {
> +        qemu_set_irq(icp->output, 0);
> +    }


This feels unrelated to what the patch claims that it does. Or 
xics_cpu_destroy() somehow indirectly calls icp_kvm_reset()?


>
>       icp_set_kvm_state(icp, 1);
>   }
> @@ -356,6 +358,14 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>       }
>   }
>
> +static void xics_kvm_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
> +{
> +    CPUState *cs = CPU(cpu);
> +    ICPState *ss = &icp->ss[cs->cpu_index];
> +
> +    ss->cs = NULL;
> +}
> +
>   static void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs, Error **errp)
>   {
>       icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
> @@ -486,6 +496,7 @@ static void xics_kvm_class_init(ObjectClass *oc, void *data)
>
>       dc->realize = xics_kvm_realize;
>       xsc->cpu_setup = xics_kvm_cpu_setup;
> +    xsc->cpu_destroy = xics_kvm_cpu_destroy;
>       xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
>       xsc->set_nr_servers = xics_kvm_set_nr_servers;
>   }
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 355a966..2faad48 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -68,6 +68,7 @@ struct XICSStateClass {
>       DeviceClass parent_class;
>
>       void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
> +    void (*cpu_destroy)(XICSState *icp, PowerPCCPU *cpu);
>       void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
>       void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers, Error **errp);
>   };
> @@ -166,5 +167,6 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align);
>   void xics_free(XICSState *icp, int irq, int num);
>
>   void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
> +void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu);
>
>   #endif /* __XICS_H__ */
>


-- 
Alexey

Re: [Qemu-devel] [PATCH v4 8/8] xics_kvm: Add cpu_destroy method to XICS

[Bharata B Rao]

On Fri, Jun 05, 2015 at 06:09:38PM +1000, Alexey Kardashevskiy wrote:
> On 06/05/2015 02:25 PM, Bharata B Rao wrote:
> >XICS is setup for each CPU during initialization. Provide a routine
> >to undo the same when CPU is unplugged. Also ensure xics reset doesn't set
> >irq for CPUs that are already unplugged.
> >
> >This allows reboot of a VM that has undergone CPU hotplug and unplug
> >to work correctly.
> >
> >Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >---
> >  hw/intc/xics.c        | 14 ++++++++++++++
> >  hw/intc/xics_kvm.c    | 15 +++++++++++++--
> >  include/hw/ppc/xics.h |  2 ++
> >  3 files changed, 29 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >index 924b1ae..3f87f82 100644
> >--- a/hw/intc/xics.c
> >+++ b/hw/intc/xics.c
> >@@ -44,6 +44,20 @@ static int get_cpu_index_by_dt_id(int cpu_dt_id)
> >      return -1;
> >  }
> >
> >+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
> 
> 
> xics_cpu_destroy() is not used by anything, may be push it later with the
> stuff which needs it?

Yeah it is not used in this patchset, will leave to David/agraf
to see if they want this dropped from this series.

> 
> 
> >+{
> >+    CPUState *cs = CPU(cpu);
> >+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
> >+    ICPState *ss = &icp->ss[cs->cpu_index];
> >+
> >+    assert(cs->cpu_index < icp->nr_servers);
> >+
> >+    ss->output = NULL;
> >+    if (info->cpu_destroy) {
> >+        info->cpu_destroy(icp, cpu);
> >+    }
> >+}
> >+
> >  void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> >  {
> >      CPUState *cs = CPU(cpu);
> >diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> >index d58729c..e35f319 100644
> >--- a/hw/intc/xics_kvm.c
> >+++ b/hw/intc/xics_kvm.c
> >@@ -109,8 +109,10 @@ static void icp_kvm_reset(DeviceState *dev)
> >      icp->pending_priority = 0xff;
> >      icp->mfrr = 0xff;
> >
> >-    /* Make all outputs are deasserted */
> >-    qemu_set_irq(icp->output, 0);
> >+    /* Make all outputs are deasserted only if the CPU thread is in use */
> >+    if (icp->output) {
> >+        qemu_set_irq(icp->output, 0);
> >+    }
> 
> 
> This feels unrelated to what the patch claims that it does. Or
> xics_cpu_destroy() somehow indirectly calls icp_kvm_reset()?

When a VM is rebooted, device_reset of icp device will result in
icp_kvm_reset() which will do qemu_set_irq(). This shouldn't happen for
a CPU that is already removed. So above check ensures that we
don't try to desssert irq for a removed CPU.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v4 3/8] cpus: Add a macro to walk CPUs in reverse

[Andreas Färber]

Am 05.06.2015 um 13:25 schrieb Bharata B Rao:
> Add CPU_FOREACH_REVERSE that walks CPUs in reverse.
> 
> Needed for PowerPC CPU device tree reorganization.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Cc: Andreas Färber <afaerber@suse.de>
> ---
>  include/qom/cpu.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 39f0f19..42f42f5 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -323,6 +323,8 @@ extern struct CPUTailQ cpus;
>  #define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
>  #define CPU_FOREACH_SAFE(cpu, next_cpu) \
>      QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
> +#define CPU_FOREACH_REVERSE(cpu) \
> +    QTAILQ_FOREACH_REVERSE(cpu, &cpus, CPUTailQ, node)
>  #define first_cpu QTAILQ_FIRST(&cpus)
>  
>  DECLARE_TLS(CPUState *, current_cpu);

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v4 3/8] cpus: Add a macro to walk CPUs in reverse

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]

On Fri, Jun 05, 2015 at 11:39:18PM +0900, Andreas Färber wrote:
> Am 05.06.2015 um 13:25 schrieb Bharata B Rao:
> > Add CPU_FOREACH_REVERSE that walks CPUs in reverse.
> > 
> > Needed for PowerPC CPU device tree reorganization.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Cc: Andreas Färber <afaerber@suse.de>
> > ---
> >  include/qom/cpu.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 39f0f19..42f42f5 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -323,6 +323,8 @@ extern struct CPUTailQ cpus;
> >  #define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
> >  #define CPU_FOREACH_SAFE(cpu, next_cpu) \
> >      QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
> > +#define CPU_FOREACH_REVERSE(cpu) \
> > +    QTAILQ_FOREACH_REVERSE(cpu, &cpus, CPUTailQ, node)
> >  #define first_cpu QTAILQ_FIRST(&cpus)
> >  
> >  DECLARE_TLS(CPUState *, current_cpu);
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>

I'm hoping to merge the parts of this series that affect the spapr
code (most of it) shortly.  Should this patch go through another tree
though?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 1/8] spapr: Consider max_cpus during xics initialization

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]

On Fri, Jun 05, 2015 at 06:01:53PM +1000, Alexey Kardashevskiy wrote:
> On 06/05/2015 05:07 PM, Bharata B Rao wrote:
> >On Fri, Jun 05, 2015 at 03:30:24PM +1000, Alexey Kardashevskiy wrote:
> >>On 06/05/2015 02:25 PM, Bharata B Rao wrote:
> >>>Use max_cpus instead of smp_cpus when intializating xics system. Also
> >>>report max_cpus in ibm,interrupt-server-ranges device tree property of
> >>>interrupt controller node.
> >>>
> >>>Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >>>---
> >>>  hw/ppc/spapr.c | 7 +++----
> >>>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>index acc7233..9270234 100644
> >>>--- a/hw/ppc/spapr.c
> >>>+++ b/hw/ppc/spapr.c
> >>>@@ -308,7 +308,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
> >>>      GString *hypertas = g_string_sized_new(256);
> >>>      GString *qemu_hypertas = g_string_sized_new(256);
> >>>      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
> >>>-    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(smp_cpus)};
> >>>+    uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(max_cpus)};
> >>>      int smt = kvmppc_smt_threads();
> >>>      unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
> >>>      QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
> >>>@@ -1454,9 +1454,8 @@ static void ppc_spapr_init(MachineState *machine)
> >>>
> >>>      /* Set up Interrupt Controller before we create the VCPUs */
> >>>      spapr->icp = xics_system_init(machine,
> >>>-                                  DIV_ROUND_UP(smp_cpus * kvmppc_smt_threads(),
> >>>-                                               smp_threads),
> >>>-                                  XICS_IRQS);
> >>>+                                  DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
> >>>+                                               smp_threads), XICS_IRQS);
> >>
> >>
> >>Please do not change the formatting of "XICS_IRQS);".
> >
> >Hmmm why ? I thought I saved a line!
> 
> 
> Looks weird. There were 3 parameters, aligned. Now there are two and third
> one hides behind DIV_ROUND_UP. And we can afford an extra line ;)
> 
> And this change is not related to what the patch does, the patch does
> s/smp_cpus/max_cpus/ and when I see another unrelated change - this confuses
> me.
> 
> 
> >Again checkpatch.pl doesn't complain.
> 
> Well, you can ignore me - after all I am not the one to takes these patches
> further :)

I don't think it's that important, but there are a couple of other
small things to fix in the series, so you might as well revert the
formatting as Alexey suggests at the same time.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 2/8] spapr: Support ibm, lrdr-capacity device tree property

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On Fri, Jun 05, 2015 at 09:55:52AM +0530, Bharata B Rao wrote:
> Add support for ibm,lrdr-capacity since this is needed by the guest
> kernel to know about the possible hot-pluggable CPUs and Memory. With
> this, pseries kernels will start reporting correct maxcpus in
> /sys/devices/system/cpu/possible.
> 
> Also define the minimum hotpluggable memory size as 256MB.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 4/8] spapr: Reorganize CPU dt generation code

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

On Fri, Jun 05, 2015 at 05:55:20PM +1000, Alexey Kardashevskiy wrote:
> On 06/05/2015 05:06 PM, Bharata B Rao wrote:
> >On Fri, Jun 05, 2015 at 04:09:48PM +1000, Alexey Kardashevskiy wrote:
> >>>
> >>>-        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
> >>>-                          pft_size_prop, sizeof(pft_size_prop));
> >>
> >>You broke formatting here, above 8 lines should not be in the patch.
> >
> >You mean above 80 chars ? It is not above 80 chars.
> 
> The chunk is:
> 
> +        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
> +                      pft_size_prop, sizeof(pft_size_prop));
> +        if (ret < 0) {
> +            return ret;
>          }
> 
> -        ret = fdt_setprop(fdt, offset, "ibm,pft-size",
> -                          pft_size_prop, sizeof(pft_size_prop));
> 
> 
> 
> pft_size_prop was under "fdt", now it is not, and this is the only change.
> 
> 
> 
> >
> >>>+    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
> >>>+                            env->dcache_line_size)));
> >>
> >>Wrong indent.
> >
> >checkpatch.pl doesn't complain, but should we start the second line
> >below ( of the first line ?
> 
> Below "(" plus 1 char right.
> 
> set expandtab, set tabstop=4, set shiftwidth=4, set cino=:0,(0  - this is
> what I use for QEMU.

Again, not a big deal, but I'd like a v2 of the series to address a
couple of other things, so you might as well correct the formatting at
the same time.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 5/8] spapr: Consolidate cpu init code into a routine

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote:
> Factor out bits of sPAPR specific CPU initialization code into
> a separate routine so that it can be called from CPU hotplug
> path too.
> 
> While at this, use MSR_EP define instead of using 6 directly.

Don't do this please.  MSR[EP] is an obsolete flag from 601.  The
MSR[IP] flag that we're controlling here just happened to re-use the
same bit position, so using the existing MSR_EP define is misleading.

A symbolic name is good, but you should create a new one for MSR[IP]
instead.

Or you could just drop this change and do the cleanup of the hardcoded
6 some other time.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 7/8] xics_kvm: Don't enable KVM_CAP_IRQ_XICS if already enabled

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]

On Fri, Jun 05, 2015 at 09:55:57AM +0530, Bharata B Rao wrote:
> When supporting CPU hot removal by parking the vCPU fd and reusing
> it during hotplug again, there can be cases where we try to reenable
> KVM_CAP_IRQ_XICS CAP for the vCPU for which it was already enabled.
> Introduce a boolean member in ICPState to track this and don't
> reenable the CAP if it was already enabled earlier.
> 
> Re-enabling this CAP should ideally work, but currently it results in
> kernel trying to create and associate ICP with this vCPU and that
> fails since there is already an ICP associated with it. Hence this
> patch is needed to work around this problem in the kernel.
> 
> This change allows CPU hot removal to work for sPAPR.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 8/8] xics_kvm: Add cpu_destroy method to XICS

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

On Fri, Jun 05, 2015 at 02:45:59PM +0530, Bharata B Rao wrote:
> On Fri, Jun 05, 2015 at 06:09:38PM +1000, Alexey Kardashevskiy wrote:
> > On 06/05/2015 02:25 PM, Bharata B Rao wrote:
> > >XICS is setup for each CPU during initialization. Provide a routine
> > >to undo the same when CPU is unplugged. Also ensure xics reset doesn't set
> > >irq for CPUs that are already unplugged.
> > >
> > >This allows reboot of a VM that has undergone CPU hotplug and unplug
> > >to work correctly.
> > >
> > >Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > >---
> > >  hw/intc/xics.c        | 14 ++++++++++++++
> > >  hw/intc/xics_kvm.c    | 15 +++++++++++++--
> > >  include/hw/ppc/xics.h |  2 ++
> > >  3 files changed, 29 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > >index 924b1ae..3f87f82 100644
> > >--- a/hw/intc/xics.c
> > >+++ b/hw/intc/xics.c
> > >@@ -44,6 +44,20 @@ static int get_cpu_index_by_dt_id(int cpu_dt_id)
> > >      return -1;
> > >  }
> > >
> > >+void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu)
> > 
> > 
> > xics_cpu_destroy() is not used by anything, may be push it later with the
> > stuff which needs it?
> 
> Yeah it is not used in this patchset, will leave to David/agraf
> to see if they want this dropped from this series.

Drop it for now, please.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 5/8] spapr: Consolidate cpu init code into a routine

[Thomas Huth]

On Mon, 15 Jun 2015 16:59:08 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote:
> > Factor out bits of sPAPR specific CPU initialization code into
> > a separate routine so that it can be called from CPU hotplug
> > path too.
> > 
> > While at this, use MSR_EP define instead of using 6 directly.
> 
> Don't do this please.  MSR[EP] is an obsolete flag from 601.  The
> MSR[IP] flag that we're controlling here just happened to re-use the
> same bit position, so using the existing MSR_EP define is misleading.

Actually, I had the same discussion with Bharata already some weeks ago:

http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html

> A symbolic name is good, but you should create a new one for MSR[IP]
> instead.

... and I had to realize that IP = EP. IP likely stands for "interrupt
prefix" (I guess), and EP simply means "exception prefix", so just two
words for the same meaning. It's just the "on 601" comment in QEMU that
is completely misleading. So IMHO it should be fine to keep the
"MSR_EP" here (and maybe update the comment in cpu.h with a separate
patch?).

 Thomas

Re: [Qemu-devel] [PATCH v4 5/8] spapr: Consolidate cpu init code into a routine

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]

On Mon, Jun 15, 2015 at 10:15:09AM +0200, Thomas Huth wrote:
> On Mon, 15 Jun 2015 16:59:08 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote:
> > > Factor out bits of sPAPR specific CPU initialization code into
> > > a separate routine so that it can be called from CPU hotplug
> > > path too.
> > > 
> > > While at this, use MSR_EP define instead of using 6 directly.
> > 
> > Don't do this please.  MSR[EP] is an obsolete flag from 601.  The
> > MSR[IP] flag that we're controlling here just happened to re-use the
> > same bit position, so using the existing MSR_EP define is misleading.
> 
> Actually, I had the same discussion with Bharata already some weeks ago:
> 
> http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html
> 
> > A symbolic name is good, but you should create a new one for MSR[IP]
> > instead.
> 
> ... and I had to realize that IP = EP. IP likely stands for "interrupt
> prefix" (I guess), and EP simply means "exception prefix", so just two
> words for the same meaning. It's just the "on 601" comment in QEMU that
> is completely misleading. So IMHO it should be fine to keep the
> "MSR_EP" here (and maybe update the comment in cpu.h with a separate
> patch?).

I don't entirely agree.  Yes EP and IP have related functions - it's
pretty common in ppc history that when an MSR bit is re-used it's for
something similar (for example IS/IR).  But MSR[IP] is still a
different name from MSR[EP], and I don't know if the semantics are
identical, though I'm sure they're similar.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 5/8] spapr: Consolidate cpu init code into a routine

[Thomas Huth]

On Tue, 16 Jun 2015 15:40:05 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Jun 15, 2015 at 10:15:09AM +0200, Thomas Huth wrote:
> > On Mon, 15 Jun 2015 16:59:08 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote:
> > > > Factor out bits of sPAPR specific CPU initialization code into
> > > > a separate routine so that it can be called from CPU hotplug
> > > > path too.
> > > > 
> > > > While at this, use MSR_EP define instead of using 6 directly.
> > > 
> > > Don't do this please.  MSR[EP] is an obsolete flag from 601.  The
> > > MSR[IP] flag that we're controlling here just happened to re-use the
> > > same bit position, so using the existing MSR_EP define is misleading.
> > 
> > Actually, I had the same discussion with Bharata already some weeks ago:
> > 
> > http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html
> > 
> > > A symbolic name is good, but you should create a new one for MSR[IP]
> > > instead.
> > 
> > ... and I had to realize that IP = EP. IP likely stands for "interrupt
> > prefix" (I guess), and EP simply means "exception prefix", so just two
> > words for the same meaning. It's just the "on 601" comment in QEMU that
> > is completely misleading. So IMHO it should be fine to keep the
> > "MSR_EP" here (and maybe update the comment in cpu.h with a separate
> > patch?).
> 
> I don't entirely agree.  Yes EP and IP have related functions - it's
> pretty common in ppc history that when an MSR bit is re-used it's for
> something similar (for example IS/IR).  But MSR[IP] is still a
> different name from MSR[EP], and I don't know if the semantics are
> identical, though I'm sure they're similar.

I had a look at the 601 User's Manual, and it says:

Bit Name Description
25   EP  Exception prefix. The setting of this bit specifies whether an
         exception vector offset is prepended with Fs or 0s.

Then, looking at the 603 User's Manual, it says:

Bit Name Description
25   IP  Exception prefix. The setting of this bit specifies whether an
         exception vector offset is prepended with Fs or 0s.

So it's the very same bit, just the name has already been changed
between the 601 and 603 already.

So I think it's either fine to keep the MSR_EP in the patch (and change
the comment for the define in a separate patch?), or to introduce
another define, MSR_IP, with a comment that it is the same as MSR_EP,
just with a different name.

However, I still wonder whether this bit applies to the spapr code at
all since it is not defined in the modern PowerISA spec anymore, as far
as I can see.

 Thomas

Re: [Qemu-devel] [PATCH v4 5/8] spapr: Consolidate cpu init code into a routine

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3414 bytes --]

On Tue, Jun 16, 2015 at 08:36:34AM +0200, Thomas Huth wrote:
> On Tue, 16 Jun 2015 15:40:05 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Jun 15, 2015 at 10:15:09AM +0200, Thomas Huth wrote:
> > > On Mon, 15 Jun 2015 16:59:08 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > > On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote:
> > > > > Factor out bits of sPAPR specific CPU initialization code into
> > > > > a separate routine so that it can be called from CPU hotplug
> > > > > path too.
> > > > > 
> > > > > While at this, use MSR_EP define instead of using 6 directly.
> > > > 
> > > > Don't do this please.  MSR[EP] is an obsolete flag from 601.  The
> > > > MSR[IP] flag that we're controlling here just happened to re-use the
> > > > same bit position, so using the existing MSR_EP define is misleading.
> > > 
> > > Actually, I had the same discussion with Bharata already some weeks ago:
> > > 
> > > http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html
> > > 
> > > > A symbolic name is good, but you should create a new one for MSR[IP]
> > > > instead.
> > > 
> > > ... and I had to realize that IP = EP. IP likely stands for "interrupt
> > > prefix" (I guess), and EP simply means "exception prefix", so just two
> > > words for the same meaning. It's just the "on 601" comment in QEMU that
> > > is completely misleading. So IMHO it should be fine to keep the
> > > "MSR_EP" here (and maybe update the comment in cpu.h with a separate
> > > patch?).
> > 
> > I don't entirely agree.  Yes EP and IP have related functions - it's
> > pretty common in ppc history that when an MSR bit is re-used it's for
> > something similar (for example IS/IR).  But MSR[IP] is still a
> > different name from MSR[EP], and I don't know if the semantics are
> > identical, though I'm sure they're similar.
> 
> I had a look at the 601 User's Manual, and it says:
> 
> Bit Name Description
> 25   EP  Exception prefix. The setting of this bit specifies whether an
>          exception vector offset is prepended with Fs or 0s.
> 
> Then, looking at the 603 User's Manual, it says:
> 
> Bit Name Description
> 25   IP  Exception prefix. The setting of this bit specifies whether an
>          exception vector offset is prepended with Fs or 0s.
> 
> So it's the very same bit, just the name has already been changed
> between the 601 and 603 already.
> 
> So I think it's either fine to keep the MSR_EP in the patch (and change
> the comment for the define in a separate patch?), or to introduce
> another define, MSR_IP, with a comment that it is the same as MSR_EP,
> just with a different name.

I'd still prefer to see the modern name in the #define.

> However, I still wonder whether this bit applies to the spapr code at
> all since it is not defined in the modern PowerISA spec anymore, as far
> as I can see.

Hrm.. yeah.  I am wondering if it is not architected, but actually
present as an implementation specific MSR bit on the modern CPUs.

I see that Bharata has reposted leaving the literal 6 as is, which is
probably a fair call until we find a consensus here.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]