From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42070) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z86iU-00030g-2z for qemu-devel@nongnu.org; Thu, 25 Jun 2015 08:57:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z86iQ-0007xH-Or for qemu-devel@nongnu.org; Thu, 25 Jun 2015 08:57:18 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:44895) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z86iQ-0007wL-Eb for qemu-devel@nongnu.org; Thu, 25 Jun 2015 08:57:14 -0400 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 25 Jun 2015 06:57:12 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20150623013234.GV13352@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> Message-ID: <20150625125659.22165.67196@loki> Date: Thu, 25 Jun 2015 07:56:59 -0500 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: David Gibson , Bharata B Rao Cc: aik@ozlabs.ru, qemu-devel@nongnu.org, agraf@suse.de, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, imammedo@redhat.com Quoting David Gibson (2015-06-22 20:32:34) > 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 granulari= ty > > in which LMBs are represented and hot-added. > > = > > LMB DR connectors will be used by the memory hotplug code. > > = > > 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(+) > > = > > 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" > > = > > #include "hw/compat.h" > > +#include "qemu-common.h" > > = > > #include > > = > > @@ -1436,10 +1437,76 @@ static void spapr_cpu_init(sPAPRMachineState *s= papr, PowerPCCPU *cpu) > > qemu_register_reset(spapr_cpu_reset, cpu); > > } > > = > > +static void spapr_drc_reset(void *opaque) > = > This function needs a different name, since it's only called for LMB > drcs, not all drcs. > = > > +{ > > + 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_rma_= 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.base; > > + } > > + > > + drc =3D spapr_dr_connector_new(qdev_get_machine(), > > + SPAPR_DR_CONNECTOR_TYPE_LMB, addr/lmb_size); > > + qemu_register_reset(spapr_drc_reset, drc); > = > 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? The qdev reset code relies on a BusState->DeviceState->BusState->... topology. Since DRCs don't reside on a bus, they don't get the automagic reset. PCI needs it as well, but since PCI DRCs are children of PHBs, they get called via spapr_phb_children_reset(). There was a suggestion from Paolo to move reset registration/unregistration into DRC realize/unrealize for these other cases so we don't have registration calls following each spapr_dr_connector_new(). That might be a nice overall cleanup, but would result in a double reset for PCI. Could maybe just mask out DRCs in spapr_phb_children_reset(). I can roll it into PHB hotplug though. > = > > + } > > +} > > + > > +/* > > + * 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 multiple " > > + "of %lld MB", i, SPAPR_MEMORY_BLOCK_SIZE/M_BY= TE); > > + 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 *machine) > > smp_threads), > > XICS_IRQS); > > = > > + spapr->dr_lmb_enabled =3D smc->dr_lmb_enabled; > = > 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. > = > > + spapr_validate_node_memory(spapr); > > + > > /* init CPUs */ > > if (machine->cpu_model =3D=3D NULL) { > > machine->cpu_model =3D kvm_enabled() ? "host" : "POWER7"; > > @@ -1577,6 +1647,10 @@ static void ppc_spapr_init(MachineState *machine) > > &spapr->hotplug_memory.mr); > > } > > = > > + if (spapr->dr_lmb_enabled) { > > + spapr_create_lmb_dr_connectors(spapr); > > + } > > + > > filename =3D qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin"); > > if (!filename) { > > error_report("Could not find LPAR rtas '%s'", "spapr-rtas.bin"= ); > > @@ -1850,6 +1924,7 @@ static void spapr_nmi(NMIState *n, int cpu_index,= Error **errp) > > 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); > > = > > @@ -1863,6 +1938,7 @@ static void spapr_machine_class_init(ObjectClass = *oc, void *data) > > mc->kvm_type =3D spapr_kvm_type; > > mc->has_dynamic_sysbus =3D true; > > = > > + smc->dr_lmb_enabled =3D false; > > fwc->get_dev_path =3D spapr_get_fw_dev_path; > > nc->nmi_monitor_handler =3D spapr_nmi; > > } > > @@ -1998,11 +2074,13 @@ static const TypeInfo spapr_machine_2_3_info = =3D { > > static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data) > > { > > MachineClass *mc =3D MACHINE_CLASS(oc); > > + sPAPRMachineClass *smc =3D SPAPR_MACHINE_CLASS(oc); > > = > > mc->name =3D "pseries-2.4"; > > mc->desc =3D "pSeries Logical Partition (PAPR compliant) v2.4"; > > mc->alias =3D "pseries"; > > mc->is_default =3D 1; > > + smc->dr_lmb_enabled =3D true; > > } > > = > > static const TypeInfo spapr_machine_2_4_info =3D { > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index 8a1929b..b3fba76 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -35,6 +35,7 @@ struct sPAPRMachineClass { > > MachineClass parent_class; > > = > > /*< public >*/ > > + bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */ > > }; > > = > > /** > > @@ -74,6 +75,7 @@ struct sPAPRMachineState { > > = > > /* RTAS state */ > > QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list; > > + bool dr_lmb_enabled; /* hotplug / dynamic-reconfiguration of LMBs = */ > > = > > /*< public >*/ > > char *kvm_type; > = > -- = > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _othe= r_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson