Re: [Qemu-ppc] [PATCH v2 5/7] ppc/pnv: add a PnvCore object

[David Gibson]

On Tue, Sep 06, 2016 at 08:14:37AM +0200, Cédric Le Goater wrote:
> On 09/05/2016 06:02 AM, David Gibson wrote:
> > On Wed, Aug 31, 2016 at 06:34:13PM +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 PnvCore objects is added to the PnvChip and the device
> >> tree is populated looping on these cores.
> >>
> >> Real HW cpu ids are now generated depending on the chip cpu model, the
> >> chip id and a core mask. This id is stored in CPUState->cpu_index and
> >> in PnvCore->core_id and it is used to populate the device tree.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>
> >>  Changes since v1:
> >>
> >>  - changed name to PnvCore
> >>  - changed PnvChip core array type to a 'PnvCore *cores'
> >>  - introduced real cpu hw ids using a core mask from the chip
> >>  - reworked powernv_create_core_node() which populates the device tree
> >>  - added missing "ibm,pa-features" property 
> >>  - smp_cpus representing threads, used smp_cores instead to create the
> >>    cores in the chip.
> >>  - removed the use of ppc_get_vcpu_dt_id() 
> >>  - added "POWER8E" and "POWER8NVL" cpu models to exercice the
> >>    PnvChipClass
> >>
> >>  hw/ppc/Makefile.objs      |   2 +-
> >>  hw/ppc/pnv.c              | 204 
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/ppc/pnv_core.c         | 170 ++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/pnv.h      |   7 ++
> >>  include/hw/ppc/pnv_core.h |  47 +++++++++++
> >>  5 files changed, 429 insertions(+), 1 deletion(-)
> >>  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 f580e5c41413..08c213c40684 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 pnv_xscom.o
> >> +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.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 b6efb5e3ef07..daf9f459ab0e 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"
> >> @@ -98,6 +99,136 @@ static int powernv_populate_memory(void *fdt)
> >>      return 0;
> >>  }
> >>  
> >> +/*
> >> + * The PowerNV cores (and threads) need to use real HW ids and not an
> >> + * incremental index like it has been done on other platforms. This HW
> >> + * id is called a PIR and is used in the device tree, in the XSCOM
> >> + * communication to address cores, in the interrupt servers.
> >> + */
> >> +static void powernv_create_core_node(PnvCore *pc, void *fdt,
> >> +                                     int cpus_offset, int chip_id)
> >> +{
> >> +    CPUCore *core = CPU_CORE(pc);
> >> +    CPUState *cs = CPU(DEVICE(pc->threads));
> >> +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +    int smt_threads = ppc_get_compat_smt_threads(cpu);
> >> +    CPUPPCState *env = &cpu->env;
> >> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> >> +    uint32_t servers_prop[smt_threads];
> >> +    uint32_t gservers_prop[smt_threads * 2];
> >> +    int i;
> >> +    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;
> >> +    const uint8_t pa_features[] = { 24, 0,
> >> +                                    0xf6, 0x3f, 0xc7, 0xc0, 0x80, 0xf0,
> >> +                                    0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
> >> +                                    0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> >> +                                    0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
> >> +    int offset;
> >> +    char *nodename;
> >> +
> >> +    nodename = g_strdup_printf("address@hidden", dc->fw_name, 
> >> core->core_id);
> >> +    offset = fdt_add_subnode(fdt, cpus_offset, nodename);
> >> +    _FDT(offset);
> >> +    g_free(nodename);
> >> +
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id", chip_id)));
> >> +
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "reg", core->core_id)));
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,pir", core->core_id)));
> >> +    _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
> >> +
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", 
> >> env->spr[SPR_PVR])));
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
> >> +                            env->dcache_line_size)));
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-line-size",
> >> +                            env->dcache_line_size)));
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-block-size",
> >> +                            env->icache_line_size)));
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-line-size",
> >> +                            env->icache_line_size)));
> >> +
> >> +    if (pcc->l1_dcache_size) {
> >> +        _FDT((fdt_setprop_cell(fdt, offset, "d-cache-size",
> >> +                               pcc->l1_dcache_size)));
> >> +    } else {
> >> +        error_report("Warning: Unknown L1 dcache size for cpu");
> >> +    }
> >> +    if (pcc->l1_icache_size) {
> >> +        _FDT((fdt_setprop_cell(fdt, offset, "i-cache-size",
> >> +                               pcc->l1_icache_size)));
> >> +    } else {
> >> +        error_report("Warning: Unknown L1 icache size for cpu");
> >> +    }
> >> +
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
> >> +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", env->slb_nr)));
> >> +    _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
> >> +    _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
> >> +
> >> +    if (env->spr_cb[SPR_PURR].oea_read) {
> >> +        _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
> >> +    }
> >> +
> >> +    if (env->mmu_model & POWERPC_MMU_1TSEG) {
> >> +        _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
> >> +                           segs, sizeof(segs))));
> >> +    }
> >> +
> >> +    /* Advertise VMX/VSX (vector extensions) if available
> >> +     *   0 / no property == no vector extensions
> >> +     *   1               == VMX / Altivec available
> >> +     *   2               == VSX available */
> >> +    if (env->insns_flags & PPC_ALTIVEC) {
> >> +        uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
> >> +
> >> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", vmx)));
> >> +    }
> >> +
> >> +    /* Advertise DFP (Decimal Floating Point) if available
> >> +     *   0 / no property == no DFP
> >> +     *   1               == DFP available */
> >> +    if (env->insns_flags2 & PPC2_DFP) {
> >> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
> >> +    }
> >> +
> >> +    page_sizes_prop_size = ppc_create_page_sizes_prop(env, 
> >> page_sizes_prop,
> >> +                                                  
> >> sizeof(page_sizes_prop));
> >> +    if (page_sizes_prop_size) {
> >> +        _FDT((fdt_setprop(fdt, offset, "ibm,segment-page-sizes",
> >> +                           page_sizes_prop, page_sizes_prop_size)));
> >> +    }
> >> +
> >> +    _FDT((fdt_setprop(fdt, offset, "ibm,pa-features",
> >> +                       pa_features, sizeof(pa_features))));
> >> +
> >> +    if (cpu->cpu_version) {
> >> +        _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", 
> >> cpu->cpu_version)));
> >> +    }
> >> +
> >> +    /* Build interrupt servers and gservers properties */
> >> +    for (i = 0; i < smt_threads; i++) {
> >> +        servers_prop[i] = cpu_to_be32(core->core_id + i);
> >> +        /* Hack, direct the group queues back to cpu 0
> >> +         *
> >> +         * FIXME: check that we still need this hack with real HW
> >> +         * ids. Probably not.
> >> +         */
> >> +        gservers_prop[i * 2] = cpu_to_be32(core->core_id + i);
> >> +        gservers_prop[i * 2 + 1] = 0;
> > 
> > I'm not sure the group servers concept even makes sense in the case of
> > powernv.  In powernv, doesn't the guest control the "real" xics,
> > including the link registers, and therefore can configure its own
> > groups, rather than being limited to what firmware has set up as for
> > PAPR?
> 
> yes. we can remove this property.  
>  
> >> +    }
> >> +    _FDT((fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
> >> +                       servers_prop, sizeof(servers_prop))));
> >> +    _FDT((fdt_setprop(fdt, offset, "ibm,ppc-interrupt-gserver#s",
> >> +                       gservers_prop, sizeof(gservers_prop))));
> >> +}
> >> +
> >>  static void *powernv_create_fdt(PnvMachineState *pnv,
> >>                                  const char *kernel_cmdline)
> >>  {
> >> @@ -106,6 +237,7 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
> >>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
> >>      int off;
> >>      int i;
> >> +    int cpus_offset;
> >>  
> >>      fdt = g_malloc0(FDT_MAX_SIZE);
> >>      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> >> @@ -150,6 +282,22 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
> >>          xscom_populate_fdt(pnv->chips[i]->xscom, fdt, 0);
> >>      }
> >>  
> >> +    /* cpus */
> >> +    cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> >> +    _FDT(cpus_offset);
> >> +    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
> >> +    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
> >> +
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        PnvChip *chip = pnv->chips[i];
> >> +        int j;
> >> +
> >> +        for (j = 0; j < chip->num_cores; j++) {
> >> +            powernv_create_core_node(&chip->cores[j], fdt, cpus_offset,
> >> +                                     chip->chip_id);
> >> +        }
> >> +    }
> >> +
> >>      return fdt;
> >>  }
> >>  
> >> @@ -230,6 +378,11 @@ static void ppc_powernv_init(MachineState *machine)
> >>      for (i = 0; i < pnv->num_chips; i++) {
> >>          Object *chip = object_new(chip_typename);
> >>          object_property_set_int(chip, CHIP_HWID(i), "chip-id", 
> >> &error_abort);
> >> +        object_property_set_int(chip, smp_cores, "num-cores", 
> >> &error_abort);
> > 
> > This should probably be &error_fatal, again.
> 
> ok.
> 
> >> +        /*
> >> +         * We could set a custom cores_mask for the chip here.
> >> +         */
> >> +
> >>          object_property_set_bool(chip, true, "realized", &error_abort);
> >>          pnv->chips[i] = PNV_CHIP(chip);
> >>      }
> >> @@ -335,19 +488,70 @@ static const TypeInfo pnv_chip_power8e_info = {
> >>      .class_init    = pnv_chip_power8e_class_init,
> >>  };
> >>  
> >> +/*
> >> + * This is different for POWER9 so we might need a ops in the chip to
> >> + * calculate the core pirs
> >> + */
> >> +#define P8_PIR(chip_id, core_id) (((chip_id) << 7) | ((core_id) << 3))
> >> +
> >>  static void pnv_chip_realize(DeviceState *dev, Error **errp)
> >>  {
> >>      PnvChip *chip = PNV_CHIP(dev);
> >>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> >> +    char *typename = pnv_core_typename(pcc->cpu_model);
> >> +    size_t typesize = object_type_get_instance_size(typename);
> >> +    int i, core_hwid;
> >> +
> >> +    if (!object_class_by_name(typename)) {
> >> +        error_setg(errp, "Unable to find PowerNV CPU Core '%s'", 
> >> typename);
> >> +        return;
> >> +    }
> >>  
> >>      /* Set up XSCOM bus */
> >>      chip->xscom = xscom_create(chip);
> >>  
> >> +    if (chip->num_cores > pcc->cores_max) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: too many cores for chip ! "
> >> +                      "Limiting to %d\n", __func__, pcc->cores_max);
> >> +        chip->num_cores = pcc->cores_max;
> >> +    }
> >> +
> >> +    chip->cores = g_new0(PnvCore, chip->num_cores);
> >> +
> >> +    /* no custom mask for this chip, let's use the default one from
> >> +     * the chip class */
> >> +    if (!chip->cores_mask) {
> >> +        chip->cores_mask = pcc->cores_mask;
> >> +    }
> >> +
> >> +    for (i = 0, core_hwid = 0; (core_hwid < sizeof(chip->cores_mask) * 8)
> >> +             && (i < chip->num_cores); core_hwid++) {
> >> +        PnvCore *pnv_core = &chip->cores[i];
> > 
> > 
> > Unfortunately, as with spapr core creating its threads you'll need
> > some fancier pointer manipulation to handle the possibility of
> > subtypes of PnvCore with a different instance size.  
> > That doesn't happen now, but it can in theory.
> 
> Yes. This is a reason why I preferred to have a Object **cores. I will 
> fix that.
> 
> I don't really like that :
> 
>       void *obj = chip->cores + i * size;
> 
> It does not feel "object-oriented".

Yeah, it's pretty clunky, but it's what we have for now.

> >> +
> >> +        if (!(chip->cores_mask & (1 << core_hwid))) {
> >> +            continue;
> >> +        }
> >> +
> >> +        object_initialize(pnv_core, typesize, typename);
> >> +        object_property_set_int(OBJECT(pnv_core), smp_threads, 
> >> "nr-threads",
> >> +                                &error_fatal);
> >> +        object_property_set_int(OBJECT(pnv_core),
> >> +                                P8_PIR(chip->chip_id, core_hwid),
> >> +                                CPU_CORE_PROP_CORE_ID, &error_fatal);
> >> +        object_property_set_bool(OBJECT(pnv_core), true, "realized",
> >> +                                 &error_fatal);
> >> +        object_unref(OBJECT(pnv_core));
> >> +        i++;
> >> +    }
> >> +    g_free(typename);
> >> +
> >>      pcc->realize(chip, errp);
> >>  }
> >>  
> >>  static Property pnv_chip_properties[] = {
> >>      DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0),
> >> +    DEFINE_PROP_UINT32("num-cores", PnvChip, num_cores, 1),
> > 
> > I suggest renaming this to "nr-cores" to match "nr-threads" inside the
> > core object.
> 
> ok. fine for me. 
> 
> >> +    DEFINE_PROP_UINT32("cores-mask", PnvChip, cores_mask, 0x0),
> >>      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..825aea1194a1
> >> --- /dev/null
> >> +++ b/hw/ppc/pnv_core.c
> >> @@ -0,0 +1,170 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV CPU Core 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());
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(machine);
> >> +
> >> +    cpu_reset(cs);
> >> +
> >> +    env->spr[SPR_PIR] = cs->cpu_index;
> > 
> > This can't work.  Your PIR values aren't contiguous, but cpu_index
> > values must be (until you get hotplug).
> 
> cpu_index are not contiguous indeed. They are assigned in pnv_core_realize()
> 
>         cs->cpu_index = cc->core_id + i;
> 
> For this to work, we need the four xics patches Nikunj is working 
> on plus some helper routines to assign and find an icp depending on 
> cs and not an index anymore.

That's not the only problem with cpu_index being non-contiguous.
Maybe we'll get to a point where that's possible but for the
forseeable future, the cpu_index will need to be contiguous (as long
as all possible cpus are present, anyway).

For now you should, if possible, derive the non-contiguous platform
ids from the contiguous cpu_index when you need them.

> I will revive the thread with an extra patch.
> 
> >> +    env->spr[SPR_HIOR] = 0;
> >> +    env->gpr[3] = pnv->fdt_addr;
> > 
> > Is the fdt address injected for all CPUs, or only the boot CPU?
> 
> skiboot will just pick just any boot cpu. My understanding is that 
> all need to have the flat device tree address in r3.

Ok.

-- 
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

signature.asc
Description: PGP signature