[Qemu-devel] [PATCH 0/5] s390: Support for Hotplug of Standby Memory

[Qemu-devel] [PATCH 0/5] s390: Support for Hotplug of Standby Memory

[Matthew Rosato]

This patchset adds support in s390 for a pool of standby memory,
which can be set online/offline by the guest (ie, via chmem).
A new option, "standby-mem={size}", is added to the QEMU
command line machine parameter to specify the size of standby
memory for the guest.  As part of this work, additional
results are provided for the Read SCP Information SCLP, and new
implentation is added for the Read Storage Element Information,
Attach Storage Element, Assign Storage and Unassign Storage SCLPs,
which enables the s390 guest to manipulate the standby memory
pool.

This patchset is based on work originally done by Jeng-Fang (Nick)
Wang.

Matthew Rosato (5):
  Add the standby-mem machine option
  virtio-ccw: Include standby memory when calculating storage increment
  target-s390: Check for standby memory specification
  sclp-s390: Define new SCLP codes and structures
  sclp-s390: Add memory hotplug SCLPs

 hw/s390x/s390-virtio-ccw.c |   30 +++++-
 hw/s390x/sclp.c            |  235 ++++++++++++++++++++++++++++++++++++++++++--
 include/hw/s390x/sclp.h    |   48 +++++++++
 qemu-options.hx            |    6 +-
 target-s390x/cpu.h         |    4 +
 target-s390x/kvm.c         |   16 +++
 vl.c                       |    6 ++
 7 files changed, 332 insertions(+), 13 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/5] Add the standby-mem machine option

[Matthew Rosato]

Add the machine=...,standby-mem={size} option and associated
documentation.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 qemu-options.hx |    6 +++++-
 vl.c            |    6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index af34483..def4493 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                kernel_irqchip=on|off controls accelerated irqchip support\n"
     "                kvm_shadow_mem=size of KVM shadow MMU\n"
     "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
-    "                mem-merge=on|off controls memory merge support (default: on)\n",
+    "                mem-merge=on|off controls memory merge support (default: on)\n"
+    "                standby-mem=size sets size of additional offline memory\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
@@ -58,6 +59,9 @@ Include guest memory in a core dump. The default is on.
 Enables or disables memory merge support. This feature, when supported by
 the host, de-duplicates identical memory pages among VMs instances
 (enabled by default).
+@item standby-mem=size
+Defines the size, in megabytes, of additional memory to be left offline for
+future hotplug by the guest.
 @end table
 ETEXI
 
diff --git a/vl.c b/vl.c
index b0399de..7d58d24 100644
--- a/vl.c
+++ b/vl.c
@@ -187,6 +187,7 @@ DisplayType display_type = DT_DEFAULT;
 static int display_remote;
 const char* keyboard_layout = NULL;
 ram_addr_t ram_size;
+ram_addr_t standby_mem_size;
 const char *mem_path = NULL;
 int mem_prealloc = 0; /* force preallocation of physical target memory */
 int nb_nics;
@@ -430,6 +431,10 @@ static QemuOptsList qemu_machine_opts = {
             .name = "firmware",
             .type = QEMU_OPT_STRING,
             .help = "firmware image",
+        },{
+            .name = "standby-mem",
+            .type = QEMU_OPT_SIZE,
+            .help = "standby memory size",
         },
         { /* End of list */ }
     },
@@ -2906,6 +2911,7 @@ int main(int argc, char **argv, char **envp)
     machine = find_default_machine();
     cpu_model = NULL;
     ram_size = 0;
+    standby_mem_size = 0;
     snapshot = 0;
     cyls = heads = secs = 0;
     translation = BIOS_ATA_TRANSLATION_AUTO;
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/5] virtio-ccw: Include standby memory when calculating storage increment

[Matthew Rosato]

When determining the memory increment size, include the standby
memory as well as the core memory.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c |   30 +++++++++++++++++++++++++-----
 target-s390x/cpu.h         |    4 ++++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 733d988..75deb24 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -17,6 +17,8 @@
 #include "css.h"
 #include "virtio-ccw.h"
 
+ram_addr_t pad_size;
+
 void io_subsystem_reset(void)
 {
     DeviceState *css, *sclp;
@@ -84,11 +86,22 @@ static void ccw_init(QEMUMachineInitArgs *args)
     int ret;
     VirtualCssBus *css_bus;
 
-    /* s390x ram size detection needs a 16bit multiplier + an increment. So
-       guests > 64GB can be specified in 2MB steps etc. */
-    while ((my_ram_size >> (20 + shift)) > 65535) {
-        shift++;
+    if (standby_mem_size) {
+        /* The storage increment size is a multiple of 1M and is a power of 2.
+         * The number of storage increments must be 1020 or fewer. */
+        while ((my_ram_size >> (20 + shift)) > 1020) {
+            shift++;
+        }
+        while ((standby_mem_size >> (20 + shift)) > 1020) {
+            shift++;
+        }
+        standby_mem_size = standby_mem_size >> (20 + shift) << (20 + shift);
+    } else {
+        while ((my_ram_size >> (20 + shift)) > 65535) {
+            shift++;
+        }
     }
+
     my_ram_size = my_ram_size >> (20 + shift) << (20 + shift);
 
     /* let's propagate the changed ram size into the global variable. */
@@ -103,11 +116,18 @@ static void ccw_init(QEMUMachineInitArgs *args)
     /* register hypercalls */
     virtio_ccw_register_hcalls();
 
-    /* allocate RAM */
+    /* allocate RAM for core */
     memory_region_init_ram(ram, NULL, "s390.ram", my_ram_size);
     vmstate_register_ram_global(ram);
     memory_region_add_subregion(sysmem, 0, ram);
 
+    if (standby_mem_size) {
+        if (my_ram_size % MEM_SECTION_SIZE) {
+            pad_size = MEM_SECTION_SIZE - my_ram_size % MEM_SECTION_SIZE;
+        }
+        my_ram_size += standby_mem_size + pad_size;
+    }
+
     /* allocate storage keys */
     storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
 
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index a2c077b..3383d55 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -385,6 +385,10 @@ unsigned s390_del_running_cpu(S390CPU *cpu);
 /* service interrupts are floating therefore we must not pass an cpustate */
 void s390_sclp_extint(uint32_t parm);
 
+/* from s390-virtio-ccw */
+#define MEM_SECTION_SIZE             0x10000000UL
+extern ram_addr_t standby_mem_size;
+
 /* from s390-virtio-bus */
 extern const hwaddr virtio_size;
 
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification

[Matthew Rosato]

When machine=...,standby-mem={size} has been specified, convert the value
to bytes and store it for use.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 target-s390x/kvm.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 02ac4ba..d4081f4 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
 
 int kvm_arch_init(KVMState *s)
 {
+    int64_t value;
+
     cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
     if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
         || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
         phys_mem_set_alloc(legacy_s390_alloc);
     }
+
+    value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
+
+    if (value < 0) {
+        fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", value);
+        exit(1);
+    }
+
+    if (value != (int64_t)(ram_addr_t)value) {
+        fprintf(stderr, "qemu: standby size too large\n");
+        exit(1);
+    }
+    standby_mem_size = value * 1024 * 1024;
+
     return 0;
 }
 
-- 
1.7.9.5

[Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures

[Matthew Rosato]

Define new SCLP codes and structures that will be needed for s390 memory
hotplug.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/sclp.c         |    2 +-
 include/hw/s390x/sclp.h |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 86d6ae0..cb53d7e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
 {
     S390SCLPDevice *sdev = get_event_facility();
 
-    switch (code) {
+    switch (code & SCLP_NO_CMD_PARM) {
     case SCLP_CMDW_READ_SCP_INFO:
     case SCLP_CMDW_READ_SCP_INFO_FORCED:
         read_SCP_info(sccb);
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 231a38a..e80cb23 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -20,18 +20,31 @@
 /* SCLP command codes */
 #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
 #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
+#define SCLP_READ_STORAGE_ELEMENT_INFO          0x00040001
+#define SCLP_ATTACH_STORAGE_ELEMENT             0x00080001
+#define SCLP_ASSIGN_STORAGE                     0x000D0001
+#define SCLP_UNASSIGN_STORAGE                   0x000C0001
 #define SCLP_CMD_READ_EVENT_DATA                0x00770005
 #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
 #define SCLP_CMD_READ_EVENT_DATA                0x00770005
 #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
 #define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
 
+/* SCLP Memory hotplug codes */
+#define SCLP_NO_CMD_PARM                        0xffff00ff
+#define SCLP_FC_ASSIGN_ATTACH_READ_STOR         0xE00000000000ULL
+#define SCLP_STARTING_SUBINCREMENT_ID           0x10001
+#define SCLP_INCREMENT_UNIT                     0x10000
+#define MAX_AVAIL_SLOTS                         32
+
 /* SCLP response codes */
 #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
 #define SCLP_RC_NORMAL_COMPLETION               0x0020
+#define SCLP_RC_SCCB_BOUNDARY_VIOLATION         0x0100
 #define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
 #define SCLP_RC_CONTAINED_EQUIPMENT_CHECK       0x0340
 #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
+#define SCLP_RC_STANDBY_READ_COMPLETION         0x0410
 #define SCLP_RC_INVALID_FUNCTION                0x40f0
 #define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
 #define SCLP_RC_INVALID_SELECTION_MASK          0x70f0
@@ -75,8 +88,41 @@ typedef struct ReadInfo {
     SCCBHeader h;
     uint16_t rnmax;
     uint8_t rnsize;
+    uint8_t  _reserved1[16 - 11];       /* 11-15 */
+    uint16_t entries_cpu;               /* 16-17 */
+    uint16_t offset_cpu;                /* 18-19 */
+    uint8_t  _reserved2[24 - 20];       /* 20-23 */
+    uint8_t  loadparm[8];               /* 24-31 */
+    uint8_t  _reserved3[48 - 32];       /* 32-47 */
+    uint64_t facilities;                /* 48-55 */
+    uint8_t  _reserved0[100 - 56];
+    uint32_t rnsize2;
+    uint64_t rnmax2;
 } QEMU_PACKED ReadInfo;
 
+typedef struct ReadStorageElementInfo {
+    SCCBHeader h;
+    uint16_t max_id;
+    uint16_t assigned;
+    uint16_t standby;
+    uint8_t _reserved0[16 - 14]; /* 14-15 */
+    uint32_t entries[0];
+} QEMU_PACKED ReadStorageElementInfo;
+
+typedef struct AttachStorageElement {
+    SCCBHeader h;
+    uint8_t _reserved0[10 - 8];  /* 8-9 */
+    uint16_t assigned;
+    uint8_t _reserved1[16 - 12]; /* 12-15 */
+    uint32_t entries[0];
+} QEMU_PACKED AttachStorageElement;
+
+typedef struct AssignStorage {
+    SCCBHeader h;
+    uint16_t rn;
+} QEMU_PACKED AssignStorage;
+
+
 typedef struct SCCB {
     SCCBHeader h;
     char data[SCCB_DATA_LEN];
-- 
1.7.9.5

[Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs

[Matthew Rosato]

Add memory information to read SCP info and add handlers for
Read Storage Element Information, Attach Storage Element,
Assign Storage and Unassign Storage.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 hw/s390x/sclp.c         |  233 +++++++++++++++++++++++++++++++++++++++++++++--
 include/hw/s390x/sclp.h |    2 +
 2 files changed, 229 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index cb53d7e..5d27fc3 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -15,9 +15,15 @@
 #include "cpu.h"
 #include "sysemu/kvm.h"
 #include "exec/memory.h"
-
+#include "exec/address-spaces.h"
 #include "hw/s390x/sclp.h"
 
+int shift;
+ram_addr_t standby_subregion_size;
+ram_addr_t padded_ram_size;
+ram_addr_t rzm;
+char *standby_state_map;
+
 static inline S390SCLPDevice *get_event_facility(void)
 {
     ObjectProperty *op = object_property_find(qdev_get_machine(),
@@ -31,16 +37,215 @@ static inline S390SCLPDevice *get_event_facility(void)
 static void read_SCP_info(SCCB *sccb)
 {
     ReadInfo *read_info = (ReadInfo *) sccb;
-    int shift = 0;
+    int rnsize, rnmax;
+    int max_avail_slots = MAX_AVAIL_SLOTS;
+#ifdef CONFIG_KVM
+    max_avail_slots = kvm_check_extension(kvm_state, KVM_CAP_NR_MEMSLOTS);
+#endif
+
+    read_info->facilities = 0;
+
+    if (standby_mem_size) {
+        /*
+         * The storage increment size is a multiple of 1M and is a power of 2.
+         * The number of storage increments must be 1020 or fewer.
+         */
+        while ((ram_size >> (20 + shift)) > 1020) {
+            shift++;
+        }
+        while ((standby_mem_size >> (20 + shift)) > 1020) {
+            shift++;
+        }
+
+        standby_subregion_size = MEM_SECTION_SIZE;
+        /* Deduct the memory slot already used for core */
+        while ((standby_subregion_size * (max_avail_slots - 1)
+               < standby_mem_size)) {
+            standby_subregion_size = standby_subregion_size << 1;
+        }
+        /*
+         * Initialize mapping of guest standby memory sections indicating which
+         * are and are not online. Assume all standby memory begins offline.
+         */
+        if (standby_mem_size % standby_subregion_size) {
+            standby_state_map = g_malloc0((standby_mem_size /
+                                           standby_subregion_size + 1) *
+                                          (standby_subregion_size /
+                                           MEM_SECTION_SIZE));
+        } else {
+            standby_state_map = g_malloc0(standby_mem_size / MEM_SECTION_SIZE);
+        }
+
+        padded_ram_size = ram_size + pad_size;
+        rzm = 1 << (20 + shift);
+
+    } else {
+        while ((ram_size >> (20 + shift)) > 65535) {
+            shift++;
+        }
+    }
 
-    while ((ram_size >> (20 + shift)) > 65535) {
-        shift++;
+    rnsize = 1 << shift;
+    if (rnsize <= 128) {
+        read_info->rnsize = rnsize;
+    } else {
+        read_info->rnsize = 0;
+        read_info->rnsize2 = cpu_to_be32(rnsize);
+    }
+
+    rnmax = (ram_size + standby_mem_size + pad_size) >> (20 + shift);
+    if (rnmax < 0x10000) {
+        read_info->rnmax = cpu_to_be16(rnmax);
+    } else {
+        read_info->rnmax = cpu_to_be16(0);
+        read_info->rnmax2 = cpu_to_be64(rnmax);
+    }
+
+    if (standby_mem_size) {
+        read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
     }
-    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
-    read_info->rnsize = 1 << shift;
     sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
 }
 
+static void read_storage_element0_info(SCCB *sccb)
+{
+    int i, assigned;
+    int subincrement_id = SCLP_STARTING_SUBINCREMENT_ID;
+    ReadStorageElementInfo *storage_info = (ReadStorageElementInfo *) sccb;
+
+    if ((ram_size >> (20 + shift)) >= 0x10000) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+        return;
+    }
+
+    storage_info->max_id = cpu_to_be16(standby_mem_size ? 1 : 0);
+    assigned = ram_size >> (20 + shift);
+    storage_info->assigned = cpu_to_be16(assigned);
+
+    for (i = 0; i < assigned; i++) {
+        storage_info->entries[i] = cpu_to_be32(subincrement_id);
+        subincrement_id += SCLP_INCREMENT_UNIT;
+    }
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
+}
+
+static void read_storage_element1_info(SCCB *sccb)
+{
+    ReadStorageElementInfo *storage_info = (ReadStorageElementInfo *) sccb;
+
+    if ((standby_mem_size >> (20 + shift)) >= 0x10000) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+        return;
+    }
+
+    storage_info->max_id = cpu_to_be16(standby_mem_size ? 1 : 0);
+    storage_info->assigned = cpu_to_be16(standby_mem_size >> (20 + shift));
+    storage_info->standby = cpu_to_be16(standby_mem_size >> (20 + shift));
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_STANDBY_READ_COMPLETION);
+}
+
+static void attach_storage_element(SCCB *sccb, uint16_t element)
+{
+    int i, assigned, subincrement_id;
+    AttachStorageElement *attach_info = (AttachStorageElement *) sccb;
+
+    if (element != 1) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        return;
+    }
+
+    assigned = standby_mem_size >> (20 + shift);
+    attach_info->assigned = cpu_to_be16(assigned);
+    subincrement_id = ((ram_size >> (20 + shift)) << 16)
+                      + SCLP_STARTING_SUBINCREMENT_ID;
+    for (i = 0; i < assigned; i++) {
+        attach_info->entries[i] = cpu_to_be32(subincrement_id);
+        subincrement_id += SCLP_INCREMENT_UNIT;
+    }
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+}
+
+static void assign_storage(SCCB *sccb)
+{
+    MemoryRegion *mr = NULL;
+    int this_subregion_size;
+    AssignStorage *assign_info = (AssignStorage *) sccb;
+    ram_addr_t assign_addr = (assign_info->rn - 1) * rzm;
+    MemoryRegion *sysmem = get_system_memory();
+
+    if ((assign_addr % MEM_SECTION_SIZE == 0) &&
+        (assign_addr >= padded_ram_size)) {
+        mr = memory_region_find(sysmem, assign_addr, 1).mr;
+        if (!mr) {
+
+            MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
+
+            /* offset to align to standby_subregion_size for allocation */
+            ram_addr_t offset = assign_addr -
+                                (assign_addr - padded_ram_size)
+                                % standby_subregion_size;
+
+            /* strlen("standby.ram") + 4 (Max of KVM_MEMORY_SLOTS) +  NULL */
+            char id[16];
+            snprintf(id, 16, "standby.ram%d", (int)((offset - padded_ram_size)
+                     / standby_subregion_size) + 1);
+
+            /* Allocate a subregion of the calculated standby_subregion_size */
+            if (offset + standby_subregion_size >
+                padded_ram_size + standby_mem_size) {
+                this_subregion_size = padded_ram_size + standby_mem_size
+                                      - offset;
+            } else {
+                this_subregion_size = standby_subregion_size;
+            }
+
+            memory_region_init_ram(standby_ram, NULL, id, this_subregion_size);
+            vmstate_register_ram_global(standby_ram);
+            memory_region_add_subregion(sysmem, offset, standby_ram);
+        }
+        standby_state_map[(assign_addr - padded_ram_size)
+                          / MEM_SECTION_SIZE] = 1;
+    }
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+}
+
+static void unassign_storage(SCCB *sccb)
+{
+    MemoryRegion *mr = NULL;
+    AssignStorage *assign_info = (AssignStorage *) sccb;
+    ram_addr_t unassign_addr = (assign_info->rn - 1) * rzm;
+    MemoryRegion *sysmem = get_system_memory();
+
+    /* if the addr is a multiple of 256 MB */
+    if ((unassign_addr % MEM_SECTION_SIZE == 0) &&
+        (unassign_addr >= padded_ram_size)) {
+        standby_state_map[(unassign_addr -
+                           padded_ram_size) / MEM_SECTION_SIZE] = 0;
+        mr = memory_region_find(sysmem, unassign_addr, 1).mr;
+        if (mr) {
+            int i;
+            int is_removable = 1;
+            ram_addr_t map_offset = (unassign_addr - padded_ram_size -
+                                     (unassign_addr - padded_ram_size)
+                                     % standby_subregion_size);
+            for (i = 0;
+                 i < (standby_subregion_size / MEM_SECTION_SIZE);
+                 i++) {
+
+                if (standby_state_map[i + map_offset / MEM_SECTION_SIZE]) {
+                    is_removable = 0;
+                    break;
+                }
+            }
+            if (is_removable) {
+                memory_region_del_subregion(sysmem, mr);
+                memory_region_destroy(mr);
+            }
+        }
+    }
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+}
+
 static void sclp_execute(SCCB *sccb, uint64_t code)
 {
     S390SCLPDevice *sdev = get_event_facility();
@@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
     case SCLP_CMDW_READ_SCP_INFO_FORCED:
         read_SCP_info(sccb);
         break;
+    case SCLP_READ_STORAGE_ELEMENT_INFO:
+        if (code & 0xff00) {
+            read_storage_element1_info(sccb);
+        } else {
+            read_storage_element0_info(sccb);
+        }
+        break;
+    case SCLP_ATTACH_STORAGE_ELEMENT:
+        attach_storage_element(sccb, (code & 0xff00) >> 8);
+        break;
+    case SCLP_ASSIGN_STORAGE:
+        assign_storage(sccb);
+        break;
+    case SCLP_UNASSIGN_STORAGE:
+        unassign_storage(sccb);
+        break;
     default:
         sdev->sclp_command_handler(sdev->ef, sccb, code);
         break;
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index e80cb23..1211d69 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -143,6 +143,8 @@ static inline int sccb_data_len(SCCB *sccb)
      OBJECT_GET_CLASS(S390SCLPDeviceClass, (obj), \
              TYPE_DEVICE_S390_SCLP)
 
+extern ram_addr_t pad_size;
+
 typedef struct SCLPEventFacility SCLPEventFacility;
 
 typedef struct S390SCLPDevice {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification

[Alexander Graf]

On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:

> When machine=...,standby-mem={size} has been specified, convert the value
> to bytes and store it for use.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> target-s390x/kvm.c |   16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> 
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 02ac4ba..d4081f4 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
> 
> int kvm_arch_init(KVMState *s)
> {
> +    int64_t value;
> +
>     cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>     if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>         || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>         phys_mem_set_alloc(legacy_s390_alloc);
>     }
> +
> +    value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
> +
> +    if (value < 0) {
> +        fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", value);
> +        exit(1);
> +    }
> +
> +    if (value != (int64_t)(ram_addr_t)value) {
> +        fprintf(stderr, "qemu: standby size too large\n");
> +        exit(1);
> +    }
> +    standby_mem_size = value * 1024 * 1024;

I would hope qemu_opt_get_size() returns a value in bytes. Why multiply it here?


Alex

Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs

[Alexander Graf]

On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:

> Add memory information to read SCP info and add handlers for
> Read Storage Element Information, Attach Storage Element,
> Assign Storage and Unassign Storage.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> hw/s390x/sclp.c         |  233 +++++++++++++++++++++++++++++++++++++++++++++--
> include/hw/s390x/sclp.h |    2 +
> 2 files changed, 229 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index cb53d7e..5d27fc3 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -15,9 +15,15 @@
> #include "cpu.h"
> #include "sysemu/kvm.h"
> #include "exec/memory.h"
> -
> +#include "exec/address-spaces.h"
> #include "hw/s390x/sclp.h"
> 
> +int shift;
> +ram_addr_t standby_subregion_size;
> +ram_addr_t padded_ram_size;
> +ram_addr_t rzm;
> +char *standby_state_map;

Do you really need all these globals?

> +
> static inline S390SCLPDevice *get_event_facility(void)
> {
>     ObjectProperty *op = object_property_find(qdev_get_machine(),
> @@ -31,16 +37,215 @@ static inline S390SCLPDevice *get_event_facility(void)
> static void read_SCP_info(SCCB *sccb)
> {
>     ReadInfo *read_info = (ReadInfo *) sccb;
> -    int shift = 0;
> +    int rnsize, rnmax;
> +    int max_avail_slots = MAX_AVAIL_SLOTS;
> +#ifdef CONFIG_KVM
> +    max_avail_slots = kvm_check_extension(kvm_state, KVM_CAP_NR_MEMSLOTS);

Please don't call kvm specific code from non-kvm specific code. Check out kvm_s390_io_interrupt() as an example how to plumb it in. Please also check kvm_enabled(), as CONFIG_KVM doesn't mean that you actually end up using KVM. It's probably best to completely abstract this away in a helper.

> +#endif
> +
> +    read_info->facilities = 0;
> +
> +    if (standby_mem_size) {

I don't understand why we have 2 code paths - one for standby and one without. Can't we just handle all cases as standby?

> +        /*
> +         * The storage increment size is a multiple of 1M and is a power of 2.
> +         * The number of storage increments must be 1020 or fewer.
> +         */
> +        while ((ram_size >> (20 + shift)) > 1020) {
> +            shift++;
> +        }
> +        while ((standby_mem_size >> (20 + shift)) > 1020) {
> +            shift++;
> +        }
> +
> +        standby_subregion_size = MEM_SECTION_SIZE;
> +        /* Deduct the memory slot already used for core */
> +        while ((standby_subregion_size * (max_avail_slots - 1)
> +               < standby_mem_size)) {
> +            standby_subregion_size = standby_subregion_size << 1;
> +        }
> +        /*
> +         * Initialize mapping of guest standby memory sections indicating which
> +         * are and are not online. Assume all standby memory begins offline.
> +         */
> +        if (standby_mem_size % standby_subregion_size) {
> +            standby_state_map = g_malloc0((standby_mem_size /
> +                                           standby_subregion_size + 1) *
> +                                          (standby_subregion_size /
> +                                           MEM_SECTION_SIZE));
> +        } else {
> +            standby_state_map = g_malloc0(standby_mem_size / MEM_SECTION_SIZE);
> +        }

You don't free this thing when the guest calls read_SCP_info twice.

> +
> +        padded_ram_size = ram_size + pad_size;
> +        rzm = 1 << (20 + shift);
> +
> +    } else {
> +        while ((ram_size >> (20 + shift)) > 65535) {
> +            shift++;
> +        }
> +    }
> 
> -    while ((ram_size >> (20 + shift)) > 65535) {
> -        shift++;
> +    rnsize = 1 << shift;
> +    if (rnsize <= 128) {
> +        read_info->rnsize = rnsize;
> +    } else {
> +        read_info->rnsize = 0;
> +        read_info->rnsize2 = cpu_to_be32(rnsize);
> +    }
> +
> +    rnmax = (ram_size + standby_mem_size + pad_size) >> (20 + shift);
> +    if (rnmax < 0x10000) {
> +        read_info->rnmax = cpu_to_be16(rnmax);
> +    } else {
> +        read_info->rnmax = cpu_to_be16(0);
> +        read_info->rnmax2 = cpu_to_be64(rnmax);
> +    }
> +
> +    if (standby_mem_size) {
> +        read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);

Any reason to make this conditional?

>     }
> -    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
> -    read_info->rnsize = 1 << shift;
>     sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> }
> 
> +static void read_storage_element0_info(SCCB *sccb)
> +{
> +    int i, assigned;
> +    int subincrement_id = SCLP_STARTING_SUBINCREMENT_ID;
> +    ReadStorageElementInfo *storage_info = (ReadStorageElementInfo *) sccb;
> +
> +    if ((ram_size >> (20 + shift)) >= 0x10000) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +        return;
> +    }
> +
> +    storage_info->max_id = cpu_to_be16(standby_mem_size ? 1 : 0);
> +    assigned = ram_size >> (20 + shift);
> +    storage_info->assigned = cpu_to_be16(assigned);
> +
> +    for (i = 0; i < assigned; i++) {
> +        storage_info->entries[i] = cpu_to_be32(subincrement_id);
> +        subincrement_id += SCLP_INCREMENT_UNIT;
> +    }
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> +}
> +
> +static void read_storage_element1_info(SCCB *sccb)
> +{
> +    ReadStorageElementInfo *storage_info = (ReadStorageElementInfo *) sccb;
> +
> +    if ((standby_mem_size >> (20 + shift)) >= 0x10000) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +        return;
> +    }
> +
> +    storage_info->max_id = cpu_to_be16(standby_mem_size ? 1 : 0);
> +    storage_info->assigned = cpu_to_be16(standby_mem_size >> (20 + shift));
> +    storage_info->standby = cpu_to_be16(standby_mem_size >> (20 + shift));
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_STANDBY_READ_COMPLETION);
> +}
> +
> +static void attach_storage_element(SCCB *sccb, uint16_t element)
> +{
> +    int i, assigned, subincrement_id;
> +    AttachStorageElement *attach_info = (AttachStorageElement *) sccb;
> +
> +    if (element != 1) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        return;
> +    }
> +
> +    assigned = standby_mem_size >> (20 + shift);
> +    attach_info->assigned = cpu_to_be16(assigned);
> +    subincrement_id = ((ram_size >> (20 + shift)) << 16)
> +                      + SCLP_STARTING_SUBINCREMENT_ID;
> +    for (i = 0; i < assigned; i++) {
> +        attach_info->entries[i] = cpu_to_be32(subincrement_id);
> +        subincrement_id += SCLP_INCREMENT_UNIT;
> +    }
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +}
> +
> +static void assign_storage(SCCB *sccb)
> +{
> +    MemoryRegion *mr = NULL;
> +    int this_subregion_size;
> +    AssignStorage *assign_info = (AssignStorage *) sccb;
> +    ram_addr_t assign_addr = (assign_info->rn - 1) * rzm;
> +    MemoryRegion *sysmem = get_system_memory();
> +
> +    if ((assign_addr % MEM_SECTION_SIZE == 0) &&
> +        (assign_addr >= padded_ram_size)) {
> +        mr = memory_region_find(sysmem, assign_addr, 1).mr;
> +        if (!mr) {
> +
> +            MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
> +
> +            /* offset to align to standby_subregion_size for allocation */
> +            ram_addr_t offset = assign_addr -
> +                                (assign_addr - padded_ram_size)
> +                                % standby_subregion_size;
> +
> +            /* strlen("standby.ram") + 4 (Max of KVM_MEMORY_SLOTS) +  NULL */
> +            char id[16];
> +            snprintf(id, 16, "standby.ram%d", (int)((offset - padded_ram_size)
> +                     / standby_subregion_size) + 1);
> +
> +            /* Allocate a subregion of the calculated standby_subregion_size */
> +            if (offset + standby_subregion_size >
> +                padded_ram_size + standby_mem_size) {
> +                this_subregion_size = padded_ram_size + standby_mem_size
> +                                      - offset;
> +            } else {
> +                this_subregion_size = standby_subregion_size;
> +            }
> +
> +            memory_region_init_ram(standby_ram, NULL, id, this_subregion_size);
> +            vmstate_register_ram_global(standby_ram);
> +            memory_region_add_subregion(sysmem, offset, standby_ram);
> +        }
> +        standby_state_map[(assign_addr - padded_ram_size)
> +                          / MEM_SECTION_SIZE] = 1;
> +    }
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +}
> +
> +static void unassign_storage(SCCB *sccb)
> +{
> +    MemoryRegion *mr = NULL;
> +    AssignStorage *assign_info = (AssignStorage *) sccb;
> +    ram_addr_t unassign_addr = (assign_info->rn - 1) * rzm;
> +    MemoryRegion *sysmem = get_system_memory();
> +
> +    /* if the addr is a multiple of 256 MB */
> +    if ((unassign_addr % MEM_SECTION_SIZE == 0) &&
> +        (unassign_addr >= padded_ram_size)) {
> +        standby_state_map[(unassign_addr -
> +                           padded_ram_size) / MEM_SECTION_SIZE] = 0;
> +        mr = memory_region_find(sysmem, unassign_addr, 1).mr;
> +        if (mr) {
> +            int i;
> +            int is_removable = 1;
> +            ram_addr_t map_offset = (unassign_addr - padded_ram_size -
> +                                     (unassign_addr - padded_ram_size)
> +                                     % standby_subregion_size);
> +            for (i = 0;
> +                 i < (standby_subregion_size / MEM_SECTION_SIZE);
> +                 i++) {
> +
> +                if (standby_state_map[i + map_offset / MEM_SECTION_SIZE]) {
> +                    is_removable = 0;
> +                    break;
> +                }
> +            }
> +            if (is_removable) {
> +                memory_region_del_subregion(sysmem, mr);
> +                memory_region_destroy(mr);

no g_free()?

> +            }
> +        }
> +    }
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +}
> +
> static void sclp_execute(SCCB *sccb, uint64_t code)
> {
>     S390SCLPDevice *sdev = get_event_facility();
> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>     case SCLP_CMDW_READ_SCP_INFO_FORCED:
>         read_SCP_info(sccb);
>         break;
> +    case SCLP_READ_STORAGE_ELEMENT_INFO:
> +        if (code & 0xff00) {
> +            read_storage_element1_info(sccb);
> +        } else {
> +            read_storage_element0_info(sccb);
> +        }
> +        break;
> +    case SCLP_ATTACH_STORAGE_ELEMENT:
> +        attach_storage_element(sccb, (code & 0xff00) >> 8);
> +        break;
> +    case SCLP_ASSIGN_STORAGE:
> +        assign_storage(sccb);
> +        break;
> +    case SCLP_UNASSIGN_STORAGE:
> +        unassign_storage(sccb);
> +        break;

Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls.

Overall, the code could use _a lot_ more comments.


Alex

Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs

[Alexander Graf]

On 16.12.2013, at 22:42, Alexander Graf <agraf@suse.de> wrote:

> 
> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
> 

[...]

> 
>> +            }
>> +        }
>> +    }
>> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
>> +}
>> +
>> static void sclp_execute(SCCB *sccb, uint64_t code)
>> {
>>    S390SCLPDevice *sdev = get_event_facility();
>> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>>    case SCLP_CMDW_READ_SCP_INFO_FORCED:
>>        read_SCP_info(sccb);
>>        break;
>> +    case SCLP_READ_STORAGE_ELEMENT_INFO:
>> +        if (code & 0xff00) {
>> +            read_storage_element1_info(sccb);
>> +        } else {
>> +            read_storage_element0_info(sccb);
>> +        }
>> +        break;
>> +    case SCLP_ATTACH_STORAGE_ELEMENT:
>> +        attach_storage_element(sccb, (code & 0xff00) >> 8);
>> +        break;
>> +    case SCLP_ASSIGN_STORAGE:
>> +        assign_storage(sccb);
>> +        break;
>> +    case SCLP_UNASSIGN_STORAGE:
>> +        unassign_storage(sccb);
>> +        break;
> 
> Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls.

Speaking of state - in the current model the "is standby storage active" bitmap doesn't get migrated, no?


Alex

Re: [Qemu-devel] [PATCH 1/5] Add the standby-mem machine option

[Paolo Bonzini]

Il 16/12/2013 21:51, Matthew Rosato ha scritto:
> Add the machine=...,standby-mem={size} option and associated
> documentation.

See how Igor Mammedov's x86 memory hotplug instead added "-m NN,maxmem=NN".

You could use these two patches:

http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03438.html
http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03439.html

Paolo

> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  qemu-options.hx |    6 +++++-
>  vl.c            |    6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index af34483..def4493 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>      "                kernel_irqchip=on|off controls accelerated irqchip support\n"
>      "                kvm_shadow_mem=size of KVM shadow MMU\n"
>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
> -    "                mem-merge=on|off controls memory merge support (default: on)\n",
> +    "                mem-merge=on|off controls memory merge support (default: on)\n"
> +    "                standby-mem=size sets size of additional offline memory\n",
>      QEMU_ARCH_ALL)
>  STEXI
>  @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
> @@ -58,6 +59,9 @@ Include guest memory in a core dump. The default is on.
>  Enables or disables memory merge support. This feature, when supported by
>  the host, de-duplicates identical memory pages among VMs instances
>  (enabled by default).
> +@item standby-mem=size
> +Defines the size, in megabytes, of additional memory to be left offline for
> +future hotplug by the guest.
>  @end table
>  ETEXI
>  
> diff --git a/vl.c b/vl.c
> index b0399de..7d58d24 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -187,6 +187,7 @@ DisplayType display_type = DT_DEFAULT;
>  static int display_remote;
>  const char* keyboard_layout = NULL;
>  ram_addr_t ram_size;
> +ram_addr_t standby_mem_size;
>  const char *mem_path = NULL;
>  int mem_prealloc = 0; /* force preallocation of physical target memory */
>  int nb_nics;
> @@ -430,6 +431,10 @@ static QemuOptsList qemu_machine_opts = {
>              .name = "firmware",
>              .type = QEMU_OPT_STRING,
>              .help = "firmware image",
> +        },{
> +            .name = "standby-mem",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "standby memory size",
>          },
>          { /* End of list */ }
>      },
> @@ -2906,6 +2911,7 @@ int main(int argc, char **argv, char **envp)
>      machine = find_default_machine();
>      cpu_model = NULL;
>      ram_size = 0;
> +    standby_mem_size = 0;
>      snapshot = 0;
>      cyls = heads = secs = 0;
>      translation = BIOS_ATA_TRANSLATION_AUTO;
> 

Re: [Qemu-devel] [PATCH 1/5] Add the standby-mem machine option

[Matthew Rosato]

On 12/17/2013 05:09 AM, Paolo Bonzini wrote:
> Il 16/12/2013 21:51, Matthew Rosato ha scritto:
>> Add the machine=...,standby-mem={size} option and associated
>> documentation.
> 
> See how Igor Mammedov's x86 memory hotplug instead added "-m NN,maxmem=NN".
> 
> You could use these two patches:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03438.html
> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03439.html
> 
> Paolo

Thanks Paolo, good point.  I'll take a look at this for the next version.

> 
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  qemu-options.hx |    6 +++++-
>>  vl.c            |    6 ++++++
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index af34483..def4493 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>      "                kernel_irqchip=on|off controls accelerated irqchip support\n"
>>      "                kvm_shadow_mem=size of KVM shadow MMU\n"
>>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>> -    "                mem-merge=on|off controls memory merge support (default: on)\n",
>> +    "                mem-merge=on|off controls memory merge support (default: on)\n"
>> +    "                standby-mem=size sets size of additional offline memory\n",
>>      QEMU_ARCH_ALL)
>>  STEXI
>>  @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
>> @@ -58,6 +59,9 @@ Include guest memory in a core dump. The default is on.
>>  Enables or disables memory merge support. This feature, when supported by
>>  the host, de-duplicates identical memory pages among VMs instances
>>  (enabled by default).
>> +@item standby-mem=size
>> +Defines the size, in megabytes, of additional memory to be left offline for
>> +future hotplug by the guest.
>>  @end table
>>  ETEXI
>>  
>> diff --git a/vl.c b/vl.c
>> index b0399de..7d58d24 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -187,6 +187,7 @@ DisplayType display_type = DT_DEFAULT;
>>  static int display_remote;
>>  const char* keyboard_layout = NULL;
>>  ram_addr_t ram_size;
>> +ram_addr_t standby_mem_size;
>>  const char *mem_path = NULL;
>>  int mem_prealloc = 0; /* force preallocation of physical target memory */
>>  int nb_nics;
>> @@ -430,6 +431,10 @@ static QemuOptsList qemu_machine_opts = {
>>              .name = "firmware",
>>              .type = QEMU_OPT_STRING,
>>              .help = "firmware image",
>> +        },{
>> +            .name = "standby-mem",
>> +            .type = QEMU_OPT_SIZE,
>> +            .help = "standby memory size",
>>          },
>>          { /* End of list */ }
>>      },
>> @@ -2906,6 +2911,7 @@ int main(int argc, char **argv, char **envp)
>>      machine = find_default_machine();
>>      cpu_model = NULL;
>>      ram_size = 0;
>> +    standby_mem_size = 0;
>>      snapshot = 0;
>>      cyls = heads = secs = 0;
>>      translation = BIOS_ATA_TRANSLATION_AUTO;
>>
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification

[Matthew Rosato]

On 12/16/2013 04:25 PM, Alexander Graf wrote:
> 
> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
> 
>> When machine=...,standby-mem={size} has been specified, convert the value
>> to bytes and store it for use.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>> target-s390x/kvm.c |   16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>> index 02ac4ba..d4081f4 100644
>> --- a/target-s390x/kvm.c
>> +++ b/target-s390x/kvm.c
>> @@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
>>
>> int kvm_arch_init(KVMState *s)
>> {
>> +    int64_t value;
>> +
>>     cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>>     if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>         || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>>         phys_mem_set_alloc(legacy_s390_alloc);
>>     }
>> +
>> +    value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
>> +
>> +    if (value < 0) {
>> +        fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", value);
>> +        exit(1);
>> +    }
>> +
>> +    if (value != (int64_t)(ram_addr_t)value) {
>> +        fprintf(stderr, "qemu: standby size too large\n");
>> +        exit(1);
>> +    }
>> +    standby_mem_size = value * 1024 * 1024;
> 
> I would hope qemu_opt_get_size() returns a value in bytes. Why multiply it here?
> 

It's actually in megabytes, and this is converting to bytes --  Based on
your comment, it sounds like qemu_opt_get_size should always be
returning a byte value.

FWIW, if I adopt Paolo's comments re: patch 1, this behavior goes away
(and the parsing moves out of s390).  Either way, I'll adjust for v2.

> 
> Alex
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs

[Matthew Rosato]

On 12/16/2013 04:42 PM, Alexander Graf wrote:
> 
> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
> 
>> Add memory information to read SCP info and add handlers for
>> Read Storage Element Information, Attach Storage Element,
>> Assign Storage and Unassign Storage.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>> hw/s390x/sclp.c         |  233 +++++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/s390x/sclp.h |    2 +
>> 2 files changed, 229 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index cb53d7e..5d27fc3 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -15,9 +15,15 @@
>> #include "cpu.h"
>> #include "sysemu/kvm.h"
>> #include "exec/memory.h"
>> -
>> +#include "exec/address-spaces.h"
>> #include "hw/s390x/sclp.h"
>>
>> +int shift;
>> +ram_addr_t standby_subregion_size;
>> +ram_addr_t padded_ram_size;
>> +ram_addr_t rzm;
>> +char *standby_state_map;
> 
> Do you really need all these globals?
> 

I think this ties into your device comment below - I agree that these
should be encapsulated.

>> +
>> static inline S390SCLPDevice *get_event_facility(void)
>> {
>>     ObjectProperty *op = object_property_find(qdev_get_machine(),
>> @@ -31,16 +37,215 @@ static inline S390SCLPDevice *get_event_facility(void)
>> static void read_SCP_info(SCCB *sccb)
>> {
>>     ReadInfo *read_info = (ReadInfo *) sccb;
>> -    int shift = 0;
>> +    int rnsize, rnmax;
>> +    int max_avail_slots = MAX_AVAIL_SLOTS;
>> +#ifdef CONFIG_KVM
>> +    max_avail_slots = kvm_check_extension(kvm_state, KVM_CAP_NR_MEMSLOTS);
> 
> Please don't call kvm specific code from non-kvm specific code. Check out kvm_s390_io_interrupt() as an example how to plumb it in. Please also check kvm_enabled(), as CONFIG_KVM doesn't mean that you actually end up using KVM. It's probably best to completely abstract this away in a helper.
> 

OK, will do.

>> +#endif
>> +
>> +    read_info->facilities = 0;
>> +
>> +    if (standby_mem_size) {
> 
> I don't understand why we have 2 code paths - one for standby and one without. Can't we just handle all cases as standby?
> 

Yes I think so -  everything should fall-through naturally and we can
just skip the malloc of standby_state_map.

>> +        /*
>> +         * The storage increment size is a multiple of 1M and is a power of 2.
>> +         * The number of storage increments must be 1020 or fewer.
>> +         */
>> +        while ((ram_size >> (20 + shift)) > 1020) {
>> +            shift++;
>> +        }
>> +        while ((standby_mem_size >> (20 + shift)) > 1020) {
>> +            shift++;
>> +        }
>> +
>> +        standby_subregion_size = MEM_SECTION_SIZE;
>> +        /* Deduct the memory slot already used for core */
>> +        while ((standby_subregion_size * (max_avail_slots - 1)
>> +               < standby_mem_size)) {
>> +            standby_subregion_size = standby_subregion_size << 1;
>> +        }
>> +        /*
>> +         * Initialize mapping of guest standby memory sections indicating which
>> +         * are and are not online. Assume all standby memory begins offline.
>> +         */
>> +        if (standby_mem_size % standby_subregion_size) {
>> +            standby_state_map = g_malloc0((standby_mem_size /
>> +                                           standby_subregion_size + 1) *
>> +                                          (standby_subregion_size /
>> +                                           MEM_SECTION_SIZE));
>> +        } else {
>> +            standby_state_map = g_malloc0(standby_mem_size / MEM_SECTION_SIZE);
>> +        }
> 
> You don't free this thing when the guest calls read_SCP_info twice.
> 

Oops.  Acknowledged.

>> +
>> +        padded_ram_size = ram_size + pad_size;
>> +        rzm = 1 << (20 + shift);
>> +
>> +    } else {
>> +        while ((ram_size >> (20 + shift)) > 65535) {
>> +            shift++;
>> +        }
>> +    }
>>
>> -    while ((ram_size >> (20 + shift)) > 65535) {
>> -        shift++;
>> +    rnsize = 1 << shift;
>> +    if (rnsize <= 128) {
>> +        read_info->rnsize = rnsize;
>> +    } else {
>> +        read_info->rnsize = 0;
>> +        read_info->rnsize2 = cpu_to_be32(rnsize);
>> +    }
>> +
>> +    rnmax = (ram_size + standby_mem_size + pad_size) >> (20 + shift);
>> +    if (rnmax < 0x10000) {
>> +        read_info->rnmax = cpu_to_be16(rnmax);
>> +    } else {
>> +        read_info->rnmax = cpu_to_be16(0);
>> +        read_info->rnmax2 = cpu_to_be64(rnmax);
>> +    }
>> +
>> +    if (standby_mem_size) {
>> +        read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
> 
> Any reason to make this conditional?
> 

I suppose not -- I'll make this unconditional.

>>     }
>> -    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
>> -    read_info->rnsize = 1 << shift;
>>     sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
>> }

[...]

>> +static void unassign_storage(SCCB *sccb)
>> +{
>> +    MemoryRegion *mr = NULL;
>> +    AssignStorage *assign_info = (AssignStorage *) sccb;
>> +    ram_addr_t unassign_addr = (assign_info->rn - 1) * rzm;
>> +    MemoryRegion *sysmem = get_system_memory();
>> +
>> +    /* if the addr is a multiple of 256 MB */
>> +    if ((unassign_addr % MEM_SECTION_SIZE == 0) &&
>> +        (unassign_addr >= padded_ram_size)) {
>> +        standby_state_map[(unassign_addr -
>> +                           padded_ram_size) / MEM_SECTION_SIZE] = 0;
>> +        mr = memory_region_find(sysmem, unassign_addr, 1).mr;
>> +        if (mr) {
>> +            int i;
>> +            int is_removable = 1;
>> +            ram_addr_t map_offset = (unassign_addr - padded_ram_size -
>> +                                     (unassign_addr - padded_ram_size)
>> +                                     % standby_subregion_size);
>> +            for (i = 0;
>> +                 i < (standby_subregion_size / MEM_SECTION_SIZE);
>> +                 i++) {
>> +
>> +                if (standby_state_map[i + map_offset / MEM_SECTION_SIZE]) {
>> +                    is_removable = 0;
>> +                    break;
>> +                }
>> +            }
>> +            if (is_removable) {
>> +                memory_region_del_subregion(sysmem, mr);
>> +                memory_region_destroy(mr);
> 
> no g_free()?

Oops.  Acknowledged.

> 
>> +            }
>> +        }
>> +    }
>> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
>> +}
>> +
>> static void sclp_execute(SCCB *sccb, uint64_t code)
>> {
>>     S390SCLPDevice *sdev = get_event_facility();
>> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>>     case SCLP_CMDW_READ_SCP_INFO_FORCED:
>>         read_SCP_info(sccb);
>>         break;
>> +    case SCLP_READ_STORAGE_ELEMENT_INFO:
>> +        if (code & 0xff00) {
>> +            read_storage_element1_info(sccb);
>> +        } else {
>> +            read_storage_element0_info(sccb);
>> +        }
>> +        break;
>> +    case SCLP_ATTACH_STORAGE_ELEMENT:
>> +        attach_storage_element(sccb, (code & 0xff00) >> 8);
>> +        break;
>> +    case SCLP_ASSIGN_STORAGE:
>> +        assign_storage(sccb);
>> +        break;
>> +    case SCLP_UNASSIGN_STORAGE:
>> +        unassign_storage(sccb);
>> +        break;
> 
> Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls.
> 

OK, let me look into this for the next version.

> Overall, the code could use _a lot_ more comments.

Will do.  Thanks for your comments.

> 
> 
> Alex
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs

[Matthew Rosato]

On 12/16/2013 06:18 PM, Alexander Graf wrote:
> 
> On 16.12.2013, at 22:42, Alexander Graf <agraf@suse.de> wrote:
> 
>>
>> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
>>
> 
> [...]
> 
>>
>>> +            }
>>> +        }
>>> +    }
>>> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
>>> +}
>>> +
>>> static void sclp_execute(SCCB *sccb, uint64_t code)
>>> {
>>>    S390SCLPDevice *sdev = get_event_facility();
>>> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>>>    case SCLP_CMDW_READ_SCP_INFO_FORCED:
>>>        read_SCP_info(sccb);
>>>        break;
>>> +    case SCLP_READ_STORAGE_ELEMENT_INFO:
>>> +        if (code & 0xff00) {
>>> +            read_storage_element1_info(sccb);
>>> +        } else {
>>> +            read_storage_element0_info(sccb);
>>> +        }
>>> +        break;
>>> +    case SCLP_ATTACH_STORAGE_ELEMENT:
>>> +        attach_storage_element(sccb, (code & 0xff00) >> 8);
>>> +        break;
>>> +    case SCLP_ASSIGN_STORAGE:
>>> +        assign_storage(sccb);
>>> +        break;
>>> +    case SCLP_UNASSIGN_STORAGE:
>>> +        unassign_storage(sccb);
>>> +        break;
>>
>> Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls.
> 
> Speaking of state - in the current model the "is standby storage active" bitmap doesn't get migrated, no?
> 

Yes, you are correct, migration support still needs to be added  - But I
wanted to get this much out for review at least.

> 
> Alex
> 
> 
> 

Re: [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification

[Alexander Graf]

On 17.12.2013, at 16:58, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:

> On 12/16/2013 04:25 PM, Alexander Graf wrote:
>> 
>> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
>> 
>>> When machine=...,standby-mem={size} has been specified, convert the value
>>> to bytes and store it for use.
>>> 
>>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>>> ---
>>> target-s390x/kvm.c |   16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>> 
>>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>>> index 02ac4ba..d4081f4 100644
>>> --- a/target-s390x/kvm.c
>>> +++ b/target-s390x/kvm.c
>>> @@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
>>> 
>>> int kvm_arch_init(KVMState *s)
>>> {
>>> +    int64_t value;
>>> +
>>>    cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>>>    if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>>        || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>>>        phys_mem_set_alloc(legacy_s390_alloc);
>>>    }
>>> +
>>> +    value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
>>> +
>>> +    if (value < 0) {
>>> +        fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", value);
>>> +        exit(1);
>>> +    }
>>> +
>>> +    if (value != (int64_t)(ram_addr_t)value) {
>>> +        fprintf(stderr, "qemu: standby size too large\n");
>>> +        exit(1);
>>> +    }
>>> +    standby_mem_size = value * 1024 * 1024;
>> 
>> I would hope qemu_opt_get_size() returns a value in bytes. Why multiply it here?
>> 
> 
> It's actually in megabytes, and this is converting to bytes --  Based on
> your comment, it sounds like qemu_opt_get_size should always be
> returning a byte value.

At least I don't see code that converts it into MB. After all, you should be able to say size=1M which with your code probably ends up as 1T, no?

> FWIW, if I adopt Paolo's comments re: patch 1, this behavior goes away
> (and the parsing moves out of s390).  Either way, I'll adjust for v2.

Ok :).


Alex

Re: [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures

[Christian Borntraeger]

On 16/12/13 21:51, Matthew Rosato wrote:
> Define new SCLP codes and structures that will be needed for s390 memory
> hotplug.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

Can you rebase this patch against 

git://github.com/borntraeger/qemu.git s390-next

and send it separately? This patch is pretty much non-controversial and I would apply
it to s390-next.
You could then base your v2 on that.

Christian


> ---
>  hw/s390x/sclp.c         |    2 +-
>  include/hw/s390x/sclp.h |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 86d6ae0..cb53d7e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>  {
>      S390SCLPDevice *sdev = get_event_facility();
> 
> -    switch (code) {
> +    switch (code & SCLP_NO_CMD_PARM) {
>      case SCLP_CMDW_READ_SCP_INFO:
>      case SCLP_CMDW_READ_SCP_INFO_FORCED:
>          read_SCP_info(sccb);
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index 231a38a..e80cb23 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -20,18 +20,31 @@
>  /* SCLP command codes */
>  #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
>  #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
> +#define SCLP_READ_STORAGE_ELEMENT_INFO          0x00040001
> +#define SCLP_ATTACH_STORAGE_ELEMENT             0x00080001
> +#define SCLP_ASSIGN_STORAGE                     0x000D0001
> +#define SCLP_UNASSIGN_STORAGE                   0x000C0001
>  #define SCLP_CMD_READ_EVENT_DATA                0x00770005
>  #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
>  #define SCLP_CMD_READ_EVENT_DATA                0x00770005
>  #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
>  #define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
> 
> +/* SCLP Memory hotplug codes */
> +#define SCLP_NO_CMD_PARM                        0xffff00ff
> +#define SCLP_FC_ASSIGN_ATTACH_READ_STOR         0xE00000000000ULL
> +#define SCLP_STARTING_SUBINCREMENT_ID           0x10001
> +#define SCLP_INCREMENT_UNIT                     0x10000
> +#define MAX_AVAIL_SLOTS                         32
> +
>  /* SCLP response codes */
>  #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
>  #define SCLP_RC_NORMAL_COMPLETION               0x0020
> +#define SCLP_RC_SCCB_BOUNDARY_VIOLATION         0x0100
>  #define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
>  #define SCLP_RC_CONTAINED_EQUIPMENT_CHECK       0x0340
>  #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
> +#define SCLP_RC_STANDBY_READ_COMPLETION         0x0410
>  #define SCLP_RC_INVALID_FUNCTION                0x40f0
>  #define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
>  #define SCLP_RC_INVALID_SELECTION_MASK          0x70f0
> @@ -75,8 +88,41 @@ typedef struct ReadInfo {
>      SCCBHeader h;
>      uint16_t rnmax;
>      uint8_t rnsize;
> +    uint8_t  _reserved1[16 - 11];       /* 11-15 */
> +    uint16_t entries_cpu;               /* 16-17 */
> +    uint16_t offset_cpu;                /* 18-19 */
> +    uint8_t  _reserved2[24 - 20];       /* 20-23 */
> +    uint8_t  loadparm[8];               /* 24-31 */
> +    uint8_t  _reserved3[48 - 32];       /* 32-47 */
> +    uint64_t facilities;                /* 48-55 */
> +    uint8_t  _reserved0[100 - 56];
> +    uint32_t rnsize2;
> +    uint64_t rnmax2;
>  } QEMU_PACKED ReadInfo;
> 
> +typedef struct ReadStorageElementInfo {
> +    SCCBHeader h;
> +    uint16_t max_id;
> +    uint16_t assigned;
> +    uint16_t standby;
> +    uint8_t _reserved0[16 - 14]; /* 14-15 */
> +    uint32_t entries[0];
> +} QEMU_PACKED ReadStorageElementInfo;
> +
> +typedef struct AttachStorageElement {
> +    SCCBHeader h;
> +    uint8_t _reserved0[10 - 8];  /* 8-9 */
> +    uint16_t assigned;
> +    uint8_t _reserved1[16 - 12]; /* 12-15 */
> +    uint32_t entries[0];
> +} QEMU_PACKED AttachStorageElement;
> +
> +typedef struct AssignStorage {
> +    SCCBHeader h;
> +    uint16_t rn;
> +} QEMU_PACKED AssignStorage;
> +
> +
>  typedef struct SCCB {
>      SCCBHeader h;
>      char data[SCCB_DATA_LEN];
> 

Re: [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures

[Christian Borntraeger]

On 16/12/13 21:51, Matthew Rosato wrote:
> Define new SCLP codes and structures that will be needed for s390 memory
> hotplug.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/s390x/sclp.c         |    2 +-
>  include/hw/s390x/sclp.h |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 86d6ae0..cb53d7e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>  {
>      S390SCLPDevice *sdev = get_event_facility();
> 
> -    switch (code) {
> +    switch (code & SCLP_NO_CMD_PARM) {

This is already done in latest upstream, so please provide a newer version of
these patches against s390-next. Will apply then.

Christian

>      case SCLP_CMDW_READ_SCP_INFO:
>      case SCLP_CMDW_READ_SCP_INFO_FORCED:
>          read_SCP_info(sccb);
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index 231a38a..e80cb23 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -20,18 +20,31 @@
>  /* SCLP command codes */
>  #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
>  #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
> +#define SCLP_READ_STORAGE_ELEMENT_INFO          0x00040001
> +#define SCLP_ATTACH_STORAGE_ELEMENT             0x00080001
> +#define SCLP_ASSIGN_STORAGE                     0x000D0001
> +#define SCLP_UNASSIGN_STORAGE                   0x000C0001
>  #define SCLP_CMD_READ_EVENT_DATA                0x00770005
>  #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
>  #define SCLP_CMD_READ_EVENT_DATA                0x00770005
>  #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
>  #define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
> 
> +/* SCLP Memory hotplug codes */
> +#define SCLP_NO_CMD_PARM                        0xffff00ff
This can then go as well.

> +#define SCLP_FC_ASSIGN_ATTACH_READ_STOR         0xE00000000000ULL
> +#define SCLP_STARTING_SUBINCREMENT_ID           0x10001
> +#define SCLP_INCREMENT_UNIT                     0x10000
> +#define MAX_AVAIL_SLOTS                         32
> +
>  /* SCLP response codes */
>  #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
>  #define SCLP_RC_NORMAL_COMPLETION               0x0020
> +#define SCLP_RC_SCCB_BOUNDARY_VIOLATION         0x0100
>  #define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
>  #define SCLP_RC_CONTAINED_EQUIPMENT_CHECK       0x0340
>  #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
> +#define SCLP_RC_STANDBY_READ_COMPLETION         0x0410
>  #define SCLP_RC_INVALID_FUNCTION                0x40f0
>  #define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
>  #define SCLP_RC_INVALID_SELECTION_MASK          0x70f0
> @@ -75,8 +88,41 @@ typedef struct ReadInfo {
>      SCCBHeader h;
>      uint16_t rnmax;
>      uint8_t rnsize;
> +    uint8_t  _reserved1[16 - 11];       /* 11-15 */
> +    uint16_t entries_cpu;               /* 16-17 */
> +    uint16_t offset_cpu;                /* 18-19 */
> +    uint8_t  _reserved2[24 - 20];       /* 20-23 */
> +    uint8_t  loadparm[8];               /* 24-31 */
> +    uint8_t  _reserved3[48 - 32];       /* 32-47 */
> +    uint64_t facilities;                /* 48-55 */
> +    uint8_t  _reserved0[100 - 56];
> +    uint32_t rnsize2;
> +    uint64_t rnmax2;
>  } QEMU_PACKED ReadInfo;
> 
> +typedef struct ReadStorageElementInfo {
> +    SCCBHeader h;
> +    uint16_t max_id;
> +    uint16_t assigned;
> +    uint16_t standby;
> +    uint8_t _reserved0[16 - 14]; /* 14-15 */
> +    uint32_t entries[0];
> +} QEMU_PACKED ReadStorageElementInfo;
> +
> +typedef struct AttachStorageElement {
> +    SCCBHeader h;
> +    uint8_t _reserved0[10 - 8];  /* 8-9 */
> +    uint16_t assigned;
> +    uint8_t _reserved1[16 - 12]; /* 12-15 */
> +    uint32_t entries[0];
> +} QEMU_PACKED AttachStorageElement;
> +
> +typedef struct AssignStorage {
> +    SCCBHeader h;
> +    uint16_t rn;
> +} QEMU_PACKED AssignStorage;
> +
> +

Only one space, I guess.

>  typedef struct SCCB {
>      SCCBHeader h;
>      char data[SCCB_DATA_LEN];
> 

Otherwise: 

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Re: [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures

[Matthew Rosato]

On 03/20/2014 05:56 AM, Christian Borntraeger wrote:
> On 16/12/13 21:51, Matthew Rosato wrote:
>> Define new SCLP codes and structures that will be needed for s390 memory
>> hotplug.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/sclp.c         |    2 +-
>>  include/hw/s390x/sclp.h |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 86d6ae0..cb53d7e 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>>  {
>>      S390SCLPDevice *sdev = get_event_facility();
>>
>> -    switch (code) {
>> +    switch (code & SCLP_NO_CMD_PARM) {
> 
> This is already done in latest upstream, so please provide a newer version of
> these patches against s390-next. Will apply then.
> 
> Christian

Hi Christian -- You already accepted this patch into s390-next with
these changes as 234eef51a12e2f0f8dfd71cb49d2469d462b1855.  Or am I
missing something else you wanted changed?

This is part of v1 -- v2 of the whole patch set excluded this patch as a
result (among other changes).

I noticed that v2 doesn't fit anymore.  I'll submit a trimmed-down and
re-fit v3 of the patch set once Igor's 'convert -m to QemuOpts' set goes
in (it just got applied to trivial).

> 
>>      case SCLP_CMDW_READ_SCP_INFO:
>>      case SCLP_CMDW_READ_SCP_INFO_FORCED:
>>          read_SCP_info(sccb);
>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
>> index 231a38a..e80cb23 100644
>> --- a/include/hw/s390x/sclp.h
>> +++ b/include/hw/s390x/sclp.h
>> @@ -20,18 +20,31 @@
>>  /* SCLP command codes */
>>  #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
>>  #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
>> +#define SCLP_READ_STORAGE_ELEMENT_INFO          0x00040001
>> +#define SCLP_ATTACH_STORAGE_ELEMENT             0x00080001
>> +#define SCLP_ASSIGN_STORAGE                     0x000D0001
>> +#define SCLP_UNASSIGN_STORAGE                   0x000C0001
>>  #define SCLP_CMD_READ_EVENT_DATA                0x00770005
>>  #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
>>  #define SCLP_CMD_READ_EVENT_DATA                0x00770005
>>  #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
>>  #define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
>>
>> +/* SCLP Memory hotplug codes */
>> +#define SCLP_NO_CMD_PARM                        0xffff00ff
> This can then go as well.
> 
>> +#define SCLP_FC_ASSIGN_ATTACH_READ_STOR         0xE00000000000ULL
>> +#define SCLP_STARTING_SUBINCREMENT_ID           0x10001
>> +#define SCLP_INCREMENT_UNIT                     0x10000
>> +#define MAX_AVAIL_SLOTS                         32
>> +
>>  /* SCLP response codes */
>>  #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
>>  #define SCLP_RC_NORMAL_COMPLETION               0x0020
>> +#define SCLP_RC_SCCB_BOUNDARY_VIOLATION         0x0100
>>  #define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
>>  #define SCLP_RC_CONTAINED_EQUIPMENT_CHECK       0x0340
>>  #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
>> +#define SCLP_RC_STANDBY_READ_COMPLETION         0x0410
>>  #define SCLP_RC_INVALID_FUNCTION                0x40f0
>>  #define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
>>  #define SCLP_RC_INVALID_SELECTION_MASK          0x70f0
>> @@ -75,8 +88,41 @@ typedef struct ReadInfo {
>>      SCCBHeader h;
>>      uint16_t rnmax;
>>      uint8_t rnsize;
>> +    uint8_t  _reserved1[16 - 11];       /* 11-15 */
>> +    uint16_t entries_cpu;               /* 16-17 */
>> +    uint16_t offset_cpu;                /* 18-19 */
>> +    uint8_t  _reserved2[24 - 20];       /* 20-23 */
>> +    uint8_t  loadparm[8];               /* 24-31 */
>> +    uint8_t  _reserved3[48 - 32];       /* 32-47 */
>> +    uint64_t facilities;                /* 48-55 */
>> +    uint8_t  _reserved0[100 - 56];
>> +    uint32_t rnsize2;
>> +    uint64_t rnmax2;
>>  } QEMU_PACKED ReadInfo;
>>
>> +typedef struct ReadStorageElementInfo {
>> +    SCCBHeader h;
>> +    uint16_t max_id;
>> +    uint16_t assigned;
>> +    uint16_t standby;
>> +    uint8_t _reserved0[16 - 14]; /* 14-15 */
>> +    uint32_t entries[0];
>> +} QEMU_PACKED ReadStorageElementInfo;
>> +
>> +typedef struct AttachStorageElement {
>> +    SCCBHeader h;
>> +    uint8_t _reserved0[10 - 8];  /* 8-9 */
>> +    uint16_t assigned;
>> +    uint8_t _reserved1[16 - 12]; /* 12-15 */
>> +    uint32_t entries[0];
>> +} QEMU_PACKED AttachStorageElement;
>> +
>> +typedef struct AssignStorage {
>> +    SCCBHeader h;
>> +    uint16_t rn;
>> +} QEMU_PACKED AssignStorage;
>> +
>> +
> 
> Only one space, I guess.
> 
>>  typedef struct SCCB {
>>      SCCBHeader h;
>>      char data[SCCB_DATA_LEN];
>>
> 
> Otherwise: 
> 
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures

[Christian Borntraeger]

On 20/03/14 17:33, Matthew Rosato wrote:
> 
> Hi Christian -- You already accepted this patch into s390-next with
> these changes as 234eef51a12e2f0f8dfd71cb49d2469d462b1855.  Or am I
> missing something else you wanted changed?

Gnarf. Yes I wanted to start review and took the wrong version :-(