[Qemu-devel] [RFC PATCH v4 0/5] Memory hotplug for PowerPC sPAPR guests

[Qemu-devel] [RFC PATCH v4 0/5] Memory hotplug for PowerPC sPAPR guests

[Bharata B Rao]

Hi,

This is the next version of memory hotplug support patchset for PowerPC
sPAPR guests. This is a split-out from the previous version (v3) that
was carrying CPU and memory hotplug together.

This patchset applies on spapr-next branch of David Gibson's tree with
the other prerequisite patchset applied. Pre-requistes patchset was
posted at:
https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05157.html

Changes in v4
-------------
- Supporting only 32 DIMM slots since PowerPC kernel supports 32.
- LMB DR connectors are now created at boot time instead of creating
  them during ibm,client-architecture-support call. This enables
  hot adding memroy to a migrated VM.
- Imposing the restriction that node memory size and max memory size
  be in multiples of SPAPR_MEMORY_BLOCK_SIZE (256M).
- Ensure memory@0 for RMA ends in the 1st node that has non-zero
  node size.
- Make hash table size dependent on maxram_size instead of ram_size.
- Allow hotplug to nodes other than NUMA node 0.
- No duplication of hotplug code from pc_dimm_plug().

v3: https://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg02910.html

The memory hotplug feature requires updated version of powerpc-utils
and ppc64-diag packages in the guest.

Bharata B Rao (5):
  spapr: Initialize hotplug memory address space
  spapr: Add LMB DR connectors
  spapr: Support ibm,dynamic-reconfiguration-memory
  spapr: Make hash table size a factor of maxram_size
  spapr: Memory hotplug support

 default-configs/ppc64-softmmu.mak |   1 +
 docs/specs/ppc-spapr-hotplug.txt  |  48 +++++
 hw/ppc/spapr.c                    | 436 ++++++++++++++++++++++++++++++++++----
 hw/ppc/spapr_events.c             |   8 +-
 hw/ppc/spapr_hcall.c              |  51 ++++-
 include/hw/ppc/spapr.h            |  29 ++-
 6 files changed, 519 insertions(+), 54 deletions(-)

-- 
2.1.0

[Qemu-devel] [RFC PATCH v4 1/5] spapr: Initialize hotplug memory address space

[Bharata B Rao]

Initialize a hotplug memory region under which all the hotplugged
memory is accommodated. Also enable memory hotplug by setting
CONFIG_MEM_HOTPLUG.

Modelled on i386 memory hotplug.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 default-configs/ppc64-softmmu.mak |  1 +
 hw/ppc/spapr.c                    | 28 ++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h            | 12 ++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index ab62cc7..e77cb1a 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -52,3 +52,4 @@ CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
 # For PReP
 CONFIG_MC146818RTC=y
 CONFIG_ISA_TESTDEV=y
+CONFIG_MEM_HOTPLUG=y
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5ca817c..87a29dc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1549,6 +1549,34 @@ static void ppc_spapr_init(MachineState *machine)
         memory_region_add_subregion(sysmem, 0, rma_region);
     }
 
+    /* initialize hotplug memory address space */
+    if (machine->ram_size < machine->maxram_size) {
+        ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
+
+        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
+            error_report("unsupported amount of memory slots: %"PRIu64,
+                          machine->ram_slots);
+            exit(EXIT_FAILURE);
+        }
+
+        spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
+                                              SPAPR_HOTPLUG_MEM_ALIGN);
+
+        hotplug_mem_size += SPAPR_HOTPLUG_MEM_ALIGN * machine->ram_slots;
+
+        if ((spapr->hotplug_memory.base + hotplug_mem_size) <
+             hotplug_mem_size) {
+            error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
+                         machine->maxram_size);
+            exit(EXIT_FAILURE);
+        }
+
+        memory_region_init(&spapr->hotplug_memory.mr, OBJECT(spapr),
+                           "hotplug-memory", hotplug_mem_size);
+        memory_region_add_subregion(sysmem, spapr->hotplug_memory.base,
+                                    &spapr->hotplug_memory.mr);
+    }
+
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
     if (!filename) {
         error_report("Could not find LPAR rtas '%s'", "spapr-rtas.bin");
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 91a61ab..8a1929b 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -5,6 +5,7 @@
 #include "hw/boards.h"
 #include "hw/ppc/xics.h"
 #include "hw/ppc/spapr_drc.h"
+#include "hw/mem/pc-dimm.h"
 
 struct VIOsPAPRBus;
 struct sPAPRPHBState;
@@ -76,6 +77,7 @@ struct sPAPRMachineState {
 
     /*< public >*/
     char *kvm_type;
+    MemoryHotplugState hotplug_memory;
 };
 
 #define H_SUCCESS         0
@@ -609,4 +611,14 @@ int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
 
 #define SPAPR_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
 
+/*
+ * This defines the maximum number of DIMM slots we can have for sPAPR
+ * guest. This is not defined by sPAPR but we are defining it to 32 slots
+ * based on default number of slots provided by PowerPC kernel.
+ */
+#define SPAPR_MAX_RAM_SLOTS     32
+
+/* 1GB alignment for hotplug memory region */
+#define SPAPR_HOTPLUG_MEM_ALIGN (1ULL << 30)
+
 #endif /* !defined (__HW_SPAPR_H__) */
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v4 1/5] spapr: Initialize hotplug memory address space

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4246 bytes --]

On Fri, Jun 19, 2015 at 03:47:53PM +0530, Bharata B Rao wrote:
> Initialize a hotplug memory region under which all the hotplugged
> memory is accommodated. Also enable memory hotplug by setting
> CONFIG_MEM_HOTPLUG.
> 
> Modelled on i386 memory hotplug.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  default-configs/ppc64-softmmu.mak |  1 +
>  hw/ppc/spapr.c                    | 28 ++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h            | 12 ++++++++++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index ab62cc7..e77cb1a 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -52,3 +52,4 @@ CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
>  # For PReP
>  CONFIG_MC146818RTC=y
>  CONFIG_ISA_TESTDEV=y
> +CONFIG_MEM_HOTPLUG=y
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5ca817c..87a29dc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1549,6 +1549,34 @@ static void ppc_spapr_init(MachineState *machine)
>          memory_region_add_subregion(sysmem, 0, rma_region);
>      }
>  
> +    /* initialize hotplug memory address space */
> +    if (machine->ram_size < machine->maxram_size) {
> +        ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> +
> +        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> +            error_report("unsupported amount of memory slots: %"PRIu64,
> +                          machine->ram_slots);
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
> +                                              SPAPR_HOTPLUG_MEM_ALIGN);
> +
> +        hotplug_mem_size += SPAPR_HOTPLUG_MEM_ALIGN * machine->ram_slots;

I'm not sure what this adjustment is about.  Are you putting a gap of
size SPAPR_HOTPLUG_MEM_ALIGN between each memory slot?  That doesn't
see to match the DRC initialization code in the next patch which
assigns all the LMBs in the hotplug area contiguous addresses.

> +        if ((spapr->hotplug_memory.base + hotplug_mem_size) <
> +             hotplug_mem_size) {
> +            error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
> +                         machine->maxram_size);
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        memory_region_init(&spapr->hotplug_memory.mr, OBJECT(spapr),
> +                           "hotplug-memory", hotplug_mem_size);
> +        memory_region_add_subregion(sysmem, spapr->hotplug_memory.base,
> +                                    &spapr->hotplug_memory.mr);
> +    }
> +
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
>      if (!filename) {
>          error_report("Could not find LPAR rtas '%s'", "spapr-rtas.bin");
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 91a61ab..8a1929b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -5,6 +5,7 @@
>  #include "hw/boards.h"
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/spapr_drc.h"
> +#include "hw/mem/pc-dimm.h"
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
> @@ -76,6 +77,7 @@ struct sPAPRMachineState {
>  
>      /*< public >*/
>      char *kvm_type;
> +    MemoryHotplugState hotplug_memory;
>  };
>  
>  #define H_SUCCESS         0
> @@ -609,4 +611,14 @@ int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
>  
>  #define SPAPR_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
>  
> +/*
> + * This defines the maximum number of DIMM slots we can have for sPAPR
> + * guest. This is not defined by sPAPR but we are defining it to 32 slots
> + * based on default number of slots provided by PowerPC kernel.
> + */
> +#define SPAPR_MAX_RAM_SLOTS     32
> +
> +/* 1GB alignment for hotplug memory region */
> +#define SPAPR_HOTPLUG_MEM_ALIGN (1ULL << 30)
> +
>  #endif /* !defined (__HW_SPAPR_H__) */

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v4 1/5] spapr: Initialize hotplug memory address space

[Bharata B Rao]

On Tue, Jun 23, 2015 at 11:33:58AM +1000, David Gibson wrote:
> On Fri, Jun 19, 2015 at 03:47:53PM +0530, Bharata B Rao wrote:
> > Initialize a hotplug memory region under which all the hotplugged
> > memory is accommodated. Also enable memory hotplug by setting
> > CONFIG_MEM_HOTPLUG.
> > 
> > Modelled on i386 memory hotplug.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  default-configs/ppc64-softmmu.mak |  1 +
> >  hw/ppc/spapr.c                    | 28 ++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h            | 12 ++++++++++++
> >  3 files changed, 41 insertions(+)
> > 
> > diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> > index ab62cc7..e77cb1a 100644
> > --- a/default-configs/ppc64-softmmu.mak
> > +++ b/default-configs/ppc64-softmmu.mak
> > @@ -52,3 +52,4 @@ CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
> >  # For PReP
> >  CONFIG_MC146818RTC=y
> >  CONFIG_ISA_TESTDEV=y
> > +CONFIG_MEM_HOTPLUG=y
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 5ca817c..87a29dc 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1549,6 +1549,34 @@ static void ppc_spapr_init(MachineState *machine)
> >          memory_region_add_subregion(sysmem, 0, rma_region);
> >      }
> >  
> > +    /* initialize hotplug memory address space */
> > +    if (machine->ram_size < machine->maxram_size) {
> > +        ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> > +
> > +        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> > +            error_report("unsupported amount of memory slots: %"PRIu64,
> > +                          machine->ram_slots);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +
> > +        spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
> > +                                              SPAPR_HOTPLUG_MEM_ALIGN);
> > +
> > +        hotplug_mem_size += SPAPR_HOTPLUG_MEM_ALIGN * machine->ram_slots;
> 
> I'm not sure what this adjustment is about.  Are you putting a gap of
> size SPAPR_HOTPLUG_MEM_ALIGN between each memory slot?  That doesn't
> see to match the DRC initialization code in the next patch which
> assigns all the LMBs in the hotplug area contiguous addresses.

Right, this is some leftover alignment bits from x86 implementation which
to me looks like not needed in Power. Let me clean this up in the next
iteration.

Regards,
Bharata.

[Qemu-devel] [RFC PATCH v4 2/5] spapr: Add LMB DR connectors

[Bharata B Rao]

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 <bharata@linux.vnet.ibm.com>
Signed-off-by: Michael Roth <mdroth@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)
+{
+    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);
+    }
+}
+
+/*
+ * 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;
+    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;
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v4 2/5] spapr: Add LMB DR connectors

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7279 bytes --]

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 <bharata@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@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?

> +    }
> +}
> +
> +/*
> + * 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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v4 2/5] spapr: Add LMB DR connectors

[Bharata B Rao]

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 <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Michael Roth <mdroth@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.

> 
> > +    }
> > +}
> > +
> > +/*
> > + * 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.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v4 2/5] spapr: Add LMB DR connectors

[Bharata B Rao]

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 <bharata@linux.vnet.ibm.com>
> > > Signed-off-by: Michael Roth <mdroth@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?

In case of PCI hotplug, the reset for DR devices are handled by
spapr_phb_reset which walks the children and does device_reset for
all DR child devices.

In case of Memory and CPU hotplug, DR devices end up as children
of spapr machine object. So we have two options.

1. The current option where individual DR devices register a reset
handler (spapr_drc_reset) which gets invoked when ppc_spapr_reset
invokes qemu_devices_reset.

2. Let individual DR objects not register their own reset handlers, but
let ppc_spapr_reset walk through its children and invoke reset for them
like below:

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 060fadc..da31152 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1061,6 +1061,16 @@ static int spapr_check_htab_fd(sPAPRMachineState *spapr)
     return rc;
 }
 
+static int spapr_drc_reset(Object *child, void *opaque)
+{
+    DeviceState *d = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE);
+
+    if (d) {
+        device_reset(d);
+    }
+    return 0;
+}
+
 static void ppc_spapr_reset(void)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
@@ -1073,6 +1083,8 @@ static void ppc_spapr_reset(void)
     /* Reset the hash table & recalc the RMA */
     spapr_reset_htab(spapr);
 
+    object_child_foreach(qdev_get_machine(), spapr_drc_reset, NULL);
+
     qemu_devices_reset();
 
     /*

Let me know which one you prefer.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v4 2/5] spapr: Add LMB DR connectors

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5912 bytes --]

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 <bharata@linux.vnet.ibm.com>
> > > Signed-off-by: Michael Roth <mdroth@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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v4 2/5] spapr: Add LMB DR connectors

[Michael Roth]

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 <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Michael Roth <mdroth@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

[Qemu-devel] [RFC PATCH v4 3/5] spapr: Support ibm, dynamic-reconfiguration-memory

[Bharata B Rao]

Parse ibm,architecture.vec table obtained from the guest and enable
memory node configuration via ibm,dynamic-reconfiguration-memory if guest
supports it. This is in preparation to support memory hotplug for
sPAPR guests.

This changes the way memory node configuration is done. Currently all
memory nodes are built upfront. But after this patch, only memory@0 node
for RMA is built upfront. Guest kernel boots with just that and rest of
the memory nodes (via memory@XXX or ibm,dynamic-reconfiguration-memory)
are built when guest does ibm,client-architecture-support call.

Note: This patch needs a SLOF enhancement which is already part of
SLOF binary in QEMU.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 docs/specs/ppc-spapr-hotplug.txt |  48 ++++++++++
 hw/ppc/spapr.c                   | 202 ++++++++++++++++++++++++++++++---------
 hw/ppc/spapr_hcall.c             |  51 ++++++++--
 include/hw/ppc/spapr.h           |  15 ++-
 4 files changed, 266 insertions(+), 50 deletions(-)

diff --git a/docs/specs/ppc-spapr-hotplug.txt b/docs/specs/ppc-spapr-hotplug.txt
index 46e0719..9d574b5 100644
--- a/docs/specs/ppc-spapr-hotplug.txt
+++ b/docs/specs/ppc-spapr-hotplug.txt
@@ -302,4 +302,52 @@ consisting of <phys>, <size> and <maxcpus>.
 pseries guests use this property to note the maximum allowed CPUs for the
 guest.
 
+== ibm,dynamic-reconfiguration-memory ==
+
+ibm,dynamic-reconfiguration-memory is a device tree node that represents
+dynamically reconfigurable logical memory blocks (LMB). This node
+is generated only when the guest advertises the support for it via
+ibm,client-architecture-support call. Memory that is not dynamically
+reconfigurable is represented by /memory nodes. The properties of this
+node that are of interest to the sPAPR memory hotplug implementation
+in QEMU are described here.
+
+ibm,lmb-size
+
+This 64bit integer defines the size of each dynamically reconfigurable LMB.
+
+ibm,associativity-lookup-arrays
+
+This property defines a lookup array in which the NUMA associativity
+information for each LMB can be found. It is a property encoded array
+that begins with an integer M, the number of associativity lists followed
+by an integer N, the number of entries per associativity list and terminated
+by M associativity lists each of length N integers.
+
+This property provides the same information as given by ibm,associativity
+property in a /memory node. Each assigned LMB has an index value between
+0 and M-1 which is used as an index into this table to select which
+associativity list to use for the LMB. This index value for each LMB
+is defined in ibm,dynamic-memory property.
+
+ibm,dynamic-memory
+
+This property describes the dynamically reconfigurable memory. It is a
+property endoded array that has an integer N, the number of LMBs followed
+by N LMB list entires.
+
+Each LMB list entry consists of the following elements:
+
+- Logical address of the start of the LMB encoded as a 64bit integer. This
+  corresponds to reg property in /memory node.
+- DRC index of the LMB that corresponds to ibm,my-drc-index property
+  in a /memory node.
+- Four bytes reserved for expansion.
+- Associativity list index for the LMB that is used an index into
+  ibm,associativity-lookup-arrays property described earlier. This
+  is used to retrieve the right associativity list to be used for this
+  LMB.
+- A 32bit flags word. The bit at bit position 0x00000008 defines whether
+  the LMB is assigned to the the partition as of boot time.
+
 [1] http://thread.gmane.org/gmane.linux.ports.ppc.embedded/75350/focus=106867
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f9af89b..2389061 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -495,44 +495,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
     return fdt;
 }
 
-int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
-                                 target_ulong addr, target_ulong size)
-{
-    void *fdt, *fdt_skel;
-    sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
-
-    size -= sizeof(hdr);
-
-    /* Create sceleton */
-    fdt_skel = g_malloc0(size);
-    _FDT((fdt_create(fdt_skel, size)));
-    _FDT((fdt_begin_node(fdt_skel, "")));
-    _FDT((fdt_end_node(fdt_skel)));
-    _FDT((fdt_finish(fdt_skel)));
-    fdt = g_malloc0(size);
-    _FDT((fdt_open_into(fdt_skel, fdt, size)));
-    g_free(fdt_skel);
-
-    /* Fix skeleton up */
-    _FDT((spapr_fixup_cpu_dt(fdt, spapr)));
-
-    /* Pack resulting tree */
-    _FDT((fdt_pack(fdt)));
-
-    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
-        trace_spapr_cas_failed(size);
-        return -1;
-    }
-
-    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
-    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
-    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
-    g_free(fdt);
-
-    return 0;
-}
-
-static void spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
+static int spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
                                        hwaddr size)
 {
     uint32_t associativity[] = {
@@ -555,6 +518,7 @@ static void spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
                       sizeof(mem_reg_property))));
     _FDT((fdt_setprop(fdt, off, "ibm,associativity", associativity,
                       sizeof(associativity))));
+    return off;
 }
 
 static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
@@ -586,7 +550,6 @@ static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
         }
         if (!mem_start) {
             /* ppc_spapr_init() checks for rma_size <= node0_size already */
-            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
             mem_start += spapr->rma_size;
             node_size -= spapr->rma_size;
         }
@@ -738,6 +701,153 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
 
 }
 
+/*
+ * Adds ibm,dynamic-reconfiguration-memory node.
+ * Refer to docs/specs/ppc-spapr-hotplug.txt for the documentation
+ * of this device tree node.
+ */
+static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+    int ret, i, offset;
+    uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
+    uint32_t prop_lmb_size[] = {0, cpu_to_be32(lmb_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;
+    uint32_t *int_buf, *cur_index, buf_len;
+
+    /* Allocate enough buffer size to fit in ibm,dynamic-memory */
+    buf_len = nr_lmbs * SPAPR_DR_LMB_LIST_ENTRY_SIZE * sizeof(uint32_t) +
+                sizeof(uint32_t);
+    cur_index = int_buf = g_malloc0(buf_len);
+
+    offset = fdt_add_subnode(fdt, 0, "ibm,dynamic-reconfiguration-memory");
+
+    ret = fdt_setprop(fdt, offset, "ibm,lmb-size", prop_lmb_size,
+                    sizeof(prop_lmb_size));
+    if (ret < 0) {
+        goto out;
+    }
+
+    ret = fdt_setprop_cell(fdt, offset, "ibm,memory-flags-mask", 0xff);
+    if (ret < 0) {
+        goto out;
+    }
+
+    ret = fdt_setprop_cell(fdt, offset, "ibm,memory-preservation-time", 0x0);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /* ibm,dynamic-memory */
+    int_buf[0] = cpu_to_be32(nr_lmbs);
+    cur_index++;
+    for (i = 0; i < nr_lmbs; i++) {
+        sPAPRDRConnector *drc;
+        sPAPRDRConnectorClass *drck;
+        uint64_t addr;
+        uint32_t *dynamic_memory = cur_index;
+
+        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_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
+                                       addr/lmb_size);
+        g_assert(drc);
+        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
+        dynamic_memory[0] = cpu_to_be32(addr >> 32);
+        dynamic_memory[1] = cpu_to_be32(addr & 0xffffffff);
+        dynamic_memory[2] = cpu_to_be32(drck->get_index(drc));
+        dynamic_memory[3] = cpu_to_be32(0); /* reserved */
+        dynamic_memory[4] = cpu_to_be32(numa_get_node(addr, NULL));
+        if (addr < machine->ram_size ||
+                    memory_region_present(get_system_memory(), addr)) {
+            dynamic_memory[5] = cpu_to_be32(SPAPR_LMB_FLAGS_ASSIGNED);
+        } else {
+            dynamic_memory[5] = cpu_to_be32(0);
+        }
+
+        cur_index += SPAPR_DR_LMB_LIST_ENTRY_SIZE;
+    }
+    ret = fdt_setprop(fdt, offset, "ibm,dynamic-memory", int_buf, buf_len);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /* ibm,associativity-lookup-arrays */
+    cur_index = int_buf;
+    int_buf[0] = cpu_to_be32(nb_numa_nodes);
+    int_buf[1] = cpu_to_be32(4); /* Number of entries per associativity list */
+    cur_index += 2;
+    for (i = 0; i < nb_numa_nodes; i++) {
+        uint32_t associativity[] = {
+            cpu_to_be32(0x0),
+            cpu_to_be32(0x0),
+            cpu_to_be32(0x0),
+            cpu_to_be32(i)
+        };
+        memcpy(cur_index, associativity, sizeof(associativity));
+        cur_index += 4;
+    }
+    ret = fdt_setprop(fdt, offset, "ibm,associativity-lookup-arrays", int_buf,
+            (cur_index - int_buf) * sizeof(uint32_t));
+out:
+    g_free(int_buf);
+    return ret;
+}
+
+int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
+                                 target_ulong addr, target_ulong size,
+                                 bool cpu_update, bool memory_update)
+{
+    void *fdt, *fdt_skel;
+    sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
+
+    size -= sizeof(hdr);
+
+    /* Create sceleton */
+    fdt_skel = g_malloc0(size);
+    _FDT((fdt_create(fdt_skel, size)));
+    _FDT((fdt_begin_node(fdt_skel, "")));
+    _FDT((fdt_end_node(fdt_skel)));
+    _FDT((fdt_finish(fdt_skel)));
+    fdt = g_malloc0(size);
+    _FDT((fdt_open_into(fdt_skel, fdt, size)));
+    g_free(fdt_skel);
+
+    /* Fixup cpu nodes */
+    if (cpu_update) {
+        _FDT((spapr_fixup_cpu_dt(fdt, spapr)));
+    }
+
+    /* Generate memory nodes or ibm,dynamic-reconfiguration-memory node */
+    if (memory_update && spapr->dr_lmb_enabled) {
+        _FDT((spapr_populate_drconf_memory(spapr, fdt)));
+    } else {
+        _FDT((spapr_populate_memory(spapr, fdt)));
+    }
+
+    /* Pack resulting tree */
+    _FDT((fdt_pack(fdt)));
+
+    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
+        trace_spapr_cas_failed(size);
+        return -1;
+    }
+
+    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
+    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
+    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
+    g_free(fdt);
+
+    return 0;
+}
+
 static void spapr_finalize_fdt(sPAPRMachineState *spapr,
                                hwaddr fdt_addr,
                                hwaddr rtas_addr,
@@ -756,10 +866,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
     /* open out the base tree into a temp buffer for the final tweaks */
     _FDT((fdt_open_into(spapr->fdt_skel, fdt, FDT_MAX_SIZE)));
 
-    ret = spapr_populate_memory(spapr, fdt);
-    if (ret < 0) {
-        fprintf(stderr, "couldn't setup memory nodes in fdt\n");
-        exit(1);
+    /*
+     * Add memory@0 node to represent RMA. Rest of the memory is either
+     * represented by memory nodes or ibm,dynamic-reconfiguration-memory
+     * node later during ibm,client-architecture-support call.
+     */
+    for (i = 0; i < nb_numa_nodes; ++i) {
+        if (numa_info[i].node_mem) {
+            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
+            break;
+        }
     }
 
     ret = spapr_populate_vdevice(spapr->vio_bus, fdt);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 652ddf6..2caac82 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -808,6 +808,32 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return ret;
 }
 
+/*
+ * Return the offset to the requested option vector @vector in the
+ * option vector table @table.
+ */
+static target_ulong cas_get_option_vector(int vector, target_ulong table)
+{
+    int i;
+    char nr_vectors, nr_entries;
+
+    if (!table) {
+        return 0;
+    }
+
+    nr_vectors = (ldl_phys(&address_space_memory, table) >> 24) + 1;
+    if (!vector || vector > nr_vectors) {
+        return 0;
+    }
+    table++; /* skip nr option vectors */
+
+    for (i = 0; i < vector - 1; i++) {
+        nr_entries = ldl_phys(&address_space_memory, table) >> 24;
+        table += nr_entries + 2;
+    }
+    return table;
+}
+
 typedef struct {
     PowerPCCPU *cpu;
     uint32_t cpu_version;
@@ -828,19 +854,22 @@ static void do_set_compat(void *arg)
     ((cpuver) == CPU_POWERPC_LOGICAL_2_06_PLUS) ? 2061 : \
     ((cpuver) == CPU_POWERPC_LOGICAL_2_07) ? 2070 : 0)
 
+#define OV5_DRCONF_MEMORY 0x20
+
 static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
                                                   sPAPRMachineState *spapr,
                                                   target_ulong opcode,
                                                   target_ulong *args)
 {
-    target_ulong list = args[0];
+    target_ulong list = args[0], ov_table;
     PowerPCCPUClass *pcc_ = POWERPC_CPU_GET_CLASS(cpu_);
     CPUState *cs;
-    bool cpu_match = false;
+    bool cpu_match = false, cpu_update = true, memory_update = false;
     unsigned old_cpu_version = cpu_->cpu_version;
     unsigned compat_lvl = 0, cpu_version = 0;
     unsigned max_lvl = get_compat_level(cpu_->max_compat);
     int counter;
+    char ov5_byte2;
 
     /* Parse PVR list */
     for (counter = 0; counter < 512; ++counter) {
@@ -890,8 +919,6 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
         }
     }
 
-    /* For the future use: here @list points to the first capability */
-
     /* Parsing finished */
     trace_spapr_cas_pvr(cpu_->cpu_version, cpu_match,
                         cpu_version, pcc_->pcr_mask);
@@ -915,14 +942,26 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
     }
 
     if (!cpu_version) {
-        return H_SUCCESS;
+        cpu_update = false;
     }
 
+    /* For the future use: here @ov_table points to the first option vector */
+    ov_table = list;
+
+    list = cas_get_option_vector(5, ov_table);
     if (!list) {
         return H_SUCCESS;
     }
 
-    if (spapr_h_cas_compose_response(spapr, args[1], args[2])) {
+    /* @list now points to OV 5 */
+    list += 2;
+    ov5_byte2 = rtas_ld(list, 0) >> 24;
+    if (ov5_byte2 & OV5_DRCONF_MEMORY) {
+        memory_update = true;
+    }
+
+    if (spapr_h_cas_compose_response(spapr, args[1], args[2],
+                                     cpu_update, memory_update)) {
         qemu_system_reset_request();
     }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index b3fba76..2d6921a 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -581,7 +581,8 @@ struct sPAPREventLogEntry {
 void spapr_events_init(sPAPRMachineState *sm);
 void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
 int spapr_h_cas_compose_response(sPAPRMachineState *sm,
-                                 target_ulong addr, target_ulong size);
+                                 target_ulong addr, target_ulong size,
+                                 bool cpu_update, bool memory_update);
 sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
                                    uint64_t bus_offset,
                                    uint32_t page_shift,
@@ -623,4 +624,16 @@ int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
 /* 1GB alignment for hotplug memory region */
 #define SPAPR_HOTPLUG_MEM_ALIGN (1ULL << 30)
 
+/*
+ * Number of 32 bit words in each LMB list entry in ibm,dynamic-memory
+ * property under ibm,dynamic-reconfiguration-memory node.
+ */
+#define SPAPR_DR_LMB_LIST_ENTRY_SIZE 6
+
+/*
+ * This flag value defines the LMB as assigned in ibm,dynamic-memory
+ * property under ibm,dynamic-reconfiguration-memory node.
+ */
+#define SPAPR_LMB_FLAGS_ASSIGNED 0x00000008
+
 #endif /* !defined (__HW_SPAPR_H__) */
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v4 3/5] spapr: Support ibm, dynamic-reconfiguration-memory

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8929 bytes --]

On Fri, Jun 19, 2015 at 03:47:55PM +0530, Bharata B Rao wrote:
> Parse ibm,architecture.vec table obtained from the guest and enable
> memory node configuration via ibm,dynamic-reconfiguration-memory if guest
> supports it. This is in preparation to support memory hotplug for
> sPAPR guests.
> 
> This changes the way memory node configuration is done. Currently all
> memory nodes are built upfront. But after this patch, only memory@0 node
> for RMA is built upfront. Guest kernel boots with just that and rest of
> the memory nodes (via memory@XXX or ibm,dynamic-reconfiguration-memory)
> are built when guest does ibm,client-architecture-support call.
> 
> Note: This patch needs a SLOF enhancement which is already part of
> SLOF binary in QEMU.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

[snip]
> +int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
> +                                 target_ulong addr, target_ulong size,
> +                                 bool cpu_update, bool memory_update)
> +{
> +    void *fdt, *fdt_skel;
> +    sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> +
> +    size -= sizeof(hdr);
> +
> +    /* Create sceleton */
> +    fdt_skel = g_malloc0(size);
> +    _FDT((fdt_create(fdt_skel, size)));
> +    _FDT((fdt_begin_node(fdt_skel, "")));
> +    _FDT((fdt_end_node(fdt_skel)));
> +    _FDT((fdt_finish(fdt_skel)));
> +    fdt = g_malloc0(size);
> +    _FDT((fdt_open_into(fdt_skel, fdt, size)));
> +    g_free(fdt_skel);
> +
> +    /* Fixup cpu nodes */
> +    if (cpu_update) {
> +        _FDT((spapr_fixup_cpu_dt(fdt, spapr)));
> +    }

The cpu_update parameter seems like its not related to memory hotplug
at all.  I'm guessing it relates to CPU hotplug, in which case please
defer it until those patches are ready to go.

> +
> +    /* Generate memory nodes or ibm,dynamic-reconfiguration-memory node */
> +    if (memory_update && spapr->dr_lmb_enabled) {
> +        _FDT((spapr_populate_drconf_memory(spapr, fdt)));
> +    } else {
> +        _FDT((spapr_populate_memory(spapr, fdt)));
> +    }
> +
> +    /* Pack resulting tree */
> +    _FDT((fdt_pack(fdt)));
> +
> +    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> +        trace_spapr_cas_failed(size);
> +        return -1;
> +    }
> +
> +    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
> +    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
> +    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> +    g_free(fdt);
> +
> +    return 0;
> +}
> +
>  static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>                                 hwaddr fdt_addr,
>                                 hwaddr rtas_addr,
> @@ -756,10 +866,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>      /* open out the base tree into a temp buffer for the final tweaks */
>      _FDT((fdt_open_into(spapr->fdt_skel, fdt, FDT_MAX_SIZE)));
>  
> -    ret = spapr_populate_memory(spapr, fdt);
> -    if (ret < 0) {
> -        fprintf(stderr, "couldn't setup memory nodes in fdt\n");
> -        exit(1);
> +    /*
> +     * Add memory@0 node to represent RMA. Rest of the memory is either
> +     * represented by memory nodes or ibm,dynamic-reconfiguration-memory
> +     * node later during ibm,client-architecture-support call.
> +     */
> +    for (i = 0; i < nb_numa_nodes; ++i) {
> +        if (numa_info[i].node_mem) {
> +            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> +            break;
> +        }

?? The code doesn't seem to match the comment - you appear to be
creating a memory@0 node for every NUMA node, not just for the RMA,
which doesn't make much sense.

>      }
>  
>      ret = spapr_populate_vdevice(spapr->vio_bus, fdt);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 652ddf6..2caac82 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -808,6 +808,32 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      return ret;
>  }
>  
> +/*
> + * Return the offset to the requested option vector @vector in the
> + * option vector table @table.
> + */
> +static target_ulong cas_get_option_vector(int vector, target_ulong table)
> +{
> +    int i;
> +    char nr_vectors, nr_entries;
> +
> +    if (!table) {
> +        return 0;
> +    }
> +
> +    nr_vectors = (ldl_phys(&address_space_memory, table) >> 24) + 1;
> +    if (!vector || vector > nr_vectors) {
> +        return 0;
> +    }
> +    table++; /* skip nr option vectors */
> +
> +    for (i = 0; i < vector - 1; i++) {
> +        nr_entries = ldl_phys(&address_space_memory, table) >> 24;
> +        table += nr_entries + 2;
> +    }
> +    return table;
> +}
> +
>  typedef struct {
>      PowerPCCPU *cpu;
>      uint32_t cpu_version;
> @@ -828,19 +854,22 @@ static void do_set_compat(void *arg)
>      ((cpuver) == CPU_POWERPC_LOGICAL_2_06_PLUS) ? 2061 : \
>      ((cpuver) == CPU_POWERPC_LOGICAL_2_07) ? 2070 : 0)
>  
> +#define OV5_DRCONF_MEMORY 0x20
> +
>  static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>                                                    sPAPRMachineState *spapr,
>                                                    target_ulong opcode,
>                                                    target_ulong *args)
>  {
> -    target_ulong list = args[0];
> +    target_ulong list = args[0], ov_table;
>      PowerPCCPUClass *pcc_ = POWERPC_CPU_GET_CLASS(cpu_);
>      CPUState *cs;
> -    bool cpu_match = false;
> +    bool cpu_match = false, cpu_update = true, memory_update = false;
>      unsigned old_cpu_version = cpu_->cpu_version;
>      unsigned compat_lvl = 0, cpu_version = 0;
>      unsigned max_lvl = get_compat_level(cpu_->max_compat);
>      int counter;
> +    char ov5_byte2;
>  
>      /* Parse PVR list */
>      for (counter = 0; counter < 512; ++counter) {
> @@ -890,8 +919,6 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>          }
>      }
>  
> -    /* For the future use: here @list points to the first capability */
> -
>      /* Parsing finished */
>      trace_spapr_cas_pvr(cpu_->cpu_version, cpu_match,
>                          cpu_version, pcc_->pcr_mask);
> @@ -915,14 +942,26 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>      }
>  
>      if (!cpu_version) {
> -        return H_SUCCESS;
> +        cpu_update = false;
>      }
>  
> +    /* For the future use: here @ov_table points to the first option vector */
> +    ov_table = list;
> +
> +    list = cas_get_option_vector(5, ov_table);
>      if (!list) {
>          return H_SUCCESS;
>      }
>  
> -    if (spapr_h_cas_compose_response(spapr, args[1], args[2])) {
> +    /* @list now points to OV 5 */
> +    list += 2;
> +    ov5_byte2 = rtas_ld(list, 0) >> 24;
> +    if (ov5_byte2 & OV5_DRCONF_MEMORY) {
> +        memory_update = true;
> +    }
> +
> +    if (spapr_h_cas_compose_response(spapr, args[1], args[2],
> +                                     cpu_update, memory_update)) {
>          qemu_system_reset_request();
>      }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index b3fba76..2d6921a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -581,7 +581,8 @@ struct sPAPREventLogEntry {
>  void spapr_events_init(sPAPRMachineState *sm);
>  void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
>  int spapr_h_cas_compose_response(sPAPRMachineState *sm,
> -                                 target_ulong addr, target_ulong size);
> +                                 target_ulong addr, target_ulong size,
> +                                 bool cpu_update, bool memory_update);
>  sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>                                     uint64_t bus_offset,
>                                     uint32_t page_shift,
> @@ -623,4 +624,16 @@ int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
>  /* 1GB alignment for hotplug memory region */
>  #define SPAPR_HOTPLUG_MEM_ALIGN (1ULL << 30)
>  
> +/*
> + * Number of 32 bit words in each LMB list entry in ibm,dynamic-memory
> + * property under ibm,dynamic-reconfiguration-memory node.
> + */
> +#define SPAPR_DR_LMB_LIST_ENTRY_SIZE 6
> +
> +/*
> + * This flag value defines the LMB as assigned in ibm,dynamic-memory
> + * property under ibm,dynamic-reconfiguration-memory node.
> + */
> +#define SPAPR_LMB_FLAGS_ASSIGNED 0x00000008
> +
>  #endif /* !defined (__HW_SPAPR_H__) */

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v4 3/5] spapr: Support ibm, dynamic-reconfiguration-memory

[Bharata B Rao]

On Tue, Jun 23, 2015 at 11:54:29AM +1000, David Gibson wrote:
> On Fri, Jun 19, 2015 at 03:47:55PM +0530, Bharata B Rao wrote:
> > Parse ibm,architecture.vec table obtained from the guest and enable
> > memory node configuration via ibm,dynamic-reconfiguration-memory if guest
> > supports it. This is in preparation to support memory hotplug for
> > sPAPR guests.
> > 
> > This changes the way memory node configuration is done. Currently all
> > memory nodes are built upfront. But after this patch, only memory@0 node
> > for RMA is built upfront. Guest kernel boots with just that and rest of
> > the memory nodes (via memory@XXX or ibm,dynamic-reconfiguration-memory)
> > are built when guest does ibm,client-architecture-support call.
> > 
> > Note: This patch needs a SLOF enhancement which is already part of
> > SLOF binary in QEMU.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> [snip]
> > +int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
> > +                                 target_ulong addr, target_ulong size,
> > +                                 bool cpu_update, bool memory_update)
> > +{
> > +    void *fdt, *fdt_skel;
> > +    sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> > +
> > +    size -= sizeof(hdr);
> > +
> > +    /* Create sceleton */
> > +    fdt_skel = g_malloc0(size);
> > +    _FDT((fdt_create(fdt_skel, size)));
> > +    _FDT((fdt_begin_node(fdt_skel, "")));
> > +    _FDT((fdt_end_node(fdt_skel)));
> > +    _FDT((fdt_finish(fdt_skel)));
> > +    fdt = g_malloc0(size);
> > +    _FDT((fdt_open_into(fdt_skel, fdt, size)));
> > +    g_free(fdt_skel);
> > +
> > +    /* Fixup cpu nodes */
> > +    if (cpu_update) {
> > +        _FDT((spapr_fixup_cpu_dt(fdt, spapr)));
> > +    }
> 
> The cpu_update parameter seems like its not related to memory hotplug
> at all.  I'm guessing it relates to CPU hotplug, in which case please
> defer it until those patches are ready to go.

This change isn't related to cpu hotplug. Earlier this compose response
routine only did CPU device tree fixup based on some conditions. I have
enabled it to check for availability DRCONF_MEMORY feature and accordingly
fixup memory DT. So this change just checks if cpu fixup is necessary
or not. Essentially we aren't changing any behaviour wrt cpu dt fixup here.

> 
> > +
> > +    /* Generate memory nodes or ibm,dynamic-reconfiguration-memory node */
> > +    if (memory_update && spapr->dr_lmb_enabled) {
> > +        _FDT((spapr_populate_drconf_memory(spapr, fdt)));
> > +    } else {
> > +        _FDT((spapr_populate_memory(spapr, fdt)));
> > +    }
> > +
> > +    /* Pack resulting tree */
> > +    _FDT((fdt_pack(fdt)));
> > +
> > +    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> > +        trace_spapr_cas_failed(size);
> > +        return -1;
> > +    }
> > +
> > +    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
> > +    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
> > +    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> > +    g_free(fdt);
> > +
> > +    return 0;
> > +}
> > +
> >  static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> >                                 hwaddr fdt_addr,
> >                                 hwaddr rtas_addr,
> > @@ -756,10 +866,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> >      /* open out the base tree into a temp buffer for the final tweaks */
> >      _FDT((fdt_open_into(spapr->fdt_skel, fdt, FDT_MAX_SIZE)));
> >  
> > -    ret = spapr_populate_memory(spapr, fdt);
> > -    if (ret < 0) {
> > -        fprintf(stderr, "couldn't setup memory nodes in fdt\n");
> > -        exit(1);
> > +    /*
> > +     * Add memory@0 node to represent RMA. Rest of the memory is either
> > +     * represented by memory nodes or ibm,dynamic-reconfiguration-memory
> > +     * node later during ibm,client-architecture-support call.
> > +     */
> > +    for (i = 0; i < nb_numa_nodes; ++i) {
> > +        if (numa_info[i].node_mem) {
> > +            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> > +            break;
> > +        }
> 
> ?? The code doesn't seem to match the comment - you appear to be
> creating a memory@0 node for every NUMA node, not just for the RMA,
> which doesn't make much sense.

I have a break there to ensure memory@0 is created only once from the 1st
memory-less node. I am slightly changing this in next version to ensure
that this works correctly even when -numa isn't specified.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v4 3/5] spapr: Support ibm, dynamic-reconfiguration-memory

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5298 bytes --]

On Wed, Jun 24, 2015 at 07:55:44AM +0530, Bharata B Rao wrote:
> On Tue, Jun 23, 2015 at 11:54:29AM +1000, David Gibson wrote:
> > On Fri, Jun 19, 2015 at 03:47:55PM +0530, Bharata B Rao wrote:
> > > Parse ibm,architecture.vec table obtained from the guest and enable
> > > memory node configuration via ibm,dynamic-reconfiguration-memory if guest
> > > supports it. This is in preparation to support memory hotplug for
> > > sPAPR guests.
> > > 
> > > This changes the way memory node configuration is done. Currently all
> > > memory nodes are built upfront. But after this patch, only memory@0 node
> > > for RMA is built upfront. Guest kernel boots with just that and rest of
> > > the memory nodes (via memory@XXX or ibm,dynamic-reconfiguration-memory)
> > > are built when guest does ibm,client-architecture-support call.
> > > 
> > > Note: This patch needs a SLOF enhancement which is already part of
> > > SLOF binary in QEMU.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > [snip]
> > > +int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
> > > +                                 target_ulong addr, target_ulong size,
> > > +                                 bool cpu_update, bool memory_update)
> > > +{
> > > +    void *fdt, *fdt_skel;
> > > +    sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> > > +
> > > +    size -= sizeof(hdr);
> > > +
> > > +    /* Create sceleton */
> > > +    fdt_skel = g_malloc0(size);
> > > +    _FDT((fdt_create(fdt_skel, size)));
> > > +    _FDT((fdt_begin_node(fdt_skel, "")));
> > > +    _FDT((fdt_end_node(fdt_skel)));
> > > +    _FDT((fdt_finish(fdt_skel)));
> > > +    fdt = g_malloc0(size);
> > > +    _FDT((fdt_open_into(fdt_skel, fdt, size)));
> > > +    g_free(fdt_skel);
> > > +
> > > +    /* Fixup cpu nodes */
> > > +    if (cpu_update) {
> > > +        _FDT((spapr_fixup_cpu_dt(fdt, spapr)));
> > > +    }
> > 
> > The cpu_update parameter seems like its not related to memory hotplug
> > at all.  I'm guessing it relates to CPU hotplug, in which case please
> > defer it until those patches are ready to go.
> 
> This change isn't related to cpu hotplug. Earlier this compose response
> routine only did CPU device tree fixup based on some conditions. I have
> enabled it to check for availability DRCONF_MEMORY feature and accordingly
> fixup memory DT. So this change just checks if cpu fixup is necessary
> or not. Essentially we aren't changing any behaviour wrt cpu dt
> fixup here.

Hm, ok.  Would there be any problem with just unconditionally doing
both fixups?  This is about as far from a hot path as its possible to
get.


> 
> > 
> > > +
> > > +    /* Generate memory nodes or ibm,dynamic-reconfiguration-memory node */
> > > +    if (memory_update && spapr->dr_lmb_enabled) {
> > > +        _FDT((spapr_populate_drconf_memory(spapr, fdt)));
> > > +    } else {
> > > +        _FDT((spapr_populate_memory(spapr, fdt)));
> > > +    }
> > > +
> > > +    /* Pack resulting tree */
> > > +    _FDT((fdt_pack(fdt)));
> > > +
> > > +    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> > > +        trace_spapr_cas_failed(size);
> > > +        return -1;
> > > +    }
> > > +
> > > +    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
> > > +    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
> > > +    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> > > +    g_free(fdt);
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> > >                                 hwaddr fdt_addr,
> > >                                 hwaddr rtas_addr,
> > > @@ -756,10 +866,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> > >      /* open out the base tree into a temp buffer for the final tweaks */
> > >      _FDT((fdt_open_into(spapr->fdt_skel, fdt, FDT_MAX_SIZE)));
> > >  
> > > -    ret = spapr_populate_memory(spapr, fdt);
> > > -    if (ret < 0) {
> > > -        fprintf(stderr, "couldn't setup memory nodes in fdt\n");
> > > -        exit(1);
> > > +    /*
> > > +     * Add memory@0 node to represent RMA. Rest of the memory is either
> > > +     * represented by memory nodes or ibm,dynamic-reconfiguration-memory
> > > +     * node later during ibm,client-architecture-support call.
> > > +     */
> > > +    for (i = 0; i < nb_numa_nodes; ++i) {
> > > +        if (numa_info[i].node_mem) {
> > > +            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> > > +            break;
> > > +        }
> > 
> > ?? The code doesn't seem to match the comment - you appear to be
> > creating a memory@0 node for every NUMA node, not just for the RMA,
> > which doesn't make much sense.
> 
> I have a break there to ensure memory@0 is created only once from the 1st
> memory-less node. I am slightly changing this in next version to ensure
> that this works correctly even when -numa isn't specified.

Ah, sorry, I missed the break;.  That should be ok then.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v4 3/5] spapr: Support ibm, dynamic-reconfiguration-memory

[Bharata B Rao]

On Wed, Jun 24, 2015 at 03:55:08PM +1000, David Gibson wrote:
> On Wed, Jun 24, 2015 at 07:55:44AM +0530, Bharata B Rao wrote:
> > On Tue, Jun 23, 2015 at 11:54:29AM +1000, David Gibson wrote:
> > > On Fri, Jun 19, 2015 at 03:47:55PM +0530, Bharata B Rao wrote:
> > > > Parse ibm,architecture.vec table obtained from the guest and enable
> > > > memory node configuration via ibm,dynamic-reconfiguration-memory if guest
> > > > supports it. This is in preparation to support memory hotplug for
> > > > sPAPR guests.
> > > > 
> > > > This changes the way memory node configuration is done. Currently all
> > > > memory nodes are built upfront. But after this patch, only memory@0 node
> > > > for RMA is built upfront. Guest kernel boots with just that and rest of
> > > > the memory nodes (via memory@XXX or ibm,dynamic-reconfiguration-memory)
> > > > are built when guest does ibm,client-architecture-support call.
> > > > 
> > > > Note: This patch needs a SLOF enhancement which is already part of
> > > > SLOF binary in QEMU.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > 
> > > [snip]
> > > > +int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
> > > > +                                 target_ulong addr, target_ulong size,
> > > > +                                 bool cpu_update, bool memory_update)
> > > > +{
> > > > +    void *fdt, *fdt_skel;
> > > > +    sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> > > > +
> > > > +    size -= sizeof(hdr);
> > > > +
> > > > +    /* Create sceleton */
> > > > +    fdt_skel = g_malloc0(size);
> > > > +    _FDT((fdt_create(fdt_skel, size)));
> > > > +    _FDT((fdt_begin_node(fdt_skel, "")));
> > > > +    _FDT((fdt_end_node(fdt_skel)));
> > > > +    _FDT((fdt_finish(fdt_skel)));
> > > > +    fdt = g_malloc0(size);
> > > > +    _FDT((fdt_open_into(fdt_skel, fdt, size)));
> > > > +    g_free(fdt_skel);
> > > > +
> > > > +    /* Fixup cpu nodes */
> > > > +    if (cpu_update) {
> > > > +        _FDT((spapr_fixup_cpu_dt(fdt, spapr)));
> > > > +    }
> > > 
> > > The cpu_update parameter seems like its not related to memory hotplug
> > > at all.  I'm guessing it relates to CPU hotplug, in which case please
> > > defer it until those patches are ready to go.
> > 
> > This change isn't related to cpu hotplug. Earlier this compose response
> > routine only did CPU device tree fixup based on some conditions. I have
> > enabled it to check for availability DRCONF_MEMORY feature and accordingly
> > fixup memory DT. So this change just checks if cpu fixup is necessary
> > or not. Essentially we aren't changing any behaviour wrt cpu dt
> > fixup here.
> 
> Hm, ok.  Would there be any problem with just unconditionally doing
> both fixups?  This is about as far from a hot path as its possible to
> get.

Right now I don't fully understand under what circumstances cpu fixup
will be done. What I can deduce is that it is conditionally done from
ibm,client-architecture-support call and I have just ensured that I
retain the same behaviour with this change.

Regards,
Bharata.

[Qemu-devel] [RFC PATCH v4 4/5] spapr: Make hash table size a factor of maxram_size

[Bharata B Rao]

The hash table size is dependent on ram_size, but since with hotplug
the memory can grow till maxram_size. Hence make hash table size dependent
on maxram_size.

This allows to hotplug huge amounts of memory to the guest.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2389061..24e5a8c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1689,7 +1689,7 @@ static void ppc_spapr_init(MachineState *machine)
      * more than needed for the Linux guests we support. */
     spapr->htab_shift = 18; /* Minimum architected size */
     while (spapr->htab_shift <= 46) {
-        if ((1ULL << (spapr->htab_shift + 7)) >= machine->ram_size) {
+        if ((1ULL << (spapr->htab_shift + 7)) >= machine->maxram_size) {
             break;
         }
         spapr->htab_shift++;
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v4 4/5] spapr: Make hash table size a factor of maxram_size

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On Fri, Jun 19, 2015 at 03:47:56PM +0530, Bharata B Rao wrote:
> The hash table size is dependent on ram_size, but since with hotplug
> the memory can grow till maxram_size. Hence make hash table size dependent
> on maxram_size.
> 
> This allows to hotplug huge amounts of memory to the guest.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [RFC PATCH v4 5/5] spapr: Memory hotplug support

[Bharata B Rao]

Make use of pc-dimm infrastructure to support memory hotplug
for PowerPC.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c        | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_events.c |   8 ++--
 2 files changed, 131 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 24e5a8c..173f639 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -33,6 +33,7 @@
 #include "sysemu/block-backend.h"
 #include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
+#include "sysemu/device_tree.h"
 #include "kvm_ppc.h"
 #include "mmu-hash64.h"
 #include "qom/cpu.h"
@@ -930,6 +931,10 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
         spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
     }
 
+    if (spapr->dr_lmb_enabled) {
+        _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
+    }
+
     _FDT((fdt_pack(fdt)));
 
     if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
@@ -2037,12 +2042,129 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
+                           uint32_t node, Error **errp)
+{
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
+    int i, fdt_offset, fdt_size;
+    void *fdt;
+    Error *local_err = NULL;
+
+    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
+        error_setg(errp, "Hotplugged memory size must be a multiple of "
+                      "%lld MB", SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
+        return;
+    }
+
+    /*
+     * Check for DRC connectors and send hotplug notification to the
+     * guest only in case of hotplugged memory. This allows cold plugged
+     * memory to be specified at boot time.
+     */
+    if (!dev->hotplugged) {
+        return;
+    }
+
+    for (i = 0; i < nr_lmbs; i++) {
+        drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
+                addr/SPAPR_MEMORY_BLOCK_SIZE);
+        g_assert(drc);
+
+        fdt = create_device_tree(&fdt_size);
+        fdt_offset = spapr_populate_memory_node(fdt, node, addr,
+                                                SPAPR_MEMORY_BLOCK_SIZE);
+
+        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+        drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
+        if (local_err) {
+            g_free(fdt);
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        spapr_hotplug_req_add_event(drc);
+        addr += SPAPR_MEMORY_BLOCK_SIZE;
+    }
+}
+
+static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                              uint32_t node, Error **errp)
+{
+    Error *local_err = NULL;
+    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
+    uint64_t align = memory_region_get_alignment(mr);
+    uint64_t addr;
+
+    pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    spapr_add_lmbs(dev, addr, memory_region_size(mr), node, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    return;
+
+out:
+    pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
+    error_propagate(errp, local_err);
+}
+
+static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        uint32_t node;
+
+        if (!spapr->dr_lmb_enabled) {
+            error_setg(errp, "Memory hotplug not supported for this machine");
+            return;
+        }
+        node = object_property_get_int(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
+        if (*errp) {
+            return;
+        }
+        spapr_memory_plug(hotplug_dev, dev, node, errp);
+    }
+}
+
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        error_setg(errp, "Memory hot unplug not supported by sPAPR");
+    }
+}
+
+static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine,
+                                             DeviceState *dev)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+    return NULL;
+}
+
 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);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = ppc_spapr_init;
     mc->reset = ppc_spapr_reset;
@@ -2053,6 +2175,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 512 * M_BYTE;
     mc->kvm_type = spapr_kvm_type;
     mc->has_dynamic_sysbus = true;
+    mc->get_hotplug_handler = spapr_get_hotpug_handler;
+    hc->plug = spapr_machine_device_plug;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = false;
     fwc->get_dev_path = spapr_get_fw_dev_path;
@@ -2070,6 +2195,7 @@ static const TypeInfo spapr_machine_info = {
     .interfaces = (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 f626eb7..98bf7ae 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -427,14 +427,16 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
     hp->hdr.section_length = cpu_to_be16(sizeof(*hp));
     hp->hdr.section_version = 1; /* includes extended modifier */
     hp->hotplug_action = hp_action;
-
+    hp->drc.index = cpu_to_be32(drck->get_index(drc));
+    hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
 
     switch (drc_type) {
     case SPAPR_DR_CONNECTOR_TYPE_PCI:
-        hp->drc.index = cpu_to_be32(drck->get_index(drc));
-        hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
         hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PCI;
         break;
+    case SPAPR_DR_CONNECTOR_TYPE_LMB:
+        hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
+        break;
     default:
         /* we shouldn't be signaling hotplug events for resources
          * that don't support them
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v4 5/5] spapr: Memory hotplug support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Fri, Jun 19, 2015 at 03:47:57PM +0530, Bharata B Rao wrote:
> Make use of pc-dimm infrastructure to support memory hotplug
> for PowerPC.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]