From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39853) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGeDw-0000b9-Mz for qemu-devel@nongnu.org; Wed, 28 Jan 2015 20:48:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGeDu-0000Zc-Ld for qemu-devel@nongnu.org; Wed, 28 Jan 2015 20:48:48 -0500 Received: from ozlabs.org ([2401:3900:2:1::2]:36367) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGeDu-0000YS-0A for qemu-devel@nongnu.org; Wed, 28 Jan 2015 20:48:46 -0500 Date: Thu, 29 Jan 2015 12:31:46 +1100 From: David Gibson Message-ID: <20150129013146.GQ14681@voom> References: <1420697420-16053-1-git-send-email-bharata@linux.vnet.ibm.com> <1420697420-16053-7-git-send-email-bharata@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="jTlsQtO0VwrbBARu" Content-Disposition: inline In-Reply-To: <1420697420-16053-7-git-send-email-bharata@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH v1 06/13] spapr: CPU hotplug support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao Cc: imammedo@redhat.com, agraf@suse.de, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com --jTlsQtO0VwrbBARu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 08, 2015 at 11:40:13AM +0530, Bharata B Rao wrote: > Support CPU hotplug via device-add command. Use the exising EPOW event > infrastructure to send CPU hotplug notification to the guest. >=20 > Signed-off-by: Bharata B Rao > --- > hw/ppc/spapr.c | 205 ++++++++++++++++++++++++++++++++++++++= +++++- > hw/ppc/spapr_events.c | 8 +- > target-ppc/translate_init.c | 6 ++ > 3 files changed, 215 insertions(+), 4 deletions(-) >=20 > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 515d770..a293a59 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -330,6 +330,8 @@ static void add_str(GString *s, const gchar *s1) > g_string_append_len(s, s1, strlen(s1) + 1); > } > =20 > +uint32_t cpus_per_socket; > + > static void *spapr_create_fdt_skel(hwaddr initrd_base, > hwaddr initrd_size, > hwaddr kernel_size, > @@ -350,9 +352,9 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base, > 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 > + cpus_per_socket =3D sockets ? (smp_cpus / sockets) : 1; > add_str(hypertas, "hcall-pft"); > add_str(hypertas, "hcall-term"); > add_str(hypertas, "hcall-dabr"); > @@ -1744,12 +1746,209 @@ static void spapr_nmi(NMIState *n, int cpu_index= , Error **errp) > } > } > =20 > +/* TODO: Duplicates code from spapr_create_fdt_skel(), Fix this */ Uh, yeah, you should fix this. I think you probably want to move the filling out of the cpu dt information from create_fdt_skel() to finalize_fdt(), then you should be able to use a common helper function. > +static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset, > + int drc_index) > +{ > + 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; > + int smpt =3D ppc_get_compat_smt_threads(cpu); > + uint32_t servers_prop[smpt]; > + uint32_t gservers_prop[smpt * 2]; > + int i; > + 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)}; > + > + _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((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index))); > + > + /* Build interrupt servers and gservers properties */ > + for (i =3D 0; i < smpt; i++) { > + servers_prop[i] =3D cpu_to_be32(index + i); > + /* Hack, direct the group queues back to cpu 0 */ > + gservers_prop[i*2] =3D cpu_to_be32(index + i); > + gservers_prop[i*2 + 1] =3D 0; > + } > + _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))); > + _FDT(fdt_setprop(fdt, offset, "ibm,pft-size", > + pft_size_prop, sizeof(pft_size_prop))); > + > + if (nb_numa_nodes > 1) { > + _FDT(fdt_setprop(fdt, offset, "ibm,associativity", associativity, > + sizeof(associativity))); > + } > +} > + > +static void spapr_cpu_hotplug_add(DeviceState *dev, CPUState *cs) > +{ > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > + DeviceClass *dc =3D DEVICE_GET_CLASS(cs); > + int id =3D ppc_get_vcpu_dt_id(cpu); > + sPAPRDRConnector *drc =3D > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id); > + sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > + int drc_index =3D drck->get_index(drc); > + void *fdt, *fdt_orig; > + int offset, i; > + char *nodename; > + > + /* add OF node for CPU and required OF DT properties */ Misleading comment, it's not really adding a node to anything but rather creating an fdt fragment which the DR code can send to the guest. > + fdt_orig =3D g_malloc0(FDT_MAX_SIZE); > + offset =3D fdt_create(fdt_orig, FDT_MAX_SIZE); > + fdt_begin_node(fdt_orig, ""); > + fdt_end_node(fdt_orig); > + fdt_finish(fdt_orig); So both the PCI and cpu hotplug code now have this idiom. I think we want a common helper that allocates some memory and populates it with an empty fdt fragment. That will mean there's only one place to fix up when we pull in a newer fdt which has a built in to do most of that. > + fdt =3D g_malloc0(FDT_MAX_SIZE); > + fdt_open_into(fdt_orig, fdt, FDT_MAX_SIZE); And like the PCI version, you don't need second malloc() - fdt_open_into() will work in place. > + nodename =3D g_strdup_printf("%s@%x", dc->fw_name, id); > + > + offset =3D fdt_add_subnode(fdt, offset, nodename); > + > + /* Set NUMA node for the added CPU */ > + for (i =3D 0; i < nb_numa_nodes; i++) { > + if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) { > + cs->numa_node =3D i; > + break; > + } > + } > + > + spapr_populate_cpu_dt(cs, fdt, offset, drc_index); > + g_free(fdt_orig); > + g_free(nodename); > + > + drck->attach(drc, dev, fdt, offset, false); > +} > + > +static void spapr_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > + Error **errp) > +{ > + Error *local_err =3D NULL; > + CPUState *cs =3D CPU(dev); > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > + sPAPRDRConnector *drc =3D > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cpu->cpu_d= t_id); > + > + /* TODO: Check if DR is enabled ? */ Again, that's a TODO that shouldn't be postponed. > + g_assert(drc); Especially since I think this could cause an assertion crash if DRC is disa= bled. > + > + spapr_cpu_reset(POWERPC_CPU(CPU(dev))); > + spapr_cpu_hotplug_add(dev, cs); > + spapr_hotplug_req_add_event(drc); > + error_propagate(errp, local_err); Nothing in the code uses local_err, so I think this is redundant. > + return; > +} > + > +static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > + if (dev->hotplugged) { > + spapr_cpu_plug(hotplug_dev, dev, errp); > + } > + } > +} > + > +static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine, > + DeviceState *dev) > +{ > + if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > + return HOTPLUG_HANDLER(machine); > + } > + return NULL; > +} > + > static void spapr_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc =3D MACHINE_CLASS(oc); > sPAPRMachineClass *smc =3D SPAPR_MACHINE_CLASS(oc); > FWPathProviderClass *fwc =3D FW_PATH_PROVIDER_CLASS(oc); > NMIClass *nc =3D NMI_CLASS(oc); > + HotplugHandlerClass *hc =3D HOTPLUG_HANDLER_CLASS(oc); > =20 > mc->init =3D ppc_spapr_init; > mc->reset =3D ppc_spapr_reset; > @@ -1759,6 +1958,9 @@ static void spapr_machine_class_init(ObjectClass *o= c, void *data) > mc->default_boot_order =3D NULL; > mc->kvm_type =3D spapr_kvm_type; > mc->has_dynamic_sysbus =3D true; > + mc->get_hotplug_handler =3D spapr_get_hotpug_handler; > + > + hc->plug =3D spapr_machine_device_plug; > smc->dr_phb_enabled =3D false; > smc->dr_cpu_enabled =3D false; > smc->dr_lmb_enabled =3D false; > @@ -1778,6 +1980,7 @@ static const TypeInfo spapr_machine_info =3D { > .interfaces =3D (InterfaceInfo[]) { > { TYPE_FW_PATH_PROVIDER }, > { TYPE_NMI }, > + { TYPE_HOTPLUG_HANDLER }, > { } > }, > }; > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 434a75d..035d8c9 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -364,14 +364,16 @@ static void spapr_hotplug_req_event(sPAPRDRConnecto= r *drc, uint8_t hp_action) > hp->hdr.section_length =3D cpu_to_be16(sizeof(*hp)); > hp->hdr.section_version =3D 1; /* includes extended modifier */ > hp->hotplug_action =3D hp_action; > - > + hp->drc.index =3D cpu_to_be32(drck->get_index(drc)); > + hp->hotplug_identifier =3D RTAS_LOG_V6_HP_ID_DRC_INDEX; > =20 > switch (drc_type) { > case SPAPR_DR_CONNECTOR_TYPE_PCI: > - hp->drc.index =3D cpu_to_be32(drck->get_index(drc)); > - hp->hotplug_identifier =3D RTAS_LOG_V6_HP_ID_DRC_INDEX; > hp->hotplug_type =3D RTAS_LOG_V6_HP_TYPE_PCI; > break; > + case SPAPR_DR_CONNECTOR_TYPE_CPU: > + hp->hotplug_type =3D RTAS_LOG_V6_HP_TYPE_CPU; > + break; > default: > /* skip notification for unknown connector types */ > g_free(new_hp); > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index 9c642a5..cf9d8d3 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -32,6 +32,7 @@ > #include "hw/qdev-properties.h" > #include "hw/ppc/spapr.h" > #include "hw/ppc/ppc.h" > +#include "sysemu/sysemu.h" > =20 > //#define PPC_DUMP_CPU > //#define PPC_DEBUG_SPR > @@ -8909,6 +8910,11 @@ static void ppc_cpu_realizefn(DeviceState *dev, Er= ror **errp) > return; > } > =20 > + if (cs->cpu_index >=3D max_cpus) { > + error_setg(errp, "Can't have more CPUs, maxcpus limit reached"); > + return; > + } > + > cpu->cpu_dt_id =3D (cs->cpu_index / smp_threads) * max_smt > + (cs->cpu_index % smp_threads); > #endif --=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 --jTlsQtO0VwrbBARu Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUyY2CAAoJEGw4ysog2bOS8HEQAIQLLde5WnK+LqLZDF8pm0KC cY+X5/NqHuy9so6cnnSKedYyl3a6Br/9Z1iljx3/d1jxmdxn6bk3wo5Ib+ypDS9u 6ivTpB7mwA1iO44IrZds2fS0apzKA/QfvsEwRZwuu3D4ZF+53oTmMWbLbUc0JqnQ tUpA6ml01OMARbIIqP7nw8TBUxIQ/RC4tSKhuEAN5j4k3GU/N7TDfGyMedPu/5Y2 WlSvxyQ2Wbl98/qayUj2fuRhejHMLI4gEV3AbMooENAcoqUtFB+yMHQbT2Nt1LIR qReB+MwmvKosP1GkJc0p7AcDlosKjlKnL6EcJ08NS4M6LGKsABdk8b3pp2VG6tR3 cUCMr6dtF3h4kByX2kWvaF/gUkfZ+bAEkHTdpkUHOCaEvro8sSa1ZvHV//O3D2Fo bempgKGFIePuKWgc47iVYNO7qqMnwpADIvJoHyZ42Tj9zMgkvavQUNW8SjrVAmpu ZKiTKAeEVNR3fPElDoqDLvEl1cMSW/uSYrzqY/uibB2UFjWeBIgZoq/pY6colu2r zVXddMsmwtp76+lnOnBBgzJxEYT0TyG7lZNoqH3/idTnVf1wzsNAnAazWqvbKIYj nqSuTbqXAH7eMlS4IkxmYyG75AS2jMUeTei4yC+Cb3gYPlP70uGm9IwHR76ScBRM eH+R/qna/tWaAVPqJuq5 =wa+i -----END PGP SIGNATURE----- --jTlsQtO0VwrbBARu--