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 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?
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 = 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.
>
> > + spapr_validate_node_memory(spapr);
> > +
> > /* init CPUs */
> > if (machine->cpu_model == NULL) {
> > machine->cpu_model = 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 = 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 = MACHINE_CLASS(oc);
> > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
> > FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(oc);
> > NMIClass *nc = NMI_CLASS(oc);
> >
> > @@ -1863,6 +1938,7 @@ static void spapr_machine_class_init(ObjectClass *oc,
> > void *data)
> > mc->kvm_type = spapr_kvm_type;
> > mc->has_dynamic_sysbus = true;
> >
> > + smc->dr_lmb_enabled = false;
> > fwc->get_dev_path = spapr_get_fw_dev_path;
> > nc->nmi_monitor_handler = spapr_nmi;
> > }
> > @@ -1998,11 +2074,13 @@ static const TypeInfo spapr_machine_2_3_info = {
> > static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data)
> > {
> > MachineClass *mc = MACHINE_CLASS(oc);
> > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
> >
> > mc->name = "pseries-2.4";
> > mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4";
> > mc->alias = "pseries";
> > mc->is_default = 1;
> > + smc->dr_lmb_enabled = true;
> > }
> >
> > static const TypeInfo spapr_machine_2_4_info = {
> > 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_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
