[PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Collin Walling]

Changelog:

    v4
    
    • added r-b's and ack's (thanks, everyone!)

    • renamed boundary and length function

    • updated header sync to reflect a change discussed in the respective
        KVM patches

    • s/data_len/offset_cpu

    • added /* fallthrough */ comment in boundary check

    v3

    • Device IOCTLs removed
        - diag 318 info is now communicated via sync_regs

    • Reset code removed
        - this is now handled in KVM
        - diag318_info is stored within the CPU reset portion of the
            S390CPUState

    • Various cleanups for ELS preliminary patches

    v2

    • QEMU now handles the instruction call
        - as such, the "enable diag 318" IOCTL has been removed

    • patch #1 now changes the read scp/cpu info functions to
      retrieve the machine state once
        - as such, I have not added any ack's or r-bs since this
          patch differs from the previous version

    • patch #3 introduces a new "get_read_scp_info_data_len"
      function in order clean-up the variable data length assignment
      in patch #7
        - a comment above this function should help clarify what's
          going on to make things a bit easier to read

    • other misc clean ups and fixes
        - s/diag318/diag_318 in order to keep the naming scheme
          consistent with Linux and other diag-related code
        - s/byte_134/fac134 to align naming scheme with Linux

-----------------------------------------------------------------------

This patch series introduces two features for an s390 KVM quest:
    - Extended-Length SCCB (els) for the Read SCP/CPU Info SCLP 
        commands
    - DIAGNOSE 0x318 (diag_318) enabling / migration handling

The diag 318 feature depends on els and KVM support.

The els feature is handled entirely with QEMU, and does not require 
KVM support.

Both features are made available starting with the zEC12-full model.

These patches are introduced together for two main reasons:
    - els allows diag 318 to exist while retaining the original 248 
        VCPU max
    - diag 318 is presented to show how els is useful

Full els support is dependant on the Linux kernel, which must react
to the SCLP response code and set an appropriate-length SCCB. 

A user should take care when tuning the CPU model for a VM.
If a user defines a VM with els support and specifies 248 CPUs, but
the guest Linux kernel cannot react to the SCLP response code, then
the guest will crash immediately upon kernel startup.


Collin L. Walling (8):
  s390/sclp: get machine once during read scp/cpu info
  s390/sclp: check sccb len before filling in data
  s390/sclp: rework sclp boundary and length checks
  s390/sclp: read sccb from mem based on sccb length
  s390/sclp: use cpu offset to locate cpu entries
  s390/sclp: add extended-length sccb support for kvm guest
  s390/kvm: header sync for diag318
  s390: guest support for diagnose 0x318

 hw/s390x/sclp.c                     | 115 ++++++++++++++++++++++------
 include/hw/s390x/sclp.h             |   4 +
 linux-headers/asm-s390/kvm.h        |   7 +-
 linux-headers/linux/kvm.h           |   1 +
 target/s390x/cpu.h                  |   2 +
 target/s390x/cpu_features.h         |   1 +
 target/s390x/cpu_features_def.inc.h |   4 +
 target/s390x/cpu_models.c           |   1 +
 target/s390x/gen-features.c         |   2 +
 target/s390x/kvm.c                  |  39 ++++++++++
 target/s390x/machine.c              |  17 ++++
 11 files changed, 166 insertions(+), 27 deletions(-)

-- 
2.26.2

[PATCH v4 1/8] s390/sclp: get machine once during read scp/cpu info

[Collin Walling]

Functions within read scp/cpu info will need access to the machine
state. Let's make a call to retrieve the machine state once and
pass the appropriate data to the respective functions.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 20aca30ac4..7875334037 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -49,9 +49,8 @@ static inline bool sclp_command_code_valid(uint32_t code)
     return false;
 }
 
-static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
+static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
     uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
     int i;
 
@@ -77,7 +76,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     IplParameterBlock *ipib = s390_ipl_get_iplb();
 
     /* CPU information */
-    prepare_cpu_entries(sclp, read_info->entries, &cpu_count);
+    prepare_cpu_entries(machine, read_info->entries, &cpu_count);
     read_info->entries_cpu = cpu_to_be16(cpu_count);
     read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
     read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
@@ -132,10 +131,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 /* Provide information about the CPU */
 static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
     int cpu_count;
 
-    prepare_cpu_entries(sclp, cpu_info->entries, &cpu_count);
+    prepare_cpu_entries(machine, cpu_info->entries, &cpu_count);
     cpu_info->nr_configured = cpu_to_be16(cpu_count);
     cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
     cpu_info->nr_standby = cpu_to_be16(0);
-- 
2.26.2

[PATCH v4 2/8] s390/sclp: check sccb len before filling in data

[Collin Walling]

The SCCB must be checked for a sufficient length before it is filled
with any data. If the length is insufficient, then the SCLP command
is suppressed and the proper response code is set in the SCCB header.

Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length")
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/sclp.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 7875334037..181ce04007 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -75,6 +75,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
 
+    if (be16_to_cpu(sccb->h.length) <
+          (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return;
+    }
+
     /* CPU information */
     prepare_cpu_entries(machine, read_info->entries, &cpu_count);
     read_info->entries_cpu = cpu_to_be16(cpu_count);
@@ -83,12 +89,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 
     read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
 
-    if (be16_to_cpu(sccb->h.length) <
-            (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
-        return;
-    }
-
     /* Configuration Characteristic (Extension) */
     s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
                          read_info->conf_char);
@@ -135,17 +135,17 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
     int cpu_count;
 
-    prepare_cpu_entries(machine, cpu_info->entries, &cpu_count);
-    cpu_info->nr_configured = cpu_to_be16(cpu_count);
-    cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
-    cpu_info->nr_standby = cpu_to_be16(0);
-
     if (be16_to_cpu(sccb->h.length) <
-            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
+          (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) {
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
         return;
     }
 
+    prepare_cpu_entries(machine, cpu_info->entries, &cpu_count);
+    cpu_info->nr_configured = cpu_to_be16(cpu_count);
+    cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
+    cpu_info->nr_standby = cpu_to_be16(0);
+
     /* The standby offset is 16-byte for each CPU */
     cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
         + cpu_info->nr_configured*sizeof(CPUEntry));
-- 
2.26.2

[PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

Rework the SCLP boundary check to account for different SCLP commands
(eventually) allowing different boundary sizes.

Move the length check code into a separate function, and introduce a
new function to determine the length of the read SCP data (i.e. the size
from the start of the struct to where the CPU entries should begin).

The format of read CPU info is unlikely to change in the future,
so we do not require a separate function to calculate its length.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 181ce04007..5899c1e3b8 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
     return false;
 }
 
+static bool sccb_verify_boundary(uint64_t sccb_addr, uint32_t code,
+                                 SCCBHeader *header)
+{
+    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1;
+    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
+
+    switch (code & SCLP_CMD_CODE_MASK) {
+    default:
+        if (sccb_max_addr < sccb_boundary) {
+            return true;
+        }
+    }
+    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    return false;
+}
+
+/* Calculates sufficient SCCB length to store a full Read SCP/CPU response */
+static bool sccb_verify_length(SCCB *sccb, int num_cpus, int offset_cpu)
+{
+    int required_len = offset_cpu + num_cpus * sizeof(CPUEntry);
+
+    if (be16_to_cpu(sccb->h.length) < required_len) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return false;
+    }
+    return true;
+}
+
 static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
 {
     uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
@@ -66,6 +94,11 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
     }
 }
 
+static inline int get_read_scp_info_offset_cpu(void)
+{
+    return offsetof(ReadInfo, entries);
+}
+
 /* Provide information about the configuration, CPUs and storage */
 static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 {
@@ -74,17 +107,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int cpu_count;
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
+    int offset_cpu = get_read_scp_info_offset_cpu();
 
-    if (be16_to_cpu(sccb->h.length) <
-          (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+    if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
         return;
     }
 
     /* CPU information */
     prepare_cpu_entries(machine, read_info->entries, &cpu_count);
     read_info->entries_cpu = cpu_to_be16(cpu_count);
-    read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
+    read_info->offset_cpu = cpu_to_be16(offset_cpu);
     read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
 
     read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
@@ -133,17 +165,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
+    int offset_cpu = offsetof(ReadCpuInfo, entries);
     int cpu_count;
 
-    if (be16_to_cpu(sccb->h.length) <
-          (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+    if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
         return;
     }
 
     prepare_cpu_entries(machine, cpu_info->entries, &cpu_count);
     cpu_info->nr_configured = cpu_to_be16(cpu_count);
-    cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
+    cpu_info->offset_configured = cpu_to_be16(offset_cpu);
     cpu_info->nr_standby = cpu_to_be16(0);
 
     /* The standby offset is 16-byte for each CPU */
@@ -229,6 +260,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
         goto out_write;
     }
 
+    if (!sccb_verify_boundary(sccb, code, &work_sccb.h)) {
+        goto out_write;
+    }
+
     sclp_c->execute(sclp, &work_sccb, code);
 out_write:
     s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
@@ -274,8 +309,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         goto out_write;
     }
 
-    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    if (!sccb_verify_boundary(sccb, code, &work_sccb.h)) {
         goto out_write;
     }
 
-- 
2.26.2

[PATCH v4 4/8] s390/sclp: read sccb from mem based on sccb length

[Collin Walling]

The header of the SCCB contains the actual length of the SCCB. Instead
of using a static 4K size, let's allow for a variable size determined
by the value set in the header. The proper checks are already in place
to ensure the SCCB length is sufficent to store a full response, and
that the length does not cross any explicitly-set boundaries.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 5899c1e3b8..1feba6f692 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -251,9 +251,8 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
     SCLPDevice *sclp = get_sclp_device();
     SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
     SCCB work_sccb;
-    hwaddr sccb_len = sizeof(SCCB);
 
-    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
+    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader));
 
     if (!sclp_command_code_valid(code)) {
         work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
@@ -264,6 +263,9 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
         goto out_write;
     }
 
+    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb,
+                         be16_to_cpu(work_sccb.h.length));
+
     sclp_c->execute(sclp, &work_sccb, code);
 out_write:
     s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
@@ -278,8 +280,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
     SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
     SCCB work_sccb;
 
-    hwaddr sccb_len = sizeof(SCCB);
-
     /* first some basic checks on program checks */
     if (env->psw.mask & PSW_MASK_PSTATE) {
         return -PGM_PRIVILEGED;
@@ -297,7 +297,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
      * from playing dirty tricks by modifying the memory content after
      * the host has checked the values
      */
-    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
+    cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader));
 
     /* Valid sccb sizes */
     if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
@@ -313,6 +313,9 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         goto out_write;
     }
 
+    /* the header contains the actual length of the sccb */
+    cpu_physical_memory_read(sccb, &work_sccb, be16_to_cpu(work_sccb.h.length));
+
     sclp_c->execute(sclp, &work_sccb, code);
 out_write:
     cpu_physical_memory_write(sccb, &work_sccb,
-- 
2.26.2

[PATCH v4 5/8] s390/sclp: use cpu offset to locate cpu entries

[Collin Walling]

The start of the CPU entry region in the Read SCP Info response data is
denoted by the offset_cpu field. As such, QEMU needs to begin creating
entries at this address. Note that the length of the Read SCP Info data
(data_len) denotes the same value as the cpu offset.

This is in preparation of when Read SCP Info inevitably introduces new
bytes that push the start of the CPUEntry field further away.

Read CPU Info is unlikely to ever change, so let's not bother
accounting for the offset there.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 1feba6f692..518f630938 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -108,13 +108,14 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
     int offset_cpu = get_read_scp_info_offset_cpu();
+    CPUEntry *entries_start = (void *)sccb + offset_cpu;
 
     if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
         return;
     }
 
     /* CPU information */
-    prepare_cpu_entries(machine, read_info->entries, &cpu_count);
+    prepare_cpu_entries(machine, entries_start, &cpu_count);
     read_info->entries_cpu = cpu_to_be16(cpu_count);
     read_info->offset_cpu = cpu_to_be16(offset_cpu);
     read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
-- 
2.26.2

[PATCH v4 6/8] s390/sclp: add extended-length sccb support for kvm guest

[Collin Walling]

As more features and facilities are added to the Read SCP Info (RSCPI)
response, more space is required to store them. The space used to store
these new features intrudes on the space originally used to store CPU
entries. This means as more features and facilities are added to the
RSCPI response, less space can be used to store CPU entries.

With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
the RSCPI command and determine if the SCCB is large enough to store a
complete reponse. If it is not large enough, then the required length
will be set in the SCCB header.

The caller of the SCLP command is responsible for creating a
large-enough SCCB to store a complete response. Proper checking should
be in place, and the caller should execute the command once-more with
the large-enough SCCB.

This facility also enables an extended SCCB for the Read CPU Info
(RCPUI) command.

When this facility is enabled, the boundary violation response cannot
be a result from the RSCPI, RSCPI Forced, or RCPUI commands.

In order to tolerate kernels that do not yet have full support for this
feature, a "fixed" offset to the start of the CPU Entries within the
Read SCP Info struct is set to allow for the original 248 max entries
when this feature is disabled.

Additionally, this is introduced as a CPU feature to protect the guest
from migrating to a machine that does not support storing an extended
SCCB. This could otherwise hinder the VM from being able to read all
available CPU entries after migration (such as during re-ipl).

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/sclp.c                     | 24 +++++++++++++++++++++++-
 include/hw/s390x/sclp.h             |  1 +
 target/s390x/cpu_features_def.inc.h |  1 +
 target/s390x/gen-features.c         |  1 +
 target/s390x/kvm.c                  |  8 ++++++++
 5 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 518f630938..2e83b4f598 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -22,6 +22,7 @@
 #include "hw/s390x/event-facility.h"
 #include "hw/s390x/s390-pci-bus.h"
 #include "hw/s390x/ipl.h"
+#include "qemu/log.h"
 
 static inline SCLPDevice *get_sclp_device(void)
 {
@@ -56,6 +57,19 @@ static bool sccb_verify_boundary(uint64_t sccb_addr, uint32_t code,
     uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
 
     switch (code & SCLP_CMD_CODE_MASK) {
+    case SCLP_CMDW_READ_SCP_INFO:
+    case SCLP_CMDW_READ_SCP_INFO_FORCED:
+    case SCLP_CMDW_READ_CPU_INFO:
+        /*
+         * An extended-length SCCB is only allowed for Read SCP/CPU Info and
+         * is allowed to exceed the 4k boundary. The respective commands will
+         * set the length field to the required length if an insufficient
+         * SCCB length is provided.
+         */
+        if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
+            return true;
+        }
+        /* fallthrough */
     default:
         if (sccb_max_addr < sccb_boundary) {
             return true;
@@ -72,6 +86,10 @@ static bool sccb_verify_length(SCCB *sccb, int num_cpus, int offset_cpu)
 
     if (be16_to_cpu(sccb->h.length) < required_len) {
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) &&
+            sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
+            sccb->h.length = required_len;
+        }
         return false;
     }
     return true;
@@ -96,7 +114,9 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
 
 static inline int get_read_scp_info_offset_cpu(void)
 {
-    return offsetof(ReadInfo, entries);
+    return s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
+           offsetof(ReadInfo, entries) :
+           SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
 }
 
 /* Provide information about the configuration, CPUs and storage */
@@ -111,6 +131,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     CPUEntry *entries_start = (void *)sccb + offset_cpu;
 
     if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "insufficient sccb size to store "
+                      "read scp info response\n");
         return;
     }
 
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 822eff4396..ef2d63eae9 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -110,6 +110,7 @@ typedef struct CPUEntry {
     uint8_t reserved1;
 } QEMU_PACKED CPUEntry;
 
+#define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET     128
 typedef struct ReadInfo {
     SCCBHeader h;
     uint16_t rnmax;
diff --git a/target/s390x/cpu_features_def.inc.h b/target/s390x/cpu_features_def.inc.h
index 5942f81f16..1c04cc18f4 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -97,6 +97,7 @@ DEF_FEAT(GUARDED_STORAGE, "gs", STFL, 133, "Guarded-storage facility")
 DEF_FEAT(VECTOR_PACKED_DECIMAL, "vxpd", STFL, 134, "Vector packed decimal facility")
 DEF_FEAT(VECTOR_ENH, "vxeh", STFL, 135, "Vector enhancements facility")
 DEF_FEAT(MULTIPLE_EPOCH, "mepoch", STFL, 139, "Multiple-epoch facility")
+DEF_FEAT(EXTENDED_LENGTH_SCCB, "els", STFL, 140, "Extended-length SCCB facility")
 DEF_FEAT(TEST_PENDING_EXT_INTERRUPTION, "tpei", STFL, 144, "Test-pending-external-interruption facility")
 DEF_FEAT(INSERT_REFERENCE_BITS_MULT, "irbm", STFL, 145, "Insert-reference-bits-multiple facility")
 DEF_FEAT(MSA_EXT_8, "msa8-base", STFL, 146, "Message-security-assist-extension-8 facility (excluding subfunctions)")
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 8ddeebc544..6857f657fb 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -522,6 +522,7 @@ static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL,
     S390_FEAT_AP_FACILITIES_TEST,
     S390_FEAT_AP,
+    S390_FEAT_EXTENDED_LENGTH_SCCB,
 };
 
 static uint16_t full_GEN12_GA2[] = {
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index f2f75d2a57..a2d5ad78f6 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2456,6 +2456,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
         set_bit(S390_FEAT_AP, model->features);
     }
+
+    /*
+     * Extended-Length SCCB is handled entirely within QEMU.
+     * For PV guests this is completely fenced by the Ultravisor, as Service
+     * Call error checking and STFLE interpretation are handled via SIE.
+     */
+    set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
+
     /* strip of features that are not part of the maximum model */
     bitmap_and(model->features, model->features, model->def->full_feat,
                S390_FEAT_MAX);
-- 
2.26.2

[PATCH v4 7/8] s390/kvm: header sync for diag318

[Collin Walling]

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 linux-headers/asm-s390/kvm.h | 7 +++++--
 linux-headers/linux/kvm.h    | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 0138ccb0d8..f053b8304a 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {
 #define KVM_SYNC_GSCB   (1UL << 9)
 #define KVM_SYNC_BPBC   (1UL << 10)
 #define KVM_SYNC_ETOKEN (1UL << 11)
+#define KVM_SYNC_DIAG318 (1UL << 12)
 
 #define KVM_SYNC_S390_VALID_FIELDS \
 	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
 	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
-	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
+	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \
+	 KVM_SYNC_DIAG318)
 
 /* length and alignment of the sdnx as a power of two */
 #define SDNXC 8
@@ -264,7 +266,8 @@ struct kvm_sync_regs {
 	__u8 reserved2 : 7;
 	__u8 padding1[51];	/* riccb needs to be 64byte aligned */
 	__u8 riccb[64];		/* runtime instrumentation controls block */
-	__u8 padding2[192];	/* sdnx needs to be 256byte aligned */
+	__u64 diag318;		/* diagnose 0x318 info */
+	__u8 padding2[184];	/* sdnx needs to be 256byte aligned */
 	union {
 		__u8 sdnx[SDNXL];  /* state description annex */
 		struct {
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 9804495a46..444fdd977f 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1017,6 +1017,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_S390_VCPU_RESETS 179
 #define KVM_CAP_S390_PROTECTED 180
 #define KVM_CAP_PPC_SECURE_GUEST 181
+#define KVM_CAP_S390_DIAG318 184
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.26.2

[PATCH v4 8/8] s390: guest support for diagnose 0x318

[Collin Walling]

DIAGNOSE 0x318 (diag318) is an s390 instruction that allows the storage
of diagnostic information that is collected by the firmware in the case
of hardware/firmware service events.

QEMU handles the instruction by storing the info in the CPU state. A
subsequent register sync will communicate the data to the hypervisor.

QEMU handles the migration via a VM State Description.

This feature depends on the Extended-Length SCCB (els) feature. If
els is not present, then a warning will be printed and the SCLP bit
that allows the Linux kernel to execute the instruction will not be
set.

Availability of this instruction is determined by byte 134 (aka fac134)
bit 0 of the SCLP Read Info block. This coincidentally expands into the
space used for CPU entries, which means VMs running with the diag318
capability may not be able to read information regarding all CPUs
unless the guest kernel supports an extended-length SCCB.

This feature is not supported in protected virtualization mode.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/sclp.c                     |  5 +++++
 include/hw/s390x/sclp.h             |  3 +++
 target/s390x/cpu.h                  |  2 ++
 target/s390x/cpu_features.h         |  1 +
 target/s390x/cpu_features_def.inc.h |  3 +++
 target/s390x/cpu_models.c           |  1 +
 target/s390x/gen-features.c         |  1 +
 target/s390x/kvm.c                  | 31 +++++++++++++++++++++++++++++
 target/s390x/machine.c              | 17 ++++++++++++++++
 9 files changed, 64 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 2e83b4f598..03068d7380 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -150,6 +150,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
                          read_info->conf_char_ext);
 
+    if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
+        s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
+                            &read_info->fac134);
+    }
+
     read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
                                         SCLP_HAS_IOA_RECONFIG);
 
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index ef2d63eae9..ccb9f0a676 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -133,6 +133,9 @@ typedef struct ReadInfo {
     uint16_t highest_cpu;
     uint8_t  _reserved5[124 - 122];     /* 122-123 */
     uint32_t hmfai;
+    uint8_t  _reserved7[134 - 128];     /* 128-133 */
+    uint8_t  fac134;
+    uint8_t  _reserved8[144 - 135];     /* 135-143 */
     struct CPUEntry entries[];
 } QEMU_PACKED ReadInfo;
 
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 035427521c..f875ebf0f4 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -112,6 +112,8 @@ struct CPUS390XState {
     uint16_t external_call_addr;
     DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
 
+    uint64_t diag318_info;
+
     /* Fields up to this point are cleared by a CPU reset */
     struct {} end_reset_fields;
 
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index da695a8346..f74f7fc3a1 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -23,6 +23,7 @@ typedef enum {
     S390_FEAT_TYPE_STFL,
     S390_FEAT_TYPE_SCLP_CONF_CHAR,
     S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
+    S390_FEAT_TYPE_SCLP_FAC134,
     S390_FEAT_TYPE_SCLP_CPU,
     S390_FEAT_TYPE_MISC,
     S390_FEAT_TYPE_PLO,
diff --git a/target/s390x/cpu_features_def.inc.h b/target/s390x/cpu_features_def.inc.h
index 1c04cc18f4..f82b4b5ec1 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -122,6 +122,9 @@ DEF_FEAT(SIE_CMMA, "cmma", SCLP_CONF_CHAR_EXT, 1, "SIE: Collaborative-memory-man
 DEF_FEAT(SIE_PFMFI, "pfmfi", SCLP_CONF_CHAR_EXT, 9, "SIE: PFMF interpretation facility")
 DEF_FEAT(SIE_IBS, "ibs", SCLP_CONF_CHAR_EXT, 10, "SIE: Interlock-and-broadcast-suppression facility")
 
+/* Features exposed via SCLP SCCB Facilities byte 134 (bit numbers relative to byte-134) */
+DEF_FEAT(DIAG_318, "diag318", SCLP_FAC134, 0, "Control program name and version codes")
+
 /* Features exposed via SCLP CPU info. */
 DEF_FEAT(SIE_F2, "sief2", SCLP_CPU, 4, "SIE: interception format 2 (Virtual SIE)")
 DEF_FEAT(SIE_SKEY, "skey", SCLP_CPU, 5, "SIE: Storage-key facility")
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 2fa609bffe..034673be54 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -827,6 +827,7 @@ static void check_consistency(const S390CPUModel *model)
         { S390_FEAT_PTFF_STOE, S390_FEAT_MULTIPLE_EPOCH },
         { S390_FEAT_PTFF_STOUE, S390_FEAT_MULTIPLE_EPOCH },
         { S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, S390_FEAT_AP },
+        { S390_FEAT_DIAG_318, S390_FEAT_EXTENDED_LENGTH_SCCB },
     };
     int i;
 
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 6857f657fb..a1f0a6f3c6 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -523,6 +523,7 @@ static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_AP_FACILITIES_TEST,
     S390_FEAT_AP,
     S390_FEAT_EXTENDED_LENGTH_SCCB,
+    S390_FEAT_DIAG_318,
 };
 
 static uint16_t full_GEN12_GA2[] = {
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a2d5ad78f6..b79feeba9f 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -105,6 +105,7 @@
 
 #define DIAG_TIMEREVENT                 0x288
 #define DIAG_IPL                        0x308
+#define DIAG_SET_CONTROL_PROGRAM_CODES  0x318
 #define DIAG_KVM_HYPERCALL              0x500
 #define DIAG_KVM_BREAKPOINT             0x501
 
@@ -602,6 +603,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
     }
 
+    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
+        cs->kvm_run->s.regs.diag318 = env->diag318_info;
+        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
+    }
+
     /* Finally the prefix */
     if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
         cs->kvm_run->s.regs.prefix = env->psa;
@@ -741,6 +747,10 @@ int kvm_arch_get_registers(CPUState *cs)
         }
     }
 
+    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
+        env->diag318_info = cs->kvm_run->s.regs.diag318;
+    }
+
     return 0;
 }
 
@@ -1601,6 +1611,19 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct kvm_run *run)
     return -ENOENT;
 }
 
+static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
+{
+    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
+    uint64_t diag318_info = run->s.regs.gprs[reg];
+
+    cpu->env.diag318_info = diag318_info;
+
+    if (can_sync_regs(CPU(cpu), KVM_SYNC_DIAG318)) {
+        run->s.regs.diag318 = diag318_info;
+        run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
+    }
+}
+
 #define DIAG_KVM_CODE_MASK 0x000000000000ffff
 
 static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
@@ -1620,6 +1643,9 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
     case DIAG_IPL:
         kvm_handle_diag_308(cpu, run);
         break;
+    case DIAG_SET_CONTROL_PROGRAM_CODES:
+        handle_diag_318(cpu, run);
+        break;
     case DIAG_KVM_HYPERCALL:
         r = handle_hypercall(cpu, run);
         break;
@@ -2464,6 +2490,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
      */
     set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
 
+    /* DIAGNOSE 0x318 is not supported under protected virtualization */
+    if (!s390_is_pv() && kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
+        set_bit(S390_FEAT_DIAG_318, model->features);
+    }
+
     /* strip of features that are not part of the maximum model */
     bitmap_and(model->features, model->features, model->def->full_feat,
                S390_FEAT_MAX);
diff --git a/target/s390x/machine.c b/target/s390x/machine.c
index 549bb6c280..5b4e82f1ab 100644
--- a/target/s390x/machine.c
+++ b/target/s390x/machine.c
@@ -234,6 +234,22 @@ const VMStateDescription vmstate_etoken = {
     }
 };
 
+static bool diag318_needed(void *opaque)
+{
+    return s390_has_feat(S390_FEAT_DIAG_318);
+}
+
+const VMStateDescription vmstate_diag318 = {
+    .name = "cpu/diag318",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = diag318_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(env.diag318_info, S390CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_s390_cpu = {
     .name = "cpu",
     .post_load = cpu_post_load,
@@ -270,6 +286,7 @@ const VMStateDescription vmstate_s390_cpu = {
         &vmstate_gscb,
         &vmstate_bpbc,
         &vmstate_etoken,
+        &vmstate_diag318,
         NULL
     },
 };
-- 
2.26.2

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Thomas Huth]

On 24/06/2020 22.23, Collin Walling wrote:
> Rework the SCLP boundary check to account for different SCLP commands
> (eventually) allowing different boundary sizes.
> 
> Move the length check code into a separate function, and introduce a
> new function to determine the length of the read SCP data (i.e. the size
> from the start of the struct to where the CPU entries should begin).
> 
> The format of read CPU info is unlikely to change in the future,
> so we do not require a separate function to calculate its length.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 44 insertions(+), 10 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 6/8] s390/sclp: add extended-length sccb support for kvm guest

[Cornelia Huck]

On Wed, 24 Jun 2020 16:23:10 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> As more features and facilities are added to the Read SCP Info (RSCPI)
> response, more space is required to store them. The space used to store
> these new features intrudes on the space originally used to store CPU
> entries. This means as more features and facilities are added to the
> RSCPI response, less space can be used to store CPU entries.
> 
> With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
> the RSCPI command and determine if the SCCB is large enough to store a
> complete reponse. If it is not large enough, then the required length
> will be set in the SCCB header.
> 
> The caller of the SCLP command is responsible for creating a
> large-enough SCCB to store a complete response. Proper checking should
> be in place, and the caller should execute the command once-more with
> the large-enough SCCB.
> 
> This facility also enables an extended SCCB for the Read CPU Info
> (RCPUI) command.
> 
> When this facility is enabled, the boundary violation response cannot
> be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
> 
> In order to tolerate kernels that do not yet have full support for this
> feature, a "fixed" offset to the start of the CPU Entries within the
> Read SCP Info struct is set to allow for the original 248 max entries
> when this feature is disabled.
> 
> Additionally, this is introduced as a CPU feature to protect the guest
> from migrating to a machine that does not support storing an extended
> SCCB. This could otherwise hinder the VM from being able to read all
> available CPU entries after migration (such as during re-ipl).
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/sclp.c                     | 24 +++++++++++++++++++++++-
>  include/hw/s390x/sclp.h             |  1 +
>  target/s390x/cpu_features_def.inc.h |  1 +
>  target/s390x/gen-features.c         |  1 +
>  target/s390x/kvm.c                  |  8 ++++++++
>  5 files changed, 34 insertions(+), 1 deletion(-)
> 

(...)

> @@ -111,6 +131,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      CPUEntry *entries_start = (void *)sccb + offset_cpu;
>  
>      if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "insufficient sccb size to store "
> +                      "read scp info response\n");

Not sure if logging needed/provided length would be helpful here.

>          return;
>      }
>  

(...)

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v4 8/8] s390: guest support for diagnose 0x318

[Cornelia Huck]

On Wed, 24 Jun 2020 16:23:12 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> DIAGNOSE 0x318 (diag318) is an s390 instruction that allows the storage
> of diagnostic information that is collected by the firmware in the case
> of hardware/firmware service events.
> 
> QEMU handles the instruction by storing the info in the CPU state. A
> subsequent register sync will communicate the data to the hypervisor.
> 
> QEMU handles the migration via a VM State Description.
> 
> This feature depends on the Extended-Length SCCB (els) feature. If
> els is not present, then a warning will be printed and the SCLP bit
> that allows the Linux kernel to execute the instruction will not be
> set.
> 
> Availability of this instruction is determined by byte 134 (aka fac134)
> bit 0 of the SCLP Read Info block. This coincidentally expands into the
> space used for CPU entries, which means VMs running with the diag318
> capability may not be able to read information regarding all CPUs
> unless the guest kernel supports an extended-length SCCB.
> 
> This feature is not supported in protected virtualization mode.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/sclp.c                     |  5 +++++
>  include/hw/s390x/sclp.h             |  3 +++
>  target/s390x/cpu.h                  |  2 ++
>  target/s390x/cpu_features.h         |  1 +
>  target/s390x/cpu_features_def.inc.h |  3 +++
>  target/s390x/cpu_models.c           |  1 +
>  target/s390x/gen-features.c         |  1 +
>  target/s390x/kvm.c                  | 31 +++++++++++++++++++++++++++++
>  target/s390x/machine.c              | 17 ++++++++++++++++
>  9 files changed, 64 insertions(+)

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v4 6/8] s390/sclp: add extended-length sccb support for kvm guest

[Collin Walling]

On 6/26/20 6:01 AM, Cornelia Huck wrote:
> On Wed, 24 Jun 2020 16:23:10 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> As more features and facilities are added to the Read SCP Info (RSCPI)
>> response, more space is required to store them. The space used to store
>> these new features intrudes on the space originally used to store CPU
>> entries. This means as more features and facilities are added to the
>> RSCPI response, less space can be used to store CPU entries.
>>
>> With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
>> the RSCPI command and determine if the SCCB is large enough to store a
>> complete reponse. If it is not large enough, then the required length
>> will be set in the SCCB header.
>>
>> The caller of the SCLP command is responsible for creating a
>> large-enough SCCB to store a complete response. Proper checking should
>> be in place, and the caller should execute the command once-more with
>> the large-enough SCCB.
>>
>> This facility also enables an extended SCCB for the Read CPU Info
>> (RCPUI) command.
>>
>> When this facility is enabled, the boundary violation response cannot
>> be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
>>
>> In order to tolerate kernels that do not yet have full support for this
>> feature, a "fixed" offset to the start of the CPU Entries within the
>> Read SCP Info struct is set to allow for the original 248 max entries
>> when this feature is disabled.
>>
>> Additionally, this is introduced as a CPU feature to protect the guest
>> from migrating to a machine that does not support storing an extended
>> SCCB. This could otherwise hinder the VM from being able to read all
>> available CPU entries after migration (such as during re-ipl).
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c                     | 24 +++++++++++++++++++++++-
>>  include/hw/s390x/sclp.h             |  1 +
>>  target/s390x/cpu_features_def.inc.h |  1 +
>>  target/s390x/gen-features.c         |  1 +
>>  target/s390x/kvm.c                  |  8 ++++++++
>>  5 files changed, 34 insertions(+), 1 deletion(-)
>>
> 
> (...)
> 
>> @@ -111,6 +131,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>      CPUEntry *entries_start = (void *)sccb + offset_cpu;
>>  
>>      if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "insufficient sccb size to store "
>> +                      "read scp info response\n");
> 
> Not sure if logging needed/provided length would be helpful here.
> 

I had the thought that it may be beneficial for kernel development -- it
gives a response if an SCCB size needs to be larger.

>>          return;
>>      }
>>  
> 
> (...)
> 
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks.

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Collin Walling]

Polite ping. Patches have been sitting on the list for a few weeks now,
and it doesn't look like any further changes are requested (hopefully I
didn't miss something).

Thanks for everyone's time and patience. Stay safe out there.

 - Collin

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Cornelia Huck]

On Wed, 15 Jul 2020 11:36:35 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> Polite ping. Patches have been sitting on the list for a few weeks now,
> and it doesn't look like any further changes are requested (hopefully I
> didn't miss something).

The only thing I had was (I think) the logging of the length you just
replied to. We can still tweak things like that later, of course.

As these patches depend on a headers sync, I could not yet queue them.
I can keep a preliminary version on a branch. I assume that the header
changes will go in during the next kernel merge window? (If I missed
something, apologies for that.)

> 
> Thanks for everyone's time and patience. Stay safe out there.
> 
>  - Collin
> 

Re: [PATCH v4 6/8] s390/sclp: add extended-length sccb support for kvm guest

[Cornelia Huck]

On Wed, 15 Jul 2020 11:35:06 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 6/26/20 6:01 AM, Cornelia Huck wrote:
> > On Wed, 24 Jun 2020 16:23:10 -0400
> > Collin Walling <walling@linux.ibm.com> wrote:
> >   
> >> As more features and facilities are added to the Read SCP Info (RSCPI)
> >> response, more space is required to store them. The space used to store
> >> these new features intrudes on the space originally used to store CPU
> >> entries. This means as more features and facilities are added to the
> >> RSCPI response, less space can be used to store CPU entries.
> >>
> >> With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
> >> the RSCPI command and determine if the SCCB is large enough to store a
> >> complete reponse. If it is not large enough, then the required length
> >> will be set in the SCCB header.
> >>
> >> The caller of the SCLP command is responsible for creating a
> >> large-enough SCCB to store a complete response. Proper checking should
> >> be in place, and the caller should execute the command once-more with
> >> the large-enough SCCB.
> >>
> >> This facility also enables an extended SCCB for the Read CPU Info
> >> (RCPUI) command.
> >>
> >> When this facility is enabled, the boundary violation response cannot
> >> be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
> >>
> >> In order to tolerate kernels that do not yet have full support for this
> >> feature, a "fixed" offset to the start of the CPU Entries within the
> >> Read SCP Info struct is set to allow for the original 248 max entries
> >> when this feature is disabled.
> >>
> >> Additionally, this is introduced as a CPU feature to protect the guest
> >> from migrating to a machine that does not support storing an extended
> >> SCCB. This could otherwise hinder the VM from being able to read all
> >> available CPU entries after migration (such as during re-ipl).
> >>
> >> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> >> ---
> >>  hw/s390x/sclp.c                     | 24 +++++++++++++++++++++++-
> >>  include/hw/s390x/sclp.h             |  1 +
> >>  target/s390x/cpu_features_def.inc.h |  1 +
> >>  target/s390x/gen-features.c         |  1 +
> >>  target/s390x/kvm.c                  |  8 ++++++++
> >>  5 files changed, 34 insertions(+), 1 deletion(-)
> >>  
> > 
> > (...)
> >   
> >> @@ -111,6 +131,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> >>      CPUEntry *entries_start = (void *)sccb + offset_cpu;
> >>  
> >>      if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "insufficient sccb size to store "
> >> +                      "read scp info response\n");  
> > 
> > Not sure if logging needed/provided length would be helpful here.
> >   
> 
> I had the thought that it may be beneficial for kernel development -- it
> gives a response if an SCCB size needs to be larger.

If we have the info, we might as well log it, I guess.

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Collin Walling]

On 7/15/20 12:04 PM, Cornelia Huck wrote:
> On Wed, 15 Jul 2020 11:36:35 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> Polite ping. Patches have been sitting on the list for a few weeks now,
>> and it doesn't look like any further changes are requested (hopefully I
>> didn't miss something).
> 
> The only thing I had was (I think) the logging of the length you just
> replied to. We can still tweak things like that later, of course.
> 
> As these patches depend on a headers sync, I could not yet queue them.
> I can keep a preliminary version on a branch. I assume that the header
> changes will go in during the next kernel merge window? (If I missed
> something, apologies for that.)
> 

Gotcha. Thanks for the update :)

There was an email on the KVM list a couple of days that made one change
to the Linux header. Just changed the integer used for the DIAG cap,
which should be reflected in QEMU as well.

https://www.spinics.net/lists/kvm/msg220548.html

Should I respin this patch series to include the new ack's and account
for the header sync?

>>
>> Thanks for everyone's time and patience. Stay safe out there.
>>
>>  - Collin
>>
> 
> 


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Cornelia Huck]

On Wed, 15 Jul 2020 12:26:20 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 7/15/20 12:04 PM, Cornelia Huck wrote:
> > On Wed, 15 Jul 2020 11:36:35 -0400
> > Collin Walling <walling@linux.ibm.com> wrote:
> >   
> >> Polite ping. Patches have been sitting on the list for a few weeks now,
> >> and it doesn't look like any further changes are requested (hopefully I
> >> didn't miss something).  
> > 
> > The only thing I had was (I think) the logging of the length you just
> > replied to. We can still tweak things like that later, of course.
> > 
> > As these patches depend on a headers sync, I could not yet queue them.
> > I can keep a preliminary version on a branch. I assume that the header
> > changes will go in during the next kernel merge window? (If I missed
> > something, apologies for that.)
> >   
> 
> Gotcha. Thanks for the update :)
> 
> There was an email on the KVM list a couple of days that made one change
> to the Linux header. Just changed the integer used for the DIAG cap,
> which should be reflected in QEMU as well.
> 
> https://www.spinics.net/lists/kvm/msg220548.html
> 
> Should I respin this patch series to include the new ack's and account
> for the header sync?

No need for that, my tooling picks up acks and the headers update needs
to be replaced with a sync against a proper Linux version anyway.

I've queued the patches on a local branch, and the only patch that did
not apply cleanly was the headers patch, which will get replaced later
anyway :) Just ping me when the kernel patches hit upstream, then I'll
do a header sync against the next -rc and queue the patches on
s390-next.

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[David Hildenbrand]

On 24.06.20 22:23, Collin Walling wrote:
> Rework the SCLP boundary check to account for different SCLP commands
> (eventually) allowing different boundary sizes.
> 
> Move the length check code into a separate function, and introduce a
> new function to determine the length of the read SCP data (i.e. the size
> from the start of the struct to where the CPU entries should begin).
> 
> The format of read CPU info is unlikely to change in the future,
> so we do not require a separate function to calculate its length.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 181ce04007..5899c1e3b8 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
>      return false;
>  }
>  
> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint32_t code,
> +                                 SCCBHeader *header)
> +{
> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1;
> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> +
> +    switch (code & SCLP_CMD_CODE_MASK) {
> +    default:
> +        if (sccb_max_addr < sccb_boundary) {
> +            return true;
> +        }
> +    }

^ what is that?

    if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
        return true;
    }

> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +    return false;

So we return "false" on success? At least I consider that weird when
returning the bool type. Maybe make it clearer what the function indicates

"sccb_boundary_is_invalid"

or leave it named as is and switch from return value "bool" to "int",
using "0" on success and "-EINVAL" on error.

> +}
> +
> +/* Calculates sufficient SCCB length to store a full Read SCP/CPU response */
> +static bool sccb_verify_length(SCCB *sccb, int num_cpus, int offset_cpu)
> +{
> +    int required_len = offset_cpu + num_cpus * sizeof(CPUEntry);
> +
> +    if (be16_to_cpu(sccb->h.length) < required_len) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>  {
>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> @@ -66,6 +94,11 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>      }
>  }
>  
> +static inline int get_read_scp_info_offset_cpu(void)
> +{
> +    return offsetof(ReadInfo, entries);
> +}
> +
>  /* Provide information about the configuration, CPUs and storage */
>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  {
> @@ -74,17 +107,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int cpu_count;
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
> +    int offset_cpu = get_read_scp_info_offset_cpu();
>  
> -    if (be16_to_cpu(sccb->h.length) <
> -          (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +    if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
>          return;
>      }
>  
>      /* CPU information */
>      prepare_cpu_entries(machine, read_info->entries, &cpu_count);
>      read_info->entries_cpu = cpu_to_be16(cpu_count);
> -    read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
> +    read_info->offset_cpu = cpu_to_be16(offset_cpu);
>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
>  
>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
> @@ -133,17 +165,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
> +    int offset_cpu = offsetof(ReadCpuInfo, entries);
>      int cpu_count;
>  
> -    if (be16_to_cpu(sccb->h.length) <
> -          (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +    if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
>          return;
>      }
>  
>      prepare_cpu_entries(machine, cpu_info->entries, &cpu_count);
>      cpu_info->nr_configured = cpu_to_be16(cpu_count);
> -    cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
> +    cpu_info->offset_configured = cpu_to_be16(offset_cpu);
>      cpu_info->nr_standby = cpu_to_be16(0);
>  
>      /* The standby offset is 16-byte for each CPU */
> @@ -229,6 +260,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> +    if (!sccb_verify_boundary(sccb, code, &work_sccb.h)) {
> +        goto out_write;
> +    }
> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> @@ -274,8 +309,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>          goto out_write;
>      }
>  
> -    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +    if (!sccb_verify_boundary(sccb, code, &work_sccb.h)) {
>          goto out_write;
>      }
>  
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v4 4/8] s390/sclp: read sccb from mem based on sccb length

[David Hildenbrand]

On 24.06.20 22:23, Collin Walling wrote:
> The header of the SCCB contains the actual length of the SCCB. Instead
> of using a static 4K size, let's allow for a variable size determined
> by the value set in the header. The proper checks are already in place
> to ensure the SCCB length is sufficent to store a full response, and
> that the length does not cross any explicitly-set boundaries.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/sclp.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 5899c1e3b8..1feba6f692 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -251,9 +251,8 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>      SCLPDevice *sclp = get_sclp_device();
>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>      SCCB work_sccb;
> -    hwaddr sccb_len = sizeof(SCCB);
>  
> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader));
>  
>      if (!sclp_command_code_valid(code)) {
>          work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> @@ -264,6 +263,9 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb,
> +                         be16_to_cpu(work_sccb.h.length));
> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> @@ -278,8 +280,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>      SCCB work_sccb;
>  
> -    hwaddr sccb_len = sizeof(SCCB);
> -
>      /* first some basic checks on program checks */
>      if (env->psw.mask & PSW_MASK_PSTATE) {
>          return -PGM_PRIVILEGED;
> @@ -297,7 +297,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>       * from playing dirty tricks by modifying the memory content after
>       * the host has checked the values
>       */
> -    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
> +    cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader));
>  
>      /* Valid sccb sizes */
>      if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
> @@ -313,6 +313,9 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>          goto out_write;
>      }
>  
> +    /* the header contains the actual length of the sccb */
> +    cpu_physical_memory_read(sccb, &work_sccb, be16_to_cpu(work_sccb.h.length));
> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      cpu_physical_memory_write(sccb, &work_sccb,
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

On 7/20/20 4:17 AM, David Hildenbrand wrote:
> On 24.06.20 22:23, Collin Walling wrote:
>> Rework the SCLP boundary check to account for different SCLP commands
>> (eventually) allowing different boundary sizes.
>>
>> Move the length check code into a separate function, and introduce a
>> new function to determine the length of the read SCP data (i.e. the size
>> from the start of the struct to where the CPU entries should begin).
>>
>> The format of read CPU info is unlikely to change in the future,
>> so we do not require a separate function to calculate its length.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 44 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 181ce04007..5899c1e3b8 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
>>      return false;
>>  }
>>  
>> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint32_t code,
>> +                                 SCCBHeader *header)
>> +{
>> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1;
>> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>> +
>> +    switch (code & SCLP_CMD_CODE_MASK) {
>> +    default:
>> +        if (sccb_max_addr < sccb_boundary) {
>> +            return true;
>> +        }
>> +    }
> 
> ^ what is that?
> 
>     if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>         return true;
>     }
> 

I agree it looks pointless in this patch, but it makes more sense in
patch #6 where we introduce cases for the SCLP commands that bypass
these checks if the extended-length sccb feature is enabled.

>> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    return false;
> 
> So we return "false" on success? At least I consider that weird when
> returning the bool type. Maybe make it clearer what the function indicates
> 

Hmmm... I figured since there were more paths that can lead to success
(i.e. when I introduce the feat check in a later patch), then it made
more sense to to return false at the end. sclp_command_code_valid has
similar logic.

But if boolean functions traditionally return true as the last return
value, I can rework it to align to coding preferences / standards.

> "sccb_boundary_is_invalid"
> 

Unless it's simply the name that is confusing?

> or leave it named as is and switch from return value "bool" to "int",
> using "0" on success and "-EINVAL" on error.
> 

Is the switch statement an overkill? I thought of it as a cleaner way to
later show which commands have a special conditions (introduced in patch
6 for the ELS stuff) instead of a nasty long if statement.

The alternative...

/* Comment explaining this check */
if ((code & SCLP_CMD_CODE_MASK) & (SCLP_CMDW_READ_SCP_INFO |
	SCLP_CMDW_READ_SCP_INFO_FORCED | SCLP_CMDW_READ_CPU_INFO) &&
	s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
	return true;
}

if (sccb_max_addr < sccb_boundary) {
        return true;
}

header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
return false;

[...]

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v4 4/8] s390/sclp: read sccb from mem based on sccb length

[Collin Walling]

On 7/20/20 4:19 AM, David Hildenbrand wrote:
> On 24.06.20 22:23, Collin Walling wrote:
>> The header of the SCCB contains the actual length of the SCCB. Instead
>> of using a static 4K size, let's allow for a variable size determined
>> by the value set in the header. The proper checks are already in place
>> to ensure the SCCB length is sufficent to store a full response, and
>> that the length does not cross any explicitly-set boundaries.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  hw/s390x/sclp.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 5899c1e3b8..1feba6f692 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -251,9 +251,8 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>      SCLPDevice *sclp = get_sclp_device();
>>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>>      SCCB work_sccb;
>> -    hwaddr sccb_len = sizeof(SCCB);
>>  
>> -    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader));
>>  
>>      if (!sclp_command_code_valid(code)) {
>>          work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> @@ -264,6 +263,9 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>          goto out_write;
>>      }
>>  
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb,
>> +                         be16_to_cpu(work_sccb.h.length));
>> +
>>      sclp_c->execute(sclp, &work_sccb, code);
>>  out_write:
>>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
>> @@ -278,8 +280,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>      SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>>      SCCB work_sccb;
>>  
>> -    hwaddr sccb_len = sizeof(SCCB);
>> -
>>      /* first some basic checks on program checks */
>>      if (env->psw.mask & PSW_MASK_PSTATE) {
>>          return -PGM_PRIVILEGED;
>> @@ -297,7 +297,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>       * from playing dirty tricks by modifying the memory content after
>>       * the host has checked the values
>>       */
>> -    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
>> +    cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader));
>>  
>>      /* Valid sccb sizes */
>>      if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
>> @@ -313,6 +313,9 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>>          goto out_write;
>>      }
>>  
>> +    /* the header contains the actual length of the sccb */
>> +    cpu_physical_memory_read(sccb, &work_sccb, be16_to_cpu(work_sccb.h.length));
>> +
>>      sclp_c->execute(sclp, &work_sccb, code);
>>  out_write:
>>      cpu_physical_memory_write(sccb, &work_sccb,
>>
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 

Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[David Hildenbrand]

[...]

>>> +    switch (code & SCLP_CMD_CODE_MASK) {
>>> +    default:
>>> +        if (sccb_max_addr < sccb_boundary) {
>>> +            return true;
>>> +        }
>>> +    }
>>
>> ^ what is that?
>>
>>     if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>>         return true;
>>     }

Oh, my tired eyes missed that it's actually only

if (sccb_max_addr < sccb_boundary) :)

>>
> 
> I agree it looks pointless in this patch, but it makes more sense in
> patch #6 where we introduce cases for the SCLP commands that bypass
> these checks if the extended-length sccb feature is enabled.

I am really a friend of introducing stuff where needed. Just use a
simple "if" here and convert to the switch in patch #6.

> 
>>> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>> +    return false;
>>
>> So we return "false" on success? At least I consider that weird when
>> returning the bool type. Maybe make it clearer what the function indicates
>>
> 
> Hmmm... I figured since there were more paths that can lead to success
> (i.e. when I introduce the feat check in a later patch), then it made
> more sense to to return false at the end. sclp_command_code_valid has
> similar logic.
> 
> But if boolean functions traditionally return true as the last return
> value, I can rework it to align to coding preferences / standards.
> 
>> "sccb_boundary_is_invalid"
>>
> 
> Unless it's simply the name that is confusing?

The options I would support are

1. "sccb_boundary_is_valid" which returns "true" if valid
2. "sccb_boundary_is_invalid" which returns "true" if invalid
3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.

Which makes reading this code a bit easier.

> 
>> or leave it named as is and switch from return value "bool" to "int",
>> using "0" on success and "-EINVAL" on error.
>>
> 
> Is the switch statement an overkill? I thought of it as a cleaner way to
> later show which commands have a special conditions (introduced in patch
> 6 for the ELS stuff) instead of a nasty long if statement.

I think the switch make sense in patch #6.

-- 
Thanks,

David / dhildenb

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

On 7/21/20 4:41 AM, David Hildenbrand wrote:
> [...]
> 
>>>> +    switch (code & SCLP_CMD_CODE_MASK) {
>>>> +    default:
>>>> +        if (sccb_max_addr < sccb_boundary) {
>>>> +            return true;
>>>> +        }
>>>> +    }
>>>
>>> ^ what is that?
>>>
>>>     if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>>>         return true;
>>>     }
> 
> Oh, my tired eyes missed that it's actually only
> 
> if (sccb_max_addr < sccb_boundary) :)
> 
>>>
>>
>> I agree it looks pointless in this patch, but it makes more sense in
>> patch #6 where we introduce cases for the SCLP commands that bypass
>> these checks if the extended-length sccb feature is enabled.
> 
> I am really a friend of introducing stuff where needed. Just use a
> simple "if" here and convert to the switch in patch #6.
> 

I can understand that. This follows the whole "each patch should be
sufficient on its own" way of thinking. A switch with no cases and a
default _does_ look silly.

>>
>>>> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>>> +    return false;
>>>
>>> So we return "false" on success? At least I consider that weird when
>>> returning the bool type. Maybe make it clearer what the function indicates
>>>
>>
>> Hmmm... I figured since there were more paths that can lead to success
>> (i.e. when I introduce the feat check in a later patch), then it made
>> more sense to to return false at the end. sclp_command_code_valid has
>> similar logic.
>>
>> But if boolean functions traditionally return true as the last return
>> value, I can rework it to align to coding preferences / standards.
>>
>>> "sccb_boundary_is_invalid"
>>>
>>
>> Unless it's simply the name that is confusing?
> 
> The options I would support are
> 
> 1. "sccb_boundary_is_valid" which returns "true" if valid
> 2. "sccb_boundary_is_invalid" which returns "true" if invalid
> 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
> 
> Which makes reading this code a bit easier.
> 

Sounds good. I'll takes this into consideration for the next round. (I
may wait just a little longer for that to allow more reviews to come in
from whoever has the time, if that's okay.)

>>
>>> or leave it named as is and switch from return value "bool" to "int",
>>> using "0" on success and "-EINVAL" on error.
>>>
>>
>> Is the switch statement an overkill? I thought of it as a cleaner way to
>> later show which commands have a special conditions (introduced in patch
>> 6 for the ELS stuff) instead of a nasty long if statement.
> 
> I think the switch make sense in patch #6.
> 


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Cornelia Huck]

On Tue, 21 Jul 2020 14:40:14 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 7/21/20 4:41 AM, David Hildenbrand wrote:

> > The options I would support are
> > 
> > 1. "sccb_boundary_is_valid" which returns "true" if valid
> > 2. "sccb_boundary_is_invalid" which returns "true" if invalid
> > 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
> > 
> > Which makes reading this code a bit easier.
> >   

Of these, I like option 1 best.

> 
> Sounds good. I'll takes this into consideration for the next round. (I
> may wait just a little longer for that to allow more reviews to come in
> from whoever has the time, if that's okay.)

We have to wait for (a) QEMU to do a release and (b) the Linux changes
to merge upstream anyway, so we're not in a hurry :)

As said before, it already looked good from my side, but the suggested
changes are fine with me as well.

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

On 7/23/20 2:26 AM, Cornelia Huck wrote:
> On Tue, 21 Jul 2020 14:40:14 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> On 7/21/20 4:41 AM, David Hildenbrand wrote:
> 
>>> The options I would support are
>>>
>>> 1. "sccb_boundary_is_valid" which returns "true" if valid
>>> 2. "sccb_boundary_is_invalid" which returns "true" if invalid
>>> 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
>>>
>>> Which makes reading this code a bit easier.
>>>   
> 
> Of these, I like option 1 best.
> 
>>
>> Sounds good. I'll takes this into consideration for the next round. (I
>> may wait just a little longer for that to allow more reviews to come in
>> from whoever has the time, if that's okay.)
> 
> We have to wait for (a) QEMU to do a release and (b) the Linux changes
> to merge upstream anyway, so we're not in a hurry :)
> 
> As said before, it already looked good from my side, but the suggested
> changes are fine with me as well.
> 
> 

Okay, thanks for the info.

I do want to send out a v5 of these patches. While working with someone
who is implementing the kernel support for the extended-length SCCB, we
found some snags. I'll highlight these changes/fixes in the respective
patches on the next version.

Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Christian Borntraeger]

On 16.07.20 14:02, Cornelia Huck wrote:
> On Wed, 15 Jul 2020 12:26:20 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> On 7/15/20 12:04 PM, Cornelia Huck wrote:
>>> On Wed, 15 Jul 2020 11:36:35 -0400
>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>   
>>>> Polite ping. Patches have been sitting on the list for a few weeks now,
>>>> and it doesn't look like any further changes are requested (hopefully I
>>>> didn't miss something).  
>>>
>>> The only thing I had was (I think) the logging of the length you just
>>> replied to. We can still tweak things like that later, of course.
>>>
>>> As these patches depend on a headers sync, I could not yet queue them.
>>> I can keep a preliminary version on a branch. I assume that the header
>>> changes will go in during the next kernel merge window? (If I missed
>>> something, apologies for that.)
>>>   
>>
>> Gotcha. Thanks for the update :)
>>
>> There was an email on the KVM list a couple of days that made one change
>> to the Linux header. Just changed the integer used for the DIAG cap,
>> which should be reflected in QEMU as well.
>>
>> https://www.spinics.net/lists/kvm/msg220548.html
>>
>> Should I respin this patch series to include the new ack's and account
>> for the header sync?
> 
> No need for that, my tooling picks up acks and the headers update needs
> to be replaced with a sync against a proper Linux version anyway.
> 
> I've queued the patches on a local branch, and the only patch that did
> not apply cleanly was the headers patch, which will get replaced later
> anyway :) Just ping me when the kernel patches hit upstream, then I'll
> do a header sync against the next -rc and queue the patches on
> s390-next.

What is the status of this patchset? The kernel part has been merged in 5.9-rc1.

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Cornelia Huck]

On Wed, 9 Sep 2020 09:54:40 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 16.07.20 14:02, Cornelia Huck wrote:
> > On Wed, 15 Jul 2020 12:26:20 -0400
> > Collin Walling <walling@linux.ibm.com> wrote:
> >   
> >> On 7/15/20 12:04 PM, Cornelia Huck wrote:  
> >>> On Wed, 15 Jul 2020 11:36:35 -0400
> >>> Collin Walling <walling@linux.ibm.com> wrote:
> >>>     
> >>>> Polite ping. Patches have been sitting on the list for a few weeks now,
> >>>> and it doesn't look like any further changes are requested (hopefully I
> >>>> didn't miss something).    
> >>>
> >>> The only thing I had was (I think) the logging of the length you just
> >>> replied to. We can still tweak things like that later, of course.
> >>>
> >>> As these patches depend on a headers sync, I could not yet queue them.
> >>> I can keep a preliminary version on a branch. I assume that the header
> >>> changes will go in during the next kernel merge window? (If I missed
> >>> something, apologies for that.)
> >>>     
> >>
> >> Gotcha. Thanks for the update :)
> >>
> >> There was an email on the KVM list a couple of days that made one change
> >> to the Linux header. Just changed the integer used for the DIAG cap,
> >> which should be reflected in QEMU as well.
> >>
> >> https://www.spinics.net/lists/kvm/msg220548.html
> >>
> >> Should I respin this patch series to include the new ack's and account
> >> for the header sync?  
> > 
> > No need for that, my tooling picks up acks and the headers update needs
> > to be replaced with a sync against a proper Linux version anyway.
> > 
> > I've queued the patches on a local branch, and the only patch that did
> > not apply cleanly was the headers patch, which will get replaced later
> > anyway :) Just ping me when the kernel patches hit upstream, then I'll
> > do a header sync against the next -rc and queue the patches on
> > s390-next.  
> 
> What is the status of this patchset? The kernel part has been merged in 5.9-rc1.
> 

Thanks for letting me know, I'll go and process this now. (Had some
busy weeks + PTO.)

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Cornelia Huck]

On Wed, 9 Sep 2020 10:46:23 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 9 Sep 2020 09:54:40 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 16.07.20 14:02, Cornelia Huck wrote:  
> > > On Wed, 15 Jul 2020 12:26:20 -0400
> > > Collin Walling <walling@linux.ibm.com> wrote:
> > >     
> > >> On 7/15/20 12:04 PM, Cornelia Huck wrote:    
> > >>> On Wed, 15 Jul 2020 11:36:35 -0400
> > >>> Collin Walling <walling@linux.ibm.com> wrote:
> > >>>       
> > >>>> Polite ping. Patches have been sitting on the list for a few weeks now,
> > >>>> and it doesn't look like any further changes are requested (hopefully I
> > >>>> didn't miss something).      
> > >>>
> > >>> The only thing I had was (I think) the logging of the length you just
> > >>> replied to. We can still tweak things like that later, of course.
> > >>>
> > >>> As these patches depend on a headers sync, I could not yet queue them.
> > >>> I can keep a preliminary version on a branch. I assume that the header
> > >>> changes will go in during the next kernel merge window? (If I missed
> > >>> something, apologies for that.)
> > >>>       
> > >>
> > >> Gotcha. Thanks for the update :)
> > >>
> > >> There was an email on the KVM list a couple of days that made one change
> > >> to the Linux header. Just changed the integer used for the DIAG cap,
> > >> which should be reflected in QEMU as well.
> > >>
> > >> https://www.spinics.net/lists/kvm/msg220548.html
> > >>
> > >> Should I respin this patch series to include the new ack's and account
> > >> for the header sync?    
> > > 
> > > No need for that, my tooling picks up acks and the headers update needs
> > > to be replaced with a sync against a proper Linux version anyway.
> > > 
> > > I've queued the patches on a local branch, and the only patch that did
> > > not apply cleanly was the headers patch, which will get replaced later
> > > anyway :) Just ping me when the kernel patches hit upstream, then I'll
> > > do a header sync against the next -rc and queue the patches on
> > > s390-next.    
> > 
> > What is the status of this patchset? The kernel part has been merged in 5.9-rc1.
> >   
> 
> Thanks for letting me know, I'll go and process this now. (Had some
> busy weeks + PTO.)

There were some minor conflicts, please double check that everything
looks sane.

Thanks, applied.

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Collin Walling]

On 9/9/20 5:43 AM, Cornelia Huck wrote:
> On Wed, 9 Sep 2020 10:46:23 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Wed, 9 Sep 2020 09:54:40 +0200
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 16.07.20 14:02, Cornelia Huck wrote:  
>>>> On Wed, 15 Jul 2020 12:26:20 -0400
>>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>>     
>>>>> On 7/15/20 12:04 PM, Cornelia Huck wrote:    
>>>>>> On Wed, 15 Jul 2020 11:36:35 -0400
>>>>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>>>>       
>>>>>>> Polite ping. Patches have been sitting on the list for a few weeks now,
>>>>>>> and it doesn't look like any further changes are requested (hopefully I
>>>>>>> didn't miss something).      
>>>>>>
>>>>>> The only thing I had was (I think) the logging of the length you just
>>>>>> replied to. We can still tweak things like that later, of course.
>>>>>>
>>>>>> As these patches depend on a headers sync, I could not yet queue them.
>>>>>> I can keep a preliminary version on a branch. I assume that the header
>>>>>> changes will go in during the next kernel merge window? (If I missed
>>>>>> something, apologies for that.)
>>>>>>       
>>>>>
>>>>> Gotcha. Thanks for the update :)
>>>>>
>>>>> There was an email on the KVM list a couple of days that made one change
>>>>> to the Linux header. Just changed the integer used for the DIAG cap,
>>>>> which should be reflected in QEMU as well.
>>>>>
>>>>> https://www.spinics.net/lists/kvm/msg220548.html
>>>>>
>>>>> Should I respin this patch series to include the new ack's and account
>>>>> for the header sync?    
>>>>
>>>> No need for that, my tooling picks up acks and the headers update needs
>>>> to be replaced with a sync against a proper Linux version anyway.
>>>>
>>>> I've queued the patches on a local branch, and the only patch that did
>>>> not apply cleanly was the headers patch, which will get replaced later
>>>> anyway :) Just ping me when the kernel patches hit upstream, then I'll
>>>> do a header sync against the next -rc and queue the patches on
>>>> s390-next.    
>>>
>>> What is the status of this patchset? The kernel part has been merged in 5.9-rc1.
>>>   
>>
>> Thanks for letting me know, I'll go and process this now. (Had some
>> busy weeks + PTO.)
> 
> There were some minor conflicts, please double check that everything
> looks sane.
> 
> Thanks, applied.
> 
> 

Has this been merged yet? There is one patch that I neglected to include
in this series (I didn't realize it until now) that properly sets the
SCCB size in QEMU based on the length provided by the kernel (right now,
these patches allocate a static 4K size for the SCCB struct, which
causes a segfault).

I can post my set with the fix as v5, or I can wait and post the fix as
a bandaid if the patches are already in.

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Cornelia Huck]

On Wed, 9 Sep 2020 14:13:55 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> Has this been merged yet? There is one patch that I neglected to include
> in this series (I didn't realize it until now) that properly sets the
> SCCB size in QEMU based on the length provided by the kernel (right now,
> these patches allocate a static 4K size for the SCCB struct, which
> causes a segfault).
> 
> I can post my set with the fix as v5, or I can wait and post the fix as
> a bandaid if the patches are already in.
> 

It's queued on s390-next right now, I can still update it.

Is that really an extra patch, or something that can be merged into the
series? If the latter, I'd prefer a v5, if the former, I'd just queue
that patch on top.

Re: [PATCH v4 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Collin Walling]

On 9/10/20 2:38 AM, Cornelia Huck wrote:
> On Wed, 9 Sep 2020 14:13:55 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> Has this been merged yet? There is one patch that I neglected to include
>> in this series (I didn't realize it until now) that properly sets the
>> SCCB size in QEMU based on the length provided by the kernel (right now,
>> these patches allocate a static 4K size for the SCCB struct, which
>> causes a segfault).
>>
>> I can post my set with the fix as v5, or I can wait and post the fix as
>> a bandaid if the patches are already in.
>>
> 
> It's queued on s390-next right now, I can still update it.
> 
> Is that really an extra patch, or something that can be merged into the
> series? If the latter, I'd prefer a v5, if the former, I'd just queue
> that patch on top.
> 
> 

I'll post a v5. Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy