From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55883) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7deA-0003Nk-T6 for qemu-devel@nongnu.org; Wed, 24 Jun 2015 01:54:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7de6-0006mR-J3 for qemu-devel@nongnu.org; Wed, 24 Jun 2015 01:54:54 -0400 Date: Wed, 24 Jun 2015 15:51:23 +1000 From: David Gibson Message-ID: <20150624055123.GA10837@voom.redhat.com> References: <1434709077-17491-1-git-send-email-bharata@linux.vnet.ibm.com> <1434709077-17491-3-git-send-email-bharata@linux.vnet.ibm.com> <20150623013234.GV13352@voom.redhat.com> <20150624021931.GC26051@in.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="FL5UXtIhxfXey3p5" Content-Disposition: inline In-Reply-To: <20150624021931.GC26051@in.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH v4 2/5] spapr: Add LMB DR connectors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao Cc: mdroth@linux.vnet.ibm.com, aik@ozlabs.ru, agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, imammedo@redhat.com --FL5UXtIhxfXey3p5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 24, 2015 at 07:49:31AM +0530, Bharata B Rao wrote: > On Tue, Jun 23, 2015 at 11:32:34AM +1000, David Gibson wrote: > > On Fri, Jun 19, 2015 at 03:47:54PM +0530, Bharata B Rao wrote: > > > Enable memory hotplug for pseries 2.4 and add LMB DR connectors. > > > With memory hotplug, enforce NUMA node memory size and maxmem to be > > > a multiple of SPAPR_MEMORY_BLOCK_SIZE (256M) since that's the granula= rity > > > in which LMBs are represented and hot-added. > > >=20 > > > LMB DR connectors will be used by the memory hotplug code. > > >=20 > > > Signed-off-by: Bharata B Rao > > > Signed-off-by: Michael Roth > > > [spapr_drc_reset implementation] > > > --- > > > hw/ppc/spapr.c | 78 ++++++++++++++++++++++++++++++++++++++++= ++++++++++ > > > include/hw/ppc/spapr.h | 2 ++ > > > 2 files changed, 80 insertions(+) > > >=20 > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 87a29dc..f9af89b 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -59,6 +59,7 @@ > > > #include "hw/nmi.h" > > > =20 > > > #include "hw/compat.h" > > > +#include "qemu-common.h" > > > =20 > > > #include > > > =20 > > > @@ -1436,10 +1437,76 @@ static void spapr_cpu_init(sPAPRMachineState = *spapr, PowerPCCPU *cpu) > > > qemu_register_reset(spapr_cpu_reset, cpu); > > > } > > > =20 > > > +static void spapr_drc_reset(void *opaque) > >=20 > > This function needs a different name, since it's only called for LMB > > drcs, not all drcs. > >=20 > > > +{ > > > + sPAPRDRConnector *drc =3D opaque; > > > + DeviceState *d =3D DEVICE(drc); > > > + > > > + if (d) { > > > + device_reset(d); > > > + } > > > +} > > > + > > > +static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) > > > +{ > > > + MachineState *machine =3D MACHINE(qdev_get_machine()); > > > + uint64_t lmb_size =3D SPAPR_MEMORY_BLOCK_SIZE; > > > + uint32_t nr_rma_lmbs =3D spapr->rma_size/lmb_size; > > > + uint32_t nr_lmbs =3D machine->maxram_size/lmb_size - nr_rma_lmbs; > > > + uint32_t nr_assigned_lmbs =3D machine->ram_size/lmb_size - nr_rm= a_lmbs; > > > + int i; > > > + > > > + for (i =3D 0; i < nr_lmbs; i++) { > > > + sPAPRDRConnector *drc; > > > + uint64_t addr; > > > + > > > + if (i < nr_assigned_lmbs) { > > > + addr =3D (i + nr_rma_lmbs) * lmb_size; > > > + } else { > > > + addr =3D (i - nr_assigned_lmbs) * lmb_size + > > > + SPAPR_MACHINE(qdev_get_machine())->hotplug_memory.ba= se; > > > + } > > > + > > > + drc =3D spapr_dr_connector_new(qdev_get_machine(), > > > + SPAPR_DR_CONNECTOR_TYPE_LMB, addr/lmb_size); > > > + qemu_register_reset(spapr_drc_reset, drc); > >=20 > > Actually.. I'm not sure what spapr_drc_reset is needed for at all. > > Won't the device reset hook get called through the normal qdev path > > anyway? The PCI hotplug code doesn't have an explicit register_reset, > > so why does the memory hotplug code need it? >=20 > I followed what Michael did for PHB hotplug. I don't see any ill-effects > of not having this special reset routine. Sorry, I'm not entirely clear on what you're saying here. Are you saying that you changed the code to remove the explicit register_reset and that looks to be working ok? >=20 > >=20 > > > + } > > > +} > > > + > > > +/* > > > + * If LMB DR is enabled node memory size and max memory size should > > > + * be a multiple of SPAPR_MEMORY_BLOCK_SIZE (256M). > > > + */ > > > +static void spapr_validate_node_memory(sPAPRMachineState *spapr) > > > +{ > > > + int i; > > > + MachineState *machine =3D MACHINE(qdev_get_machine()); > > > + > > > + if (!spapr->dr_lmb_enabled) { > > > + return; > > > + } > > > + > > > + if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE) { > > > + error_report("maxmem should be a multiple of %lld MB", > > > + SPAPR_MEMORY_BLOCK_SIZE/M_BYTE); > > > + exit(EXIT_FAILURE); > > > + } > > > + > > > + for (i =3D 0; i < nb_numa_nodes; i++) { > > > + if (numa_info[i].node_mem && > > > + numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) { > > > + error_report("Memory size on node %d should be a multipl= e " > > > + "of %lld MB", i, SPAPR_MEMORY_BLOCK_SIZE/M_= BYTE); > > > + exit(EXIT_FAILURE); > > > + } > > > + } > > > +} > > > + > > > /* pSeries LPAR / sPAPR hardware init */ > > > static void ppc_spapr_init(MachineState *machine) > > > { > > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(machine); > > > + sPAPRMachineClass *smc =3D SPAPR_MACHINE_GET_CLASS(machine); > > > const char *kernel_filename =3D machine->kernel_filename; > > > const char *kernel_cmdline =3D machine->kernel_cmdline; > > > const char *initrd_filename =3D machine->initrd_filename; > > > @@ -1518,6 +1585,9 @@ static void ppc_spapr_init(MachineState *machin= e) > > > smp_threads), > > > XICS_IRQS); > > > =20 > > > + spapr->dr_lmb_enabled =3D smc->dr_lmb_enabled; > >=20 > > I don't see any point to copying this value into the MachineState - > > I'm guessing this is a leftover from sPAPREnvironment. Anywhere you > > have the MachineState you can get to the MachineClass and use the > > value directly from there. >=20 > Correct. Will fix this in next version. Ok. Please try to send the next version ASAP - we're getting close enough that a shorter iteration time would be good. --=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 --FL5UXtIhxfXey3p5 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVikVaAAoJEGw4ysog2bOSTwwP/i2I1tAV+OS9GDajW775yzXk tipDD5yDO1vMdicrsMgpW0IlPIm9lYwSg5KiqtLbtLpP1T8t61zxcbsXkI+Spg1j kzsvXBi0JOvKpewE5MqztjuPAA3z7LI6R0PcUbDlcrVhkLi6sIjXQ0PnjpEDA2lu 0ZvUIN8YPm0vXvQGpwHjL7H6w7DmS+ywDE64h398ejZMnkCGP6I3r05PW5et1G78 8/giCDxXZKDJrKO4NeYSspDqZxA6/Yc5e4qBs1Dex/dGvgHx93VoHaCMo2V9pU3V 49tX4a7i3CjuHynCaKKPg65GF3LTyvwnGLMD2sFu3qESPktRfk5ZG5HGI90TqLEh MSjP7z9gfRuUZk8N8hvXMzh0OlJOsfzERD7ODb9Z21G3D4O1n8KE6vdw2Ooox9zk IEcjoQ3gwz5apPWYfFcieIAhFGc/cZn6BiHamK+dErqYUE21CWz5USRyAaCI3tLY h4rDtDCgXtCS6HB3faOVppP4MM1UwLNUrUm26BzECcXPxXOB8TDysTnvCkHnrzaE 6L2WKODXmNOiZUQdo3buUffvG1vqs0MXQw7UiQ84Cq0zAF6taUCPt0duFK/hGaQQ ZFo5yqu9D0QsdMAoIYPaciKNh9rjdRT7iL7gKbnwnIqVKNDdoU0vnGohzU1UKVQQ Xi/Jqv+QvlAogFLONIyj =/Xxa -----END PGP SIGNATURE----- --FL5UXtIhxfXey3p5--