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 granularity
> > > 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 <bhar...@linux.vnet.ibm.com>
> > > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> > > [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 <libfdt.h>
> > >
> > > @@ -1436,10 +1437,76 @@ static void spapr_cpu_init(sPAPRMachineState
> > > *spapr, 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 = opaque;
> > > + DeviceState *d = DEVICE(drc);
> > > +
> > > + if (d) {
> > > + device_reset(d);
> > > + }
> > > +}
> > > +
> > > +static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
> > > +{
> > > + MachineState *machine = MACHINE(qdev_get_machine());
> > > + uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
> > > + uint32_t nr_rma_lmbs = spapr->rma_size/lmb_size;
> > > + uint32_t nr_lmbs = machine->maxram_size/lmb_size - nr_rma_lmbs;
> > > + uint32_t nr_assigned_lmbs = machine->ram_size/lmb_size - nr_rma_lmbs;
> > > + int i;
> > > +
> > > + for (i = 0; i < nr_lmbs; i++) {
> > > + sPAPRDRConnector *drc;
> > > + uint64_t addr;
> > > +
> > > + if (i < nr_assigned_lmbs) {
> > > + addr = (i + nr_rma_lmbs) * lmb_size;
> > > + } else {
> > > + addr = (i - nr_assigned_lmbs) * lmb_size +
> > > + SPAPR_MACHINE(qdev_get_machine())->hotplug_memory.base;
> > > + }
> > > +
> > > + drc = 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?
>
> 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?
>
> >
> > > + }
> > > +}
> > > +
> > > +/*
> > > + * 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 = 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 = 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_BYTE);
> > > + exit(EXIT_FAILURE);
> > > + }
> > > + }
> > > +}
> > > +
> > > /* pSeries LPAR / sPAPR hardware init */
> > > static void ppc_spapr_init(MachineState *machine)
> > > {
> > > sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> > > const char *kernel_filename = machine->kernel_filename;
> > > const char *kernel_cmdline = machine->kernel_cmdline;
> > > const char *initrd_filename = machine->initrd_filename;
> > > @@ -1518,6 +1585,9 @@ static void ppc_spapr_init(MachineState *machine)
> > > smp_threads),
> > > XICS_IRQS);
> > >
> > > + spapr->dr_lmb_enabled = 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.
>
> 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.
--
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
pgpy5l_xzPuTS.pgp
Description: PGP signature
