[Qemu-devel] [PATCH 0/3] ppc/pnc: add a minimal platform

[Qemu-devel] [PATCH 0/3] ppc/pnc: add a minimal platform

[Cédric Le Goater]

In this version, the initial patch sent by Ben was trimmed down to its
minimal : a platform with some RAM to load initial ROMs and a device
tree built from the machine reset op.

A PnvChip object comes next to act as a container for all the
'chiplets' required to run a system. First of these are the cores,
represented by the PowerNVCPUCore objects.

The PowerNV platform does not provide enough support to be useful but
yet, it can be run under qemu, so you can check the qom tree. This is
the first step to add the missing models. XICS and XCSOM should come
next.

Thanks,

C. 

Benjamin Herrenschmidt (1):
  ppc/pnv: add skeleton PowerNV platform

Cédric Le Goater (2):
  ppc/pnv: add a PnvChip object
  ppc/pnv: add a PowerNVCPUCore object

 default-configs/ppc64-softmmu.mak |   1 +
 hw/ppc/Makefile.objs              |   2 +
 hw/ppc/pnv.c                      | 484 ++++++++++++++++++++++++++++++++++++++
 hw/ppc/pnv_core.c                 | 171 ++++++++++++++
 include/hw/ppc/pnv.h              |  58 +++++
 include/hw/ppc/pnv_core.h         |  47 ++++
 6 files changed, 763 insertions(+)
 create mode 100644 hw/ppc/pnv.c
 create mode 100644 hw/ppc/pnv_core.c
 create mode 100644 include/hw/ppc/pnv.h
 create mode 100644 include/hw/ppc/pnv_core.h

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] ppc/pnv: add skeleton PowerNV platform

[Cédric Le Goater]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

The goal is to emulate a PowerNV system at the level of the skiboot
firmware, which loads the OS and provides some runtime services. Power
Systems have a lower firmware (HostBoot) that does low level system
initialization, like DRAM training. This is beyond the scope of what
qemu will address in a PowerNV guest.

No devices yet, not even an interrupt controller. Just to get started,
some RAM to load the skiboot firmware, the kernel and initrd. The
device tree is fully created in the machine reset op.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[clg: - updated for qemu-2.7
      - replaced fprintf by error_report
      - used a common definition of _FDT macro
      - removed VMStateDescription as migration is not yet supported
      - added IBM Copyright statements
      - reworked kernel_filename handling
      - merged PnvSystem and sPowerNVMachineState
      - removed PHANDLE_XICP
      - added ppc_create_page_sizes_prop helper
      - removed nmi support
      - removed kvm support
      - updated powernv machine to version 2.8
      - removed chips and cpus, They will be provided in another patches
      - added a machine reset routine to initialize the device tree (also)
      - french has a squelette and english a skeleton.
      - improved commit log.
      - reworked prototypes parameters
      - added a check on the ram size (thanks to Michael Ellerman)
      - fixed chip-id cell
      - and then, I got lost with the changes.
]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 default-configs/ppc64-softmmu.mak |   1 +
 hw/ppc/Makefile.objs              |   2 +
 hw/ppc/pnv.c                      | 283 ++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/pnv.h              |  36 +++++
 4 files changed, 322 insertions(+)
 create mode 100644 hw/ppc/pnv.c
 create mode 100644 include/hw/ppc/pnv.h

diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index c4be59f638ed..516a6e25aba3 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -40,6 +40,7 @@ CONFIG_I8259=y
 CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
 CONFIG_PSERIES=y
+CONFIG_POWERNV=y
 CONFIG_PREP=y
 CONFIG_MAC=y
 CONFIG_E500=y
diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 99a0d4e581bf..8105db7d5600 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -5,6 +5,8 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
 obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
 obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
+# IBM PowerNV
+obj-$(CONFIG_POWERNV) += pnv.o
 ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
 obj-y += spapr_pci_vfio.o
 endif
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
new file mode 100644
index 000000000000..3bb6a240c25b
--- /dev/null
+++ b/hw/ppc/pnv.c
@@ -0,0 +1,283 @@
+/*
+ * QEMU PowerPC PowerNV model
+ *
+ * Copyright (c) 2004-2007 Fabrice Bellard
+ * Copyright (c) 2007 Jocelyn Mayer
+ * Copyright (c) 2010 David Gibson, IBM Corporation.
+ * Copyright (c) 2014-2016 BenH, IBM Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/numa.h"
+#include "hw/hw.h"
+#include "target-ppc/cpu.h"
+#include "qemu/log.h"
+#include "hw/ppc/fdt.h"
+#include "hw/ppc/ppc.h"
+#include "hw/ppc/pnv.h"
+#include "hw/loader.h"
+#include "exec/address-spaces.h"
+#include "qemu/cutils.h"
+
+#include <libfdt.h>
+
+#define FDT_ADDR                0x01000000
+#define FDT_MAX_SIZE            0x00100000
+#define FW_MAX_SIZE             0x00400000
+#define FW_FILE_NAME            "skiboot.lid"
+
+#define MAX_CPUS                255
+
+static void powernv_populate_memory_node(void *fdt, int nodeid, hwaddr start,
+                                         hwaddr size)
+{
+    /* Probably bogus, need to match with what's going on in CPU nodes */
+    uint32_t chip_id = nodeid;
+    char *mem_name;
+    uint64_t mem_reg_property[2];
+
+    mem_reg_property[0] = cpu_to_be64(start);
+    mem_reg_property[1] = cpu_to_be64(size);
+
+    mem_name = g_strdup_printf("memory@"TARGET_FMT_lx, start);
+    _FDT((fdt_begin_node(fdt, mem_name)));
+    g_free(mem_name);
+    _FDT((fdt_property_string(fdt, "device_type", "memory")));
+    _FDT((fdt_property(fdt, "reg", mem_reg_property,
+                       sizeof(mem_reg_property))));
+    _FDT((fdt_property_cell(fdt, "ibm,chip-id", chip_id)));
+    _FDT((fdt_end_node(fdt)));
+}
+
+static int powernv_populate_memory(void *fdt)
+{
+    hwaddr mem_start, node_size;
+    int i, nb_nodes = nb_numa_nodes;
+    NodeInfo *nodes = numa_info;
+    NodeInfo ramnode;
+
+    /* No NUMA nodes, assume there is just one node with whole RAM */
+    if (!nb_numa_nodes) {
+        nb_nodes = 1;
+        ramnode.node_mem = ram_size;
+        nodes = &ramnode;
+    }
+
+    for (i = 0, mem_start = 0; i < nb_nodes; ++i) {
+        if (!nodes[i].node_mem) {
+            continue;
+        }
+        if (mem_start >= ram_size) {
+            node_size = 0;
+        } else {
+            node_size = nodes[i].node_mem;
+            if (node_size > ram_size - mem_start) {
+                node_size = ram_size - mem_start;
+            }
+        }
+        for ( ; node_size; ) {
+            hwaddr sizetmp = pow2floor(node_size);
+
+            /* mem_start != 0 here */
+            if (ctzl(mem_start) < ctzl(sizetmp)) {
+                sizetmp = 1ULL << ctzl(mem_start);
+            }
+
+            powernv_populate_memory_node(fdt, i, mem_start, sizetmp);
+            node_size -= sizetmp;
+            mem_start += sizetmp;
+        }
+    }
+
+    return 0;
+}
+
+static void *powernv_create_fdt(sPowerNVMachineState *pnv,
+                                const char *kernel_cmdline)
+{
+    void *fdt;
+    uint32_t start_prop = cpu_to_be32(pnv->initrd_base);
+    uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
+    char *buf;
+    const char plat_compat[] = "qemu,powernv\0ibm,powernv";
+
+    fdt = g_malloc0(FDT_MAX_SIZE);
+    _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
+    _FDT((fdt_finish_reservemap(fdt)));
+
+    /* Root node */
+    _FDT((fdt_begin_node(fdt, "")));
+    _FDT((fdt_property_string(fdt, "model", "IBM PowerNV (emulated by qemu)")));
+    _FDT((fdt_property(fdt, "compatible", plat_compat, sizeof(plat_compat))));
+
+    buf = g_strdup_printf(UUID_FMT, qemu_uuid[0], qemu_uuid[1],
+                          qemu_uuid[2], qemu_uuid[3], qemu_uuid[4],
+                          qemu_uuid[5], qemu_uuid[6], qemu_uuid[7],
+                          qemu_uuid[8], qemu_uuid[9], qemu_uuid[10],
+                          qemu_uuid[11], qemu_uuid[12], qemu_uuid[13],
+                          qemu_uuid[14], qemu_uuid[15]);
+
+    _FDT((fdt_property_string(fdt, "vm,uuid", buf)));
+    g_free(buf);
+
+    _FDT((fdt_begin_node(fdt, "chosen")));
+    if (kernel_cmdline) {
+        _FDT((fdt_property_string(fdt, "bootargs", kernel_cmdline)));
+    }
+    _FDT((fdt_property(fdt, "linux,initrd-start",
+                       &start_prop, sizeof(start_prop))));
+    _FDT((fdt_property(fdt, "linux,initrd-end",
+                       &end_prop, sizeof(end_prop))));
+    _FDT((fdt_end_node(fdt)));
+
+    _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
+    _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
+
+    /* Memory */
+    _FDT((powernv_populate_memory(fdt)));
+
+    _FDT((fdt_end_node(fdt))); /* close root node */
+    _FDT((fdt_finish(fdt)));
+
+    return fdt;
+}
+
+static void ppc_powernv_reset(void)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
+    void *fdt;
+
+    qemu_devices_reset();
+
+    fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
+
+    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
+}
+
+static void ppc_powernv_init(MachineState *machine)
+{
+    ram_addr_t ram_size = machine->ram_size;
+    const char *kernel_filename = machine->kernel_filename;
+    const char *initrd_filename = machine->initrd_filename;
+    MemoryRegion *sysmem = get_system_memory();
+    MemoryRegion *ram = g_new(MemoryRegion, 1);
+    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
+    long fw_size;
+    char *filename;
+
+    if (ram_size < (1 * G_BYTE)) {
+        error_report("Warning: skiboot may not work with < 1GB of RAM");
+    }
+
+    /* allocate RAM */
+    memory_region_allocate_system_memory(ram, NULL, "ppc_powernv.ram",
+                                         ram_size);
+    memory_region_add_subregion(sysmem, 0, ram);
+
+    if (bios_name == NULL) {
+        bios_name = FW_FILE_NAME;
+    }
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
+    if (fw_size < 0) {
+        hw_error("qemu: could not load OPAL '%s'\n", filename);
+        exit(1);
+    }
+    g_free(filename);
+
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, kernel_filename);
+    if (!filename) {
+        hw_error("qemu: could find kernel '%s'\n", kernel_filename);
+        exit(1);
+    }
+
+    fw_size = load_image_targphys(filename, 0x20000000, 0x2000000);
+    if (fw_size < 0) {
+        hw_error("qemu: could not load kernel'%s'\n", filename);
+        exit(1);
+    }
+    g_free(filename);
+
+    /* load initrd */
+    if (initrd_filename) {
+            /* Try to locate the initrd in the gap between the kernel
+             * and the firmware. Add a bit of space just in case
+             */
+            pnv->initrd_base = 0x40000000;
+            pnv->initrd_size = load_image_targphys(initrd_filename,
+                            pnv->initrd_base, 0x10000000); /* 128MB max */
+            if (pnv->initrd_size < 0) {
+                    error_report("qemu: could not load initial ram disk '%s'",
+                            initrd_filename);
+                    exit(1);
+            }
+    } else {
+            pnv->initrd_base = 0;
+            pnv->initrd_size = 0;
+    }
+}
+
+static void powernv_machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->init = ppc_powernv_init;
+    mc->reset = ppc_powernv_reset;
+    mc->block_default_type = IF_IDE;
+    mc->max_cpus = MAX_CPUS;
+    mc->no_parallel = 1;
+    mc->default_boot_order = NULL;
+    mc->default_ram_size = 1 * G_BYTE;
+}
+
+static const TypeInfo powernv_machine_info = {
+    .name          = TYPE_POWERNV_MACHINE,
+    .parent        = TYPE_MACHINE,
+    .abstract      = true,
+    .instance_size = sizeof(sPowerNVMachineState),
+    .class_init    = powernv_machine_class_init,
+};
+
+static void powernv_machine_2_8_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->name = "powernv-2.8";
+    mc->desc = "PowerNV v2.8";
+    mc->alias = "powernv";
+}
+
+static const TypeInfo powernv_machine_2_8_info = {
+    .name          = MACHINE_TYPE_NAME("powernv-2.8"),
+    .parent        = TYPE_POWERNV_MACHINE,
+    .class_init    = powernv_machine_2_8_class_init,
+};
+
+static void powernv_machine_register_types(void)
+{
+    type_register_static(&powernv_machine_info);
+    type_register_static(&powernv_machine_2_8_info);
+}
+
+type_init(powernv_machine_register_types)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
new file mode 100644
index 000000000000..2990f691672d
--- /dev/null
+++ b/include/hw/ppc/pnv.h
@@ -0,0 +1,36 @@
+/*
+ * QEMU PowerNV various definitions
+ *
+ * Copyright (c) 2014-2016 BenH, IBM Corporation.
+ *
+ * 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 _PPC_PNV_H
+#define _PPC_PNV_H
+
+#include "hw/boards.h"
+
+#define TYPE_POWERNV_MACHINE      "powernv-machine"
+#define POWERNV_MACHINE(obj) \
+    OBJECT_CHECK(sPowerNVMachineState, (obj), TYPE_POWERNV_MACHINE)
+
+typedef struct sPowerNVMachineState {
+    /*< private >*/
+    MachineState parent_obj;
+
+    uint32_t initrd_base;
+    long initrd_size;
+} sPowerNVMachineState;
+
+#endif /* _PPC_PNV_H */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] ppc/pnv: add skeleton PowerNV platform

[David Gibson]

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

On Fri, Aug 05, 2016 at 11:15:35AM +0200, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> The goal is to emulate a PowerNV system at the level of the skiboot
> firmware, which loads the OS and provides some runtime services. Power
> Systems have a lower firmware (HostBoot) that does low level system
> initialization, like DRAM training. This is beyond the scope of what
> qemu will address in a PowerNV guest.
> 
> No devices yet, not even an interrupt controller. Just to get started,
> some RAM to load the skiboot firmware, the kernel and initrd. The
> device tree is fully created in the machine reset op.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> [clg: - updated for qemu-2.7
>       - replaced fprintf by error_report
>       - used a common definition of _FDT macro
>       - removed VMStateDescription as migration is not yet supported
>       - added IBM Copyright statements
>       - reworked kernel_filename handling
>       - merged PnvSystem and sPowerNVMachineState
>       - removed PHANDLE_XICP
>       - added ppc_create_page_sizes_prop helper
>       - removed nmi support
>       - removed kvm support
>       - updated powernv machine to version 2.8
>       - removed chips and cpus, They will be provided in another patches
>       - added a machine reset routine to initialize the device tree (also)
>       - french has a squelette and english a skeleton.
>       - improved commit log.
>       - reworked prototypes parameters
>       - added a check on the ram size (thanks to Michael Ellerman)
>       - fixed chip-id cell
>       - and then, I got lost with the changes.
> ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  default-configs/ppc64-softmmu.mak |   1 +
>  hw/ppc/Makefile.objs              |   2 +
>  hw/ppc/pnv.c                      | 283 ++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h              |  36 +++++
>  4 files changed, 322 insertions(+)
>  create mode 100644 hw/ppc/pnv.c
>  create mode 100644 include/hw/ppc/pnv.h
> 
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index c4be59f638ed..516a6e25aba3 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -40,6 +40,7 @@ CONFIG_I8259=y
>  CONFIG_XILINX=y
>  CONFIG_XILINX_ETHLITE=y
>  CONFIG_PSERIES=y
> +CONFIG_POWERNV=y
>  CONFIG_PREP=y
>  CONFIG_MAC=y
>  CONFIG_E500=y
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 99a0d4e581bf..8105db7d5600 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -5,6 +5,8 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> +# IBM PowerNV
> +obj-$(CONFIG_POWERNV) += pnv.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> new file mode 100644
> index 000000000000..3bb6a240c25b
> --- /dev/null
> +++ b/hw/ppc/pnv.c
> @@ -0,0 +1,283 @@
> +/*
> + * QEMU PowerPC PowerNV model
> + *
> + * Copyright (c) 2004-2007 Fabrice Bellard
> + * Copyright (c) 2007 Jocelyn Mayer
> + * Copyright (c) 2010 David Gibson, IBM Corporation.
> + * Copyright (c) 2014-2016 BenH, IBM Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + *
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/numa.h"
> +#include "hw/hw.h"
> +#include "target-ppc/cpu.h"
> +#include "qemu/log.h"
> +#include "hw/ppc/fdt.h"
> +#include "hw/ppc/ppc.h"
> +#include "hw/ppc/pnv.h"
> +#include "hw/loader.h"
> +#include "exec/address-spaces.h"
> +#include "qemu/cutils.h"
> +
> +#include <libfdt.h>
> +
> +#define FDT_ADDR                0x01000000
> +#define FDT_MAX_SIZE            0x00100000
> +#define FW_MAX_SIZE             0x00400000
> +#define FW_FILE_NAME            "skiboot.lid"
> +
> +#define MAX_CPUS                255

So, this is copied from pseries, which in turn copied it from either
mac99 or pc (I forget which).  But having a MAX_CPUS which is not a
multiple of the maximum threads-per-core is kind of dumb, especially
in light of the new hotplug mechanisms.  So let's not repeat this make
another time, and round this off to a multiple of 8.

> +
> +static void powernv_populate_memory_node(void *fdt, int nodeid, hwaddr start,
> +                                         hwaddr size)
> +{
> +    /* Probably bogus, need to match with what's going on in CPU nodes */
> +    uint32_t chip_id = nodeid;
> +    char *mem_name;
> +    uint64_t mem_reg_property[2];
> +
> +    mem_reg_property[0] = cpu_to_be64(start);
> +    mem_reg_property[1] = cpu_to_be64(size);
> +
> +    mem_name = g_strdup_printf("memory@"TARGET_FMT_lx, start);
> +    _FDT((fdt_begin_node(fdt, mem_name)));
> +    g_free(mem_name);
> +    _FDT((fdt_property_string(fdt, "device_type", "memory")));
> +    _FDT((fdt_property(fdt, "reg", mem_reg_property,
> +                       sizeof(mem_reg_property))));
> +    _FDT((fdt_property_cell(fdt, "ibm,chip-id", chip_id)));
> +    _FDT((fdt_end_node(fdt)));
> +}
> +
> +static int powernv_populate_memory(void *fdt)
> +{
> +    hwaddr mem_start, node_size;
> +    int i, nb_nodes = nb_numa_nodes;
> +    NodeInfo *nodes = numa_info;
> +    NodeInfo ramnode;
> +
> +    /* No NUMA nodes, assume there is just one node with whole RAM */
> +    if (!nb_numa_nodes) {
> +        nb_nodes = 1;
> +        ramnode.node_mem = ram_size;
> +        nodes = &ramnode;
> +    }
> +
> +    for (i = 0, mem_start = 0; i < nb_nodes; ++i) {
> +        if (!nodes[i].node_mem) {
> +            continue;
> +        }
> +        if (mem_start >= ram_size) {
> +            node_size = 0;
> +        } else {
> +            node_size = nodes[i].node_mem;
> +            if (node_size > ram_size - mem_start) {
> +                node_size = ram_size - mem_start;
> +            }
> +        }
> +        for ( ; node_size; ) {

This is equivalent to just while (node_size), which would be clearer.

> +            hwaddr sizetmp = pow2floor(node_size);
> +
> +            /* mem_start != 0 here */

Um.. that's not true on the very first iteration, AFAICT..

> +            if (ctzl(mem_start) < ctzl(sizetmp)) {
> +                sizetmp = 1ULL << ctzl(mem_start);
> +            }
> +
> +            powernv_populate_memory_node(fdt, i, mem_start, sizetmp);
> +            node_size -= sizetmp;
> +            mem_start += sizetmp;
> +        }

IIUC this code is basically breaking the memory representation up into
naturally aligned chunks.  Is that right?  A comment to that effect
might make it easier to follow.

> +    }
> +
> +    return 0;
> +}
> +
> +static void *powernv_create_fdt(sPowerNVMachineState *pnv,
> +                                const char *kernel_cmdline)
> +{
> +    void *fdt;
> +    uint32_t start_prop = cpu_to_be32(pnv->initrd_base);
> +    uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
> +    char *buf;
> +    const char plat_compat[] = "qemu,powernv\0ibm,powernv";
> +
> +    fdt = g_malloc0(FDT_MAX_SIZE);
> +    _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
> +    _FDT((fdt_finish_reservemap(fdt)));
> +
> +    /* Root node */
> +    _FDT((fdt_begin_node(fdt, "")));
> +    _FDT((fdt_property_string(fdt, "model", "IBM PowerNV (emulated by qemu)")));
> +    _FDT((fdt_property(fdt, "compatible", plat_compat, sizeof(plat_compat))));
> +
> +    buf = g_strdup_printf(UUID_FMT, qemu_uuid[0], qemu_uuid[1],
> +                          qemu_uuid[2], qemu_uuid[3], qemu_uuid[4],
> +                          qemu_uuid[5], qemu_uuid[6], qemu_uuid[7],
> +                          qemu_uuid[8], qemu_uuid[9], qemu_uuid[10],
> +                          qemu_uuid[11], qemu_uuid[12], qemu_uuid[13],
> +                          qemu_uuid[14], qemu_uuid[15]);
> +
> +    _FDT((fdt_property_string(fdt, "vm,uuid", buf)));
> +    g_free(buf);
> +
> +    _FDT((fdt_begin_node(fdt, "chosen")));
> +    if (kernel_cmdline) {
> +        _FDT((fdt_property_string(fdt, "bootargs", kernel_cmdline)));
> +    }
> +    _FDT((fdt_property(fdt, "linux,initrd-start",
> +                       &start_prop, sizeof(start_prop))));
> +    _FDT((fdt_property(fdt, "linux,initrd-end",
> +                       &end_prop, sizeof(end_prop))));
> +    _FDT((fdt_end_node(fdt)));
> +
> +    _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
> +    _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));

You're writing the fdt sequentially, which means all properties for a
node need to be constructed before any subnodes, so this can't work.
I'm not quite sure what will happen here, but I suspect you only get
away with it because these are the default values for #address-cells
and #size-cells anyway.

Ideally, fdt_property() would give an error in this case, but that's
not at all trivial to implement.

Alternatively, you could change to using the fdt "rw" functions
(random access) instead of the "sw" (sequential) functions.  That
would let you build things in any order, and might be a bit easier to
convert to a "live" tree representation, which I'm hoping to introduce
in the qemu-2.8 timeframe.


> +    /* Memory */
> +    _FDT((powernv_populate_memory(fdt)));
> +
> +    _FDT((fdt_end_node(fdt))); /* close root node */
> +    _FDT((fdt_finish(fdt)));
> +
> +    return fdt;
> +}
> +
> +static void ppc_powernv_reset(void)
> +{
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> +    void *fdt;
> +
> +    qemu_devices_reset();
> +
> +    fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
> +
> +    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
> +}
> +
> +static void ppc_powernv_init(MachineState *machine)
> +{
> +    ram_addr_t ram_size = machine->ram_size;
> +    const char *kernel_filename = machine->kernel_filename;
> +    const char *initrd_filename = machine->initrd_filename;
> +    MemoryRegion *sysmem = get_system_memory();
> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> +    long fw_size;
> +    char *filename;
> +
> +    if (ram_size < (1 * G_BYTE)) {
> +        error_report("Warning: skiboot may not work with < 1GB of RAM");
> +    }
> +
> +    /* allocate RAM */
> +    memory_region_allocate_system_memory(ram, NULL, "ppc_powernv.ram",
> +                                         ram_size);
> +    memory_region_add_subregion(sysmem, 0, ram);
> +
> +    if (bios_name == NULL) {
> +        bios_name = FW_FILE_NAME;
> +    }
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
> +    if (fw_size < 0) {
> +        hw_error("qemu: could not load OPAL '%s'\n", filename);
> +        exit(1);
> +    }
> +    g_free(filename);
> +
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, kernel_filename);

I don't think qemu_find_file(QEMU_FILE_TYPE_BIOS) is usually used for
kernels.  Is this really what you want?

> +    if (!filename) {
> +        hw_error("qemu: could find kernel '%s'\n", kernel_filename);
> +        exit(1);
> +    }
> +
> +    fw_size = load_image_targphys(filename, 0x20000000, 0x2000000);
> +    if (fw_size < 0) {
> +        hw_error("qemu: could not load kernel'%s'\n", filename);
> +        exit(1);
> +    }
> +    g_free(filename);
> +
> +    /* load initrd */
> +    if (initrd_filename) {
> +            /* Try to locate the initrd in the gap between the kernel
> +             * and the firmware. Add a bit of space just in case
> +             */
> +            pnv->initrd_base = 0x40000000;
> +            pnv->initrd_size = load_image_targphys(initrd_filename,
> +                            pnv->initrd_base, 0x10000000); /* 128MB max */
> +            if (pnv->initrd_size < 0) {
> +                    error_report("qemu: could not load initial ram disk '%s'",
> +                            initrd_filename);
> +                    exit(1);
> +            }
> +    } else {
> +            pnv->initrd_base = 0;
> +            pnv->initrd_size = 0;
> +    }
> +}
> +
> +static void powernv_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->init = ppc_powernv_init;
> +    mc->reset = ppc_powernv_reset;
> +    mc->block_default_type = IF_IDE;

IDE?  Really?

> +    mc->max_cpus = MAX_CPUS;
> +    mc->no_parallel = 1;
> +    mc->default_boot_order = NULL;
> +    mc->default_ram_size = 1 * G_BYTE;
> +}
> +
> +static const TypeInfo powernv_machine_info = {
> +    .name          = TYPE_POWERNV_MACHINE,
> +    .parent        = TYPE_MACHINE,
> +    .abstract      = true,
> +    .instance_size = sizeof(sPowerNVMachineState),
> +    .class_init    = powernv_machine_class_init,
> +};
> +
> +static void powernv_machine_2_8_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->name = "powernv-2.8";
> +    mc->desc = "PowerNV v2.8";
> +    mc->alias = "powernv";
> +}
> +
> +static const TypeInfo powernv_machine_2_8_info = {
> +    .name          = MACHINE_TYPE_NAME("powernv-2.8"),
> +    .parent        = TYPE_POWERNV_MACHINE,
> +    .class_init    = powernv_machine_2_8_class_init,
> +};

It might be simpler to just begin with an "unversioned" machine type.
You only really need to start worrying about versions once its
sufficiently stable that you care about migration between different
qemu versions.

> +static void powernv_machine_register_types(void)
> +{
> +    type_register_static(&powernv_machine_info);
> +    type_register_static(&powernv_machine_2_8_info);
> +}
> +
> +type_init(powernv_machine_register_types)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> new file mode 100644
> index 000000000000..2990f691672d
> --- /dev/null
> +++ b/include/hw/ppc/pnv.h
> @@ -0,0 +1,36 @@
> +/*
> + * QEMU PowerNV various definitions
> + *
> + * Copyright (c) 2014-2016 BenH, IBM Corporation.
> + *
> + * 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 _PPC_PNV_H
> +#define _PPC_PNV_H
> +
> +#include "hw/boards.h"
> +
> +#define TYPE_POWERNV_MACHINE      "powernv-machine"
> +#define POWERNV_MACHINE(obj) \
> +    OBJECT_CHECK(sPowerNVMachineState, (obj), TYPE_POWERNV_MACHINE)
> +
> +typedef struct sPowerNVMachineState {

What's the "s" at the beginning for?  Looks like it's copied from
sPAPR into a context where it doesn't really make sense.

> +    /*< private >*/
> +    MachineState parent_obj;
> +
> +    uint32_t initrd_base;
> +    long initrd_size;
> +} sPowerNVMachineState;
> +
> +#endif /* _PPC_PNV_H */

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/3] ppc/pnv: add skeleton PowerNV platform

[Cédric Le Goater]

On 08/16/2016 04:12 AM, David Gibson wrote:
> On Fri, Aug 05, 2016 at 11:15:35AM +0200, Cédric Le Goater wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> The goal is to emulate a PowerNV system at the level of the skiboot
>> firmware, which loads the OS and provides some runtime services. Power
>> Systems have a lower firmware (HostBoot) that does low level system
>> initialization, like DRAM training. This is beyond the scope of what
>> qemu will address in a PowerNV guest.
>>
>> No devices yet, not even an interrupt controller. Just to get started,
>> some RAM to load the skiboot firmware, the kernel and initrd. The
>> device tree is fully created in the machine reset op.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> [clg: - updated for qemu-2.7
>>       - replaced fprintf by error_report
>>       - used a common definition of _FDT macro
>>       - removed VMStateDescription as migration is not yet supported
>>       - added IBM Copyright statements
>>       - reworked kernel_filename handling
>>       - merged PnvSystem and sPowerNVMachineState
>>       - removed PHANDLE_XICP
>>       - added ppc_create_page_sizes_prop helper
>>       - removed nmi support
>>       - removed kvm support
>>       - updated powernv machine to version 2.8
>>       - removed chips and cpus, They will be provided in another patches
>>       - added a machine reset routine to initialize the device tree (also)
>>       - french has a squelette and english a skeleton.
>>       - improved commit log.
>>       - reworked prototypes parameters
>>       - added a check on the ram size (thanks to Michael Ellerman)
>>       - fixed chip-id cell
>>       - and then, I got lost with the changes.
>> ]
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  default-configs/ppc64-softmmu.mak |   1 +
>>  hw/ppc/Makefile.objs              |   2 +
>>  hw/ppc/pnv.c                      | 283 ++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv.h              |  36 +++++
>>  4 files changed, 322 insertions(+)
>>  create mode 100644 hw/ppc/pnv.c
>>  create mode 100644 include/hw/ppc/pnv.h
>>
>> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
>> index c4be59f638ed..516a6e25aba3 100644
>> --- a/default-configs/ppc64-softmmu.mak
>> +++ b/default-configs/ppc64-softmmu.mak
>> @@ -40,6 +40,7 @@ CONFIG_I8259=y
>>  CONFIG_XILINX=y
>>  CONFIG_XILINX_ETHLITE=y
>>  CONFIG_PSERIES=y
>> +CONFIG_POWERNV=y
>>  CONFIG_PREP=y
>>  CONFIG_MAC=y
>>  CONFIG_E500=y
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index 99a0d4e581bf..8105db7d5600 100644
>> --- a/hw/ppc/Makefile.objs
>> +++ b/hw/ppc/Makefile.objs
>> @@ -5,6 +5,8 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>> +# IBM PowerNV
>> +obj-$(CONFIG_POWERNV) += pnv.o
>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>  obj-y += spapr_pci_vfio.o
>>  endif
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> new file mode 100644
>> index 000000000000..3bb6a240c25b
>> --- /dev/null
>> +++ b/hw/ppc/pnv.c
>> @@ -0,0 +1,283 @@
>> +/*
>> + * QEMU PowerPC PowerNV model
>> + *
>> + * Copyright (c) 2004-2007 Fabrice Bellard
>> + * Copyright (c) 2007 Jocelyn Mayer
>> + * Copyright (c) 2010 David Gibson, IBM Corporation.
>> + * Copyright (c) 2014-2016 BenH, IBM Corporation.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + *
>> + */
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/numa.h"
>> +#include "hw/hw.h"
>> +#include "target-ppc/cpu.h"
>> +#include "qemu/log.h"
>> +#include "hw/ppc/fdt.h"
>> +#include "hw/ppc/ppc.h"
>> +#include "hw/ppc/pnv.h"
>> +#include "hw/loader.h"
>> +#include "exec/address-spaces.h"
>> +#include "qemu/cutils.h"
>> +
>> +#include <libfdt.h>
>> +
>> +#define FDT_ADDR                0x01000000
>> +#define FDT_MAX_SIZE            0x00100000
>> +#define FW_MAX_SIZE             0x00400000
>> +#define FW_FILE_NAME            "skiboot.lid"
>> +
>> +#define MAX_CPUS                255
> 
> So, this is copied from pseries, which in turn copied it from either
> mac99 or pc (I forget which).  But having a MAX_CPUS which is not a
> multiple of the maximum threads-per-core is kind of dumb, especially
> in light of the new hotplug mechanisms.  So let's not repeat this make
> another time, and round this off to a multiple of 8.

yes. I made that 2048.

>> +
>> +static void powernv_populate_memory_node(void *fdt, int nodeid, hwaddr start,
>> +                                         hwaddr size)
>> +{
>> +    /* Probably bogus, need to match with what's going on in CPU nodes */
>> +    uint32_t chip_id = nodeid;
>> +    char *mem_name;
>> +    uint64_t mem_reg_property[2];
>> +
>> +    mem_reg_property[0] = cpu_to_be64(start);
>> +    mem_reg_property[1] = cpu_to_be64(size);
>> +
>> +    mem_name = g_strdup_printf("memory@"TARGET_FMT_lx, start);
>> +    _FDT((fdt_begin_node(fdt, mem_name)));
>> +    g_free(mem_name);
>> +    _FDT((fdt_property_string(fdt, "device_type", "memory")));
>> +    _FDT((fdt_property(fdt, "reg", mem_reg_property,
>> +                       sizeof(mem_reg_property))));
>> +    _FDT((fdt_property_cell(fdt, "ibm,chip-id", chip_id)));
>> +    _FDT((fdt_end_node(fdt)));
>> +}
>> +
>> +static int powernv_populate_memory(void *fdt)
>> +{
>> +    hwaddr mem_start, node_size;
>> +    int i, nb_nodes = nb_numa_nodes;
>> +    NodeInfo *nodes = numa_info;
>> +    NodeInfo ramnode;
>> +
>> +    /* No NUMA nodes, assume there is just one node with whole RAM */
>> +    if (!nb_numa_nodes) {
>> +        nb_nodes = 1;
>> +        ramnode.node_mem = ram_size;
>> +        nodes = &ramnode;
>> +    }
>> +
>> +    for (i = 0, mem_start = 0; i < nb_nodes; ++i) {
>> +        if (!nodes[i].node_mem) {
>> +            continue;
>> +        }
>> +        if (mem_start >= ram_size) {
>> +            node_size = 0;
>> +        } else {
>> +            node_size = nodes[i].node_mem;
>> +            if (node_size > ram_size - mem_start) {
>> +                node_size = ram_size - mem_start;
>> +            }
>> +        }
>> +        for ( ; node_size; ) {
> 
> This is equivalent to just while (node_size), which would be clearer.
> 
>> +            hwaddr sizetmp = pow2floor(node_size);
>> +
>> +            /* mem_start != 0 here */
> 
> Um.. that's not true on the very first iteration, AFAICT..
> 
>> +            if (ctzl(mem_start) < ctzl(sizetmp)) {
>> +                sizetmp = 1ULL << ctzl(mem_start);
>> +            }
>> +
>> +            powernv_populate_memory_node(fdt, i, mem_start, sizetmp);
>> +            node_size -= sizetmp;
>> +            mem_start += sizetmp;
>> +        }
> 
> IIUC this code is basically breaking the memory representation up into
> naturally aligned chunks.  Is that right?  A comment to that effect
> might make it easier to follow.

That routine came from spar with some minor hacks. 

So, in the current patchset, I removed all of it as on powernv Memory nodes 
are created by hostboot, one for each range of memory that has a different 
"affinity". In practice, it means one range per chip.

We will start with one chip, so one memory node with all the RAM in it. Then, 
when we add more chips, we will have to figure out how to assign RAM to some 
and no RAM to others. I guess the numa API will come into play but I haven't
look deeper yet.

>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>> +                                const char *kernel_cmdline)
>> +{
>> +    void *fdt;
>> +    uint32_t start_prop = cpu_to_be32(pnv->initrd_base);
>> +    uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
>> +    char *buf;
>> +    const char plat_compat[] = "qemu,powernv\0ibm,powernv";
>> +
>> +    fdt = g_malloc0(FDT_MAX_SIZE);
>> +    _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
>> +    _FDT((fdt_finish_reservemap(fdt)));
>> +
>> +    /* Root node */
>> +    _FDT((fdt_begin_node(fdt, "")));
>> +    _FDT((fdt_property_string(fdt, "model", "IBM PowerNV (emulated by qemu)")));
>> +    _FDT((fdt_property(fdt, "compatible", plat_compat, sizeof(plat_compat))));
>> +
>> +    buf = g_strdup_printf(UUID_FMT, qemu_uuid[0], qemu_uuid[1],
>> +                          qemu_uuid[2], qemu_uuid[3], qemu_uuid[4],
>> +                          qemu_uuid[5], qemu_uuid[6], qemu_uuid[7],
>> +                          qemu_uuid[8], qemu_uuid[9], qemu_uuid[10],
>> +                          qemu_uuid[11], qemu_uuid[12], qemu_uuid[13],
>> +                          qemu_uuid[14], qemu_uuid[15]);
>> +
>> +    _FDT((fdt_property_string(fdt, "vm,uuid", buf)));
>> +    g_free(buf);
>> +
>> +    _FDT((fdt_begin_node(fdt, "chosen")));
>> +    if (kernel_cmdline) {
>> +        _FDT((fdt_property_string(fdt, "bootargs", kernel_cmdline)));
>> +    }
>> +    _FDT((fdt_property(fdt, "linux,initrd-start",
>> +                       &start_prop, sizeof(start_prop))));
>> +    _FDT((fdt_property(fdt, "linux,initrd-end",
>> +                       &end_prop, sizeof(end_prop))));
>> +    _FDT((fdt_end_node(fdt)));
>> +
>> +    _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
>> +    _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
> 
> You're writing the fdt sequentially, which means all properties for a
> node need to be constructed before any subnodes, so this can't work.
> I'm not quite sure what will happen here, but I suspect you only get
> away with it because these are the default values for #address-cells
> and #size-cells anyway.
> 
> Ideally, fdt_property() would give an error in this case, but that's
> not at all trivial to implement.
> 
> Alternatively, you could change to using the fdt "rw" functions
> (random access) instead of the "sw" (sequential) functions.  That
> would let you build things in any order, and might be a bit easier to
> convert to a "live" tree representation, which I'm hoping to introduce
> in the qemu-2.8 timeframe.

I have changed the whole patchset to use the fdt "rw" functions. It 
made my life easier to populate the device tree with devices found 
on the command line, like the IPMI BT device. 

A few questions on that topic, 

what is the difference between the 'fdt_' api and the 'qemu_fdt_' api ? 
Which one should we use ? 

I duplicated (again) create_device_tree() because of the fixed size. 
May be we could introduce something like :

+static void *powernv_create_device_tree(int size)
+{
+    void *fdt = g_malloc0(size);
+
+    _FDT((fdt_create(fdt, size)));
+    _FDT((fdt_finish_reservemap(fdt)));
+    _FDT((fdt_begin_node(fdt, "")));
+    _FDT((fdt_end_node(fdt)));
+    _FDT((fdt_finish(fdt)));
+    _FDT((fdt_open_into(fdt, fdt, size)));
+
+    return fdt;
+}

? 

>> +    /* Memory */
>> +    _FDT((powernv_populate_memory(fdt)));
>> +
>> +    _FDT((fdt_end_node(fdt))); /* close root node */
>> +    _FDT((fdt_finish(fdt)));
>> +
>> +    return fdt;
>> +}
>> +
>> +static void ppc_powernv_reset(void)
>> +{
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>> +    void *fdt;
>> +
>> +    qemu_devices_reset();
>> +
>> +    fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
>> +
>> +    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
>> +}
>> +
>> +static void ppc_powernv_init(MachineState *machine)
>> +{
>> +    ram_addr_t ram_size = machine->ram_size;
>> +    const char *kernel_filename = machine->kernel_filename;
>> +    const char *initrd_filename = machine->initrd_filename;
>> +    MemoryRegion *sysmem = get_system_memory();
>> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
>> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>> +    long fw_size;
>> +    char *filename;
>> +
>> +    if (ram_size < (1 * G_BYTE)) {
>> +        error_report("Warning: skiboot may not work with < 1GB of RAM");
>> +    }
>> +
>> +    /* allocate RAM */
>> +    memory_region_allocate_system_memory(ram, NULL, "ppc_powernv.ram",
>> +                                         ram_size);
>> +    memory_region_add_subregion(sysmem, 0, ram);
>> +
>> +    if (bios_name == NULL) {
>> +        bios_name = FW_FILE_NAME;
>> +    }
>> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>> +    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
>> +    if (fw_size < 0) {
>> +        hw_error("qemu: could not load OPAL '%s'\n", filename);
>> +        exit(1);
>> +    }
>> +    g_free(filename);
>> +
>> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, kernel_filename);
> 
> I don't think qemu_find_file(QEMU_FILE_TYPE_BIOS) is usually used for
> kernels.  Is this really what you want?

no. fixed.

>> +    if (!filename) {
>> +        hw_error("qemu: could find kernel '%s'\n", kernel_filename);
>> +        exit(1);
>> +    }
>> +
>> +    fw_size = load_image_targphys(filename, 0x20000000, 0x2000000);
>> +    if (fw_size < 0) {
>> +        hw_error("qemu: could not load kernel'%s'\n", filename);
>> +        exit(1);
>> +    }
>> +    g_free(filename);
>> +
>> +    /* load initrd */
>> +    if (initrd_filename) {
>> +            /* Try to locate the initrd in the gap between the kernel
>> +             * and the firmware. Add a bit of space just in case
>> +             */
>> +            pnv->initrd_base = 0x40000000;
>> +            pnv->initrd_size = load_image_targphys(initrd_filename,
>> +                            pnv->initrd_base, 0x10000000); /* 128MB max */
>> +            if (pnv->initrd_size < 0) {
>> +                    error_report("qemu: could not load initial ram disk '%s'",
>> +                            initrd_filename);
>> +                    exit(1);
>> +            }
>> +    } else {
>> +            pnv->initrd_base = 0;
>> +            pnv->initrd_size = 0;
>> +    }
>> +}
>> +
>> +static void powernv_machine_class_init(ObjectClass *oc, void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +
>> +    mc->init = ppc_powernv_init;
>> +    mc->reset = ppc_powernv_reset;
>> +    mc->block_default_type = IF_IDE;
> 
> IDE?  Really?

nah :) It's SCSI again now.

I was trying to reconcile all the little hunks in the different 
patches of Ben. IF_IDE was introduced at the end of the patchset. 
I suppose that adding a ISA bus to PowerNV had some influence :)

> 
>> +    mc->max_cpus = MAX_CPUS;
>> +    mc->no_parallel = 1;
>> +    mc->default_boot_order = NULL;
>> +    mc->default_ram_size = 1 * G_BYTE;
>> +}
>> +
>> +static const TypeInfo powernv_machine_info = {
>> +    .name          = TYPE_POWERNV_MACHINE,
>> +    .parent        = TYPE_MACHINE,
>> +    .abstract      = true,
>> +    .instance_size = sizeof(sPowerNVMachineState),
>> +    .class_init    = powernv_machine_class_init,
>> +};
>> +
>> +static void powernv_machine_2_8_class_init(ObjectClass *oc, void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +
>> +    mc->name = "powernv-2.8";
>> +    mc->desc = "PowerNV v2.8";
>> +    mc->alias = "powernv";
>> +}
>> +
>> +static const TypeInfo powernv_machine_2_8_info = {
>> +    .name          = MACHINE_TYPE_NAME("powernv-2.8"),
>> +    .parent        = TYPE_POWERNV_MACHINE,
>> +    .class_init    = powernv_machine_2_8_class_init,
>> +};
> 
> It might be simpler to just begin with an "unversioned" machine type.

yes. fixed.

> You only really need to start worrying about versions once its
> sufficiently stable that you care about migration between different
> qemu versions.
>
>> +static void powernv_machine_register_types(void)
>> +{
>> +    type_register_static(&powernv_machine_info);
>> +    type_register_static(&powernv_machine_2_8_info);
>> +}
>> +
>> +type_init(powernv_machine_register_types)
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> new file mode 100644
>> index 000000000000..2990f691672d
>> --- /dev/null
>> +++ b/include/hw/ppc/pnv.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * QEMU PowerNV various definitions
>> + *
>> + * Copyright (c) 2014-2016 BenH, IBM Corporation.
>> + *
>> + * 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 _PPC_PNV_H
>> +#define _PPC_PNV_H
>> +
>> +#include "hw/boards.h"
>> +
>> +#define TYPE_POWERNV_MACHINE      "powernv-machine"
>> +#define POWERNV_MACHINE(obj) \
>> +    OBJECT_CHECK(sPowerNVMachineState, (obj), TYPE_POWERNV_MACHINE)
>> +
>> +typedef struct sPowerNVMachineState {
> 
> What's the "s" at the beginning for?  Looks like it's copied from
> sPAPR into a context where it doesn't really make sense.

It is now called PnvMachineState.

You can see the changes with your inputs here:

	https://github.com/legoater/qemu/commits/powernv-ipmi-2.8?page=2

I would like to get the chips, the cores and the xscom bus 'mostly' right 
first, so the rest is in a working state.

Thanks,

C. 

>> +    /*< private >*/
>> +    MachineState parent_obj;
>> +
>> +    uint32_t initrd_base;
>> +    long initrd_size;
>> +} sPowerNVMachineState;
>> +
>> +#endif /* _PPC_PNV_H */
> 

Re: [Qemu-devel] [PATCH 1/3] ppc/pnv: add skeleton PowerNV platform

[Benjamin Herrenschmidt]

On Fri, 2016-08-26 at 16:47 +0200, Cédric Le Goater wrote:
> >> +static void powernv_machine_class_init(ObjectClass *oc, void
> *data)
> >> +{
> >> +    MachineClass *mc = MACHINE_CLASS(oc);
> >> +
> >> +    mc->init = ppc_powernv_init;
> >> +    mc->reset = ppc_powernv_reset;
> >> +    mc->block_default_type = IF_IDE;
> > 
> > IDE?  Really?
> 
> nah :) It's SCSI again now.
> 
> I was trying to reconcile all the little hunks in the different 
> patches of Ben. IF_IDE was introduced at the end of the patchset. 
> I suppose that adding a ISA bus to PowerNV had some influence :)

It's meant to be IF_IDE, the default interface is AHCI.

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 1/3] ppc/pnv: add skeleton PowerNV platform

[David Gibson]

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

On Sat, Aug 27, 2016 at 08:48:10AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2016-08-26 at 16:47 +0200, Cédric Le Goater wrote:
> > >> +static void powernv_machine_class_init(ObjectClass *oc, void
> > *data)
> > >> +{
> > >> +    MachineClass *mc = MACHINE_CLASS(oc);
> > >> +
> > >> +    mc->init = ppc_powernv_init;
> > >> +    mc->reset = ppc_powernv_reset;
> > >> +    mc->block_default_type = IF_IDE;
> > > 
> > > IDE?  Really?
> > 
> > nah :) It's SCSI again now.
> > 
> > I was trying to reconcile all the little hunks in the different 
> > patches of Ben. IF_IDE was introduced at the end of the patchset. 
> > I suppose that adding a ISA bus to PowerNV had some influence :)
> 
> It's meant to be IF_IDE, the default interface is AHCI.

Ah, right.  A comment mentioning AHCI would help to clarify this.


-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH 1/3] ppc/pnv: add skeleton PowerNV platform

[David Gibson]

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

On Fri, Aug 26, 2016 at 04:47:08PM +0200, Cédric Le Goater wrote:
> On 08/16/2016 04:12 AM, David Gibson wrote:
> > On Fri, Aug 05, 2016 at 11:15:35AM +0200, Cédric Le Goater wrote:
> >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>
> >> The goal is to emulate a PowerNV system at the level of the skiboot
> >> firmware, which loads the OS and provides some runtime services. Power
> >> Systems have a lower firmware (HostBoot) that does low level system
> >> initialization, like DRAM training. This is beyond the scope of what
> >> qemu will address in a PowerNV guest.
> >>
> >> No devices yet, not even an interrupt controller. Just to get started,
> >> some RAM to load the skiboot firmware, the kernel and initrd. The
> >> device tree is fully created in the machine reset op.
> >>
> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> [clg: - updated for qemu-2.7
> >>       - replaced fprintf by error_report
> >>       - used a common definition of _FDT macro
> >>       - removed VMStateDescription as migration is not yet supported
> >>       - added IBM Copyright statements
> >>       - reworked kernel_filename handling
> >>       - merged PnvSystem and sPowerNVMachineState
> >>       - removed PHANDLE_XICP
> >>       - added ppc_create_page_sizes_prop helper
> >>       - removed nmi support
> >>       - removed kvm support
> >>       - updated powernv machine to version 2.8
> >>       - removed chips and cpus, They will be provided in another patches
> >>       - added a machine reset routine to initialize the device tree (also)
> >>       - french has a squelette and english a skeleton.
> >>       - improved commit log.
> >>       - reworked prototypes parameters
> >>       - added a check on the ram size (thanks to Michael Ellerman)
> >>       - fixed chip-id cell
> >>       - and then, I got lost with the changes.
> >> ]
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  default-configs/ppc64-softmmu.mak |   1 +
> >>  hw/ppc/Makefile.objs              |   2 +
> >>  hw/ppc/pnv.c                      | 283 ++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/pnv.h              |  36 +++++
> >>  4 files changed, 322 insertions(+)
> >>  create mode 100644 hw/ppc/pnv.c
> >>  create mode 100644 include/hw/ppc/pnv.h
> >>
> >> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> >> index c4be59f638ed..516a6e25aba3 100644
> >> --- a/default-configs/ppc64-softmmu.mak
> >> +++ b/default-configs/ppc64-softmmu.mak
> >> @@ -40,6 +40,7 @@ CONFIG_I8259=y
> >>  CONFIG_XILINX=y
> >>  CONFIG_XILINX_ETHLITE=y
> >>  CONFIG_PSERIES=y
> >> +CONFIG_POWERNV=y
> >>  CONFIG_PREP=y
> >>  CONFIG_MAC=y
> >>  CONFIG_E500=y
> >> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >> index 99a0d4e581bf..8105db7d5600 100644
> >> --- a/hw/ppc/Makefile.objs
> >> +++ b/hw/ppc/Makefile.objs
> >> @@ -5,6 +5,8 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> >>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >> +# IBM PowerNV
> >> +obj-$(CONFIG_POWERNV) += pnv.o
> >>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >>  obj-y += spapr_pci_vfio.o
> >>  endif
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> new file mode 100644
> >> index 000000000000..3bb6a240c25b
> >> --- /dev/null
> >> +++ b/hw/ppc/pnv.c
> >> @@ -0,0 +1,283 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV model
> >> + *
> >> + * Copyright (c) 2004-2007 Fabrice Bellard
> >> + * Copyright (c) 2007 Jocelyn Mayer
> >> + * Copyright (c) 2010 David Gibson, IBM Corporation.
> >> + * Copyright (c) 2014-2016 BenH, IBM Corporation.
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> >> + * of this software and associated documentation files (the "Software"), to deal
> >> + * in the Software without restriction, including without limitation the rights
> >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> >> + * copies of the Software, and to permit persons to whom the Software is
> >> + * furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice shall be included in
> >> + * all copies or substantial portions of the Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> >> + * THE SOFTWARE.
> >> + *
> >> + */
> >> +#include "qemu/osdep.h"
> >> +#include "qapi/error.h"
> >> +#include "sysemu/sysemu.h"
> >> +#include "sysemu/numa.h"
> >> +#include "hw/hw.h"
> >> +#include "target-ppc/cpu.h"
> >> +#include "qemu/log.h"
> >> +#include "hw/ppc/fdt.h"
> >> +#include "hw/ppc/ppc.h"
> >> +#include "hw/ppc/pnv.h"
> >> +#include "hw/loader.h"
> >> +#include "exec/address-spaces.h"
> >> +#include "qemu/cutils.h"
> >> +
> >> +#include <libfdt.h>
> >> +
> >> +#define FDT_ADDR                0x01000000
> >> +#define FDT_MAX_SIZE            0x00100000
> >> +#define FW_MAX_SIZE             0x00400000
> >> +#define FW_FILE_NAME            "skiboot.lid"
> >> +
> >> +#define MAX_CPUS                255
> > 
> > So, this is copied from pseries, which in turn copied it from either
> > mac99 or pc (I forget which).  But having a MAX_CPUS which is not a
> > multiple of the maximum threads-per-core is kind of dumb, especially
> > in light of the new hotplug mechanisms.  So let's not repeat this make
> > another time, and round this off to a multiple of 8.
> 
> yes. I made that 2048.

Ok.

> >> +static void powernv_populate_memory_node(void *fdt, int nodeid, hwaddr start,
> >> +                                         hwaddr size)
> >> +{
> >> +    /* Probably bogus, need to match with what's going on in CPU nodes */
> >> +    uint32_t chip_id = nodeid;
> >> +    char *mem_name;
> >> +    uint64_t mem_reg_property[2];
> >> +
> >> +    mem_reg_property[0] = cpu_to_be64(start);
> >> +    mem_reg_property[1] = cpu_to_be64(size);
> >> +
> >> +    mem_name = g_strdup_printf("memory@"TARGET_FMT_lx, start);
> >> +    _FDT((fdt_begin_node(fdt, mem_name)));
> >> +    g_free(mem_name);
> >> +    _FDT((fdt_property_string(fdt, "device_type", "memory")));
> >> +    _FDT((fdt_property(fdt, "reg", mem_reg_property,
> >> +                       sizeof(mem_reg_property))));
> >> +    _FDT((fdt_property_cell(fdt, "ibm,chip-id", chip_id)));
> >> +    _FDT((fdt_end_node(fdt)));
> >> +}
> >> +
> >> +static int powernv_populate_memory(void *fdt)
> >> +{
> >> +    hwaddr mem_start, node_size;
> >> +    int i, nb_nodes = nb_numa_nodes;
> >> +    NodeInfo *nodes = numa_info;
> >> +    NodeInfo ramnode;
> >> +
> >> +    /* No NUMA nodes, assume there is just one node with whole RAM */
> >> +    if (!nb_numa_nodes) {
> >> +        nb_nodes = 1;
> >> +        ramnode.node_mem = ram_size;
> >> +        nodes = &ramnode;
> >> +    }
> >> +
> >> +    for (i = 0, mem_start = 0; i < nb_nodes; ++i) {
> >> +        if (!nodes[i].node_mem) {
> >> +            continue;
> >> +        }
> >> +        if (mem_start >= ram_size) {
> >> +            node_size = 0;
> >> +        } else {
> >> +            node_size = nodes[i].node_mem;
> >> +            if (node_size > ram_size - mem_start) {
> >> +                node_size = ram_size - mem_start;
> >> +            }
> >> +        }
> >> +        for ( ; node_size; ) {
> > 
> > This is equivalent to just while (node_size), which would be clearer.
> > 
> >> +            hwaddr sizetmp = pow2floor(node_size);
> >> +
> >> +            /* mem_start != 0 here */
> > 
> > Um.. that's not true on the very first iteration, AFAICT..
> > 
> >> +            if (ctzl(mem_start) < ctzl(sizetmp)) {
> >> +                sizetmp = 1ULL << ctzl(mem_start);
> >> +            }
> >> +
> >> +            powernv_populate_memory_node(fdt, i, mem_start, sizetmp);
> >> +            node_size -= sizetmp;
> >> +            mem_start += sizetmp;
> >> +        }
> > 
> > IIUC this code is basically breaking the memory representation up into
> > naturally aligned chunks.  Is that right?  A comment to that effect
> > might make it easier to follow.
> 
> That routine came from spar with some minor hacks. 

Ah, so it is.  I'd forgotten that code there, guess it needs a clean
up.

> So, in the current patchset, I removed all of it as on powernv Memory nodes 
> are created by hostboot, one for each range of memory that has a different 
> "affinity". In practice, it means one range per chip.

Ok.

> We will start with one chip, so one memory node with all the RAM in it. Then, 
> when we add more chips, we will have to figure out how to assign RAM to some 
> and no RAM to others. I guess the numa API will come into play but I haven't
> look deeper yet.

Right, there will be some complexities here.  Let's cross that bridge
when we come to it.

> 
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static void *powernv_create_fdt(sPowerNVMachineState *pnv,
> >> +                                const char *kernel_cmdline)
> >> +{
> >> +    void *fdt;
> >> +    uint32_t start_prop = cpu_to_be32(pnv->initrd_base);
> >> +    uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
> >> +    char *buf;
> >> +    const char plat_compat[] = "qemu,powernv\0ibm,powernv";
> >> +
> >> +    fdt = g_malloc0(FDT_MAX_SIZE);
> >> +    _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
> >> +    _FDT((fdt_finish_reservemap(fdt)));
> >> +
> >> +    /* Root node */
> >> +    _FDT((fdt_begin_node(fdt, "")));
> >> +    _FDT((fdt_property_string(fdt, "model", "IBM PowerNV (emulated by qemu)")));
> >> +    _FDT((fdt_property(fdt, "compatible", plat_compat, sizeof(plat_compat))));
> >> +
> >> +    buf = g_strdup_printf(UUID_FMT, qemu_uuid[0], qemu_uuid[1],
> >> +                          qemu_uuid[2], qemu_uuid[3], qemu_uuid[4],
> >> +                          qemu_uuid[5], qemu_uuid[6], qemu_uuid[7],
> >> +                          qemu_uuid[8], qemu_uuid[9], qemu_uuid[10],
> >> +                          qemu_uuid[11], qemu_uuid[12], qemu_uuid[13],
> >> +                          qemu_uuid[14], qemu_uuid[15]);
> >> +
> >> +    _FDT((fdt_property_string(fdt, "vm,uuid", buf)));
> >> +    g_free(buf);
> >> +
> >> +    _FDT((fdt_begin_node(fdt, "chosen")));
> >> +    if (kernel_cmdline) {
> >> +        _FDT((fdt_property_string(fdt, "bootargs", kernel_cmdline)));
> >> +    }
> >> +    _FDT((fdt_property(fdt, "linux,initrd-start",
> >> +                       &start_prop, sizeof(start_prop))));
> >> +    _FDT((fdt_property(fdt, "linux,initrd-end",
> >> +                       &end_prop, sizeof(end_prop))));
> >> +    _FDT((fdt_end_node(fdt)));
> >> +
> >> +    _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
> >> +    _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
> > 
> > You're writing the fdt sequentially, which means all properties for a
> > node need to be constructed before any subnodes, so this can't work.
> > I'm not quite sure what will happen here, but I suspect you only get
> > away with it because these are the default values for #address-cells
> > and #size-cells anyway.
> > 
> > Ideally, fdt_property() would give an error in this case, but that's
> > not at all trivial to implement.
> > 
> > Alternatively, you could change to using the fdt "rw" functions
> > (random access) instead of the "sw" (sequential) functions.  That
> > would let you build things in any order, and might be a bit easier to
> > convert to a "live" tree representation, which I'm hoping to introduce
> > in the qemu-2.8 timeframe.
> 
> I have changed the whole patchset to use the fdt "rw" functions. It 
> made my life easier to populate the device tree with devices found 
> on the command line, like the IPMI BT device. 

Sounds reasonable.

> A few questions on that topic, 
> 
> what is the difference between the 'fdt_' api and the 'qemu_fdt_' api ? 

Heh.

So, the fdt_* functions are the raw API exported by libfdt itself.
The qemu_fdt_* functions are a limited wrapper around these originally
created for some of the embedded machine types.

IMO, there's not actually a lot of value to that wrapper, and little
reason not to use the fdt api directly.  So, I'd encourage using the
plain fdt_* functions, at least until my in-the-works patches for
better handling of dt creation within qemu get somewhere.

> Which one should we use ? 
> 
> I duplicated (again) create_device_tree() because of the fixed size. 
> May be we could introduce something like :
> 
> +static void *powernv_create_device_tree(int size)
> +{
> +    void *fdt = g_malloc0(size);
> +
> +    _FDT((fdt_create(fdt, size)));
> +    _FDT((fdt_finish_reservemap(fdt)));
> +    _FDT((fdt_begin_node(fdt, "")));
> +    _FDT((fdt_end_node(fdt)));
> +    _FDT((fdt_finish(fdt)));
> +    _FDT((fdt_open_into(fdt, fdt, size)));
> +
> +    return fdt;
> +}
> 
> ? 

So, current libfdt actually has such a function built in -
fdt_create_empty_tree().  We should really make sure the submodule
includes a recent enough version and use that.

> >> +    /* Memory */
> >> +    _FDT((powernv_populate_memory(fdt)));
> >> +
> >> +    _FDT((fdt_end_node(fdt))); /* close root node */
> >> +    _FDT((fdt_finish(fdt)));
> >> +
> >> +    return fdt;
> >> +}
> >> +
> >> +static void ppc_powernv_reset(void)
> >> +{
> >> +    MachineState *machine = MACHINE(qdev_get_machine());
> >> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> >> +    void *fdt;
> >> +
> >> +    qemu_devices_reset();
> >> +
> >> +    fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
> >> +
> >> +    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
> >> +}
> >> +
> >> +static void ppc_powernv_init(MachineState *machine)
> >> +{
> >> +    ram_addr_t ram_size = machine->ram_size;
> >> +    const char *kernel_filename = machine->kernel_filename;
> >> +    const char *initrd_filename = machine->initrd_filename;
> >> +    MemoryRegion *sysmem = get_system_memory();
> >> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
> >> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> >> +    long fw_size;
> >> +    char *filename;
> >> +
> >> +    if (ram_size < (1 * G_BYTE)) {
> >> +        error_report("Warning: skiboot may not work with < 1GB of RAM");
> >> +    }
> >> +
> >> +    /* allocate RAM */
> >> +    memory_region_allocate_system_memory(ram, NULL, "ppc_powernv.ram",
> >> +                                         ram_size);
> >> +    memory_region_add_subregion(sysmem, 0, ram);
> >> +
> >> +    if (bios_name == NULL) {
> >> +        bios_name = FW_FILE_NAME;
> >> +    }
> >> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> >> +    fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
> >> +    if (fw_size < 0) {
> >> +        hw_error("qemu: could not load OPAL '%s'\n", filename);
> >> +        exit(1);
> >> +    }
> >> +    g_free(filename);
> >> +
> >> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, kernel_filename);
> > 
> > I don't think qemu_find_file(QEMU_FILE_TYPE_BIOS) is usually used for
> > kernels.  Is this really what you want?
> 
> no. fixed.
> 
> >> +    if (!filename) {
> >> +        hw_error("qemu: could find kernel '%s'\n", kernel_filename);
> >> +        exit(1);
> >> +    }
> >> +
> >> +    fw_size = load_image_targphys(filename, 0x20000000, 0x2000000);
> >> +    if (fw_size < 0) {
> >> +        hw_error("qemu: could not load kernel'%s'\n", filename);
> >> +        exit(1);
> >> +    }
> >> +    g_free(filename);
> >> +
> >> +    /* load initrd */
> >> +    if (initrd_filename) {
> >> +            /* Try to locate the initrd in the gap between the kernel
> >> +             * and the firmware. Add a bit of space just in case
> >> +             */
> >> +            pnv->initrd_base = 0x40000000;
> >> +            pnv->initrd_size = load_image_targphys(initrd_filename,
> >> +                            pnv->initrd_base, 0x10000000); /* 128MB max */
> >> +            if (pnv->initrd_size < 0) {
> >> +                    error_report("qemu: could not load initial ram disk '%s'",
> >> +                            initrd_filename);
> >> +                    exit(1);
> >> +            }
> >> +    } else {
> >> +            pnv->initrd_base = 0;
> >> +            pnv->initrd_size = 0;
> >> +    }
> >> +}
> >> +
> >> +static void powernv_machine_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +    MachineClass *mc = MACHINE_CLASS(oc);
> >> +
> >> +    mc->init = ppc_powernv_init;
> >> +    mc->reset = ppc_powernv_reset;
> >> +    mc->block_default_type = IF_IDE;
> > 
> > IDE?  Really?
> 
> nah :) It's SCSI again now.
> 
> I was trying to reconcile all the little hunks in the different 
> patches of Ben. IF_IDE was introduced at the end of the patchset. 
> I suppose that adding a ISA bus to PowerNV had some influence :)
> 
> > 
> >> +    mc->max_cpus = MAX_CPUS;
> >> +    mc->no_parallel = 1;
> >> +    mc->default_boot_order = NULL;
> >> +    mc->default_ram_size = 1 * G_BYTE;
> >> +}
> >> +
> >> +static const TypeInfo powernv_machine_info = {
> >> +    .name          = TYPE_POWERNV_MACHINE,
> >> +    .parent        = TYPE_MACHINE,
> >> +    .abstract      = true,
> >> +    .instance_size = sizeof(sPowerNVMachineState),
> >> +    .class_init    = powernv_machine_class_init,
> >> +};
> >> +
> >> +static void powernv_machine_2_8_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +    MachineClass *mc = MACHINE_CLASS(oc);
> >> +
> >> +    mc->name = "powernv-2.8";
> >> +    mc->desc = "PowerNV v2.8";
> >> +    mc->alias = "powernv";
> >> +}
> >> +
> >> +static const TypeInfo powernv_machine_2_8_info = {
> >> +    .name          = MACHINE_TYPE_NAME("powernv-2.8"),
> >> +    .parent        = TYPE_POWERNV_MACHINE,
> >> +    .class_init    = powernv_machine_2_8_class_init,
> >> +};
> > 
> > It might be simpler to just begin with an "unversioned" machine type.
> 
> yes. fixed.
> 
> > You only really need to start worrying about versions once its
> > sufficiently stable that you care about migration between different
> > qemu versions.
> >
> >> +static void powernv_machine_register_types(void)
> >> +{
> >> +    type_register_static(&powernv_machine_info);
> >> +    type_register_static(&powernv_machine_2_8_info);
> >> +}
> >> +
> >> +type_init(powernv_machine_register_types)
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> new file mode 100644
> >> index 000000000000..2990f691672d
> >> --- /dev/null
> >> +++ b/include/hw/ppc/pnv.h
> >> @@ -0,0 +1,36 @@
> >> +/*
> >> + * QEMU PowerNV various definitions
> >> + *
> >> + * Copyright (c) 2014-2016 BenH, IBM Corporation.
> >> + *
> >> + * 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 _PPC_PNV_H
> >> +#define _PPC_PNV_H
> >> +
> >> +#include "hw/boards.h"
> >> +
> >> +#define TYPE_POWERNV_MACHINE      "powernv-machine"
> >> +#define POWERNV_MACHINE(obj) \
> >> +    OBJECT_CHECK(sPowerNVMachineState, (obj), TYPE_POWERNV_MACHINE)
> >> +
> >> +typedef struct sPowerNVMachineState {
> > 
> > What's the "s" at the beginning for?  Looks like it's copied from
> > sPAPR into a context where it doesn't really make sense.
> 
> It is now called PnvMachineState.
> 
> You can see the changes with your inputs here:
> 
> 	https://github.com/legoater/qemu/commits/powernv-ipmi-2.8?page=2
> 
> I would like to get the chips, the cores and the xscom bus 'mostly' right 
> first, so the rest is in a working state.
> 
> Thanks,
> 
> C. 
> 
> >> +    /*< private >*/
> >> +    MachineState parent_obj;
> >> +
> >> +    uint32_t initrd_base;
> >> +    long initrd_size;
> >> +} sPowerNVMachineState;
> >> +
> >> +#endif /* _PPC_PNV_H */
> > 
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[Qemu-devel] [PATCH 2/3] ppc/pnv: add a PnvChip object

[Cédric Le Goater]

This is is an abstraction of a P8 chip which is a set of cores plus
other 'units', like the pervasive unit, the interrupt controller, the
memory controller, the on-chip microcontroller, etc. The whole can be
seen as a socket.

We start with an empty PnvChip which we will grow in the subsequent
patches with controllers required to run the system.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c         | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/pnv.h | 15 +++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3bb6a240c25b..a680780e9dea 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -185,6 +185,7 @@ static void ppc_powernv_init(MachineState *machine)
     sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
     long fw_size;
     char *filename;
+    int i;
 
     if (ram_size < (1 * G_BYTE)) {
         error_report("Warning: skiboot may not work with < 1GB of RAM");
@@ -236,6 +237,23 @@ static void ppc_powernv_init(MachineState *machine)
             pnv->initrd_base = 0;
             pnv->initrd_size = 0;
     }
+
+    /* Create PowerNV chips
+     *
+     * FIXME: We should decide how many chips to create based on
+     * #cores and Venice vs. Murano vs. Naples chip type etc..., for
+     * now, just create one chip, with all the cores.
+     */
+    pnv->num_chips = 1;
+
+    pnv->chips = g_new0(PnvChip, pnv->num_chips);
+    for (i = 0; i < pnv->num_chips; i++) {
+        PnvChip *chip = &pnv->chips[i];
+
+        object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
+        object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
+        object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
+    }
 }
 
 static void powernv_machine_class_init(ObjectClass *oc, void *data)
@@ -274,10 +292,39 @@ static const TypeInfo powernv_machine_2_8_info = {
     .class_init    = powernv_machine_2_8_class_init,
 };
 
+
+static void pnv_chip_realize(DeviceState *dev, Error **errp)
+{
+    ;
+}
+
+static Property pnv_chip_properties[] = {
+    DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pnv_chip_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = pnv_chip_realize;
+    dc->props = pnv_chip_properties;
+    dc->desc = "PowerNV Chip";
+ }
+
+static const TypeInfo pnv_chip_info = {
+    .name          = TYPE_PNV_CHIP,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PnvChip),
+    .class_init    = pnv_chip_class_init,
+};
+
+
 static void powernv_machine_register_types(void)
 {
     type_register_static(&powernv_machine_info);
     type_register_static(&powernv_machine_2_8_info);
+    type_register_static(&pnv_chip_info);
 }
 
 type_init(powernv_machine_register_types)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 2990f691672d..6907dc9e5c3d 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -20,6 +20,18 @@
 #define _PPC_PNV_H
 
 #include "hw/boards.h"
+#include "hw/sysbus.h"
+
+#define TYPE_PNV_CHIP "powernv-chip"
+#define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
+
+typedef struct PnvChip {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    uint32_t     chip_id;
+} PnvChip;
 
 #define TYPE_POWERNV_MACHINE      "powernv-machine"
 #define POWERNV_MACHINE(obj) \
@@ -31,6 +43,9 @@ typedef struct sPowerNVMachineState {
 
     uint32_t initrd_base;
     long initrd_size;
+
+    uint32_t  num_chips;
+    PnvChip   *chips;
 } sPowerNVMachineState;
 
 #endif /* _PPC_PNV_H */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] ppc/pnv: add a PnvChip object

[Benjamin Herrenschmidt]

On Fri, 2016-08-05 at 11:15 +0200, Cédric Le Goater wrote:
> This is is an abstraction of a P8 chip which is a set of cores plus
> other 'units', like the pervasive unit, the interrupt controller, the
> memory controller, the on-chip microcontroller, etc. The whole can be
> seen as a socket.
> 
> We start with an empty PnvChip which we will grow in the subsequent
> patches with controllers required to run the system..

We should create a subclass PnvChipP8 which we instanciate for now
since P9 is around the corner and will be a bit different

Cheers,
Ben.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/pnv.c         | 47
> +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h | 15 +++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3bb6a240c25b..a680780e9dea 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -185,6 +185,7 @@ static void ppc_powernv_init(MachineState
> *machine)
>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>      long fw_size;
>      char *filename;
> +    int i;
>  
>      if (ram_size < (1 * G_BYTE)) {
>          error_report("Warning: skiboot may not work with < 1GB of
> RAM");
> @@ -236,6 +237,23 @@ static void ppc_powernv_init(MachineState
> *machine)
>              pnv->initrd_base = 0;
>              pnv->initrd_size = 0;
>      }
> +
> +    /* Create PowerNV chips
> +     *
> +     * FIXME: We should decide how many chips to create based on
> +     * #cores and Venice vs. Murano vs. Naples chip type etc..., for
> +     * now, just create one chip, with all the cores.
> +     */
> +    pnv->num_chips = 1;
> +
> +    pnv->chips = g_new0(PnvChip, pnv->num_chips);
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        PnvChip *chip = &pnv->chips[i];
> +
> +        object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
> +        object_property_set_int(OBJECT(chip), i, "chip-id",
> &error_abort);
> +        object_property_set_bool(OBJECT(chip), true, "realized",
> &error_abort);
> +    }
>  }
>  
>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
> @@ -274,10 +292,39 @@ static const TypeInfo powernv_machine_2_8_info
> = {
>      .class_init    = powernv_machine_2_8_class_init,
>  };
>  
> +
> +static void pnv_chip_realize(DeviceState *dev, Error **errp)
> +{
> +    ;
> +}
> +
> +static Property pnv_chip_properties[] = {
> +    DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pnv_chip_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = pnv_chip_realize;
> +    dc->props = pnv_chip_properties;
> +    dc->desc = "PowerNV Chip";
> + }
> +
> +static const TypeInfo pnv_chip_info = {
> +    .name          = TYPE_PNV_CHIP,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PnvChip),
> +    .class_init    = pnv_chip_class_init,
> +};
> +
> +
>  static void powernv_machine_register_types(void)
>  {
>      type_register_static(&powernv_machine_info);
>      type_register_static(&powernv_machine_2_8_info);
> +    type_register_static(&pnv_chip_info);
>  }
>  
>  type_init(powernv_machine_register_types)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 2990f691672d..6907dc9e5c3d 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -20,6 +20,18 @@
>  #define _PPC_PNV_H
>  
>  #include "hw/boards.h"
> +#include "hw/sysbus.h"
> +
> +#define TYPE_PNV_CHIP "powernv-chip"
> +#define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
> +
> +typedef struct PnvChip {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    uint32_t     chip_id;
> +} PnvChip;
>  
>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
>  #define POWERNV_MACHINE(obj) \
> @@ -31,6 +43,9 @@ typedef struct sPowerNVMachineState {
>  
>      uint32_t initrd_base;
>      long initrd_size;
> +
> +    uint32_t  num_chips;
> +    PnvChip   *chips;
>  } sPowerNVMachineState;
>  
>  #endif /* _PPC_PNV_H */

Re: [Qemu-devel] [PATCH 2/3] ppc/pnv: add a PnvChip object

[Cédric Le Goater]

On 08/05/2016 11:44 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2016-08-05 at 11:15 +0200, Cédric Le Goater wrote:
>> This is is an abstraction of a P8 chip which is a set of cores plus
>> other 'units', like the pervasive unit, the interrupt controller, the
>> memory controller, the on-chip microcontroller, etc. The whole can be
>> seen as a socket.
>>
>> We start with an empty PnvChip which we will grow in the subsequent
>> patches with controllers required to run the system..
> 
> We should create a subclass PnvChipP8 which we instanciate for now
> since P9 is around the corner and will be a bit different

Yes. I gave it a try on a new 2.8 branch, check it here : 

	https://github.com/legoater/qemu/commit/60a6206c2b31d897d1af2ea3641870c2cc0e8c41

It defines a PnvChip base class with a realize routine in which 
we can handle specific attributes of child classes like the 
PnvChipPower8. I will send it in v2 for review when David has 
taken a look at v1.

All the controllers are still under the base class, so all looks
good. But when the PnvChipPower9 comes in play, we will need to 
redispatch and see how it fits the purpose of supporting multiple
Chip models.

The core initialization should be ok but building the device 
tree might be a bit of a burden if we have to 'cast' in the chip 
type we need. We will see.


So what would be the big differences with what we have today ?   

Thanks,

C.
 
> Cheers,
> Ben.
> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ppc/pnv.c         | 47
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv.h | 15 +++++++++++++++
>>  2 files changed, 62 insertions(+)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 3bb6a240c25b..a680780e9dea 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -185,6 +185,7 @@ static void ppc_powernv_init(MachineState
>> *machine)
>>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>>      long fw_size;
>>      char *filename;
>> +    int i;
>>  
>>      if (ram_size < (1 * G_BYTE)) {
>>          error_report("Warning: skiboot may not work with < 1GB of
>> RAM");
>> @@ -236,6 +237,23 @@ static void ppc_powernv_init(MachineState
>> *machine)
>>              pnv->initrd_base = 0;
>>              pnv->initrd_size = 0;
>>      }
>> +
>> +    /* Create PowerNV chips
>> +     *
>> +     * FIXME: We should decide how many chips to create based on
>> +     * #cores and Venice vs. Murano vs. Naples chip type etc..., for
>> +     * now, just create one chip, with all the cores.
>> +     */
>> +    pnv->num_chips = 1;
>> +
>> +    pnv->chips = g_new0(PnvChip, pnv->num_chips);
>> +    for (i = 0; i < pnv->num_chips; i++) {
>> +        PnvChip *chip = &pnv->chips[i];
>> +
>> +        object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
>> +        object_property_set_int(OBJECT(chip), i, "chip-id",
>> &error_abort);
>> +        object_property_set_bool(OBJECT(chip), true, "realized",
>> &error_abort);
>> +    }
>>  }
>>  
>>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>> @@ -274,10 +292,39 @@ static const TypeInfo powernv_machine_2_8_info
>> = {
>>      .class_init    = powernv_machine_2_8_class_init,
>>  };
>>  
>> +
>> +static void pnv_chip_realize(DeviceState *dev, Error **errp)
>> +{
>> +    ;
>> +}
>> +
>> +static Property pnv_chip_properties[] = {
>> +    DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pnv_chip_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = pnv_chip_realize;
>> +    dc->props = pnv_chip_properties;
>> +    dc->desc = "PowerNV Chip";
>> + }
>> +
>> +static const TypeInfo pnv_chip_info = {
>> +    .name          = TYPE_PNV_CHIP,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(PnvChip),
>> +    .class_init    = pnv_chip_class_init,
>> +};
>> +
>> +
>>  static void powernv_machine_register_types(void)
>>  {
>>      type_register_static(&powernv_machine_info);
>>      type_register_static(&powernv_machine_2_8_info);
>> +    type_register_static(&pnv_chip_info);
>>  }
>>  
>>  type_init(powernv_machine_register_types)
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 2990f691672d..6907dc9e5c3d 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -20,6 +20,18 @@
>>  #define _PPC_PNV_H
>>  
>>  #include "hw/boards.h"
>> +#include "hw/sysbus.h"
>> +
>> +#define TYPE_PNV_CHIP "powernv-chip"
>> +#define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
>> +
>> +typedef struct PnvChip {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +
>> +    /*< public >*/
>> +    uint32_t     chip_id;
>> +} PnvChip;
>>  
>>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
>>  #define POWERNV_MACHINE(obj) \
>> @@ -31,6 +43,9 @@ typedef struct sPowerNVMachineState {
>>  
>>      uint32_t initrd_base;
>>      long initrd_size;
>> +
>> +    uint32_t  num_chips;
>> +    PnvChip   *chips;
>>  } sPowerNVMachineState;
>>  
>>  #endif /* _PPC_PNV_H */

Re: [Qemu-devel] [PATCH 2/3] ppc/pnv: add a PnvChip object

[Benjamin Herrenschmidt]

On Fri, 2016-08-05 at 18:48 +0200, Cédric Le Goater wrote:
> The core initialization should be ok but building the device 
> tree might be a bit of a burden if we have to 'cast' in the chip 
> type we need. We will see.

We don't cast, we use a method.

> So what would be the big differences with what we have today ?   

The XSCOM controller has a different address decoding scheme, so
we'll have two variants and a base class there. The number and
location of functional units changes, so we'll probably need to make
them properties or something, the core XSCOM addressing is completely
different, we use PHB4 rather than PHB3, etc...

(Note that for PHB we should probably rename PHB3 to PnvPhb with a
subclass as well as there is a lot in common between the two, though
the PBCQ bit is quite different).

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 2/3] ppc/pnv: add a PnvChip object

[David Gibson]

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

On Sat, Aug 06, 2016 at 08:43:21AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2016-08-05 at 18:48 +0200, Cédric Le Goater wrote:
> > The core initialization should be ok but building the device 
> > tree might be a bit of a burden if we have to 'cast' in the chip 
> > type we need. We will see.
> 
> We don't cast, we use a method.

Right.  You can have a look at the sPAPR VIO stuff for an example
where we use a method in the individual devices to do the device
specific fdt construction.

> 
> > So what would be the big differences with what we have today ?   
> 
> The XSCOM controller has a different address decoding scheme, so
> we'll have two variants and a base class there. The number and
> location of functional units changes, so we'll probably need to make
> them properties or something, the core XSCOM addressing is completely
> different, we use PHB4 rather than PHB3, etc...
> 
> (Note that for PHB we should probably rename PHB3 to PnvPhb with a
> subclass as well as there is a lot in common between the two, though
> the PBCQ bit is quite different).
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/3] ppc/pnv: add a PnvChip object

[David Gibson]

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

On Fri, Aug 05, 2016 at 11:15:36AM +0200, Cédric Le Goater wrote:
> This is is an abstraction of a P8 chip which is a set of cores plus
> other 'units', like the pervasive unit, the interrupt controller, the
> memory controller, the on-chip microcontroller, etc. The whole can be
> seen as a socket.
> 
> We start with an empty PnvChip which we will grow in the subsequent
> patches with controllers required to run the system.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/pnv.c         | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h | 15 +++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3bb6a240c25b..a680780e9dea 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -185,6 +185,7 @@ static void ppc_powernv_init(MachineState *machine)
>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>      long fw_size;
>      char *filename;
> +    int i;
>  
>      if (ram_size < (1 * G_BYTE)) {
>          error_report("Warning: skiboot may not work with < 1GB of RAM");
> @@ -236,6 +237,23 @@ static void ppc_powernv_init(MachineState *machine)
>              pnv->initrd_base = 0;
>              pnv->initrd_size = 0;
>      }
> +
> +    /* Create PowerNV chips
> +     *
> +     * FIXME: We should decide how many chips to create based on
> +     * #cores and Venice vs. Murano vs. Naples chip type etc..., for
> +     * now, just create one chip, with all the cores.
> +     */
> +    pnv->num_chips = 1;
> +
> +    pnv->chips = g_new0(PnvChip, pnv->num_chips);
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        PnvChip *chip = &pnv->chips[i];
> +
> +        object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);

I think you'd be better off having an array of pointers, each one you
allocate with object_new() rather than doing an explicit g_new0() for
the whole array then using object_initialize().

For one thing, if certain chip subtypes need to allocate more space
for their instance, then this approach will break, whereas
object_new() will get that right.

> +        object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
> +        object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
> +    }
>  }
>  
>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
> @@ -274,10 +292,39 @@ static const TypeInfo powernv_machine_2_8_info = {
>      .class_init    = powernv_machine_2_8_class_init,
>  };
>  
> +
> +static void pnv_chip_realize(DeviceState *dev, Error **errp)
> +{
> +    ;
> +}
> +
> +static Property pnv_chip_properties[] = {
> +    DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pnv_chip_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = pnv_chip_realize;
> +    dc->props = pnv_chip_properties;
> +    dc->desc = "PowerNV Chip";
> + }
> +
> +static const TypeInfo pnv_chip_info = {
> +    .name          = TYPE_PNV_CHIP,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PnvChip),
> +    .class_init    = pnv_chip_class_init,
> +};
> +
> +
>  static void powernv_machine_register_types(void)
>  {
>      type_register_static(&powernv_machine_info);
>      type_register_static(&powernv_machine_2_8_info);
> +    type_register_static(&pnv_chip_info);
>  }
>  
>  type_init(powernv_machine_register_types)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 2990f691672d..6907dc9e5c3d 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -20,6 +20,18 @@
>  #define _PPC_PNV_H
>  
>  #include "hw/boards.h"
> +#include "hw/sysbus.h"
> +
> +#define TYPE_PNV_CHIP "powernv-chip"
> +#define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
> +
> +typedef struct PnvChip {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    uint32_t     chip_id;
> +} PnvChip;
>  
>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
>  #define POWERNV_MACHINE(obj) \
> @@ -31,6 +43,9 @@ typedef struct sPowerNVMachineState {
>  
>      uint32_t initrd_base;
>      long initrd_size;
> +
> +    uint32_t  num_chips;
> +    PnvChip   *chips;
>  } sPowerNVMachineState;
>  
>  #endif /* _PPC_PNV_H */

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/3] ppc/pnv: add a PnvChip object

[Cédric Le Goater]

On 08/16/2016 04:21 AM, David Gibson wrote:
> On Fri, Aug 05, 2016 at 11:15:36AM +0200, Cédric Le Goater wrote:
>> This is is an abstraction of a P8 chip which is a set of cores plus
>> other 'units', like the pervasive unit, the interrupt controller, the
>> memory controller, the on-chip microcontroller, etc. The whole can be
>> seen as a socket.
>>
>> We start with an empty PnvChip which we will grow in the subsequent
>> patches with controllers required to run the system.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ppc/pnv.c         | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv.h | 15 +++++++++++++++
>>  2 files changed, 62 insertions(+)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 3bb6a240c25b..a680780e9dea 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -185,6 +185,7 @@ static void ppc_powernv_init(MachineState *machine)
>>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>>      long fw_size;
>>      char *filename;
>> +    int i;
>>  
>>      if (ram_size < (1 * G_BYTE)) {
>>          error_report("Warning: skiboot may not work with < 1GB of RAM");
>> @@ -236,6 +237,23 @@ static void ppc_powernv_init(MachineState *machine)
>>              pnv->initrd_base = 0;
>>              pnv->initrd_size = 0;
>>      }
>> +
>> +    /* Create PowerNV chips
>> +     *
>> +     * FIXME: We should decide how many chips to create based on
>> +     * #cores and Venice vs. Murano vs. Naples chip type etc..., for
>> +     * now, just create one chip, with all the cores.
>> +     */
>> +    pnv->num_chips = 1;
>> +
>> +    pnv->chips = g_new0(PnvChip, pnv->num_chips);
>> +    for (i = 0; i < pnv->num_chips; i++) {
>> +        PnvChip *chip = &pnv->chips[i];
>> +
>> +        object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
> 
> I think you'd be better off having an array of pointers, each one you
> allocate with object_new() rather than doing an explicit g_new0() for
> the whole array then using object_initialize().
> 
> For one thing, if certain chip subtypes need to allocate more space
> for their instance, then this approach will break, whereas
> object_new() will get that right.

ok. I did not know that. This is the approach I have taken in the 
new patchset but for another reason. I made the type of the PnvChip
object depend on the cpu_model. It should be useful for other chiplets 
which can behave differently.  

A first specificity to handle is the support of the LPC interrupts via 
the LPC controller for the Power8NVL chip. Ben just worked that out. 
I need to see how this comes together with the model I have.

Thanks,

C. 


>> +        object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
>> +        object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
>> +    }
>>  }
>>  
>>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>> @@ -274,10 +292,39 @@ static const TypeInfo powernv_machine_2_8_info = {
>>      .class_init    = powernv_machine_2_8_class_init,
>>  };
>>  
>> +
>> +static void pnv_chip_realize(DeviceState *dev, Error **errp)
>> +{
>> +    ;
>> +}
>> +
>> +static Property pnv_chip_properties[] = {
>> +    DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pnv_chip_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = pnv_chip_realize;
>> +    dc->props = pnv_chip_properties;
>> +    dc->desc = "PowerNV Chip";
>> + }
>> +
>> +static const TypeInfo pnv_chip_info = {
>> +    .name          = TYPE_PNV_CHIP,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(PnvChip),
>> +    .class_init    = pnv_chip_class_init,
>> +};
>> +
>> +
>>  static void powernv_machine_register_types(void)
>>  {
>>      type_register_static(&powernv_machine_info);
>>      type_register_static(&powernv_machine_2_8_info);
>> +    type_register_static(&pnv_chip_info);
>>  }
>>  
>>  type_init(powernv_machine_register_types)
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 2990f691672d..6907dc9e5c3d 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -20,6 +20,18 @@
>>  #define _PPC_PNV_H
>>  
>>  #include "hw/boards.h"
>> +#include "hw/sysbus.h"
>> +
>> +#define TYPE_PNV_CHIP "powernv-chip"
>> +#define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
>> +
>> +typedef struct PnvChip {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +
>> +    /*< public >*/
>> +    uint32_t     chip_id;
>> +} PnvChip;
>>  
>>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
>>  #define POWERNV_MACHINE(obj) \
>> @@ -31,6 +43,9 @@ typedef struct sPowerNVMachineState {
>>  
>>      uint32_t initrd_base;
>>      long initrd_size;
>> +
>> +    uint32_t  num_chips;
>> +    PnvChip   *chips;
>>  } sPowerNVMachineState;
>>  
>>  #endif /* _PPC_PNV_H */
> 

[Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[Cédric Le Goater]

This is largy inspired by sPAPRCPUCore with some simplification, no
hotplug for instance. But the differences are small and the objects
could possibly be merged.

A set of PowerNVCPUCore objects is added to the PnvChip and the device
tree is populated looping on these cores. Core ids in the device tree
are still a little fuzy. To be checked.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/Makefile.objs      |   2 +-
 hw/ppc/pnv.c              | 160 ++++++++++++++++++++++++++++++++++++++++++-
 hw/ppc/pnv_core.c         | 171 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/pnv.h      |   7 ++
 include/hw/ppc/pnv_core.h |  47 +++++++++++++
 5 files changed, 383 insertions(+), 4 deletions(-)
 create mode 100644 hw/ppc/pnv_core.c
 create mode 100644 include/hw/ppc/pnv_core.h

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 8105db7d5600..f8c7d1db9ade 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
 obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
 obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
 # IBM PowerNV
-obj-$(CONFIG_POWERNV) += pnv.o
+obj-$(CONFIG_POWERNV) += pnv.o pnv_core.o
 ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
 obj-y += spapr_pci_vfio.o
 endif
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index a680780e9dea..1219493c7218 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -35,6 +35,7 @@
 #include "hw/ppc/fdt.h"
 #include "hw/ppc/ppc.h"
 #include "hw/ppc/pnv.h"
+#include "hw/ppc/pnv_core.h"
 #include "hw/loader.h"
 #include "exec/address-spaces.h"
 #include "qemu/cutils.h"
@@ -112,6 +113,114 @@ static int powernv_populate_memory(void *fdt)
     return 0;
 }
 
+static void powernv_create_core_node(void *fdt, CPUState *cs, uint32_t chip_id)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    int smt_threads = ppc_get_compat_smt_threads(cpu);
+    CPUPPCState *env = &cpu->env;
+    DeviceClass *dc = DEVICE_GET_CLASS(cs);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
+    uint32_t servers_prop[smt_threads];
+    uint32_t gservers_prop[smt_threads * 2];
+    int i, index = ppc_get_vcpu_dt_id(cpu);
+    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
+                       0xffffffff, 0xffffffff};
+    uint32_t tbfreq = PNV_TIMEBASE_FREQ;
+    uint32_t cpufreq = 1000000000;
+    uint32_t page_sizes_prop[64];
+    size_t page_sizes_prop_size;
+    char *nodename;
+
+    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 {
+        error_report("Warning: Unknown L1 dcache size for cpu");
+    }
+    if (pcc->l1_icache_size) {
+        _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_size)));
+    } else {
+        error_report("Warning: Unknown L1 icache size for cpu");
+    }
+
+    _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 = ppc_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", chip_id)));
+
+    if (cpu->cpu_version) {
+        _FDT((fdt_property_cell(fdt, "cpu-version", cpu->cpu_version)));
+    }
+
+    /* Build interrupt servers and gservers properties */
+    for (i = 0; i < smt_threads; i++) {
+        servers_prop[i] = cpu_to_be32(index + i);
+        /* Hack, direct the group queues back to cpu 0 */
+        gservers_prop[i * 2] = cpu_to_be32(index + i);
+        gservers_prop[i * 2 + 1] = 0;
+    }
+    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-server#s",
+                       servers_prop, sizeof(servers_prop))));
+    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s",
+                       gservers_prop, sizeof(gservers_prop))));
+
+    _FDT((fdt_end_node(fdt)));
+}
+
 static void *powernv_create_fdt(sPowerNVMachineState *pnv,
                                 const char *kernel_cmdline)
 {
@@ -120,6 +229,7 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
     uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
     char *buf;
     const char plat_compat[] = "qemu,powernv\0ibm,powernv";
+    int i;
 
     fdt = g_malloc0(FDT_MAX_SIZE);
     _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
@@ -156,6 +266,23 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
     /* Memory */
     _FDT((powernv_populate_memory(fdt)));
 
+    /* cpus */
+    _FDT((fdt_begin_node(fdt, "cpus")));
+    _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
+    _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
+
+    for (i = 0; i < pnv->num_chips; i++) {
+        PnvChip *chip = &pnv->chips[i];
+        int j;
+
+        for (j = 0; j < chip->num_cores; j++) {
+            PowerNVCPUCore *pc = POWERNV_CPU_CORE(chip->cores[j]);
+            CPUState *cs = CPU(DEVICE(pc->threads));
+            powernv_create_core_node(fdt, cs, chip->chip_id);
+        }
+    }
+    _FDT((fdt_end_node(fdt)));
+
     _FDT((fdt_end_node(fdt))); /* close root node */
     _FDT((fdt_finish(fdt)));
 
@@ -168,11 +295,13 @@ static void ppc_powernv_reset(void)
     sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
     void *fdt;
 
+    pnv->fdt_addr = FDT_ADDR;
+
     qemu_devices_reset();
 
     fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
 
-    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
+    cpu_physical_memory_write(pnv->fdt_addr, fdt, fdt_totalsize(fdt));
 }
 
 static void ppc_powernv_init(MachineState *machine)
@@ -252,6 +381,10 @@ static void ppc_powernv_init(MachineState *machine)
 
         object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
         object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
+        object_property_set_int(OBJECT(chip), smp_cpus, "num-cores",
+                                &error_abort);
+        object_property_set_str(OBJECT(chip), machine->cpu_model, "cpu-model",
+                                &error_abort);
         object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
     }
 }
@@ -292,14 +425,35 @@ static const TypeInfo powernv_machine_2_8_info = {
     .class_init    = powernv_machine_2_8_class_init,
 };
 
-
 static void pnv_chip_realize(DeviceState *dev, Error **errp)
 {
-    ;
+    int i;
+    PnvChip *chip = PNV_CHIP(dev);
+    char *typename = powernv_cpu_core_typename(chip->cpu_model);
+
+    if (!object_class_by_name(typename)) {
+        error_setg(errp, "Unable to find PowerNV CPU Core definition");
+        return;
+    }
+
+    chip->cores = g_new0(Object *, chip->num_cores);
+    for (i = 0; i < chip->num_cores; i++) {
+        int core_id = i * smp_threads;
+        chip->cores[i] = object_new(typename);
+        object_property_set_int(chip->cores[i], smp_threads, "nr-threads",
+                                &error_fatal);
+        object_property_set_int(chip->cores[i], core_id, CPU_CORE_PROP_CORE_ID,
+                                &error_fatal);
+        object_property_set_bool(chip->cores[i], true, "realized",
+                                 &error_fatal);
+    }
+    g_free(typename);
 }
 
 static Property pnv_chip_properties[] = {
     DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
+    DEFINE_PROP_STRING("cpu-model", PnvChip, cpu_model),
+    DEFINE_PROP_UINT32("num-cores", PnvChip, num_cores, 1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
new file mode 100644
index 000000000000..1e36709db993
--- /dev/null
+++ b/hw/ppc/pnv_core.c
@@ -0,0 +1,171 @@
+/*
+ * QEMU PowerPC PowerNV CPU model
+ *
+ * Copyright (c) IBM Corporation.
+ *
+ * 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 "qemu/osdep.h"
+#include "sysemu/sysemu.h"
+#include "qapi/error.h"
+#include "target-ppc/cpu.h"
+#include "hw/ppc/ppc.h"
+#include "hw/ppc/pnv.h"
+#include "hw/ppc/pnv_core.h"
+
+static void powernv_cpu_reset(void *opaque)
+{
+    PowerPCCPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
+    CPUPPCState *env = &cpu->env;
+    MachineState *machine = MACHINE(qdev_get_machine());
+    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
+
+    cpu_reset(cs);
+
+    env->spr[SPR_PIR] = ppc_get_vcpu_dt_id(cpu);
+    env->spr[SPR_HIOR] = 0;
+    env->gpr[3] = pnv->fdt_addr;
+    env->nip = 0x10;
+    env->msr |= MSR_HVB;
+}
+
+static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
+{
+    CPUPPCState *env = &cpu->env;
+
+    /* Set time-base frequency to 512 MHz */
+    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
+
+    /* MSR[IP] doesn't exist nowadays */
+    env->msr_mask &= ~(1 << 6);
+
+    qemu_register_reset(powernv_cpu_reset, cpu);
+    powernv_cpu_reset(cpu);
+}
+
+static void powernv_cpu_core_realize_child(Object *child, Error **errp)
+{
+    Error *local_err = NULL;
+    CPUState *cs = CPU(child);
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    object_property_set_bool(child, true, "realized", &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    powernv_cpu_init(cpu, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
+static void powernv_cpu_core_realize(DeviceState *dev, Error **errp)
+{
+    PowerNVCPUCore *pc = POWERNV_CPU_CORE(OBJECT(dev));
+    CPUCore *cc = CPU_CORE(OBJECT(dev));
+    PowerNVCPUClass *pcc = POWERNV_CPU_GET_CLASS(OBJECT(dev));
+    const char *typename = object_class_get_name(pcc->cpu_oc);
+    size_t size = object_type_get_instance_size(typename);
+    Error *local_err = NULL;
+    void *obj;
+    int i, j;
+
+
+    pc->threads = g_malloc0(size * cc->nr_threads);
+    for (i = 0; i < cc->nr_threads; i++) {
+        char id[32];
+        CPUState *cs;
+
+        obj = pc->threads + i * size;
+
+        object_initialize(obj, size, typename);
+        cs = CPU(obj);
+        cs->cpu_index = cc->core_id + i;
+        snprintf(id, sizeof(id), "thread[%d]", i);
+        object_property_add_child(OBJECT(pc), id, obj, &local_err);
+        if (local_err) {
+            goto err;
+        }
+        object_unref(obj);
+    }
+
+    for (j = 0; j < cc->nr_threads; j++) {
+        obj = pc->threads + j * size;
+
+        powernv_cpu_core_realize_child(obj, &local_err);
+        if (local_err) {
+            goto err;
+        }
+    }
+    return;
+
+err:
+    while (--i >= 0) {
+        obj = pc->threads + i * size;
+        object_unparent(obj);
+    }
+    g_free(pc->threads);
+    error_propagate(errp, local_err);
+}
+
+/*
+ * Grow this list or merge with SPAPRCoreInfo which is very similar
+ */
+static const char *powernv_core_models[] = { "POWER8" };
+
+static void powernv_cpu_core_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    PowerNVCPUClass *pcc = POWERNV_CPU_CLASS(oc);
+
+    dc->realize = powernv_cpu_core_realize;
+    pcc->cpu_oc = cpu_class_by_name(TYPE_POWERPC_CPU, data);
+}
+
+static const TypeInfo powernv_cpu_core_info = {
+    .name           = TYPE_POWERNV_CPU_CORE,
+    .parent         = TYPE_CPU_CORE,
+    .instance_size  = sizeof(PowerNVCPUCore),
+    .class_size     = sizeof(PowerNVCPUClass),
+    .abstract       = true,
+};
+
+static void powernv_cpu_core_register_types(void)
+{
+    int i ;
+
+    type_register_static(&powernv_cpu_core_info);
+    for (i = 0; i < ARRAY_SIZE(powernv_core_models); ++i) {
+        TypeInfo ti = {
+            .parent = TYPE_POWERNV_CPU_CORE,
+            .instance_size = sizeof(PowerNVCPUCore),
+            .class_init = powernv_cpu_core_class_init,
+            .class_data = (void *) powernv_core_models[i],
+        };
+        ti.name = powernv_cpu_core_typename(powernv_core_models[i]);
+        type_register(&ti);
+        g_free((void *)ti.name);
+    }
+}
+
+type_init(powernv_cpu_core_register_types)
+
+char *powernv_cpu_core_typename(const char *model)
+{
+    return g_strdup_printf("%s-" TYPE_POWERNV_CPU_CORE, model);
+}
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 6907dc9e5c3d..9eac4b34a9b0 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -31,6 +31,10 @@ typedef struct PnvChip {
 
     /*< public >*/
     uint32_t     chip_id;
+    uint32_t     num_cores;
+    char *cpu_model;
+
+    Object **cores;
 } PnvChip;
 
 #define TYPE_POWERNV_MACHINE      "powernv-machine"
@@ -43,9 +47,12 @@ typedef struct sPowerNVMachineState {
 
     uint32_t initrd_base;
     long initrd_size;
+    hwaddr fdt_addr;
 
     uint32_t  num_chips;
     PnvChip   *chips;
 } sPowerNVMachineState;
 
+#define PNV_TIMEBASE_FREQ           512000000ULL
+
 #endif /* _PPC_PNV_H */
diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
new file mode 100644
index 000000000000..88a09b0fd1c6
--- /dev/null
+++ b/include/hw/ppc/pnv_core.h
@@ -0,0 +1,47 @@
+/*
+ * QEMU PowerPC PowerNV CPU model
+ *
+ * Copyright (c) 2016 IBM Corporation.
+ *
+ * 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 _PPC_PNV_CORE_H
+#define _PPC_PNV_CORE_H
+
+#include "hw/cpu/core.h"
+
+#define TYPE_POWERNV_CPU_CORE "powernv-cpu-core"
+#define POWERNV_CPU_CORE(obj) \
+    OBJECT_CHECK(PowerNVCPUCore, (obj), TYPE_POWERNV_CPU_CORE)
+#define POWERNV_CPU_CLASS(klass) \
+     OBJECT_CLASS_CHECK(PowerNVCPUClass, (klass), TYPE_POWERNV_CPU_CORE)
+#define POWERNV_CPU_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(PowerNVCPUClass, (obj), TYPE_POWERNV_CPU_CORE)
+
+typedef struct PowerNVCPUCore {
+    /*< private >*/
+    CPUCore parent_obj;
+
+    /*< public >*/
+    void *threads;
+} PowerNVCPUCore;
+
+typedef struct PowerNVCPUClass {
+    DeviceClass parent_class;
+    ObjectClass *cpu_oc;
+}   PowerNVCPUClass;
+
+extern char *powernv_cpu_core_typename(const char *model);
+
+#endif /* _PPC_PNV_CORE_H */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[David Gibson]

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

On Fri, Aug 05, 2016 at 11:15:37AM +0200, Cédric Le Goater wrote:
> This is largy inspired by sPAPRCPUCore with some simplification, no
> hotplug for instance. But the differences are small and the objects
> could possibly be merged.
> 
> A set of PowerNVCPUCore objects is added to the PnvChip and the device
> tree is populated looping on these cores. Core ids in the device tree
> are still a little fuzy. To be checked.

So, it's not immediately obvious to me if you want an actual core
object.  You could potentially create the actual vcpu objects directly
from the chip object.  That assumes that any hotplug will only be at
chip granularity, not core granularity, but I'm guessing that's the
case anyway.

That said, if having the intermediate core object is helpful, you're
certainly free to have it.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/Makefile.objs      |   2 +-
>  hw/ppc/pnv.c              | 160 ++++++++++++++++++++++++++++++++++++++++++-
>  hw/ppc/pnv_core.c         | 171 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h      |   7 ++
>  include/hw/ppc/pnv_core.h |  47 +++++++++++++
>  5 files changed, 383 insertions(+), 4 deletions(-)
>  create mode 100644 hw/ppc/pnv_core.c
>  create mode 100644 include/hw/ppc/pnv_core.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 8105db7d5600..f8c7d1db9ade 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>  # IBM PowerNV
> -obj-$(CONFIG_POWERNV) += pnv.o
> +obj-$(CONFIG_POWERNV) += pnv.o pnv_core.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index a680780e9dea..1219493c7218 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -35,6 +35,7 @@
>  #include "hw/ppc/fdt.h"
>  #include "hw/ppc/ppc.h"
>  #include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_core.h"
>  #include "hw/loader.h"
>  #include "exec/address-spaces.h"
>  #include "qemu/cutils.h"
> @@ -112,6 +113,114 @@ static int powernv_populate_memory(void *fdt)
>      return 0;
>  }
>  
> +static void powernv_create_core_node(void *fdt, CPUState *cs, uint32_t chip_id)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    int smt_threads = ppc_get_compat_smt_threads(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> +    uint32_t servers_prop[smt_threads];
> +    uint32_t gservers_prop[smt_threads * 2];
> +    int i, index = ppc_get_vcpu_dt_id(cpu);
> +    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
> +                       0xffffffff, 0xffffffff};
> +    uint32_t tbfreq = PNV_TIMEBASE_FREQ;
> +    uint32_t cpufreq = 1000000000;
> +    uint32_t page_sizes_prop[64];
> +    size_t page_sizes_prop_size;
> +    char *nodename;
> +
> +    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")));

The handling of dt_id is going to collide with cleanups I want to do
in this area (for spapr and the ppc cpu core).  Not sure there's a lot
you can do to avoid that at this stage.

> +    _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 {
> +        error_report("Warning: Unknown L1 dcache size for cpu");
> +    }
> +    if (pcc->l1_icache_size) {
> +        _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_size)));
> +    } else {
> +        error_report("Warning: Unknown L1 icache size for cpu");
> +    }
> +
> +    _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 = ppc_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", chip_id)));
> +
> +    if (cpu->cpu_version) {
> +        _FDT((fdt_property_cell(fdt, "cpu-version", cpu->cpu_version)));
> +    }
> +
> +    /* Build interrupt servers and gservers properties */
> +    for (i = 0; i < smt_threads; i++) {
> +        servers_prop[i] = cpu_to_be32(index + i);
> +        /* Hack, direct the group queues back to cpu 0 */
> +        gservers_prop[i * 2] = cpu_to_be32(index + i);
> +        gservers_prop[i * 2 + 1] = 0;
> +    }
> +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-server#s",
> +                       servers_prop, sizeof(servers_prop))));
> +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s",
> +                       gservers_prop, sizeof(gservers_prop))));
> +
> +    _FDT((fdt_end_node(fdt)));
> +}
> +
>  static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>                                  const char *kernel_cmdline)
>  {
> @@ -120,6 +229,7 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>      uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
>      char *buf;
>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
> +    int i;
>  
>      fdt = g_malloc0(FDT_MAX_SIZE);
>      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
> @@ -156,6 +266,23 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>      /* Memory */
>      _FDT((powernv_populate_memory(fdt)));
>  
> +    /* cpus */
> +    _FDT((fdt_begin_node(fdt, "cpus")));
> +    _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
> +    _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
> +
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        PnvChip *chip = &pnv->chips[i];
> +        int j;
> +
> +        for (j = 0; j < chip->num_cores; j++) {
> +            PowerNVCPUCore *pc = POWERNV_CPU_CORE(chip->cores[j]);
> +            CPUState *cs = CPU(DEVICE(pc->threads));
> +            powernv_create_core_node(fdt, cs, chip->chip_id);

I think it would be nicer to define the fdt creation function in terms
of the core object, and have it retrieve the representative thread itself.

> +        }
> +    }
> +    _FDT((fdt_end_node(fdt)));
> +
>      _FDT((fdt_end_node(fdt))); /* close root node */
>      _FDT((fdt_finish(fdt)));
>  
> @@ -168,11 +295,13 @@ static void ppc_powernv_reset(void)
>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>      void *fdt;
>  
> +    pnv->fdt_addr = FDT_ADDR;
> +
>      qemu_devices_reset();
>  
>      fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
>  
> -    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
> +    cpu_physical_memory_write(pnv->fdt_addr, fdt, fdt_totalsize(fdt));

These look like not really related changes, that should be maybe
folded into 1/3.

>  }
>  
>  static void ppc_powernv_init(MachineState *machine)
> @@ -252,6 +381,10 @@ static void ppc_powernv_init(MachineState *machine)
>  
>          object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
>          object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
> +        object_property_set_int(OBJECT(chip), smp_cpus, "num-cores",
> +                                &error_abort);
> +        object_property_set_str(OBJECT(chip), machine->cpu_model, "cpu-model",
> +                                &error_abort);

So various drafts of the spapr cores had a cpu-model parameter, but we
eventually rejected it in favour of having different core types
corresponding to the different, well, core types.  Unless there's a
compelling reason otherwise, I think it would be nicer to do the same
for the pnvchip objects - a p8pnvchip object will always create p8
cores and so forth.

>          object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
>      }
>  }
> @@ -292,14 +425,35 @@ static const TypeInfo powernv_machine_2_8_info = {
>      .class_init    = powernv_machine_2_8_class_init,
>  };
>  
> -
>  static void pnv_chip_realize(DeviceState *dev, Error **errp)
>  {
> -    ;
> +    int i;
> +    PnvChip *chip = PNV_CHIP(dev);
> +    char *typename = powernv_cpu_core_typename(chip->cpu_model);
> +
> +    if (!object_class_by_name(typename)) {
> +        error_setg(errp, "Unable to find PowerNV CPU Core definition");
> +        return;
> +    }
> +
> +    chip->cores = g_new0(Object *, chip->num_cores);
> +    for (i = 0; i < chip->num_cores; i++) {
> +        int core_id = i * smp_threads;
> +        chip->cores[i] = object_new(typename);
> +        object_property_set_int(chip->cores[i], smp_threads, "nr-threads",
> +                                &error_fatal);
> +        object_property_set_int(chip->cores[i], core_id, CPU_CORE_PROP_CORE_ID,
> +                                &error_fatal);
> +        object_property_set_bool(chip->cores[i], true, "realized",
> +                                 &error_fatal);
> +    }
> +    g_free(typename);
>  }
>  
>  static Property pnv_chip_properties[] = {
>      DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
> +    DEFINE_PROP_STRING("cpu-model", PnvChip, cpu_model),
> +    DEFINE_PROP_UINT32("num-cores", PnvChip, num_cores, 1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> new file mode 100644
> index 000000000000..1e36709db993
> --- /dev/null
> +++ b/hw/ppc/pnv_core.c
> @@ -0,0 +1,171 @@
> +/*
> + * QEMU PowerPC PowerNV CPU model
> + *
> + * Copyright (c) IBM Corporation.
> + *
> + * 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 "qemu/osdep.h"
> +#include "sysemu/sysemu.h"
> +#include "qapi/error.h"
> +#include "target-ppc/cpu.h"
> +#include "hw/ppc/ppc.h"
> +#include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_core.h"
> +
> +static void powernv_cpu_reset(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUState *cs = CPU(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> +
> +    cpu_reset(cs);
> +
> +    env->spr[SPR_PIR] = ppc_get_vcpu_dt_id(cpu);

Is the PIR writable?  If not it might be better to move this to init
rather than reset time.

> +    env->spr[SPR_HIOR] = 0;
> +    env->gpr[3] = pnv->fdt_addr;
> +    env->nip = 0x10;
> +    env->msr |= MSR_HVB;
> +}
> +
> +static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    /* Set time-base frequency to 512 MHz */
> +    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> +
> +    /* MSR[IP] doesn't exist nowadays */
> +    env->msr_mask &= ~(1 << 6);
> +
> +    qemu_register_reset(powernv_cpu_reset, cpu);
> +    powernv_cpu_reset(cpu);
> +}
> +
> +static void powernv_cpu_core_realize_child(Object *child, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    CPUState *cs = CPU(child);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    object_property_set_bool(child, true, "realized", &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    powernv_cpu_init(cpu, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +}
> +
> +static void powernv_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> +    PowerNVCPUCore *pc = POWERNV_CPU_CORE(OBJECT(dev));
> +    CPUCore *cc = CPU_CORE(OBJECT(dev));
> +    PowerNVCPUClass *pcc = POWERNV_CPU_GET_CLASS(OBJECT(dev));
> +    const char *typename = object_class_get_name(pcc->cpu_oc);
> +    size_t size = object_type_get_instance_size(typename);
> +    Error *local_err = NULL;
> +    void *obj;
> +    int i, j;
> +
> +
> +    pc->threads = g_malloc0(size * cc->nr_threads);
> +    for (i = 0; i < cc->nr_threads; i++) {
> +        char id[32];
> +        CPUState *cs;
> +
> +        obj = pc->threads + i * size;
> +
> +        object_initialize(obj, size, typename);
> +        cs = CPU(obj);
> +        cs->cpu_index = cc->core_id + i;
> +        snprintf(id, sizeof(id), "thread[%d]", i);
> +        object_property_add_child(OBJECT(pc), id, obj, &local_err);
> +        if (local_err) {
> +            goto err;
> +        }
> +        object_unref(obj);
> +    }
> +
> +    for (j = 0; j < cc->nr_threads; j++) {
> +        obj = pc->threads + j * size;
> +
> +        powernv_cpu_core_realize_child(obj, &local_err);
> +        if (local_err) {
> +            goto err;
> +        }
> +    }
> +    return;
> +
> +err:
> +    while (--i >= 0) {
> +        obj = pc->threads + i * size;
> +        object_unparent(obj);
> +    }
> +    g_free(pc->threads);
> +    error_propagate(errp, local_err);
> +}
> +
> +/*
> + * Grow this list or merge with SPAPRCoreInfo which is very similar
> + */
> +static const char *powernv_core_models[] = { "POWER8" };
> +
> +static void powernv_cpu_core_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    PowerNVCPUClass *pcc = POWERNV_CPU_CLASS(oc);
> +
> +    dc->realize = powernv_cpu_core_realize;
> +    pcc->cpu_oc = cpu_class_by_name(TYPE_POWERPC_CPU, data);
> +}
> +
> +static const TypeInfo powernv_cpu_core_info = {
> +    .name           = TYPE_POWERNV_CPU_CORE,
> +    .parent         = TYPE_CPU_CORE,
> +    .instance_size  = sizeof(PowerNVCPUCore),
> +    .class_size     = sizeof(PowerNVCPUClass),
> +    .abstract       = true,
> +};
> +
> +static void powernv_cpu_core_register_types(void)
> +{
> +    int i ;
> +
> +    type_register_static(&powernv_cpu_core_info);
> +    for (i = 0; i < ARRAY_SIZE(powernv_core_models); ++i) {
> +        TypeInfo ti = {
> +            .parent = TYPE_POWERNV_CPU_CORE,
> +            .instance_size = sizeof(PowerNVCPUCore),
> +            .class_init = powernv_cpu_core_class_init,
> +            .class_data = (void *) powernv_core_models[i],
> +        };
> +        ti.name = powernv_cpu_core_typename(powernv_core_models[i]);
> +        type_register(&ti);
> +        g_free((void *)ti.name);
> +    }
> +}
> +
> +type_init(powernv_cpu_core_register_types)
> +
> +char *powernv_cpu_core_typename(const char *model)
> +{
> +    return g_strdup_printf("%s-" TYPE_POWERNV_CPU_CORE, model);
> +}
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 6907dc9e5c3d..9eac4b34a9b0 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -31,6 +31,10 @@ typedef struct PnvChip {
>  
>      /*< public >*/
>      uint32_t     chip_id;
> +    uint32_t     num_cores;
> +    char *cpu_model;
> +
> +    Object **cores;

So, unlike the chips within the machine, the cores within the chip
should probably be done like the threads within the core - a single
array with object_initialize() rather than an array of pointers.  AIUI
this is because having a (potentially) user instantiable object
automatically object_new() other things breaks QOM lifetime rules.  I
can't say I understand this point terribly well though, but i know it
was something we went several rounds on during the spapr core work
though.

>  } PnvChip;
>  
>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
> @@ -43,9 +47,12 @@ typedef struct sPowerNVMachineState {
>  
>      uint32_t initrd_base;
>      long initrd_size;
> +    hwaddr fdt_addr;
>  
>      uint32_t  num_chips;
>      PnvChip   *chips;
>  } sPowerNVMachineState;
>  
> +#define PNV_TIMEBASE_FREQ           512000000ULL
> +
>  #endif /* _PPC_PNV_H */
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> new file mode 100644
> index 000000000000..88a09b0fd1c6
> --- /dev/null
> +++ b/include/hw/ppc/pnv_core.h
> @@ -0,0 +1,47 @@
> +/*
> + * QEMU PowerPC PowerNV CPU model
> + *
> + * Copyright (c) 2016 IBM Corporation.
> + *
> + * 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 _PPC_PNV_CORE_H
> +#define _PPC_PNV_CORE_H
> +
> +#include "hw/cpu/core.h"
> +
> +#define TYPE_POWERNV_CPU_CORE "powernv-cpu-core"
> +#define POWERNV_CPU_CORE(obj) \
> +    OBJECT_CHECK(PowerNVCPUCore, (obj), TYPE_POWERNV_CPU_CORE)
> +#define POWERNV_CPU_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(PowerNVCPUClass, (klass), TYPE_POWERNV_CPU_CORE)
> +#define POWERNV_CPU_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(PowerNVCPUClass, (obj), TYPE_POWERNV_CPU_CORE)
> +
> +typedef struct PowerNVCPUCore {
> +    /*< private >*/
> +    CPUCore parent_obj;
> +
> +    /*< public >*/
> +    void *threads;
> +} PowerNVCPUCore;
> +
> +typedef struct PowerNVCPUClass {
> +    DeviceClass parent_class;
> +    ObjectClass *cpu_oc;
> +}   PowerNVCPUClass;
> +
> +extern char *powernv_cpu_core_typename(const char *model);
> +
> +#endif /* _PPC_PNV_CORE_H */

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[Benjamin Herrenschmidt]

On Tue, 2016-08-16 at 12:39 +1000, David Gibson wrote:
> On Fri, Aug 05, 2016 at 11:15:37AM +0200, Cédric Le Goater wrote:
> > 
> > This is largy inspired by sPAPRCPUCore with some simplification, no
> > hotplug for instance. But the differences are small and the objects
> > could possibly be merged.
> > 
> > A set of PowerNVCPUCore objects is added to the PnvChip and the device
> > tree is populated looping on these cores. Core ids in the device tree
> > are still a little fuzy. To be checked.

What about P9 where cores are in pairs to form EX and in pairs of EX
(ie, quads) to form EPs ?

> > So, it's not immediately obvious to me if you want an actual core
> object.  You could potentially create the actual vcpu objects directly
> from the chip object.  That assumes that any hotplug will only be at
> chip granularity, not core granularity, but I'm guessing that's the
> case anyway.

Well we want to add some of the core XSCOMs so it makes sense to have
a core object that is a XSCOM device

> > That said, if having the intermediate core object is helpful, you're
> certainly free to have it.
> 
> > 
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  hw/ppc/Makefile.objs      |   2 +-
> >  hw/ppc/pnv.c              | 160 ++++++++++++++++++++++++++++++++++++++++++-
> >  hw/ppc/pnv_core.c         | 171 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/pnv.h      |   7 ++
> >  include/hw/ppc/pnv_core.h |  47 +++++++++++++
> >  5 files changed, 383 insertions(+), 4 deletions(-)
> >  create mode 100644 hw/ppc/pnv_core.c
> >  create mode 100644 include/hw/ppc/pnv_core.h
> > 
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index 8105db7d5600..f8c7d1db9ade 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> >  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >  # IBM PowerNV
> > -obj-$(CONFIG_POWERNV) += pnv.o
> > +obj-$(CONFIG_POWERNV) += pnv.o pnv_core.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >  obj-y += spapr_pci_vfio.o
> >  endif
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index a680780e9dea..1219493c7218 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -35,6 +35,7 @@
> >  #include "hw/ppc/fdt.h"
> >  #include "hw/ppc/ppc.h"
> >  #include "hw/ppc/pnv.h"
> > +#include "hw/ppc/pnv_core.h"
> >  #include "hw/loader.h"
> >  #include "exec/address-spaces.h"
> >  #include "qemu/cutils.h"
> > @@ -112,6 +113,114 @@ static int powernv_populate_memory(void *fdt)
> >      return 0;
> >  }
> >  
> > +static void powernv_create_core_node(void *fdt, CPUState *cs, uint32_t chip_id)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    int smt_threads = ppc_get_compat_smt_threads(cpu);
> > +    CPUPPCState *env = &cpu->env;
> > +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> > +    uint32_t servers_prop[smt_threads];
> > +    uint32_t gservers_prop[smt_threads * 2];
> > +    int i, index = ppc_get_vcpu_dt_id(cpu);
> > +    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
> > +                       0xffffffff, 0xffffffff};
> > +    uint32_t tbfreq = PNV_TIMEBASE_FREQ;
> > +    uint32_t cpufreq = 1000000000;
> > +    uint32_t page_sizes_prop[64];
> > +    size_t page_sizes_prop_size;
> > +    char *nodename;
> > +
> > +    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")));
> 
> The handling of dt_id is going to collide with cleanups I want to do
> in this area (for spapr and the ppc cpu core).  Not sure there's a lot
> you can do to avoid that at this stage.
> 
> > 
> > +    _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 {
> > +        error_report("Warning: Unknown L1 dcache size for cpu");
> > +    }
> > +    if (pcc->l1_icache_size) {
> > +        _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_size)));
> > +    } else {
> > +        error_report("Warning: Unknown L1 icache size for cpu");
> > +    }
> > +
> > +    _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 = ppc_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", chip_id)));
> > +
> > +    if (cpu->cpu_version) {
> > +        _FDT((fdt_property_cell(fdt, "cpu-version", cpu->cpu_version)));
> > +    }
> > +
> > +    /* Build interrupt servers and gservers properties */
> > +    for (i = 0; i < smt_threads; i++) {
> > +        servers_prop[i] = cpu_to_be32(index + i);
> > +        /* Hack, direct the group queues back to cpu 0 */
> > +        gservers_prop[i * 2] = cpu_to_be32(index + i);
> > +        gservers_prop[i * 2 + 1] = 0;
> > +    }
> > +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-server#s",
> > +                       servers_prop, sizeof(servers_prop))));
> > +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s",
> > +                       gservers_prop, sizeof(gservers_prop))));
> > +
> > +    _FDT((fdt_end_node(fdt)));
> > +}
> > +
> >  static void *powernv_create_fdt(sPowerNVMachineState *pnv,
> >                                  const char *kernel_cmdline)
> >  {
> > @@ -120,6 +229,7 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
> >      uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
> >      char *buf;
> >      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
> > +    int i;
> >  
> >      fdt = g_malloc0(FDT_MAX_SIZE);
> >      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
> > @@ -156,6 +266,23 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
> >      /* Memory */
> >      _FDT((powernv_populate_memory(fdt)));
> >  
> > +    /* cpus */
> > +    _FDT((fdt_begin_node(fdt, "cpus")));
> > +    _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
> > +    _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
> > +
> > +    for (i = 0; i < pnv->num_chips; i++) {
> > +        PnvChip *chip = &pnv->chips[i];
> > +        int j;
> > +
> > +        for (j = 0; j < chip->num_cores; j++) {
> > +            PowerNVCPUCore *pc = POWERNV_CPU_CORE(chip->cores[j]);
> > +            CPUState *cs = CPU(DEVICE(pc->threads));
> > +            powernv_create_core_node(fdt, cs, chip->chip_id);
> 
> I think it would be nicer to define the fdt creation function in terms
> of the core object, and have it retrieve the representative thread itself.
> 
> > 
> > +        }
> > +    }
> > +    _FDT((fdt_end_node(fdt)));
> > +
> >      _FDT((fdt_end_node(fdt))); /* close root node */
> >      _FDT((fdt_finish(fdt)));
> >  
> > @@ -168,11 +295,13 @@ static void ppc_powernv_reset(void)
> >      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> >      void *fdt;
> >  
> > +    pnv->fdt_addr = FDT_ADDR;
> > +
> >      qemu_devices_reset();
> >  
> >      fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
> >  
> > -    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
> > +    cpu_physical_memory_write(pnv->fdt_addr, fdt, fdt_totalsize(fdt));
> 
> These look like not really related changes, that should be maybe
> folded into 1/3.
> 
> > 
> >  }
> >  
> >  static void ppc_powernv_init(MachineState *machine)
> > @@ -252,6 +381,10 @@ static void ppc_powernv_init(MachineState *machine)
> >  
> >          object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
> >          object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
> > +        object_property_set_int(OBJECT(chip), smp_cpus, "num-cores",
> > +                                &error_abort);
> > +        object_property_set_str(OBJECT(chip), machine->cpu_model, "cpu-model",
> > +                                &error_abort);
> 
> So various drafts of the spapr cores had a cpu-model parameter, but we
> eventually rejected it in favour of having different core types
> corresponding to the different, well, core types.  Unless there's a
> compelling reason otherwise, I think it would be nicer to do the same
> for the pnvchip objects - a p8pnvchip object will always create p8
> cores and so forth.
> 
> > 
> >          object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
> >      }
> >  }
> > @@ -292,14 +425,35 @@ static const TypeInfo powernv_machine_2_8_info = {
> >      .class_init    = powernv_machine_2_8_class_init,
> >  };
> >  
> > -
> >  static void pnv_chip_realize(DeviceState *dev, Error **errp)
> >  {
> > -    ;
> > +    int i;
> > +    PnvChip *chip = PNV_CHIP(dev);
> > +    char *typename = powernv_cpu_core_typename(chip->cpu_model);
> > +
> > +    if (!object_class_by_name(typename)) {
> > +        error_setg(errp, "Unable to find PowerNV CPU Core definition");
> > +        return;
> > +    }
> > +
> > +    chip->cores = g_new0(Object *, chip->num_cores);
> > +    for (i = 0; i < chip->num_cores; i++) {
> > +        int core_id = i * smp_threads;
> > +        chip->cores[i] = object_new(typename);
> > +        object_property_set_int(chip->cores[i], smp_threads, "nr-threads",
> > +                                &error_fatal);
> > +        object_property_set_int(chip->cores[i], core_id, CPU_CORE_PROP_CORE_ID,
> > +                                &error_fatal);
> > +        object_property_set_bool(chip->cores[i], true, "realized",
> > +                                 &error_fatal);
> > +    }
> > +    g_free(typename);
> >  }
> >  
> >  static Property pnv_chip_properties[] = {
> >      DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
> > +    DEFINE_PROP_STRING("cpu-model", PnvChip, cpu_model),
> > +    DEFINE_PROP_UINT32("num-cores", PnvChip, num_cores, 1),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > new file mode 100644
> > index 000000000000..1e36709db993
> > --- /dev/null
> > +++ b/hw/ppc/pnv_core.c
> > @@ -0,0 +1,171 @@
> > +/*
> > + * QEMU PowerPC PowerNV CPU model
> > + *
> > + * Copyright (c) IBM Corporation.
> > + *
> > + * 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 "qemu/osdep.h"
> > +#include "sysemu/sysemu.h"
> > +#include "qapi/error.h"
> > +#include "target-ppc/cpu.h"
> > +#include "hw/ppc/ppc.h"
> > +#include "hw/ppc/pnv.h"
> > +#include "hw/ppc/pnv_core.h"
> > +
> > +static void powernv_cpu_reset(void *opaque)
> > +{
> > +    PowerPCCPU *cpu = opaque;
> > +    CPUState *cs = CPU(cpu);
> > +    CPUPPCState *env = &cpu->env;
> > +    MachineState *machine = MACHINE(qdev_get_machine());
> > +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> > +
> > +    cpu_reset(cs);
> > +
> > +    env->spr[SPR_PIR] = ppc_get_vcpu_dt_id(cpu);
> 
> Is the PIR writable?  If not it might be better to move this to init
> rather than reset time.
> 
> > 
> > +    env->spr[SPR_HIOR] = 0;
> > +    env->gpr[3] = pnv->fdt_addr;
> > +    env->nip = 0x10;
> > +    env->msr |= MSR_HVB;
> > +}
> > +
> > +static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
> > +{
> > +    CPUPPCState *env = &cpu->env;
> > +
> > +    /* Set time-base frequency to 512 MHz */
> > +    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> > +
> > +    /* MSR[IP] doesn't exist nowadays */
> > +    env->msr_mask &= ~(1 << 6);
> > +
> > +    qemu_register_reset(powernv_cpu_reset, cpu);
> > +    powernv_cpu_reset(cpu);
> > +}
> > +
> > +static void powernv_cpu_core_realize_child(Object *child, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    CPUState *cs = CPU(child);
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +    object_property_set_bool(child, true, "realized", &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    powernv_cpu_init(cpu, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +}
> > +
> > +static void powernv_cpu_core_realize(DeviceState *dev, Error **errp)
> > +{
> > +    PowerNVCPUCore *pc = POWERNV_CPU_CORE(OBJECT(dev));
> > +    CPUCore *cc = CPU_CORE(OBJECT(dev));
> > +    PowerNVCPUClass *pcc = POWERNV_CPU_GET_CLASS(OBJECT(dev));
> > +    const char *typename = object_class_get_name(pcc->cpu_oc);
> > +    size_t size = object_type_get_instance_size(typename);
> > +    Error *local_err = NULL;
> > +    void *obj;
> > +    int i, j;
> > +
> > +
> > +    pc->threads = g_malloc0(size * cc->nr_threads);
> > +    for (i = 0; i < cc->nr_threads; i++) {
> > +        char id[32];
> > +        CPUState *cs;
> > +
> > +        obj = pc->threads + i * size;
> > +
> > +        object_initialize(obj, size, typename);
> > +        cs = CPU(obj);
> > +        cs->cpu_index = cc->core_id + i;
> > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > +        object_property_add_child(OBJECT(pc), id, obj, &local_err);
> > +        if (local_err) {
> > +            goto err;
> > +        }
> > +        object_unref(obj);
> > +    }
> > +
> > +    for (j = 0; j < cc->nr_threads; j++) {
> > +        obj = pc->threads + j * size;
> > +
> > +        powernv_cpu_core_realize_child(obj, &local_err);
> > +        if (local_err) {
> > +            goto err;
> > +        }
> > +    }
> > +    return;
> > +
> > +err:
> > +    while (--i >= 0) {
> > +        obj = pc->threads + i * size;
> > +        object_unparent(obj);
> > +    }
> > +    g_free(pc->threads);
> > +    error_propagate(errp, local_err);
> > +}
> > +
> > +/*
> > + * Grow this list or merge with SPAPRCoreInfo which is very similar
> > + */
> > +static const char *powernv_core_models[] = { "POWER8" };
> > +
> > +static void powernv_cpu_core_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +    PowerNVCPUClass *pcc = POWERNV_CPU_CLASS(oc);
> > +
> > +    dc->realize = powernv_cpu_core_realize;
> > +    pcc->cpu_oc = cpu_class_by_name(TYPE_POWERPC_CPU, data);
> > +}
> > +
> > +static const TypeInfo powernv_cpu_core_info = {
> > +    .name           = TYPE_POWERNV_CPU_CORE,
> > +    .parent         = TYPE_CPU_CORE,
> > +    .instance_size  = sizeof(PowerNVCPUCore),
> > +    .class_size     = sizeof(PowerNVCPUClass),
> > +    .abstract       = true,
> > +};
> > +
> > +static void powernv_cpu_core_register_types(void)
> > +{
> > +    int i ;
> > +
> > +    type_register_static(&powernv_cpu_core_info);
> > +    for (i = 0; i < ARRAY_SIZE(powernv_core_models); ++i) {
> > +        TypeInfo ti = {
> > +            .parent = TYPE_POWERNV_CPU_CORE,
> > +            .instance_size = sizeof(PowerNVCPUCore),
> > +            .class_init = powernv_cpu_core_class_init,
> > +            .class_data = (void *) powernv_core_models[i],
> > +        };
> > +        ti.name = powernv_cpu_core_typename(powernv_core_models[i]);
> > +        type_register(&ti);
> > +        g_free((void *)ti.name);
> > +    }
> > +}
> > +
> > +type_init(powernv_cpu_core_register_types)
> > +
> > +char *powernv_cpu_core_typename(const char *model)
> > +{
> > +    return g_strdup_printf("%s-" TYPE_POWERNV_CPU_CORE, model);
> > +}
> > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> > index 6907dc9e5c3d..9eac4b34a9b0 100644
> > --- a/include/hw/ppc/pnv.h
> > +++ b/include/hw/ppc/pnv.h
> > @@ -31,6 +31,10 @@ typedef struct PnvChip {
> >  
> >      /*< public >*/
> >      uint32_t     chip_id;
> > +    uint32_t     num_cores;
> > +    char *cpu_model;
> > +
> > +    Object **cores;
> 
> So, unlike the chips within the machine, the cores within the chip
> should probably be done like the threads within the core - a single
> array with object_initialize() rather than an array of pointers.  AIUI
> this is because having a (potentially) user instantiable object
> automatically object_new() other things breaks QOM lifetime rules.  I
> can't say I understand this point terribly well though, but i know it
> was something we went several rounds on during the spapr core work
> though.
> 
> > 
> >  } PnvChip;
> >  
> >  #define TYPE_POWERNV_MACHINE      "powernv-machine"
> > @@ -43,9 +47,12 @@ typedef struct sPowerNVMachineState {
> >  
> >      uint32_t initrd_base;
> >      long initrd_size;
> > +    hwaddr fdt_addr;
> >  
> >      uint32_t  num_chips;
> >      PnvChip   *chips;
> >  } sPowerNVMachineState;
> >  
> > +#define PNV_TIMEBASE_FREQ           512000000ULL
> > +
> >  #endif /* _PPC_PNV_H */
> > diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> > new file mode 100644
> > index 000000000000..88a09b0fd1c6
> > --- /dev/null
> > +++ b/include/hw/ppc/pnv_core.h
> > @@ -0,0 +1,47 @@
> > +/*
> > + * QEMU PowerPC PowerNV CPU model
> > + *
> > + * Copyright (c) 2016 IBM Corporation.
> > + *
> > + * 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 _PPC_PNV_CORE_H
> > +#define _PPC_PNV_CORE_H
> > +
> > +#include "hw/cpu/core.h"
> > +
> > +#define TYPE_POWERNV_CPU_CORE "powernv-cpu-core"
> > +#define POWERNV_CPU_CORE(obj) \
> > +    OBJECT_CHECK(PowerNVCPUCore, (obj), TYPE_POWERNV_CPU_CORE)
> > +#define POWERNV_CPU_CLASS(klass) \
> > +     OBJECT_CLASS_CHECK(PowerNVCPUClass, (klass), TYPE_POWERNV_CPU_CORE)
> > +#define POWERNV_CPU_GET_CLASS(obj) \
> > +     OBJECT_GET_CLASS(PowerNVCPUClass, (obj), TYPE_POWERNV_CPU_CORE)
> > +
> > +typedef struct PowerNVCPUCore {
> > +    /*< private >*/
> > +    CPUCore parent_obj;
> > +
> > +    /*< public >*/
> > +    void *threads;
> > +} PowerNVCPUCore;
> > +
> > +typedef struct PowerNVCPUClass {
> > +    DeviceClass parent_class;
> > +    ObjectClass *cpu_oc;
> > +}   PowerNVCPUClass;
> > +
> > +extern char *powernv_cpu_core_typename(const char *model);
> > +
> > +#endif /* _PPC_PNV_CORE_H */
> 

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[Cédric Le Goater]

On 08/16/2016 07:02 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2016-08-16 at 12:39 +1000, David Gibson wrote:
>> On Fri, Aug 05, 2016 at 11:15:37AM +0200, Cédric Le Goater wrote:
>>>
>>> This is largy inspired by sPAPRCPUCore with some simplification, no
>>> hotplug for instance. But the differences are small and the objects
>>> could possibly be merged.
>>>
>>> A set of PowerNVCPUCore objects is added to the PnvChip and the device
>>> tree is populated looping on these cores. Core ids in the device tree
>>> are still a little fuzy. To be checked.
> 
> What about P9 where cores are in pairs to form EX and in pairs of EX
> (ie, quads) to form EPs ?

Sounds fun ! I need to dig in the P9 specs to have a better idea. I lack 
the knowledge right now.

>>> So, it's not immediately obvious to me if you want an actual core
>> object.  You could potentially create the actual vcpu objects directly
>> from the chip object.  That assumes that any hotplug will only be at
>> chip granularity, not core granularity, but I'm guessing that's the
>> case anyway.
> 
> Well we want to add some of the core XSCOMs so it makes sense to have
> a core object that is a XSCOM device

ok. I have will look into that. 

Thanks,

C. 

>>> That said, if having the intermediate core object is helpful, you're
>> certainly free to have it.
>>
>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  hw/ppc/Makefile.objs      |   2 +-
>>>  hw/ppc/pnv.c              | 160 ++++++++++++++++++++++++++++++++++++++++++-
>>>  hw/ppc/pnv_core.c         | 171 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/ppc/pnv.h      |   7 ++
>>>  include/hw/ppc/pnv_core.h |  47 +++++++++++++
>>>  5 files changed, 383 insertions(+), 4 deletions(-)
>>>  create mode 100644 hw/ppc/pnv_core.c
>>>  create mode 100644 include/hw/ppc/pnv_core.h
>>>
>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>> index 8105db7d5600..f8c7d1db9ade 100644
>>> --- a/hw/ppc/Makefile.objs
>>> +++ b/hw/ppc/Makefile.objs
>>> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>>>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>>>  # IBM PowerNV
>>> -obj-$(CONFIG_POWERNV) += pnv.o
>>> +obj-$(CONFIG_POWERNV) += pnv.o pnv_core.o
>>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>>  obj-y += spapr_pci_vfio.o
>>>  endif
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index a680780e9dea..1219493c7218 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -35,6 +35,7 @@
>>>  #include "hw/ppc/fdt.h"
>>>  #include "hw/ppc/ppc.h"
>>>  #include "hw/ppc/pnv.h"
>>> +#include "hw/ppc/pnv_core.h"
>>>  #include "hw/loader.h"
>>>  #include "exec/address-spaces.h"
>>>  #include "qemu/cutils.h"
>>> @@ -112,6 +113,114 @@ static int powernv_populate_memory(void *fdt)
>>>      return 0;
>>>  }
>>>  
>>> +static void powernv_create_core_node(void *fdt, CPUState *cs, uint32_t chip_id)
>>> +{
>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> +    int smt_threads = ppc_get_compat_smt_threads(cpu);
>>> +    CPUPPCState *env = &cpu->env;
>>> +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
>>> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
>>> +    uint32_t servers_prop[smt_threads];
>>> +    uint32_t gservers_prop[smt_threads * 2];
>>> +    int i, index = ppc_get_vcpu_dt_id(cpu);
>>> +    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
>>> +                       0xffffffff, 0xffffffff};
>>> +    uint32_t tbfreq = PNV_TIMEBASE_FREQ;
>>> +    uint32_t cpufreq = 1000000000;
>>> +    uint32_t page_sizes_prop[64];
>>> +    size_t page_sizes_prop_size;
>>> +    char *nodename;
>>> +
>>> +    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")));
>>
>> The handling of dt_id is going to collide with cleanups I want to do
>> in this area (for spapr and the ppc cpu core).  Not sure there's a lot
>> you can do to avoid that at this stage.
>>
>>>
>>> +    _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 {
>>> +        error_report("Warning: Unknown L1 dcache size for cpu");
>>> +    }
>>> +    if (pcc->l1_icache_size) {
>>> +        _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_size)));
>>> +    } else {
>>> +        error_report("Warning: Unknown L1 icache size for cpu");
>>> +    }
>>> +
>>> +    _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 = ppc_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", chip_id)));
>>> +
>>> +    if (cpu->cpu_version) {
>>> +        _FDT((fdt_property_cell(fdt, "cpu-version", cpu->cpu_version)));
>>> +    }
>>> +
>>> +    /* Build interrupt servers and gservers properties */
>>> +    for (i = 0; i < smt_threads; i++) {
>>> +        servers_prop[i] = cpu_to_be32(index + i);
>>> +        /* Hack, direct the group queues back to cpu 0 */
>>> +        gservers_prop[i * 2] = cpu_to_be32(index + i);
>>> +        gservers_prop[i * 2 + 1] = 0;
>>> +    }
>>> +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-server#s",
>>> +                       servers_prop, sizeof(servers_prop))));
>>> +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s",
>>> +                       gservers_prop, sizeof(gservers_prop))));
>>> +
>>> +    _FDT((fdt_end_node(fdt)));
>>> +}
>>> +
>>>  static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>>>                                  const char *kernel_cmdline)
>>>  {
>>> @@ -120,6 +229,7 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>>>      uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
>>>      char *buf;
>>>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
>>> +    int i;
>>>  
>>>      fdt = g_malloc0(FDT_MAX_SIZE);
>>>      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
>>> @@ -156,6 +266,23 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>>>      /* Memory */
>>>      _FDT((powernv_populate_memory(fdt)));
>>>  
>>> +    /* cpus */
>>> +    _FDT((fdt_begin_node(fdt, "cpus")));
>>> +    _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
>>> +    _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
>>> +
>>> +    for (i = 0; i < pnv->num_chips; i++) {
>>> +        PnvChip *chip = &pnv->chips[i];
>>> +        int j;
>>> +
>>> +        for (j = 0; j < chip->num_cores; j++) {
>>> +            PowerNVCPUCore *pc = POWERNV_CPU_CORE(chip->cores[j]);
>>> +            CPUState *cs = CPU(DEVICE(pc->threads));
>>> +            powernv_create_core_node(fdt, cs, chip->chip_id);
>>
>> I think it would be nicer to define the fdt creation function in terms
>> of the core object, and have it retrieve the representative thread itself.
>>
>>>
>>> +        }
>>> +    }
>>> +    _FDT((fdt_end_node(fdt)));
>>> +
>>>      _FDT((fdt_end_node(fdt))); /* close root node */
>>>      _FDT((fdt_finish(fdt)));
>>>  
>>> @@ -168,11 +295,13 @@ static void ppc_powernv_reset(void)
>>>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>>>      void *fdt;
>>>  
>>> +    pnv->fdt_addr = FDT_ADDR;
>>> +
>>>      qemu_devices_reset();
>>>  
>>>      fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
>>>  
>>> -    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
>>> +    cpu_physical_memory_write(pnv->fdt_addr, fdt, fdt_totalsize(fdt));
>>
>> These look like not really related changes, that should be maybe
>> folded into 1/3.
>>
>>>
>>>  }
>>>  
>>>  static void ppc_powernv_init(MachineState *machine)
>>> @@ -252,6 +381,10 @@ static void ppc_powernv_init(MachineState *machine)
>>>  
>>>          object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
>>>          object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
>>> +        object_property_set_int(OBJECT(chip), smp_cpus, "num-cores",
>>> +                                &error_abort);
>>> +        object_property_set_str(OBJECT(chip), machine->cpu_model, "cpu-model",
>>> +                                &error_abort);
>>
>> So various drafts of the spapr cores had a cpu-model parameter, but we
>> eventually rejected it in favour of having different core types
>> corresponding to the different, well, core types.  Unless there's a
>> compelling reason otherwise, I think it would be nicer to do the same
>> for the pnvchip objects - a p8pnvchip object will always create p8
>> cores and so forth.
>>
>>>
>>>          object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
>>>      }
>>>  }
>>> @@ -292,14 +425,35 @@ static const TypeInfo powernv_machine_2_8_info = {
>>>      .class_init    = powernv_machine_2_8_class_init,
>>>  };
>>>  
>>> -
>>>  static void pnv_chip_realize(DeviceState *dev, Error **errp)
>>>  {
>>> -    ;
>>> +    int i;
>>> +    PnvChip *chip = PNV_CHIP(dev);
>>> +    char *typename = powernv_cpu_core_typename(chip->cpu_model);
>>> +
>>> +    if (!object_class_by_name(typename)) {
>>> +        error_setg(errp, "Unable to find PowerNV CPU Core definition");
>>> +        return;
>>> +    }
>>> +
>>> +    chip->cores = g_new0(Object *, chip->num_cores);
>>> +    for (i = 0; i < chip->num_cores; i++) {
>>> +        int core_id = i * smp_threads;
>>> +        chip->cores[i] = object_new(typename);
>>> +        object_property_set_int(chip->cores[i], smp_threads, "nr-threads",
>>> +                                &error_fatal);
>>> +        object_property_set_int(chip->cores[i], core_id, CPU_CORE_PROP_CORE_ID,
>>> +                                &error_fatal);
>>> +        object_property_set_bool(chip->cores[i], true, "realized",
>>> +                                 &error_fatal);
>>> +    }
>>> +    g_free(typename);
>>>  }
>>>  
>>>  static Property pnv_chip_properties[] = {
>>>      DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
>>> +    DEFINE_PROP_STRING("cpu-model", PnvChip, cpu_model),
>>> +    DEFINE_PROP_UINT32("num-cores", PnvChip, num_cores, 1),
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>  
>>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>>> new file mode 100644
>>> index 000000000000..1e36709db993
>>> --- /dev/null
>>> +++ b/hw/ppc/pnv_core.c
>>> @@ -0,0 +1,171 @@
>>> +/*
>>> + * QEMU PowerPC PowerNV CPU model
>>> + *
>>> + * Copyright (c) IBM Corporation.
>>> + *
>>> + * 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 "qemu/osdep.h"
>>> +#include "sysemu/sysemu.h"
>>> +#include "qapi/error.h"
>>> +#include "target-ppc/cpu.h"
>>> +#include "hw/ppc/ppc.h"
>>> +#include "hw/ppc/pnv.h"
>>> +#include "hw/ppc/pnv_core.h"
>>> +
>>> +static void powernv_cpu_reset(void *opaque)
>>> +{
>>> +    PowerPCCPU *cpu = opaque;
>>> +    CPUState *cs = CPU(cpu);
>>> +    CPUPPCState *env = &cpu->env;
>>> +    MachineState *machine = MACHINE(qdev_get_machine());
>>> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>>> +
>>> +    cpu_reset(cs);
>>> +
>>> +    env->spr[SPR_PIR] = ppc_get_vcpu_dt_id(cpu);
>>
>> Is the PIR writable?  If not it might be better to move this to init
>> rather than reset time.
>>
>>>
>>> +    env->spr[SPR_HIOR] = 0;
>>> +    env->gpr[3] = pnv->fdt_addr;
>>> +    env->nip = 0x10;
>>> +    env->msr |= MSR_HVB;
>>> +}
>>> +
>>> +static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
>>> +{
>>> +    CPUPPCState *env = &cpu->env;
>>> +
>>> +    /* Set time-base frequency to 512 MHz */
>>> +    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
>>> +
>>> +    /* MSR[IP] doesn't exist nowadays */
>>> +    env->msr_mask &= ~(1 << 6);
>>> +
>>> +    qemu_register_reset(powernv_cpu_reset, cpu);
>>> +    powernv_cpu_reset(cpu);
>>> +}
>>> +
>>> +static void powernv_cpu_core_realize_child(Object *child, Error **errp)
>>> +{
>>> +    Error *local_err = NULL;
>>> +    CPUState *cs = CPU(child);
>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> +
>>> +    object_property_set_bool(child, true, "realized", &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>> +    powernv_cpu_init(cpu, &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +}
>>> +
>>> +static void powernv_cpu_core_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    PowerNVCPUCore *pc = POWERNV_CPU_CORE(OBJECT(dev));
>>> +    CPUCore *cc = CPU_CORE(OBJECT(dev));
>>> +    PowerNVCPUClass *pcc = POWERNV_CPU_GET_CLASS(OBJECT(dev));
>>> +    const char *typename = object_class_get_name(pcc->cpu_oc);
>>> +    size_t size = object_type_get_instance_size(typename);
>>> +    Error *local_err = NULL;
>>> +    void *obj;
>>> +    int i, j;
>>> +
>>> +
>>> +    pc->threads = g_malloc0(size * cc->nr_threads);
>>> +    for (i = 0; i < cc->nr_threads; i++) {
>>> +        char id[32];
>>> +        CPUState *cs;
>>> +
>>> +        obj = pc->threads + i * size;
>>> +
>>> +        object_initialize(obj, size, typename);
>>> +        cs = CPU(obj);
>>> +        cs->cpu_index = cc->core_id + i;
>>> +        snprintf(id, sizeof(id), "thread[%d]", i);
>>> +        object_property_add_child(OBJECT(pc), id, obj, &local_err);
>>> +        if (local_err) {
>>> +            goto err;
>>> +        }
>>> +        object_unref(obj);
>>> +    }
>>> +
>>> +    for (j = 0; j < cc->nr_threads; j++) {
>>> +        obj = pc->threads + j * size;
>>> +
>>> +        powernv_cpu_core_realize_child(obj, &local_err);
>>> +        if (local_err) {
>>> +            goto err;
>>> +        }
>>> +    }
>>> +    return;
>>> +
>>> +err:
>>> +    while (--i >= 0) {
>>> +        obj = pc->threads + i * size;
>>> +        object_unparent(obj);
>>> +    }
>>> +    g_free(pc->threads);
>>> +    error_propagate(errp, local_err);
>>> +}
>>> +
>>> +/*
>>> + * Grow this list or merge with SPAPRCoreInfo which is very similar
>>> + */
>>> +static const char *powernv_core_models[] = { "POWER8" };
>>> +
>>> +static void powernv_cpu_core_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>>> +    PowerNVCPUClass *pcc = POWERNV_CPU_CLASS(oc);
>>> +
>>> +    dc->realize = powernv_cpu_core_realize;
>>> +    pcc->cpu_oc = cpu_class_by_name(TYPE_POWERPC_CPU, data);
>>> +}
>>> +
>>> +static const TypeInfo powernv_cpu_core_info = {
>>> +    .name           = TYPE_POWERNV_CPU_CORE,
>>> +    .parent         = TYPE_CPU_CORE,
>>> +    .instance_size  = sizeof(PowerNVCPUCore),
>>> +    .class_size     = sizeof(PowerNVCPUClass),
>>> +    .abstract       = true,
>>> +};
>>> +
>>> +static void powernv_cpu_core_register_types(void)
>>> +{
>>> +    int i ;
>>> +
>>> +    type_register_static(&powernv_cpu_core_info);
>>> +    for (i = 0; i < ARRAY_SIZE(powernv_core_models); ++i) {
>>> +        TypeInfo ti = {
>>> +            .parent = TYPE_POWERNV_CPU_CORE,
>>> +            .instance_size = sizeof(PowerNVCPUCore),
>>> +            .class_init = powernv_cpu_core_class_init,
>>> +            .class_data = (void *) powernv_core_models[i],
>>> +        };
>>> +        ti.name = powernv_cpu_core_typename(powernv_core_models[i]);
>>> +        type_register(&ti);
>>> +        g_free((void *)ti.name);
>>> +    }
>>> +}
>>> +
>>> +type_init(powernv_cpu_core_register_types)
>>> +
>>> +char *powernv_cpu_core_typename(const char *model)
>>> +{
>>> +    return g_strdup_printf("%s-" TYPE_POWERNV_CPU_CORE, model);
>>> +}
>>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>>> index 6907dc9e5c3d..9eac4b34a9b0 100644
>>> --- a/include/hw/ppc/pnv.h
>>> +++ b/include/hw/ppc/pnv.h
>>> @@ -31,6 +31,10 @@ typedef struct PnvChip {
>>>  
>>>      /*< public >*/
>>>      uint32_t     chip_id;
>>> +    uint32_t     num_cores;
>>> +    char *cpu_model;
>>> +
>>> +    Object **cores;
>>
>> So, unlike the chips within the machine, the cores within the chip
>> should probably be done like the threads within the core - a single
>> array with object_initialize() rather than an array of pointers.  AIUI
>> this is because having a (potentially) user instantiable object
>> automatically object_new() other things breaks QOM lifetime rules.  I
>> can't say I understand this point terribly well though, but i know it
>> was something we went several rounds on during the spapr core work
>> though.
>>
>>>
>>>  } PnvChip;
>>>  
>>>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
>>> @@ -43,9 +47,12 @@ typedef struct sPowerNVMachineState {
>>>  
>>>      uint32_t initrd_base;
>>>      long initrd_size;
>>> +    hwaddr fdt_addr;
>>>  
>>>      uint32_t  num_chips;
>>>      PnvChip   *chips;
>>>  } sPowerNVMachineState;
>>>  
>>> +#define PNV_TIMEBASE_FREQ           512000000ULL
>>> +
>>>  #endif /* _PPC_PNV_H */
>>> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
>>> new file mode 100644
>>> index 000000000000..88a09b0fd1c6
>>> --- /dev/null
>>> +++ b/include/hw/ppc/pnv_core.h
>>> @@ -0,0 +1,47 @@
>>> +/*
>>> + * QEMU PowerPC PowerNV CPU model
>>> + *
>>> + * Copyright (c) 2016 IBM Corporation.
>>> + *
>>> + * 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 _PPC_PNV_CORE_H
>>> +#define _PPC_PNV_CORE_H
>>> +
>>> +#include "hw/cpu/core.h"
>>> +
>>> +#define TYPE_POWERNV_CPU_CORE "powernv-cpu-core"
>>> +#define POWERNV_CPU_CORE(obj) \
>>> +    OBJECT_CHECK(PowerNVCPUCore, (obj), TYPE_POWERNV_CPU_CORE)
>>> +#define POWERNV_CPU_CLASS(klass) \
>>> +     OBJECT_CLASS_CHECK(PowerNVCPUClass, (klass), TYPE_POWERNV_CPU_CORE)
>>> +#define POWERNV_CPU_GET_CLASS(obj) \
>>> +     OBJECT_GET_CLASS(PowerNVCPUClass, (obj), TYPE_POWERNV_CPU_CORE)
>>> +
>>> +typedef struct PowerNVCPUCore {
>>> +    /*< private >*/
>>> +    CPUCore parent_obj;
>>> +
>>> +    /*< public >*/
>>> +    void *threads;
>>> +} PowerNVCPUCore;
>>> +
>>> +typedef struct PowerNVCPUClass {
>>> +    DeviceClass parent_class;
>>> +    ObjectClass *cpu_oc;
>>> +}   PowerNVCPUClass;
>>> +
>>> +extern char *powernv_cpu_core_typename(const char *model);
>>> +
>>> +#endif /* _PPC_PNV_CORE_H */
>>

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[Cédric Le Goater]

On 08/16/2016 04:39 AM, David Gibson wrote:
> On Fri, Aug 05, 2016 at 11:15:37AM +0200, Cédric Le Goater wrote:
>> This is largy inspired by sPAPRCPUCore with some simplification, no
>> hotplug for instance. But the differences are small and the objects
>> could possibly be merged.
>>
>> A set of PowerNVCPUCore objects is added to the PnvChip and the device
>> tree is populated looping on these cores. Core ids in the device tree
>> are still a little fuzy. To be checked.
> 
> So, it's not immediately obvious to me if you want an actual core
> object.  You could potentially create the actual vcpu objects directly
> from the chip object.  That assumes that any hotplug will only be at
> chip granularity, not core granularity, but I'm guessing that's the
> case anyway.
> 
> That said, if having the intermediate core object is helpful, you're
> certainly free to have it.

I would like to find some common ground with the spapr core. It should
be possible but for the sake of simplicity let's keep it that way.

>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ppc/Makefile.objs      |   2 +-
>>  hw/ppc/pnv.c              | 160 ++++++++++++++++++++++++++++++++++++++++++-
>>  hw/ppc/pnv_core.c         | 171 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv.h      |   7 ++
>>  include/hw/ppc/pnv_core.h |  47 +++++++++++++
>>  5 files changed, 383 insertions(+), 4 deletions(-)
>>  create mode 100644 hw/ppc/pnv_core.c
>>  create mode 100644 include/hw/ppc/pnv_core.h
>>
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index 8105db7d5600..f8c7d1db9ade 100644
>> --- a/hw/ppc/Makefile.objs
>> +++ b/hw/ppc/Makefile.objs
>> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>>  # IBM PowerNV
>> -obj-$(CONFIG_POWERNV) += pnv.o
>> +obj-$(CONFIG_POWERNV) += pnv.o pnv_core.o
>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>  obj-y += spapr_pci_vfio.o
>>  endif
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index a680780e9dea..1219493c7218 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -35,6 +35,7 @@
>>  #include "hw/ppc/fdt.h"
>>  #include "hw/ppc/ppc.h"
>>  #include "hw/ppc/pnv.h"
>> +#include "hw/ppc/pnv_core.h"
>>  #include "hw/loader.h"
>>  #include "exec/address-spaces.h"
>>  #include "qemu/cutils.h"
>> @@ -112,6 +113,114 @@ static int powernv_populate_memory(void *fdt)
>>      return 0;
>>  }
>>  
>> +static void powernv_create_core_node(void *fdt, CPUState *cs, uint32_t chip_id)
>> +{
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    int smt_threads = ppc_get_compat_smt_threads(cpu);
>> +    CPUPPCState *env = &cpu->env;
>> +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
>> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
>> +    uint32_t servers_prop[smt_threads];
>> +    uint32_t gservers_prop[smt_threads * 2];
>> +    int i, index = ppc_get_vcpu_dt_id(cpu);
>> +    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
>> +                       0xffffffff, 0xffffffff};
>> +    uint32_t tbfreq = PNV_TIMEBASE_FREQ;
>> +    uint32_t cpufreq = 1000000000;
>> +    uint32_t page_sizes_prop[64];
>> +    size_t page_sizes_prop_size;
>> +    char *nodename;
>> +
>> +    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")));
> 
> The handling of dt_id is going to collide with cleanups I want to do
> in this area (for spapr and the ppc cpu core).  Not sure there's a lot
> you can do to avoid that at this stage.

We will adapt. When I reworked the device tree to use the "rw" functions, 
I took a closer look at the powernv needs. Nothing alarming.

The cores are numbered in the (processor) chip. The maximum number of 
cores depends on the model (the max of the max is 12 for Venice) and 
they are not contiguous so we should be activating them with a bitmap 
per chip. 

The core id plus the chip id make a PIR, which is what we use as a dt_id.
For example :

	PowerPC,POWER8@8a8/ibm,chip-id
			 00000011 (17)
	PowerPC,POWER8@8a8/reg
			 000008a8 (2216)
	PowerPC,POWER8@8a8/ibm,pir
			 000008a8 (2216)	

Ben provided a good summary in skiboot, here :

	https://github.com/open-power/skiboot/blob/master/include/chip.h

May be we can find a good formula for cpu->cpu_dt_id. to be studied.


>> +    _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 {
>> +        error_report("Warning: Unknown L1 dcache size for cpu");
>> +    }
>> +    if (pcc->l1_icache_size) {
>> +        _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_size)));
>> +    } else {
>> +        error_report("Warning: Unknown L1 icache size for cpu");
>> +    }
>> +
>> +    _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 = ppc_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", chip_id)));
>> +
>> +    if (cpu->cpu_version) {
>> +        _FDT((fdt_property_cell(fdt, "cpu-version", cpu->cpu_version)));
>> +    }
>> +
>> +    /* Build interrupt servers and gservers properties */
>> +    for (i = 0; i < smt_threads; i++) {
>> +        servers_prop[i] = cpu_to_be32(index + i);
>> +        /* Hack, direct the group queues back to cpu 0 */
>> +        gservers_prop[i * 2] = cpu_to_be32(index + i);
>> +        gservers_prop[i * 2 + 1] = 0;
>> +    }
>> +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-server#s",
>> +                       servers_prop, sizeof(servers_prop))));
>> +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s",
>> +                       gservers_prop, sizeof(gservers_prop))));
>> +
>> +    _FDT((fdt_end_node(fdt)));
>> +}
>> +
>>  static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>>                                  const char *kernel_cmdline)
>>  {
>> @@ -120,6 +229,7 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>>      uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
>>      char *buf;
>>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
>> +    int i;
>>  
>>      fdt = g_malloc0(FDT_MAX_SIZE);
>>      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
>> @@ -156,6 +266,23 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
>>      /* Memory */
>>      _FDT((powernv_populate_memory(fdt)));
>>  
>> +    /* cpus */
>> +    _FDT((fdt_begin_node(fdt, "cpus")));
>> +    _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
>> +    _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
>> +
>> +    for (i = 0; i < pnv->num_chips; i++) {
>> +        PnvChip *chip = &pnv->chips[i];
>> +        int j;
>> +
>> +        for (j = 0; j < chip->num_cores; j++) {
>> +            PowerNVCPUCore *pc = POWERNV_CPU_CORE(chip->cores[j]);
>> +            CPUState *cs = CPU(DEVICE(pc->threads));
>> +            powernv_create_core_node(fdt, cs, chip->chip_id);
> 
> I think it would be nicer to define the fdt creation function in terms
> of the core object, and have it retrieve the representative thread itself.

ok. yes, I will try to refresh that.

>> +        }
>> +    }
>> +    _FDT((fdt_end_node(fdt)));
>> +
>>      _FDT((fdt_end_node(fdt))); /* close root node */
>>      _FDT((fdt_finish(fdt)));
>>  
>> @@ -168,11 +295,13 @@ static void ppc_powernv_reset(void)
>>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>>      void *fdt;
>>  
>> +    pnv->fdt_addr = FDT_ADDR;
>> +
>>      qemu_devices_reset();
>>  
>>      fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
>>  
>> -    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
>> +    cpu_physical_memory_write(pnv->fdt_addr, fdt, fdt_totalsize(fdt));
> 
> These look like not really related changes, that should be maybe
> folded into 1/3.

yes.

>>  }
>>  
>>  static void ppc_powernv_init(MachineState *machine)
>> @@ -252,6 +381,10 @@ static void ppc_powernv_init(MachineState *machine)
>>  
>>          object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
>>          object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
>> +        object_property_set_int(OBJECT(chip), smp_cpus, "num-cores",
>> +                                &error_abort);
>> +        object_property_set_str(OBJECT(chip), machine->cpu_model, "cpu-model",
>> +                                &error_abort);
> 
> So various drafts of the spapr cores had a cpu-model parameter, but we
> eventually rejected it in favour of having different core types
> corresponding to the different, well, core types.  Unless there's a
> compelling reason otherwise, I think it would be nicer to do the same
> for the pnvchip objects - a p8pnvchip object will always create p8
> cores and so forth.

yes. This is fixed in the current patchset. 

>>          object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
>>      }
>>  }
>> @@ -292,14 +425,35 @@ static const TypeInfo powernv_machine_2_8_info = {
>>      .class_init    = powernv_machine_2_8_class_init,
>>  };
>>  
>> -
>>  static void pnv_chip_realize(DeviceState *dev, Error **errp)
>>  {
>> -    ;
>> +    int i;
>> +    PnvChip *chip = PNV_CHIP(dev);
>> +    char *typename = powernv_cpu_core_typename(chip->cpu_model);
>> +
>> +    if (!object_class_by_name(typename)) {
>> +        error_setg(errp, "Unable to find PowerNV CPU Core definition");
>> +        return;
>> +    }
>> +
>> +    chip->cores = g_new0(Object *, chip->num_cores);
>> +    for (i = 0; i < chip->num_cores; i++) {
>> +        int core_id = i * smp_threads;
>> +        chip->cores[i] = object_new(typename);
>> +        object_property_set_int(chip->cores[i], smp_threads, "nr-threads",
>> +                                &error_fatal);
>> +        object_property_set_int(chip->cores[i], core_id, CPU_CORE_PROP_CORE_ID,
>> +                                &error_fatal);
>> +        object_property_set_bool(chip->cores[i], true, "realized",
>> +                                 &error_fatal);
>> +    }
>> +    g_free(typename);
>>  }
>>  
>>  static Property pnv_chip_properties[] = {
>>      DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
>> +    DEFINE_PROP_STRING("cpu-model", PnvChip, cpu_model),
>> +    DEFINE_PROP_UINT32("num-cores", PnvChip, num_cores, 1),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>> new file mode 100644
>> index 000000000000..1e36709db993
>> --- /dev/null
>> +++ b/hw/ppc/pnv_core.c
>> @@ -0,0 +1,171 @@
>> +/*
>> + * QEMU PowerPC PowerNV CPU model
>> + *
>> + * Copyright (c) IBM Corporation.
>> + *
>> + * 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 "qemu/osdep.h"
>> +#include "sysemu/sysemu.h"
>> +#include "qapi/error.h"
>> +#include "target-ppc/cpu.h"
>> +#include "hw/ppc/ppc.h"
>> +#include "hw/ppc/pnv.h"
>> +#include "hw/ppc/pnv_core.h"
>> +
>> +static void powernv_cpu_reset(void *opaque)
>> +{
>> +    PowerPCCPU *cpu = opaque;
>> +    CPUState *cs = CPU(cpu);
>> +    CPUPPCState *env = &cpu->env;
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>> +
>> +    cpu_reset(cs);
>> +
>> +    env->spr[SPR_PIR] = ppc_get_vcpu_dt_id(cpu);
> 
> Is the PIR writable?  If not it might be better to move this to init
> rather than reset time.

It should not. OK.

>> +    env->spr[SPR_HIOR] = 0;
>> +    env->gpr[3] = pnv->fdt_addr;
>> +    env->nip = 0x10;
>> +    env->msr |= MSR_HVB;
>> +}
>> +
>> +static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    /* Set time-base frequency to 512 MHz */
>> +    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
>> +
>> +    /* MSR[IP] doesn't exist nowadays */
>> +    env->msr_mask &= ~(1 << 6);
>> +
>> +    qemu_register_reset(powernv_cpu_reset, cpu);
>> +    powernv_cpu_reset(cpu);
>> +}
>> +
>> +static void powernv_cpu_core_realize_child(Object *child, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    CPUState *cs = CPU(child);
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +
>> +    object_property_set_bool(child, true, "realized", &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    powernv_cpu_init(cpu, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +}
>> +
>> +static void powernv_cpu_core_realize(DeviceState *dev, Error **errp)
>> +{
>> +    PowerNVCPUCore *pc = POWERNV_CPU_CORE(OBJECT(dev));
>> +    CPUCore *cc = CPU_CORE(OBJECT(dev));
>> +    PowerNVCPUClass *pcc = POWERNV_CPU_GET_CLASS(OBJECT(dev));
>> +    const char *typename = object_class_get_name(pcc->cpu_oc);
>> +    size_t size = object_type_get_instance_size(typename);
>> +    Error *local_err = NULL;
>> +    void *obj;
>> +    int i, j;
>> +
>> +
>> +    pc->threads = g_malloc0(size * cc->nr_threads);
>> +    for (i = 0; i < cc->nr_threads; i++) {
>> +        char id[32];
>> +        CPUState *cs;
>> +
>> +        obj = pc->threads + i * size;
>> +
>> +        object_initialize(obj, size, typename);
>> +        cs = CPU(obj);
>> +        cs->cpu_index = cc->core_id + i;
>> +        snprintf(id, sizeof(id), "thread[%d]", i);
>> +        object_property_add_child(OBJECT(pc), id, obj, &local_err);
>> +        if (local_err) {
>> +            goto err;
>> +        }
>> +        object_unref(obj);
>> +    }
>> +
>> +    for (j = 0; j < cc->nr_threads; j++) {
>> +        obj = pc->threads + j * size;
>> +
>> +        powernv_cpu_core_realize_child(obj, &local_err);
>> +        if (local_err) {
>> +            goto err;
>> +        }
>> +    }
>> +    return;
>> +
>> +err:
>> +    while (--i >= 0) {
>> +        obj = pc->threads + i * size;
>> +        object_unparent(obj);
>> +    }
>> +    g_free(pc->threads);
>> +    error_propagate(errp, local_err);
>> +}
>> +
>> +/*
>> + * Grow this list or merge with SPAPRCoreInfo which is very similar
>> + */
>> +static const char *powernv_core_models[] = { "POWER8" };
>> +
>> +static void powernv_cpu_core_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    PowerNVCPUClass *pcc = POWERNV_CPU_CLASS(oc);
>> +
>> +    dc->realize = powernv_cpu_core_realize;
>> +    pcc->cpu_oc = cpu_class_by_name(TYPE_POWERPC_CPU, data);
>> +}
>> +
>> +static const TypeInfo powernv_cpu_core_info = {
>> +    .name           = TYPE_POWERNV_CPU_CORE,
>> +    .parent         = TYPE_CPU_CORE,
>> +    .instance_size  = sizeof(PowerNVCPUCore),
>> +    .class_size     = sizeof(PowerNVCPUClass),
>> +    .abstract       = true,
>> +};
>> +
>> +static void powernv_cpu_core_register_types(void)
>> +{
>> +    int i ;
>> +
>> +    type_register_static(&powernv_cpu_core_info);
>> +    for (i = 0; i < ARRAY_SIZE(powernv_core_models); ++i) {
>> +        TypeInfo ti = {
>> +            .parent = TYPE_POWERNV_CPU_CORE,
>> +            .instance_size = sizeof(PowerNVCPUCore),
>> +            .class_init = powernv_cpu_core_class_init,
>> +            .class_data = (void *) powernv_core_models[i],
>> +        };
>> +        ti.name = powernv_cpu_core_typename(powernv_core_models[i]);
>> +        type_register(&ti);
>> +        g_free((void *)ti.name);
>> +    }
>> +}
>> +
>> +type_init(powernv_cpu_core_register_types)
>> +
>> +char *powernv_cpu_core_typename(const char *model)
>> +{
>> +    return g_strdup_printf("%s-" TYPE_POWERNV_CPU_CORE, model);
>> +}
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 6907dc9e5c3d..9eac4b34a9b0 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -31,6 +31,10 @@ typedef struct PnvChip {
>>  
>>      /*< public >*/
>>      uint32_t     chip_id;
>> +    uint32_t     num_cores;
>> +    char *cpu_model;
>> +
>> +    Object **cores;
> 
> So, unlike the chips within the machine, the cores within the chip
> should probably be done like the threads within the core - a single
> array with object_initialize() rather than an array of pointers.  AIUI
> this is because having a (potentially) user instantiable object
> automatically object_new() other things breaks QOM lifetime rules.  I
> can't say I understand this point terribly well though, but i know it
> was something we went several rounds on during the spapr core work
> though.

ok. I will look into it if this is possible, but I see : 

	struct sPAPRMachineState {
	    ...
	    Object **cores;
	};

So spapr is not instantiating cores in the prefered way ? That does
mean pnv should do the same.

Thanks,

C. 


>>  } PnvChip;
>>  
>>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
>> @@ -43,9 +47,12 @@ typedef struct sPowerNVMachineState {
>>  
>>      uint32_t initrd_base;
>>      long initrd_size;
>> +    hwaddr fdt_addr;
>>  
>>      uint32_t  num_chips;
>>      PnvChip   *chips;
>>  } sPowerNVMachineState;
>>  
>> +#define PNV_TIMEBASE_FREQ           512000000ULL
>> +
>>  #endif /* _PPC_PNV_H */
>> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
>> new file mode 100644
>> index 000000000000..88a09b0fd1c6
>> --- /dev/null
>> +++ b/include/hw/ppc/pnv_core.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * QEMU PowerPC PowerNV CPU model
>> + *
>> + * Copyright (c) 2016 IBM Corporation.
>> + *
>> + * 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 _PPC_PNV_CORE_H
>> +#define _PPC_PNV_CORE_H
>> +
>> +#include "hw/cpu/core.h"
>> +
>> +#define TYPE_POWERNV_CPU_CORE "powernv-cpu-core"
>> +#define POWERNV_CPU_CORE(obj) \
>> +    OBJECT_CHECK(PowerNVCPUCore, (obj), TYPE_POWERNV_CPU_CORE)
>> +#define POWERNV_CPU_CLASS(klass) \
>> +     OBJECT_CLASS_CHECK(PowerNVCPUClass, (klass), TYPE_POWERNV_CPU_CORE)
>> +#define POWERNV_CPU_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(PowerNVCPUClass, (obj), TYPE_POWERNV_CPU_CORE)
>> +
>> +typedef struct PowerNVCPUCore {
>> +    /*< private >*/
>> +    CPUCore parent_obj;
>> +
>> +    /*< public >*/
>> +    void *threads;
>> +} PowerNVCPUCore;
>> +
>> +typedef struct PowerNVCPUClass {
>> +    DeviceClass parent_class;
>> +    ObjectClass *cpu_oc;
>> +}   PowerNVCPUClass;
>> +
>> +extern char *powernv_cpu_core_typename(const char *model);
>> +
>> +#endif /* _PPC_PNV_CORE_H */
> 

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[David Gibson]

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

On Fri, Aug 26, 2016 at 07:49:20PM +0200, Cédric Le Goater wrote:
> On 08/16/2016 04:39 AM, David Gibson wrote:
> > On Fri, Aug 05, 2016 at 11:15:37AM +0200, Cédric Le Goater wrote:
> >> This is largy inspired by sPAPRCPUCore with some simplification, no
> >> hotplug for instance. But the differences are small and the objects
> >> could possibly be merged.
> >>
> >> A set of PowerNVCPUCore objects is added to the PnvChip and the device
> >> tree is populated looping on these cores. Core ids in the device tree
> >> are still a little fuzy. To be checked.
> > 
> > So, it's not immediately obvious to me if you want an actual core
> > object.  You could potentially create the actual vcpu objects directly
> > from the chip object.  That assumes that any hotplug will only be at
> > chip granularity, not core granularity, but I'm guessing that's the
> > case anyway.
> > 
> > That said, if having the intermediate core object is helpful, you're
> > certainly free to have it.
> 
> I would like to find some common ground with the spapr core. It should
> be possible but for the sake of simplicity let's keep it that way.

TBH, I don't think there will be any value sharing anything with the
spapr core object specifically.  Note that there is already an
entirely generic 'cpu-core' type, which you should inherit from.

> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/ppc/Makefile.objs      |   2 +-
> >>  hw/ppc/pnv.c              | 160 ++++++++++++++++++++++++++++++++++++++++++-
> >>  hw/ppc/pnv_core.c         | 171 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/pnv.h      |   7 ++
> >>  include/hw/ppc/pnv_core.h |  47 +++++++++++++
> >>  5 files changed, 383 insertions(+), 4 deletions(-)
> >>  create mode 100644 hw/ppc/pnv_core.c
> >>  create mode 100644 include/hw/ppc/pnv_core.h
> >>
> >> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >> index 8105db7d5600..f8c7d1db9ade 100644
> >> --- a/hw/ppc/Makefile.objs
> >> +++ b/hw/ppc/Makefile.objs
> >> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> >>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >>  # IBM PowerNV
> >> -obj-$(CONFIG_POWERNV) += pnv.o
> >> +obj-$(CONFIG_POWERNV) += pnv.o pnv_core.o
> >>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >>  obj-y += spapr_pci_vfio.o
> >>  endif
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index a680780e9dea..1219493c7218 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -35,6 +35,7 @@
> >>  #include "hw/ppc/fdt.h"
> >>  #include "hw/ppc/ppc.h"
> >>  #include "hw/ppc/pnv.h"
> >> +#include "hw/ppc/pnv_core.h"
> >>  #include "hw/loader.h"
> >>  #include "exec/address-spaces.h"
> >>  #include "qemu/cutils.h"
> >> @@ -112,6 +113,114 @@ static int powernv_populate_memory(void *fdt)
> >>      return 0;
> >>  }
> >>  
> >> +static void powernv_create_core_node(void *fdt, CPUState *cs, uint32_t chip_id)
> >> +{
> >> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +    int smt_threads = ppc_get_compat_smt_threads(cpu);
> >> +    CPUPPCState *env = &cpu->env;
> >> +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> >> +    uint32_t servers_prop[smt_threads];
> >> +    uint32_t gservers_prop[smt_threads * 2];
> >> +    int i, index = ppc_get_vcpu_dt_id(cpu);
> >> +    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
> >> +                       0xffffffff, 0xffffffff};
> >> +    uint32_t tbfreq = PNV_TIMEBASE_FREQ;
> >> +    uint32_t cpufreq = 1000000000;
> >> +    uint32_t page_sizes_prop[64];
> >> +    size_t page_sizes_prop_size;
> >> +    char *nodename;
> >> +
> >> +    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")));
> > 
> > The handling of dt_id is going to collide with cleanups I want to do
> > in this area (for spapr and the ppc cpu core).  Not sure there's a lot
> > you can do to avoid that at this stage.
> 
> We will adapt. When I reworked the device tree to use the "rw" functions, 
> I took a closer look at the powernv needs. Nothing alarming.

Using "rw" throught will certainly make the conversion easier.
Merging the "sw" and "rw" portions is one of the motivations for this
cleanup on spapr.

> The cores are numbered in the (processor) chip. The maximum number of 
> cores depends on the model (the max of the max is 12 for Venice) and 
> they are not contiguous so we should be activating them with a bitmap 
> per chip. 

Ok, this will need some caution to get sane cpu_index values.
However, this isn't urgent until you get to the point of supporting
migration across versions.

> The core id plus the chip id make a PIR, which is what we use as a dt_id.
> For example :
> 
> 	PowerPC,POWER8@8a8/ibm,chip-id
> 			 00000011 (17)
> 	PowerPC,POWER8@8a8/reg
> 			 000008a8 (2216)
> 	PowerPC,POWER8@8a8/ibm,pir
> 			 000008a8 (2216)	
> 
> Ben provided a good summary in skiboot, here :
> 
> 	https://github.com/open-power/skiboot/blob/master/include/chip.h
> 
> May be we can find a good formula for cpu->cpu_dt_id. to be studied.

Possibly.  In fact, I'm planning to eliminate cpu->cpu_dt_id at some
point, in favour of having the machine type construct the id when it
actually builds the dt.  It's not really a cpu level construct.

> >> +    _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 {
> >> +        error_report("Warning: Unknown L1 dcache size for cpu");
> >> +    }
> >> +    if (pcc->l1_icache_size) {
> >> +        _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_size)));
> >> +    } else {
> >> +        error_report("Warning: Unknown L1 icache size for cpu");
> >> +    }
> >> +
> >> +    _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 = ppc_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", chip_id)));
> >> +
> >> +    if (cpu->cpu_version) {
> >> +        _FDT((fdt_property_cell(fdt, "cpu-version", cpu->cpu_version)));
> >> +    }
> >> +
> >> +    /* Build interrupt servers and gservers properties */
> >> +    for (i = 0; i < smt_threads; i++) {
> >> +        servers_prop[i] = cpu_to_be32(index + i);
> >> +        /* Hack, direct the group queues back to cpu 0 */
> >> +        gservers_prop[i * 2] = cpu_to_be32(index + i);
> >> +        gservers_prop[i * 2 + 1] = 0;
> >> +    }
> >> +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-server#s",
> >> +                       servers_prop, sizeof(servers_prop))));
> >> +    _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s",
> >> +                       gservers_prop, sizeof(gservers_prop))));
> >> +
> >> +    _FDT((fdt_end_node(fdt)));
> >> +}
> >> +
> >>  static void *powernv_create_fdt(sPowerNVMachineState *pnv,
> >>                                  const char *kernel_cmdline)
> >>  {
> >> @@ -120,6 +229,7 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
> >>      uint32_t end_prop = cpu_to_be32(pnv->initrd_base + pnv->initrd_size);
> >>      char *buf;
> >>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
> >> +    int i;
> >>  
> >>      fdt = g_malloc0(FDT_MAX_SIZE);
> >>      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
> >> @@ -156,6 +266,23 @@ static void *powernv_create_fdt(sPowerNVMachineState *pnv,
> >>      /* Memory */
> >>      _FDT((powernv_populate_memory(fdt)));
> >>  
> >> +    /* cpus */
> >> +    _FDT((fdt_begin_node(fdt, "cpus")));
> >> +    _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
> >> +    _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
> >> +
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        PnvChip *chip = &pnv->chips[i];
> >> +        int j;
> >> +
> >> +        for (j = 0; j < chip->num_cores; j++) {
> >> +            PowerNVCPUCore *pc = POWERNV_CPU_CORE(chip->cores[j]);
> >> +            CPUState *cs = CPU(DEVICE(pc->threads));
> >> +            powernv_create_core_node(fdt, cs, chip->chip_id);
> > 
> > I think it would be nicer to define the fdt creation function in terms
> > of the core object, and have it retrieve the representative thread itself.
> 
> ok. yes, I will try to refresh that.
> 
> >> +        }
> >> +    }
> >> +    _FDT((fdt_end_node(fdt)));
> >> +
> >>      _FDT((fdt_end_node(fdt))); /* close root node */
> >>      _FDT((fdt_finish(fdt)));
> >>  
> >> @@ -168,11 +295,13 @@ static void ppc_powernv_reset(void)
> >>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> >>      void *fdt;
> >>  
> >> +    pnv->fdt_addr = FDT_ADDR;
> >> +
> >>      qemu_devices_reset();
> >>  
> >>      fdt = powernv_create_fdt(pnv, machine->kernel_cmdline);
> >>  
> >> -    cpu_physical_memory_write(FDT_ADDR, fdt, fdt_totalsize(fdt));
> >> +    cpu_physical_memory_write(pnv->fdt_addr, fdt, fdt_totalsize(fdt));
> > 
> > These look like not really related changes, that should be maybe
> > folded into 1/3.
> 
> yes.
> 
> >>  }
> >>  
> >>  static void ppc_powernv_init(MachineState *machine)
> >> @@ -252,6 +381,10 @@ static void ppc_powernv_init(MachineState *machine)
> >>  
> >>          object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
> >>          object_property_set_int(OBJECT(chip), i, "chip-id", &error_abort);
> >> +        object_property_set_int(OBJECT(chip), smp_cpus, "num-cores",
> >> +                                &error_abort);
> >> +        object_property_set_str(OBJECT(chip), machine->cpu_model, "cpu-model",
> >> +                                &error_abort);
> > 
> > So various drafts of the spapr cores had a cpu-model parameter, but we
> > eventually rejected it in favour of having different core types
> > corresponding to the different, well, core types.  Unless there's a
> > compelling reason otherwise, I think it would be nicer to do the same
> > for the pnvchip objects - a p8pnvchip object will always create p8
> > cores and so forth.
> 
> yes. This is fixed in the current patchset. 
> 
> >>          object_property_set_bool(OBJECT(chip), true, "realized", &error_abort);
> >>      }
> >>  }
> >> @@ -292,14 +425,35 @@ static const TypeInfo powernv_machine_2_8_info = {
> >>      .class_init    = powernv_machine_2_8_class_init,
> >>  };
> >>  
> >> -
> >>  static void pnv_chip_realize(DeviceState *dev, Error **errp)
> >>  {
> >> -    ;
> >> +    int i;
> >> +    PnvChip *chip = PNV_CHIP(dev);
> >> +    char *typename = powernv_cpu_core_typename(chip->cpu_model);
> >> +
> >> +    if (!object_class_by_name(typename)) {
> >> +        error_setg(errp, "Unable to find PowerNV CPU Core definition");
> >> +        return;
> >> +    }
> >> +
> >> +    chip->cores = g_new0(Object *, chip->num_cores);
> >> +    for (i = 0; i < chip->num_cores; i++) {
> >> +        int core_id = i * smp_threads;
> >> +        chip->cores[i] = object_new(typename);
> >> +        object_property_set_int(chip->cores[i], smp_threads, "nr-threads",
> >> +                                &error_fatal);
> >> +        object_property_set_int(chip->cores[i], core_id, CPU_CORE_PROP_CORE_ID,
> >> +                                &error_fatal);
> >> +        object_property_set_bool(chip->cores[i], true, "realized",
> >> +                                 &error_fatal);
> >> +    }
> >> +    g_free(typename);
> >>  }
> >>  
> >>  static Property pnv_chip_properties[] = {
> >>      DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
> >> +    DEFINE_PROP_STRING("cpu-model", PnvChip, cpu_model),
> >> +    DEFINE_PROP_UINT32("num-cores", PnvChip, num_cores, 1),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> >> new file mode 100644
> >> index 000000000000..1e36709db993
> >> --- /dev/null
> >> +++ b/hw/ppc/pnv_core.c
> >> @@ -0,0 +1,171 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV CPU model
> >> + *
> >> + * Copyright (c) IBM Corporation.
> >> + *
> >> + * 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 "qemu/osdep.h"
> >> +#include "sysemu/sysemu.h"
> >> +#include "qapi/error.h"
> >> +#include "target-ppc/cpu.h"
> >> +#include "hw/ppc/ppc.h"
> >> +#include "hw/ppc/pnv.h"
> >> +#include "hw/ppc/pnv_core.h"
> >> +
> >> +static void powernv_cpu_reset(void *opaque)
> >> +{
> >> +    PowerPCCPU *cpu = opaque;
> >> +    CPUState *cs = CPU(cpu);
> >> +    CPUPPCState *env = &cpu->env;
> >> +    MachineState *machine = MACHINE(qdev_get_machine());
> >> +    sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
> >> +
> >> +    cpu_reset(cs);
> >> +
> >> +    env->spr[SPR_PIR] = ppc_get_vcpu_dt_id(cpu);
> > 
> > Is the PIR writable?  If not it might be better to move this to init
> > rather than reset time.
> 
> It should not. OK.
> 
> >> +    env->spr[SPR_HIOR] = 0;
> >> +    env->gpr[3] = pnv->fdt_addr;
> >> +    env->nip = 0x10;
> >> +    env->msr |= MSR_HVB;
> >> +}
> >> +
> >> +static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
> >> +{
> >> +    CPUPPCState *env = &cpu->env;
> >> +
> >> +    /* Set time-base frequency to 512 MHz */
> >> +    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> >> +
> >> +    /* MSR[IP] doesn't exist nowadays */
> >> +    env->msr_mask &= ~(1 << 6);
> >> +
> >> +    qemu_register_reset(powernv_cpu_reset, cpu);
> >> +    powernv_cpu_reset(cpu);
> >> +}
> >> +
> >> +static void powernv_cpu_core_realize_child(Object *child, Error **errp)
> >> +{
> >> +    Error *local_err = NULL;
> >> +    CPUState *cs = CPU(child);
> >> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +
> >> +    object_property_set_bool(child, true, "realized", &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >> +
> >> +    powernv_cpu_init(cpu, &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >> +}
> >> +
> >> +static void powernv_cpu_core_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    PowerNVCPUCore *pc = POWERNV_CPU_CORE(OBJECT(dev));
> >> +    CPUCore *cc = CPU_CORE(OBJECT(dev));
> >> +    PowerNVCPUClass *pcc = POWERNV_CPU_GET_CLASS(OBJECT(dev));
> >> +    const char *typename = object_class_get_name(pcc->cpu_oc);
> >> +    size_t size = object_type_get_instance_size(typename);
> >> +    Error *local_err = NULL;
> >> +    void *obj;
> >> +    int i, j;
> >> +
> >> +
> >> +    pc->threads = g_malloc0(size * cc->nr_threads);
> >> +    for (i = 0; i < cc->nr_threads; i++) {
> >> +        char id[32];
> >> +        CPUState *cs;
> >> +
> >> +        obj = pc->threads + i * size;
> >> +
> >> +        object_initialize(obj, size, typename);
> >> +        cs = CPU(obj);
> >> +        cs->cpu_index = cc->core_id + i;
> >> +        snprintf(id, sizeof(id), "thread[%d]", i);
> >> +        object_property_add_child(OBJECT(pc), id, obj, &local_err);
> >> +        if (local_err) {
> >> +            goto err;
> >> +        }
> >> +        object_unref(obj);
> >> +    }
> >> +
> >> +    for (j = 0; j < cc->nr_threads; j++) {
> >> +        obj = pc->threads + j * size;
> >> +
> >> +        powernv_cpu_core_realize_child(obj, &local_err);
> >> +        if (local_err) {
> >> +            goto err;
> >> +        }
> >> +    }
> >> +    return;
> >> +
> >> +err:
> >> +    while (--i >= 0) {
> >> +        obj = pc->threads + i * size;
> >> +        object_unparent(obj);
> >> +    }
> >> +    g_free(pc->threads);
> >> +    error_propagate(errp, local_err);
> >> +}
> >> +
> >> +/*
> >> + * Grow this list or merge with SPAPRCoreInfo which is very similar
> >> + */
> >> +static const char *powernv_core_models[] = { "POWER8" };
> >> +
> >> +static void powernv_cpu_core_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +    DeviceClass *dc = DEVICE_CLASS(oc);
> >> +    PowerNVCPUClass *pcc = POWERNV_CPU_CLASS(oc);
> >> +
> >> +    dc->realize = powernv_cpu_core_realize;
> >> +    pcc->cpu_oc = cpu_class_by_name(TYPE_POWERPC_CPU, data);
> >> +}
> >> +
> >> +static const TypeInfo powernv_cpu_core_info = {
> >> +    .name           = TYPE_POWERNV_CPU_CORE,
> >> +    .parent         = TYPE_CPU_CORE,
> >> +    .instance_size  = sizeof(PowerNVCPUCore),
> >> +    .class_size     = sizeof(PowerNVCPUClass),
> >> +    .abstract       = true,
> >> +};
> >> +
> >> +static void powernv_cpu_core_register_types(void)
> >> +{
> >> +    int i ;
> >> +
> >> +    type_register_static(&powernv_cpu_core_info);
> >> +    for (i = 0; i < ARRAY_SIZE(powernv_core_models); ++i) {
> >> +        TypeInfo ti = {
> >> +            .parent = TYPE_POWERNV_CPU_CORE,
> >> +            .instance_size = sizeof(PowerNVCPUCore),
> >> +            .class_init = powernv_cpu_core_class_init,
> >> +            .class_data = (void *) powernv_core_models[i],
> >> +        };
> >> +        ti.name = powernv_cpu_core_typename(powernv_core_models[i]);
> >> +        type_register(&ti);
> >> +        g_free((void *)ti.name);
> >> +    }
> >> +}
> >> +
> >> +type_init(powernv_cpu_core_register_types)
> >> +
> >> +char *powernv_cpu_core_typename(const char *model)
> >> +{
> >> +    return g_strdup_printf("%s-" TYPE_POWERNV_CPU_CORE, model);
> >> +}
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index 6907dc9e5c3d..9eac4b34a9b0 100644
> >> --- a/include/hw/ppc/pnv.h
> >> +++ b/include/hw/ppc/pnv.h
> >> @@ -31,6 +31,10 @@ typedef struct PnvChip {
> >>  
> >>      /*< public >*/
> >>      uint32_t     chip_id;
> >> +    uint32_t     num_cores;
> >> +    char *cpu_model;
> >> +
> >> +    Object **cores;
> > 
> > So, unlike the chips within the machine, the cores within the chip
> > should probably be done like the threads within the core - a single
> > array with object_initialize() rather than an array of pointers.  AIUI
> > this is because having a (potentially) user instantiable object
> > automatically object_new() other things breaks QOM lifetime rules.  I
> > can't say I understand this point terribly well though, but i know it
> > was something we went several rounds on during the spapr core work
> > though.
> 
> ok. I will look into it if this is possible, but I see : 
> 
> 	struct sPAPRMachineState {
> 	    ...
> 	    Object **cores;
> 	};
> 
> So spapr is not instantiating cores in the prefered way ? That does
> mean pnv should do the same.

So, the difference here is that while the spapr machine keeps track of
the cores, they're instantiated directly by the user with -device, and
they can be dynamically added or removed with device_{add,del}.

Well.. in practice for backwards compatibility spapr does instantiate
the initial CPUs.

For the cores in pnv, however, IIUC, the lifetime is locked to the
lifetime of the chips - you can't dynamically add or remove a core
from a chip.

Another way to look at it is that the relationship of pnv cores to pnv
chips is somewhat like the relationship of papr threads to papr
cores.  The relationship of papr cores to the papr machine is instead
mirrored in the relationship of pnv *chips* to the pnv machine.

> 
> Thanks,
> 
> C. 
> 
> 
> >>  } PnvChip;
> >>  
> >>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
> >> @@ -43,9 +47,12 @@ typedef struct sPowerNVMachineState {
> >>  
> >>      uint32_t initrd_base;
> >>      long initrd_size;
> >> +    hwaddr fdt_addr;
> >>  
> >>      uint32_t  num_chips;
> >>      PnvChip   *chips;
> >>  } sPowerNVMachineState;
> >>  
> >> +#define PNV_TIMEBASE_FREQ           512000000ULL
> >> +
> >>  #endif /* _PPC_PNV_H */
> >> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> >> new file mode 100644
> >> index 000000000000..88a09b0fd1c6
> >> --- /dev/null
> >> +++ b/include/hw/ppc/pnv_core.h
> >> @@ -0,0 +1,47 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV CPU model
> >> + *
> >> + * Copyright (c) 2016 IBM Corporation.
> >> + *
> >> + * 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 _PPC_PNV_CORE_H
> >> +#define _PPC_PNV_CORE_H
> >> +
> >> +#include "hw/cpu/core.h"
> >> +
> >> +#define TYPE_POWERNV_CPU_CORE "powernv-cpu-core"
> >> +#define POWERNV_CPU_CORE(obj) \
> >> +    OBJECT_CHECK(PowerNVCPUCore, (obj), TYPE_POWERNV_CPU_CORE)
> >> +#define POWERNV_CPU_CLASS(klass) \
> >> +     OBJECT_CLASS_CHECK(PowerNVCPUClass, (klass), TYPE_POWERNV_CPU_CORE)
> >> +#define POWERNV_CPU_GET_CLASS(obj) \
> >> +     OBJECT_GET_CLASS(PowerNVCPUClass, (obj), TYPE_POWERNV_CPU_CORE)
> >> +
> >> +typedef struct PowerNVCPUCore {
> >> +    /*< private >*/
> >> +    CPUCore parent_obj;
> >> +
> >> +    /*< public >*/
> >> +    void *threads;
> >> +} PowerNVCPUCore;
> >> +
> >> +typedef struct PowerNVCPUClass {
> >> +    DeviceClass parent_class;
> >> +    ObjectClass *cpu_oc;
> >> +}   PowerNVCPUClass;
> >> +
> >> +extern char *powernv_cpu_core_typename(const char *model);
> >> +
> >> +#endif /* _PPC_PNV_CORE_H */
> > 
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[Benjamin Herrenschmidt]

On Mon, 2016-08-29 at 10:30 -0400, David Gibson wrote:
> 
> Possibly.  In fact, I'm planning to eliminate cpu->cpu_dt_id at some
> point, in favour of having the machine type construct the id when it
> actually builds the dt.  It's not really a cpu level construct.

On PowerNV it is as it's equal to the PIR, the HW interrupt server,
etc...

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[David Gibson]

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

On Tue, Aug 30, 2016 at 04:15:35PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2016-08-29 at 10:30 -0400, David Gibson wrote:
> > 
> > Possibly.  In fact, I'm planning to eliminate cpu->cpu_dt_id at some
> > point, in favour of having the machine type construct the id when it
> > actually builds the dt.  It's not really a cpu level construct.
> 
> On PowerNV it is as it's equal to the PIR, the HW interrupt server,
> etc...

No.. the PIR itself is a cpu level construct (and we already have a
place for that in the cpu state).  The DT id as such isn't, although
it happens to have the same value.  The fact it has the same value is
itself a machine type property.

[Aside: removing dt_id from the cpu will require disentangling it from
the kvm vcpu id]

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[Benjamin Herrenschmidt]

On Tue, 2016-08-30 at 02:28 -0400, David Gibson wrote:
> No.. the PIR itself is a cpu level construct (and we already have a
> place for that in the cpu state).  The DT id as such isn't, although
> it happens to have the same value.  The fact it has the same value is
> itself a machine type property.
> 
> [Aside: removing dt_id from the cpu will require disentangling it from
> the kvm vcpu id]

On P8 and P9 the PIR of a thread is a chip property, as it encodes
the HW node, chip, core and thread ID (hint: it's not 0 based on P8,
well the core isn't).

So it has to match accordingly for things like core XSCOMs which we
want to start supporting some of.

It also has to match what's in the device-tree as a pretty standard
requirement of all powerpc device-trees.

Finally it also happen to be the target interrupt server on all known
implementations.

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[Cédric Le Goater]

On 08/30/2016 08:15 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2016-08-29 at 10:30 -0400, David Gibson wrote:
>>
>> Possibly.  In fact, I'm planning to eliminate cpu->cpu_dt_id at some
>> point, in favour of having the machine type construct the id when it
>> actually builds the dt.  It's not really a cpu level construct.

>From my understanding, cs->cpu_index is becoming the main CPU identifier.
sPAPRCPUCore assigns it :

	cs->cpu_index = cc->core_id + i

which I reused in PnvCPUCore to hold the real HW identifiers. 
ppc_get_vcpu_by_dt_id() can also safely use cs->cpu_index I think. 

So pnv mostly work without ->cpu_dt_id but there is :

> On PowerNV it is as it's equal to the PIR, the HW interrupt server,
> etc...

xics in the way ... Now that pnv uses real hw core ids, it is 
interesting to see how lost it gets without a cpu with index=0 ...

The most obvious issue is the way we look for the ICPState of a cpu :

    ICPState *ss = &xics->ss[cs->cpu_index];

how about introducing a helper like (this one I hacked) :

+ICPState *xics_find_icp(XICSState *xics, int cpu_index)
+{
+    int i;
+
+    for (i = 0 ; i < xics->nr_servers; i++) {
+        ICPState *ss = &xics->ss[i];
+        if (ss->cs && ss->cs->cpu_index == cpu_index)
+            return ss;
+    }
+
+    return NULL;
+}
+

That might have been already discussed on the mailing list ? 

Thanks,

C.

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[David Gibson]

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

On Tue, Aug 30, 2016 at 09:23:40AM +0200, Cédric Le Goater wrote:
> On 08/30/2016 08:15 AM, Benjamin Herrenschmidt wrote:
> > On Mon, 2016-08-29 at 10:30 -0400, David Gibson wrote:
> >>
> >> Possibly.  In fact, I'm planning to eliminate cpu->cpu_dt_id at some
> >> point, in favour of having the machine type construct the id when it
> >> actually builds the dt.  It's not really a cpu level construct.
> 
> >From my understanding, cs->cpu_index is becoming the main CPU identifier.
> sPAPRCPUCore assigns it :
> 
> 	cs->cpu_index = cc->core_id + i

Uh.. yes and no.  It's the main internal identifier, and it's become
stable at least on platforms which support cpu hotplug.  This means
that it should be possible to derive any other platform specific
identifiers from cpu_index in a consistent way.

However, cpu_index values must still lie in the range 0..max_cpus-1,
which means it's not suitable for directly representing non-contiguous
platform identifiers.

> which I reused in PnvCPUCore to hold the real HW identifiers. 
> ppc_get_vcpu_by_dt_id() can also safely use cs->cpu_index I think. 

Well, there's a different function for getting a cpu by cpu_index.

> So pnv mostly work without ->cpu_dt_id but there is :
> 
> > On PowerNV it is as it's equal to the PIR, the HW interrupt server,
> > etc...
> 
> xics in the way ... Now that pnv uses real hw core ids, it is 
> interesting to see how lost it gets without a cpu with index=0 ...
> 
> The most obvious issue is the way we look for the ICPState of a cpu :
> 
>     ICPState *ss = &xics->ss[cs->cpu_index];
> 
> how about introducing a helper like (this one I hacked) :
> 
> +ICPState *xics_find_icp(XICSState *xics, int cpu_index)
> +{
> +    int i;
> +
> +    for (i = 0 ; i < xics->nr_servers; i++) {
> +        ICPState *ss = &xics->ss[i];
> +        if (ss->cs && ss->cs->cpu_index == cpu_index)
> +            return ss;
> +    }
> +
> +    return NULL;
> +}

That's probably reasonable.

> +
> 
> That might have been already discussed on the mailing list ? 
> 
> Thanks,
> 
> C.
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: add a PowerNVCPUCore object

[Cédric Le Goater]

On 09/05/2016 03:45 AM, David Gibson wrote:
> On Tue, Aug 30, 2016 at 09:23:40AM +0200, Cédric Le Goater wrote:
>> On 08/30/2016 08:15 AM, Benjamin Herrenschmidt wrote:
>>> On Mon, 2016-08-29 at 10:30 -0400, David Gibson wrote:
>>>>
>>>> Possibly.  In fact, I'm planning to eliminate cpu->cpu_dt_id at some
>>>> point, in favour of having the machine type construct the id when it
>>>> actually builds the dt.  It's not really a cpu level construct.
>>
>> >From my understanding, cs->cpu_index is becoming the main CPU identifier.
>> sPAPRCPUCore assigns it :
>>
>> 	cs->cpu_index = cc->core_id + i
> 
> Uh.. yes and no.  It's the main internal identifier, and it's become
> stable at least on platforms which support cpu hotplug.  This means
> that it should be possible to derive any other platform specific
> identifiers from cpu_index in a consistent way.
> 
> However, cpu_index values must still lie in the range 0..max_cpus-1,
> which means it's not suitable for directly representing non-contiguous
> platform identifiers.

ah ok. So I might be doing something wrong on the pnv platform. As
cc->core_id contains the cpu pir and cs->cpu_index is assigned doing:

	cs->cpu_index = cc->core_id + i

It is useful to compute the threads pir but causes some issues in xics.
These are solved with a couple of helpers to look for the ICPState* 
using the CPUState* as an index. 

I will see if I can adapt pnv to use a contiguous cpu_index. 

Thanks,

C.