From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51413) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaaXR-0001pY-7c for qemu-devel@nongnu.org; Tue, 24 Mar 2015 21:55:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaaXP-00033Y-Av for qemu-devel@nongnu.org; Tue, 24 Mar 2015 21:55:21 -0400 Date: Wed, 25 Mar 2015 12:36:38 +1100 From: David Gibson Message-ID: <20150325013638.GP25043@voom.fritz.box> References: <1427117764-23008-1-git-send-email-bharata@linux.vnet.ibm.com> <1427117764-23008-6-git-send-email-bharata@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="uRJ407LMRYUmu53i" Content-Disposition: inline In-Reply-To: <1427117764-23008-6-git-send-email-bharata@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH v2 05/23] spapr: Reorganize CPU dt generation code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao Cc: mdroth@linux.vnet.ibm.com, agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, imammedo@redhat.com, afaerber@suse.de --uRJ407LMRYUmu53i Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 23, 2015 at 07:05:46PM +0530, Bharata B Rao wrote: > Reorganize CPU device tree generation code so that it be reused from > hotplug path. CPU dt entries are now generated from spapr_finalize_fdt() > instead of spapr_create_fdt_skel(). >=20 > Signed-off-by: Bharata B Rao > --- > hw/ppc/spapr.c | 288 ++++++++++++++++++++++++++++++---------------------= ------ > 1 file changed, 154 insertions(+), 134 deletions(-) >=20 > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 36ff754..1a25cc0 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -206,24 +206,50 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int of= fset, PowerPCCPU *cpu, > return ret; > } > =20 > +static int spapr_fixup_cpu_numa_smt_dt(void *fdt, int offset, CPUState *= cs, > + sPAPREnvironment *spapr) > +{ > + int ret; > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > + int index =3D ppc_get_vcpu_dt_id(cpu); > + uint32_t pft_size_prop[] =3D {0, cpu_to_be32(spapr->htab_shift)}; > + uint32_t associativity[] =3D {cpu_to_be32(0x5), > + cpu_to_be32(0x0), > + cpu_to_be32(0x0), > + cpu_to_be32(0x0), > + cpu_to_be32(cs->numa_node), > + cpu_to_be32(index)}; > + > + /* Advertise NUMA via ibm,associativity */ > + if (nb_numa_nodes > 1) { > + ret =3D fdt_setprop(fdt, offset, "ibm,associativity", associativ= ity, > + sizeof(associativity)); > + if (ret < 0) { > + return ret; > + } > + } > + > + ret =3D fdt_setprop(fdt, offset, "ibm,pft-size", > + pft_size_prop, sizeof(pft_size_prop)); > + if (ret < 0) { > + return ret; > + } The pft-size property isn't actually related to NUMA, so it doesn't really belong in this function. > + return spapr_fixup_cpu_smt_dt(fdt, offset, cpu, > + ppc_get_compat_smt_threads(cpu)); Likewise calling the smt fixup function from the numa fixup function just seems odd to me; just be explicit and call the two sequentially. Overall this seems an odd way to split things. Why not just make a spapr_fixup_one_cpu_dt() function, or similar, which should do all the necessary pieces. > +} > + > static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr) > { > int ret =3D 0, offset, cpus_offset; > CPUState *cs; > char cpu_model[32]; > int smt =3D kvmppc_smt_threads(); > - uint32_t pft_size_prop[] =3D {0, cpu_to_be32(spapr->htab_shift)}; > =20 > CPU_FOREACH(cs) { > PowerPCCPU *cpu =3D POWERPC_CPU(cs); > DeviceClass *dc =3D DEVICE_GET_CLASS(cs); > int index =3D ppc_get_vcpu_dt_id(cpu); > - uint32_t associativity[] =3D {cpu_to_be32(0x5), > - cpu_to_be32(0x0), > - cpu_to_be32(0x0), > - cpu_to_be32(0x0), > - cpu_to_be32(cs->numa_node), > - cpu_to_be32(index)}; > =20 > if ((index % smt) !=3D 0) { > continue; > @@ -247,22 +273,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnviro= nment *spapr) > } > } > =20 > - if (nb_numa_nodes > 1) { > - ret =3D fdt_setprop(fdt, offset, "ibm,associativity", associ= ativity, > - sizeof(associativity)); > - if (ret < 0) { > - return ret; > - } > - } > - > - ret =3D fdt_setprop(fdt, offset, "ibm,pft-size", > - pft_size_prop, sizeof(pft_size_prop)); > - if (ret < 0) { > - return ret; > - } > - > - ret =3D spapr_fixup_cpu_smt_dt(fdt, offset, cpu, > - ppc_get_compat_smt_threads(cpu)); > + ret =3D spapr_fixup_cpu_numa_smt_dt(fdt, offset, cs, spapr); > if (ret < 0) { > return ret; > } > @@ -341,18 +352,13 @@ static void *spapr_create_fdt_skel(hwaddr initrd_ba= se, > uint32_t epow_irq) > { > void *fdt; > - CPUState *cs; > uint32_t start_prop =3D cpu_to_be32(initrd_base); > uint32_t end_prop =3D cpu_to_be32(initrd_base + initrd_size); > GString *hypertas =3D g_string_sized_new(256); > GString *qemu_hypertas =3D g_string_sized_new(256); > uint32_t refpoints[] =3D {cpu_to_be32(0x4), cpu_to_be32(0x4)}; > uint32_t interrupt_server_ranges_prop[] =3D {0, cpu_to_be32(max_cpus= )}; > - int smt =3D kvmppc_smt_threads(); > unsigned char vec5[] =3D {0x0, 0x0, 0x0, 0x0, 0x0, 0x80}; > - QemuOpts *opts =3D qemu_opts_find(qemu_find_opts("smp-opts"), NULL); > - unsigned sockets =3D opts ? qemu_opt_get_number(opts, "sockets", 0) = : 0; > - uint32_t cpus_per_socket =3D sockets ? (smp_cpus / sockets) : 1; > char *buf; > =20 > add_str(hypertas, "hcall-pft"); > @@ -441,107 +447,6 @@ static void *spapr_create_fdt_skel(hwaddr initrd_ba= se, > =20 > _FDT((fdt_end_node(fdt))); > =20 > - /* cpus */ > - _FDT((fdt_begin_node(fdt, "cpus"))); > - > - _FDT((fdt_property_cell(fdt, "#address-cells", 0x1))); > - _FDT((fdt_property_cell(fdt, "#size-cells", 0x0))); > - > - CPU_FOREACH(cs) { > - PowerPCCPU *cpu =3D POWERPC_CPU(cs); > - CPUPPCState *env =3D &cpu->env; > - DeviceClass *dc =3D DEVICE_GET_CLASS(cs); > - PowerPCCPUClass *pcc =3D POWERPC_CPU_GET_CLASS(cs); > - int index =3D ppc_get_vcpu_dt_id(cpu); > - char *nodename; > - uint32_t segs[] =3D {cpu_to_be32(28), cpu_to_be32(40), > - 0xffffffff, 0xffffffff}; > - uint32_t tbfreq =3D kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBA= SE_FREQ; > - uint32_t cpufreq =3D kvm_enabled() ? kvmppc_get_clockfreq() : 10= 00000000; > - uint32_t page_sizes_prop[64]; > - size_t page_sizes_prop_size; > - > - if ((index % smt) !=3D 0) { > - continue; > - } > - > - nodename =3D g_strdup_printf("%s@%x", dc->fw_name, index); > - > - _FDT((fdt_begin_node(fdt, nodename))); > - > - g_free(nodename); > - > - _FDT((fdt_property_cell(fdt, "reg", index))); > - _FDT((fdt_property_string(fdt, "device_type", "cpu"))); > - > - _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR]))); > - _FDT((fdt_property_cell(fdt, "d-cache-block-size", > - env->dcache_line_size))); > - _FDT((fdt_property_cell(fdt, "d-cache-line-size", > - env->dcache_line_size))); > - _FDT((fdt_property_cell(fdt, "i-cache-block-size", > - env->icache_line_size))); > - _FDT((fdt_property_cell(fdt, "i-cache-line-size", > - env->icache_line_size))); > - > - if (pcc->l1_dcache_size) { > - _FDT((fdt_property_cell(fdt, "d-cache-size", pcc->l1_dcache_= size))); > - } else { > - fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n"); > - } > - if (pcc->l1_icache_size) { > - _FDT((fdt_property_cell(fdt, "i-cache-size", pcc->l1_icache_= size))); > - } else { > - fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n"); > - } > - > - _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq))); > - _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq))); > - _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr))); > - _FDT((fdt_property_string(fdt, "status", "okay"))); > - _FDT((fdt_property(fdt, "64-bit", NULL, 0))); > - > - if (env->spr_cb[SPR_PURR].oea_read) { > - _FDT((fdt_property(fdt, "ibm,purr", NULL, 0))); > - } > - > - if (env->mmu_model & POWERPC_MMU_1TSEG) { > - _FDT((fdt_property(fdt, "ibm,processor-segment-sizes", > - segs, sizeof(segs)))); > - } > - > - /* Advertise VMX/VSX (vector extensions) if available > - * 0 / no property =3D=3D no vector extensions > - * 1 =3D=3D VMX / Altivec available > - * 2 =3D=3D VSX available */ > - if (env->insns_flags & PPC_ALTIVEC) { > - uint32_t vmx =3D (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 =3D=3D no DFP > - * 1 =3D=3D DFP available */ > - if (env->insns_flags2 & PPC2_DFP) { > - _FDT((fdt_property_cell(fdt, "ibm,dfp", 1))); > - } > - > - page_sizes_prop_size =3D create_page_sizes_prop(env, page_sizes_= prop, > - sizeof(page_sizes_= prop)); > - if (page_sizes_prop_size) { > - _FDT((fdt_property(fdt, "ibm,segment-page-sizes", > - page_sizes_prop, page_sizes_prop_size))); > - } > - > - _FDT((fdt_property_cell(fdt, "ibm,chip-id", > - cs->cpu_index / cpus_per_socket))); > - > - _FDT((fdt_end_node(fdt))); > - } > - > - _FDT((fdt_end_node(fdt))); > - > /* RTAS */ > _FDT((fdt_begin_node(fdt, "rtas"))); > =20 > @@ -739,6 +644,124 @@ static int spapr_populate_memory(sPAPREnvironment *= spapr, void *fdt) > return 0; > } > =20 > +static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset) > +{ > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > + CPUPPCState *env =3D &cpu->env; > + PowerPCCPUClass *pcc =3D POWERPC_CPU_GET_CLASS(cs); > + int index =3D ppc_get_vcpu_dt_id(cpu); > + uint32_t segs[] =3D {cpu_to_be32(28), cpu_to_be32(40), > + 0xffffffff, 0xffffffff}; > + uint32_t tbfreq =3D kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_F= REQ; > + uint32_t cpufreq =3D kvm_enabled() ? kvmppc_get_clockfreq() : 100000= 0000; > + uint32_t page_sizes_prop[64]; > + size_t page_sizes_prop_size; > + QemuOpts *opts =3D qemu_opts_find(qemu_find_opts("smp-opts"), NULL); > + unsigned sockets =3D opts ? qemu_opt_get_number(opts, "sockets", 0) = : 0; > + uint32_t cpus_per_socket =3D sockets ? (smp_cpus / sockets) : 1; > + > + _FDT((fdt_setprop_cell(fdt, offset, "reg", index))); > + _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu"))); > + > + _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", env->spr[SPR_PVR]= ))); > + _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size", > + env->dcache_line_size))); > + _FDT((fdt_setprop_cell(fdt, offset, "d-cache-line-size", > + env->dcache_line_size))); > + _FDT((fdt_setprop_cell(fdt, offset, "i-cache-block-size", > + env->icache_line_size))); > + _FDT((fdt_setprop_cell(fdt, offset, "i-cache-line-size", > + env->icache_line_size))); > + > + if (pcc->l1_dcache_size) { > + _FDT((fdt_setprop_cell(fdt, offset, "d-cache-size", > + pcc->l1_dcache_size))); > + } else { > + fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n"); > + } > + if (pcc->l1_icache_size) { > + _FDT((fdt_setprop_cell(fdt, offset, "i-cache-size", > + pcc->l1_icache_size))); > + } else { > + fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n"); > + } > + > + _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq))); > + _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq))); > + _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", env->slb_nr))); > + _FDT((fdt_setprop_string(fdt, offset, "status", "okay"))); > + _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0))); > + > + if (env->spr_cb[SPR_PURR].oea_read) { > + _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0))); > + } > + > + if (env->mmu_model & POWERPC_MMU_1TSEG) { > + _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes", > + segs, sizeof(segs)))); > + } > + > + /* Advertise VMX/VSX (vector extensions) if available > + * 0 / no property =3D=3D no vector extensions > + * 1 =3D=3D VMX / Altivec available > + * 2 =3D=3D VSX available */ > + if (env->insns_flags & PPC_ALTIVEC) { > + uint32_t vmx =3D (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 =3D=3D no DFP > + * 1 =3D=3D DFP available */ > + if (env->insns_flags2 & PPC2_DFP) { > + _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1))); > + } > + > + page_sizes_prop_size =3D create_page_sizes_prop(env, page_sizes_prop, > + sizeof(page_sizes_prop= )); > + if (page_sizes_prop_size) { > + _FDT((fdt_setprop(fdt, offset, "ibm,segment-page-sizes", > + page_sizes_prop, page_sizes_prop_size))); > + } > + > + _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id", > + cs->cpu_index / cpus_per_socket))); > + > + _FDT(spapr_fixup_cpu_numa_smt_dt(fdt, offset, cs, spapr)); > +} > + > +static void spapr_populate_cpu_dt_node(void *fdt, sPAPREnvironment *spap= r) I'd suggest s/cpu/cpus/. If anything "populate_cpu_dt_node" sounds more like it covers a single cpu than "populate_cpu_dt". > +{ > + CPUState *cs; > + int cpus_offset; > + char *nodename; > + int smt =3D kvmppc_smt_threads(); > + > + cpus_offset =3D 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))); > + > + CPU_FOREACH(cs) { > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > + int index =3D ppc_get_vcpu_dt_id(cpu); > + DeviceClass *dc =3D DEVICE_GET_CLASS(cs); > + int offset; > + > + if ((index % smt) !=3D 0) { > + continue; > + } > + > + nodename =3D g_strdup_printf("%s@%x", dc->fw_name, index); > + offset =3D fdt_add_subnode(fdt, cpus_offset, nodename); > + g_free(nodename); > + _FDT(offset); > + spapr_populate_cpu_dt(cs, fdt, offset); > + } > + > +} > + > static void spapr_finalize_fdt(sPAPREnvironment *spapr, > hwaddr fdt_addr, > hwaddr rtas_addr, > @@ -782,11 +805,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spa= pr, > fprintf(stderr, "Couldn't set up RTAS device tree properties\n"); > } > =20 > - /* Advertise NUMA via ibm,associativity */ > - ret =3D spapr_fixup_cpu_dt(fdt, spapr); > - if (ret < 0) { > - fprintf(stderr, "Couldn't finalize CPU device tree properties\n"= ); > - } > + /* cpus */ > + spapr_populate_cpu_dt_node(fdt, spapr); > =20 > bootlist =3D get_boot_devices_list(&cb, true); > if (cb && bootlist) { --=20 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 --uRJ407LMRYUmu53i Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVEhEmAAoJEGw4ysog2bOSKncP/jwFxIYoGj2LLQm3+ynJ96/n PTvHxK52d3MgWjCu3zOWm87YDn44fFOGudo538SYkBHErRhazoJW5pJ9NP8ReAui afJSzcvhaawQ1S5YTlRwxSJdk7eeAHJBlIIX/BoljtTMrrydpD8UvCjLdBq4cn/R cpezf2dfu3Xe1LnCWCxb5wbvh+r35LMnvRH19LPZJYQh9yGwAjfNaFnSftqv9mDi aWbutr619ZgLyVyjEFKlRPE4svBNAQmaS5ONUdECSdVar/H2kuGI6k1D9Mvcubia CBDg0kOySFIg9jNXFZgopI/Xk31RzY6ugUDSis0CPTzUugykoeSItwthnGU7aXcl cqRNBhpcntuiK3kdPZk9aT8U3l0N8Xi9twfrz8q6AnDZ7QOMoHDqeuMdQ03gpo/w 9xXk8Lf7+ecmd92P6hf9km5M+pulLo6Wp3XWArLf8EMR2R+obdjMGfyAbwv4hwdo KRgpJxxIHpxZ2VpEgnusz4xhtRH8XPvOdRK5GiXZ9RZJB4F+OVlY0+Z/KU7t4SNn 92nO6RUFJjdXTB7wQ/Bzm7pm0f12hsbbANC1hnpqseHIbBV5JPgdpPZ3Lw5aNznl b7kwS55Rs3Bd62S7n9c4XXLWrXLuxz2nqfL3BCtWT/rpZ8aOoUhSA47GWEHahXkB ivsE04B/lyoRJnm/o5Yr =jOQd -----END PGP SIGNATURE----- --uRJ407LMRYUmu53i--