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

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

[Collin Walling]

Changelog:

    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/s390-virtio-ccw.c          |  45 +++++++++++
 hw/s390x/sclp.c                     | 117 ++++++++++++++++++++++------
 include/hw/s390x/s390-virtio-ccw.h  |   1 +
 include/hw/s390x/sclp.h             |   4 +
 linux-headers/asm-s390/kvm.h        |   4 +
 target/s390x/cpu.c                  |  23 ++++++
 target/s390x/cpu.h                  |   4 +
 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-stub.c             |  10 +++
 target/s390x/kvm.c                  |  44 +++++++++++
 target/s390x/kvm_s390x.h            |   2 +
 14 files changed, 237 insertions(+), 25 deletions(-)

-- 
2.21.3

[PATCH v2 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>
---
 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 ede056b3ef..61e2e2839c 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.21.3

[PATCH v2 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>
---
 hw/s390x/sclp.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 61e2e2839c..2bd618515e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -75,6 +75,11 @@ 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) + cpu_count * 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 +88,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 +134,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
     int cpu_count;
 
+    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * 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);
 
-    if (be16_to_cpu(sccb->h.length) <
-            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
-        return;
-    }
-
     /* 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.21.3

[PATCH v2 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).

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
+                                    SCCBHeader *header)
+{
+    uint64_t current_len = sccb_addr + be16_to_cpu(header->length);
+    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
+
+    switch (code & SCLP_CMD_CODE_MASK) {
+    default:
+        if (current_len <= allowed_len) {
+            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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
+{
+    int required_len = data_len + 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,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
     }
 }
 
+/*
+ * The data length denotes the start of the struct to where the first
+ * CPU entry is to be allocated. This value also denotes the offset_cpu
+ * field.
+ */
+static int get_read_scp_info_data_len(void)
+{
+    return offsetof(ReadInfo, entries);
+}
+
 /* Provide information about the configuration, CPUs and storage */
 static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 {
@@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int cpu_count;
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
+    int data_len = get_read_scp_info_data_len();
 
-    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);
+    if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
         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(data_len);
     read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
 
     read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
@@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
     ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
+    int data_len = offsetof(ReadCpuInfo, entries);
     int cpu_count;
 
-    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+    if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
         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(data_len);
     cpu_info->nr_standby = cpu_to_be16(0);
 
     /* The standby offset is 16-byte for each CPU */
@@ -227,6 +265,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
         goto out_write;
     }
 
+    if (!sccb_has_valid_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,
@@ -272,8 +314,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_has_valid_boundary(sccb, code, &work_sccb.h)) {
         goto out_write;
     }
 
-- 
2.21.3

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

[Collin Walling]

The header 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>
---
 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 987699e3c4..5d6e98ae64 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -256,9 +256,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);
@@ -269,6 +268,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,
@@ -283,8 +285,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;
@@ -302,7 +302,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)) {
@@ -318,6 +318,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.21.3

[PATCH v2 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>
---
 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 5d6e98ae64..755f5f3fab 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -113,13 +113,14 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     IplParameterBlock *ipib = s390_ipl_get_iplb();
     int data_len = get_read_scp_info_data_len();
+    CPUEntry *entries_start = (void *)sccb + data_len;
 
     if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
         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(data_len);
     read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
-- 
2.21.3

[PATCH v2 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                     | 21 ++++++++++++++++++++-
 include/hw/s390x/sclp.h             |  1 +
 target/s390x/cpu_features_def.inc.h |  1 +
 target/s390x/gen-features.c         |  1 +
 target/s390x/kvm.c                  |  4 ++++
 5 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 755f5f3fab..bde4c5420e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
     uint64_t allowed_len = (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 RSCPI and RSCPU 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;
+        }
     default:
         if (current_len <= allowed_len) {
             return true;
@@ -72,6 +84,10 @@ static bool sccb_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
 
     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;
@@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
  */
 static int get_read_scp_info_data_len(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 */
@@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     CPUEntry *entries_start = (void *)sccb + data_len;
 
     if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
+        warn_report("insufficient sccb size to store full read scp info response");
         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 60db28351d..3548d65a69 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 69881a0da0..380fb81822 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2456,6 +2456,10 @@ 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 */
+    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.21.3

[PATCH v2 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 0138ccb0d8..3d85817ec2 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
 #define KVM_S390_VM_CRYPTO		2
 #define KVM_S390_VM_CPU_MODEL		3
 #define KVM_S390_VM_MIGRATION		4
+#define KVM_S390_VM_MISC		5
 
 /* kvm attributes for mem_ctrl */
 #define KVM_S390_VM_MEM_ENABLE_CMMA	0
@@ -171,6 +172,9 @@ struct kvm_s390_vm_cpu_subfunc {
 #define KVM_S390_VM_MIGRATION_START	1
 #define KVM_S390_VM_MIGRATION_STATUS	2
 
+/* kvm attributes for KVM_S390_VM_MISC */
+#define KVM_S390_VM_MISC_DIAG_318	0
+
 /* for KVM_GET_REGS and KVM_SET_REGS */
 struct kvm_regs {
 	/* general purpose regs for s390 */
-- 
2.21.3

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

[Collin Walling]

DIAGNOSE 0x318 (diag 318) allows the storage of diagnostic data that
is collected by the firmware in the case of hardware/firmware service
events.

The instruction is invoked in the Linux kernel and is handled,
migrated, and reset (modified clear and load normal) by QEMU.
KVM assists with the get/set of the relevent data via IOCTLs.

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 diag 318
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>
---
 hw/s390x/s390-virtio-ccw.c          | 45 +++++++++++++++++++++++++++++
 hw/s390x/sclp.c                     |  5 ++++
 include/hw/s390x/s390-virtio-ccw.h  |  1 +
 include/hw/s390x/sclp.h             |  3 ++
 target/s390x/cpu.c                  | 23 +++++++++++++++
 target/s390x/cpu.h                  |  4 +++
 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-stub.c             | 10 +++++++
 target/s390x/kvm.c                  | 40 +++++++++++++++++++++++++
 target/s390x/kvm_s390x.h            |  2 ++
 13 files changed, 139 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 45292fb5a8..9cc5f4cede 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -242,6 +242,40 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
     qdev_init_nofail(dev);
 }
 
+static int diag_318_post_load(void *opaque, int version_id)
+{
+    S390CcwMachineState *d = opaque;
+
+    s390_set_diag_318_info(d->diag_318_info);
+    return 0;
+}
+
+static int diag_318_pre_save(void *opaque)
+{
+    S390CcwMachineState *d = opaque;
+
+    s390_get_diag_318_info(&d->diag_318_info);
+    return 0;
+}
+
+static bool diag_318_needed(void *opaque)
+{
+    return s390_diag_318_is_allowed();
+}
+
+const VMStateDescription vmstate_diag_318 = {
+    .name = "vmstate_diag_318",
+    .post_load = diag_318_post_load,
+    .pre_save = diag_318_pre_save,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = diag_318_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(diag_318_info, S390CcwMachineState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void ccw_init(MachineState *machine)
 {
     int ret;
@@ -299,6 +333,8 @@ static void ccw_init(MachineState *machine)
 
     /* init the TOD clock */
     s390_init_tod();
+
+    vmstate_register(NULL, 0, &vmstate_diag_318, machine);
 }
 
 static void s390_cpu_plug(HotplugHandler *hotplug_dev,
@@ -404,6 +440,13 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
     s390_pv_perf_clear_reset();
 }
 
+static void s390_diag_318_reset(void)
+{
+    if (s390_diag_318_is_allowed()) {
+        s390_set_diag_318_info(0);
+    }
+}
+
 static void s390_machine_reset(MachineState *machine)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
@@ -440,6 +483,7 @@ static void s390_machine_reset(MachineState *machine)
         subsystem_reset();
         s390_crypto_reset();
         s390_pv_prepare_reset(ms);
+        s390_diag_318_reset();
         CPU_FOREACH(t) {
             run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
         }
@@ -452,6 +496,7 @@ static void s390_machine_reset(MachineState *machine)
          */
         subsystem_reset();
         s390_pv_prepare_reset(ms);
+        s390_diag_318_reset();
         CPU_FOREACH(t) {
             if (t == cs) {
                 continue;
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index bde4c5420e..b55fa955c8 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -152,6 +152,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/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index cd1dccc6e3..c9bf7b7096 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -30,6 +30,7 @@ typedef struct S390CcwMachineState {
     bool dea_key_wrap;
     bool pv;
     uint8_t loadparm[8];
+    uint64_t diag_318_info;
 } S390CcwMachineState;
 
 typedef struct S390CcwMachineClass {
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.c b/target/s390x/cpu.c
index f2ccf0a06a..367a54c173 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -446,6 +446,29 @@ void s390_enable_css_support(S390CPU *cpu)
         kvm_s390_enable_css_support(cpu);
     }
 }
+
+void s390_get_diag_318_info(uint64_t *info)
+{
+    if (kvm_enabled()) {
+        kvm_s390_get_diag_318_info(info);
+    }
+}
+
+void s390_set_diag_318_info(uint64_t info)
+{
+    if (kvm_enabled()) {
+        kvm_s390_set_diag_318_info(info);
+    }
+}
+
+bool s390_diag_318_is_allowed(void)
+{
+    if (kvm_enabled()) {
+        return s390_has_feat(S390_FEAT_DIAG_318) &&
+               s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB);
+    }
+    return false;
+}
 #endif
 
 static gchar *s390_gdb_arch_name(CPUState *cs)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 035427521c..205738999e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -769,6 +769,10 @@ void s390_cmma_reset(void);
 void s390_enable_css_support(S390CPU *cpu);
 int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
                                 int vq, bool assign);
+void s390_get_diag_318_info(uint64_t *info);
+void s390_set_diag_318_info(uint64_t info);
+bool s390_diag_318_is_allowed(void);
+
 #ifndef CONFIG_USER_ONLY
 unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu);
 #else
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 3548d65a69..8c5bbfa0ea 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, "diag_318", 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 7c32180269..1d8fc76f7b 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-stub.c b/target/s390x/kvm-stub.c
index aa185017a2..bb5c0d770a 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -120,3 +120,13 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
 void kvm_s390_restart_interrupt(S390CPU *cpu)
 {
 }
+
+int kvm_s390_get_diag_318_info(uint64_t *info)
+{
+    return 0;
+}
+
+int kvm_s390_set_diag_318_info(uint64_t info)
+{
+    return 0;
+}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 380fb81822..5d7dc60c85 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
 
@@ -814,6 +815,28 @@ int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_low)
     return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
 }
 
+int kvm_s390_get_diag_318_info(uint64_t *info)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_MISC,
+        .attr = KVM_S390_VM_MISC_DIAG_318,
+        .addr = (uint64_t)info,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
+}
+
+int kvm_s390_set_diag_318_info(uint64_t info)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_MISC,
+        .attr = KVM_S390_VM_MISC_DIAG_318,
+        .addr = (uint64_t)&info,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
+}
+
 /**
  * kvm_s390_mem_op:
  * @addr:      the logical start address in guest memory
@@ -1601,6 +1624,14 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct kvm_run *run)
     return -ENOENT;
 }
 
+static void kvm_handle_diag_318(struct kvm_run *run)
+{
+    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
+    uint64_t info = run->s.regs.gprs[reg];
+
+    kvm_s390_set_diag_318_info(info);
+}
+
 #define DIAG_KVM_CODE_MASK 0x000000000000ffff
 
 static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
@@ -1620,6 +1651,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:
+        kvm_handle_diag_318(run);
+        break;
     case DIAG_KVM_HYPERCALL:
         r = handle_hypercall(cpu, run);
         break;
@@ -2460,6 +2494,12 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
     /* Extended-Length SCCB is handled entirely within QEMU */
     set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
 
+    /* Allow diag 0x318 iff KVM supported and not in PV mode */
+    if (!s390_is_pv() && kvm_vm_check_attr(kvm_state,
+        KVM_S390_VM_MISC, KVM_S390_VM_MISC_DIAG_318)) {
+        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/kvm_s390x.h b/target/s390x/kvm_s390x.h
index 6ab17c81b7..a9123b3821 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -32,6 +32,8 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
 int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
 int kvm_s390_set_clock(uint8_t tod_high, uint64_t tod_clock);
 int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_clock);
+int kvm_s390_get_diag_318_info(uint64_t *info);
+int kvm_s390_set_diag_318_info(uint64_t info);
 void kvm_s390_enable_css_support(S390CPU *cpu);
 int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign);
-- 
2.21.3

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200515222032.18838-1-walling@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200515222032.18838-1-walling@linux.ibm.com
Subject: [PATCH v2 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f8cb821 s390: guest support for diagnose 0x318
6b87c59 s390/kvm: header sync for diag318
af06627 s390/sclp: add extended-length sccb support for kvm guest
39b848c s390/sclp: use cpu offset to locate cpu entries
1dd8e02 s390/sclp: read sccb from mem based on sccb length
aad956d s390/sclp: rework sclp boundary and length checks
428b1e4 s390/sclp: check sccb len before filling in data
850e1b8 s390/sclp: get machine once during read scp/cpu info

=== OUTPUT BEGIN ===
1/8 Checking commit 850e1b88729f (s390/sclp: get machine once during read scp/cpu info)
2/8 Checking commit 428b1e46e016 (s390/sclp: check sccb len before filling in data)
WARNING: line over 80 characters
#23: FILE: hw/s390x/sclp.c:78:
+    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {

ERROR: line over 90 characters
#48: FILE: hw/s390x/sclp.c:137:
+    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {

total: 1 errors, 1 warnings, 45 lines checked

Patch 2/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/8 Checking commit aad956d5ac92 (s390/sclp: rework sclp boundary and length checks)
4/8 Checking commit 1dd8e02af7b2 (s390/sclp: read sccb from mem based on sccb length)
5/8 Checking commit 39b848c3be15 (s390/sclp: use cpu offset to locate cpu entries)
6/8 Checking commit af06627cc5fb (s390/sclp: add extended-length sccb support for kvm guest)
WARNING: line over 80 characters
#91: FILE: hw/s390x/sclp.c:137:
+        warn_report("insufficient sccb size to store full read scp info response");

WARNING: line over 80 characters
#115: FILE: target/s390x/cpu_features_def.inc.h:100:
+DEF_FEAT(EXTENDED_LENGTH_SCCB, "els", STFL, 140, "Extended-length SCCB facility")

total: 0 errors, 2 warnings, 76 lines checked

Patch 6/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/8 Checking commit 6b87c5992768 (s390/kvm: header sync for diag318)
8/8 Checking commit f8cb821134a7 (s390: guest support for diagnose 0x318)
ERROR: line over 90 characters
#226: FILE: target/s390x/cpu_features_def.inc.h:125:
+/* Features exposed via SCLP SCCB Facilities byte 134 (bit numbers relative to byte-134) */

WARNING: line over 80 characters
#227: FILE: target/s390x/cpu_features_def.inc.h:126:
+DEF_FEAT(DIAG_318, "diag_318", SCLP_FAC134, 0, "Control program name and version codes")

total: 1 errors, 1 warnings, 262 lines checked

Patch 8/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200515222032.18838-1-walling@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2856 bytes --]

On 5/16/20 12:20 AM, Collin Walling wrote:
> 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>

> ---
>  hw/s390x/sclp.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 61e2e2839c..2bd618515e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -75,6 +75,11 @@ 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) + cpu_count * 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 +88,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 +134,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>      int cpu_count;
>  
> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * 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);
>  
> -    if (be16_to_cpu(sccb->h.length) <
> -            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> -        return;
> -    }
> -
>      /* 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));
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Hildenbrand]

On 16.05.20 00:20, Collin Walling wrote:
> 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>
> ---
>  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 ede056b3ef..61e2e2839c 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);
> 

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

-- 
Thanks,

David / dhildenb

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 5712 bytes --]

On 5/16/20 12:20 AM, 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).
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
> +                                    SCCBHeader *header)
> +{
> +    uint64_t current_len = sccb_addr + be16_to_cpu(header->length);
> +    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;

Those are addresses not length indications and the names should reflect
that.
Also don't we need to use PAGE_SIZE - 1?

I'm still trying to wake up, so take this with a grain of salt.

> +
> +    switch (code & SCLP_CMD_CODE_MASK) {
> +    default:
> +        if (current_len <= allowed_len) {
> +            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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
> +{
> +    int required_len = data_len + 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;
> +}

Hm, from the function name alone I'd not have expected it to also set
the response code.

> +
>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>  {
>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> @@ -66,6 +94,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>      }
>  }
>  
> +/*
> + * The data length denotes the start of the struct to where the first
> + * CPU entry is to be allocated. This value also denotes the offset_cpu
> + * field.
> + */
> +static int get_read_scp_info_data_len(void)
> +{
> +    return offsetof(ReadInfo, entries);
> +}

Not sure what the policy for this is, but maybe this can go into a
header file?
David and Conny will surely make that clear to me :)

> +
>  /* Provide information about the configuration, CPUs and storage */
>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  {
> @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int cpu_count;
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
> +    int data_len = get_read_scp_info_data_len();
>  
> -    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);
> +    if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>          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(data_len);
>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
>  
>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
> @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
> +    int data_len = offsetof(ReadCpuInfo, entries);
>      int cpu_count;
>  
> -    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +    if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>          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(data_len);
>      cpu_info->nr_standby = cpu_to_be16(0);
>  
>      /* The standby offset is 16-byte for each CPU */
> @@ -227,6 +265,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> +    if (!sccb_has_valid_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,
> @@ -272,8 +314,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_has_valid_boundary(sccb, code, &work_sccb.h)) {
>          goto out_write;
>      }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 7074 bytes --]

On 5/16/20 12:20 AM, Collin Walling 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                     | 21 ++++++++++++++++++++-
>  include/hw/s390x/sclp.h             |  1 +
>  target/s390x/cpu_features_def.inc.h |  1 +
>  target/s390x/gen-features.c         |  1 +
>  target/s390x/kvm.c                  |  4 ++++
>  5 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 755f5f3fab..bde4c5420e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
>      uint64_t allowed_len = (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 RSCPI and RSCPU 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;
> +        }
>      default:
>          if (current_len <= allowed_len) {
>              return true;
> @@ -72,6 +84,10 @@ static bool sccb_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
>  
>      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;
> @@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>   */
>  static int get_read_scp_info_data_len(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 */
> @@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      CPUEntry *entries_start = (void *)sccb + data_len;
>  
>      if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
> +        warn_report("insufficient sccb size to store full read scp info response");
>          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 60db28351d..3548d65a69 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 69881a0da0..380fb81822 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2456,6 +2456,10 @@ 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 */
> +    set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
> +

We need to fence this for secure guests as the SIDA is only 4k at the
moment.

Do we need to take extra steps for migration safety?
I guess this is only available with host-passthrough or -model?

>      /* strip of features that are not part of the maximum model */
>      bitmap_and(model->features, model->features, model->def->full_feat,
>                 S390_FEAT_MAX);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Hildenbrand]

On 16.05.20 00:20, Collin Walling wrote:
> 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>
> ---
>  hw/s390x/sclp.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 61e2e2839c..2bd618515e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -75,6 +75,11 @@ 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) + cpu_count * sizeof(CPUEntry))) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +

(replied to v1 by mistake)

Lines too long.

Please run scripts/checkpatch.pl before submitting.

-- 
Thanks,

David / dhildenb

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

[Collin Walling]

On 5/18/20 4:55 AM, Janosch Frank wrote:
> On 5/16/20 12:20 AM, Collin Walling 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                     | 21 ++++++++++++++++++++-
>>  include/hw/s390x/sclp.h             |  1 +
>>  target/s390x/cpu_features_def.inc.h |  1 +
>>  target/s390x/gen-features.c         |  1 +
>>  target/s390x/kvm.c                  |  4 ++++
>>  5 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 755f5f3fab..bde4c5420e 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
>>      uint64_t allowed_len = (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 RSCPI and RSCPU 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;
>> +        }
>>      default:
>>          if (current_len <= allowed_len) {
>>              return true;
>> @@ -72,6 +84,10 @@ static bool sccb_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
>>  
>>      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;
>> @@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>   */
>>  static int get_read_scp_info_data_len(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 */
>> @@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>      CPUEntry *entries_start = (void *)sccb + data_len;
>>  
>>      if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>> +        warn_report("insufficient sccb size to store full read scp info response");
>>          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 60db28351d..3548d65a69 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 69881a0da0..380fb81822 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2456,6 +2456,10 @@ 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 */
>> +    set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>> +
> 
> We need to fence this for secure guests as the SIDA is only 4k at the
> moment.
> 

I don't know much about the SE stuff, so I'll take your word for it.
Should this follow the same fencing as diag 318 and simply check for pv
mode?

> Do we need to take extra steps for migration safety?
> I guess this is only available with host-passthrough or -model?
> 
>>      /* strip of features that are not part of the maximum model */
>>      bitmap_and(model->features, model->features, model->def->full_feat,
>>                 S390_FEAT_MAX);
>>
> 
> 


-- 
--
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 5/18/20 7:46 AM, David Hildenbrand wrote:
> On 16.05.20 00:20, Collin Walling wrote:
>> 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>
>> ---
>>  hw/s390x/sclp.c | 22 ++++++++++------------
>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 61e2e2839c..2bd618515e 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -75,6 +75,11 @@ 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) + cpu_count * sizeof(CPUEntry))) {
>> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>> +        return;
>> +    }
>> +
> 
> (replied to v1 by mistake)
> 
> Lines too long.
> 
> Please run scripts/checkpatch.pl before submitting.
> 

I do. The changes in this patch are replaced by the #3. I opted to be
sloppy here for ease of readability.

But if it's truly an issue I can clean it up for next round.

-- 
--
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 5/18/20 4:50 AM, Janosch Frank wrote:
> On 5/16/20 12:20 AM, 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).
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 49 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
>> +                                    SCCBHeader *header)
>> +{
>> +    uint64_t current_len = sccb_addr + be16_to_cpu(header->length);
>> +    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> 
> Those are addresses not length indications and the names should reflect
> that.

True

> Also don't we need to use PAGE_SIZE - 1?
> 

Technically we need to -1 on both sides since length denotes the size of
the sccb in bytes, not the max address.

How about this:

s/current_len/sccb_max_addr
s/allowed_len/sccb_boundary

-1 to sccb_max_addr

Change the check to: sccb_max_addr < sccb_boundary

?

> I'm still trying to wake up, so take this with a grain of salt.
> 

No worries. I appreciate the review nonetheless :)

>> +
>> +    switch (code & SCLP_CMD_CODE_MASK) {
>> +    default:
>> +        if (current_len <= allowed_len) {
>> +            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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
>> +{
>> +    int required_len = data_len + 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;
>> +}
> 
> Hm, from the function name alone I'd not have expected it to also set
> the response code.
> 

It also sets the required length in the header for an extended-length
sccb. Perhaps this function name doesn't hold up well.

Does sccb_check_sufficient_len make more sense?

I think the same could be said of the boundary check function, which
also sets the response code.

What about setting the response code outside the function, similar to
what sclp_comand_code_valid does?

>> +
>>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>  {
>>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
>> @@ -66,6 +94,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>      }
>>  }
>>  
>> +/*
>> + * The data length denotes the start of the struct to where the first
>> + * CPU entry is to be allocated. This value also denotes the offset_cpu
>> + * field.
>> + */
>> +static int get_read_scp_info_data_len(void)
>> +{
>> +    return offsetof(ReadInfo, entries);
>> +}
> 
> Not sure what the policy for this is, but maybe this can go into a
> header file?
> David and Conny will surely make that clear to me :)
> 

Not sure either. If anything it might be a good candidate for an inline
function.

>> +
>>  /* Provide information about the configuration, CPUs and storage */
>>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>  {
>> @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>      int cpu_count;
>>      int rnsize, rnmax;
>>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>> +    int data_len = get_read_scp_info_data_len();
>>  
>> -    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);
>> +    if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>>          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(data_len);
>>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
>>  
>>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
>> @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>>  {
>>      MachineState *machine = MACHINE(qdev_get_machine());
>>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>> +    int data_len = offsetof(ReadCpuInfo, entries);
>>      int cpu_count;
>>  
>> -    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
>> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>> +    if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>>          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(data_len);
>>      cpu_info->nr_standby = cpu_to_be16(0);
>>  
>>      /* The standby offset is 16-byte for each CPU */
>> @@ -227,6 +265,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>>          goto out_write;
>>      }
>>  
>> +    if (!sccb_has_valid_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,
>> @@ -272,8 +314,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_has_valid_boundary(sccb, code, &work_sccb.h)) {
>>          goto out_write;
>>      }
>>  
>>
> 
> 


-- 
--
Regards,
Collin

Stay safe and stay healthy

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

[David Hildenbrand]

On 18.05.20 16:32, Collin Walling wrote:
> On 5/18/20 7:46 AM, David Hildenbrand wrote:
>> On 16.05.20 00:20, Collin Walling wrote:
>>> 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>
>>> ---
>>>  hw/s390x/sclp.c | 22 ++++++++++------------
>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>> index 61e2e2839c..2bd618515e 100644
>>> --- a/hw/s390x/sclp.c
>>> +++ b/hw/s390x/sclp.c
>>> @@ -75,6 +75,11 @@ 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) + cpu_count * sizeof(CPUEntry))) {
>>> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>>> +        return;
>>> +    }
>>> +
>>
>> (replied to v1 by mistake)
>>
>> Lines too long.
>>
>> Please run scripts/checkpatch.pl before submitting.
>>
> 
> I do. The changes in this patch are replaced by the #3. I opted to be
> sloppy here for ease of readability.
> 
> But if it's truly an issue I can clean it up for next round.

No good reason to be sloppy and make checkpatch (+ David) complain ;)


-- 
Thanks,

David / dhildenb

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

[Collin Walling]

On 5/18/20 4:38 AM, David Hildenbrand wrote:
> On 16.05.20 00:20, Collin Walling wrote:
>> 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>
>> ---
>>  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 ede056b3ef..61e2e2839c 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);
>>
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 

Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 5/18/20 11:43 AM, David Hildenbrand wrote:
> On 18.05.20 16:32, Collin Walling wrote:
>> On 5/18/20 7:46 AM, David Hildenbrand wrote:
>>> On 16.05.20 00:20, Collin Walling wrote:
>>>> 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>
>>>> ---
>>>>  hw/s390x/sclp.c | 22 ++++++++++------------
>>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>>> index 61e2e2839c..2bd618515e 100644
>>>> --- a/hw/s390x/sclp.c
>>>> +++ b/hw/s390x/sclp.c
>>>> @@ -75,6 +75,11 @@ 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) + cpu_count * sizeof(CPUEntry))) {
>>>> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>> (replied to v1 by mistake)
>>>
>>> Lines too long.
>>>
>>> Please run scripts/checkpatch.pl before submitting.
>>>
>>
>> I do. The changes in this patch are replaced by the #3. I opted to be
>> sloppy here for ease of readability.
>>
>> But if it's truly an issue I can clean it up for next round.
> 
> No good reason to be sloppy and make checkpatch (+ David) complain ;)
> 
> 

Fair enough. I'll make the fix!

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 5/16/20 2:41 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200515222032.18838-1-walling@linux.ibm.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 20200515222032.18838-1-walling@linux.ibm.com
> Subject: [PATCH v2 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> f8cb821 s390: guest support for diagnose 0x318
> 6b87c59 s390/kvm: header sync for diag318
> af06627 s390/sclp: add extended-length sccb support for kvm guest
> 39b848c s390/sclp: use cpu offset to locate cpu entries
> 1dd8e02 s390/sclp: read sccb from mem based on sccb length
> aad956d s390/sclp: rework sclp boundary and length checks
> 428b1e4 s390/sclp: check sccb len before filling in data
> 850e1b8 s390/sclp: get machine once during read scp/cpu info
> 
> === OUTPUT BEGIN ===
> 1/8 Checking commit 850e1b88729f (s390/sclp: get machine once during read scp/cpu info)
> 2/8 Checking commit 428b1e46e016 (s390/sclp: check sccb len before filling in data)
> WARNING: line over 80 characters
> #23: FILE: hw/s390x/sclp.c:78:
> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
> 
> ERROR: line over 90 characters
> #48: FILE: hw/s390x/sclp.c:137:
> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> 
> total: 1 errors, 1 warnings, 45 lines checked
> 
> Patch 2/8 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 3/8 Checking commit aad956d5ac92 (s390/sclp: rework sclp boundary and length checks)
> 4/8 Checking commit 1dd8e02af7b2 (s390/sclp: read sccb from mem based on sccb length)
> 5/8 Checking commit 39b848c3be15 (s390/sclp: use cpu offset to locate cpu entries)
> 6/8 Checking commit af06627cc5fb (s390/sclp: add extended-length sccb support for kvm guest)
> WARNING: line over 80 characters
> #91: FILE: hw/s390x/sclp.c:137:
> +        warn_report("insufficient sccb size to store full read scp info response");
> 
> WARNING: line over 80 characters
> #115: FILE: target/s390x/cpu_features_def.inc.h:100:
> +DEF_FEAT(EXTENDED_LENGTH_SCCB, "els", STFL, 140, "Extended-length SCCB facility")
> 
> total: 0 errors, 2 warnings, 76 lines checked
> 
> Patch 6/8 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 7/8 Checking commit 6b87c5992768 (s390/kvm: header sync for diag318)
> 8/8 Checking commit f8cb821134a7 (s390: guest support for diagnose 0x318)
> ERROR: line over 90 characters
> #226: FILE: target/s390x/cpu_features_def.inc.h:125:
> +/* Features exposed via SCLP SCCB Facilities byte 134 (bit numbers relative to byte-134) */
> 
> WARNING: line over 80 characters
> #227: FILE: target/s390x/cpu_features_def.inc.h:126:
> +DEF_FEAT(DIAG_318, "diag_318", SCLP_FAC134, 0, "Control program name and version codes")
> 
> total: 1 errors, 1 warnings, 262 lines checked
> 
> Patch 8/8 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> The full log is available at
> http://patchew.org/logs/20200515222032.18838-1-walling@linux.ibm.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 

I'll have these fixed for next round, but there's not much I can do with
regards to the styling in cpu_features_def.inc.h :/

-- 
Regards,
Collin

Stay safe and stay healthy

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

[David Hildenbrand]

> Am 18.05.2020 um 19:34 schrieb Collin Walling <walling@linux.ibm.com>:
> 
> On 5/16/20 2:41 AM, no-reply@patchew.org wrote:
>> Patchew URL: https://patchew.org/QEMU/20200515222032.18838-1-walling@linux.ibm.com/
>> 
>> 
>> 
>> Hi,
>> 
>> This series seems to have some coding style problems. See output below for
>> more information:
>> 
>> Message-id: 20200515222032.18838-1-walling@linux.ibm.com
>> Subject: [PATCH v2 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318
>> Type: series
>> 
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>> git rev-parse base > /dev/null || exit 0
>> git config --local diff.renamelimit 0
>> git config --local diff.renames True
>> git config --local diff.algorithm histogram
>> ./scripts/checkpatch.pl --mailback base..
>> === TEST SCRIPT END ===
>> 
>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>> Switched to a new branch 'test'
>> f8cb821 s390: guest support for diagnose 0x318
>> 6b87c59 s390/kvm: header sync for diag318
>> af06627 s390/sclp: add extended-length sccb support for kvm guest
>> 39b848c s390/sclp: use cpu offset to locate cpu entries
>> 1dd8e02 s390/sclp: read sccb from mem based on sccb length
>> aad956d s390/sclp: rework sclp boundary and length checks
>> 428b1e4 s390/sclp: check sccb len before filling in data
>> 850e1b8 s390/sclp: get machine once during read scp/cpu info
>> 
>> === OUTPUT BEGIN ===
>> 1/8 Checking commit 850e1b88729f (s390/sclp: get machine once during read scp/cpu info)
>> 2/8 Checking commit 428b1e46e016 (s390/sclp: check sccb len before filling in data)
>> WARNING: line over 80 characters
>> #23: FILE: hw/s390x/sclp.c:78:
>> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
>> 
>> ERROR: line over 90 characters
>> #48: FILE: hw/s390x/sclp.c:137:
>> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
>> 
>> total: 1 errors, 1 warnings, 45 lines checked
>> 
>> Patch 2/8 has style problems, please review.  If any of these errors
>> are false positives report them to the maintainer, see
>> CHECKPATCH in MAINTAINERS.
>> 
>> 3/8 Checking commit aad956d5ac92 (s390/sclp: rework sclp boundary and length checks)
>> 4/8 Checking commit 1dd8e02af7b2 (s390/sclp: read sccb from mem based on sccb length)
>> 5/8 Checking commit 39b848c3be15 (s390/sclp: use cpu offset to locate cpu entries)
>> 6/8 Checking commit af06627cc5fb (s390/sclp: add extended-length sccb support for kvm guest)
>> WARNING: line over 80 characters
>> #91: FILE: hw/s390x/sclp.c:137:
>> +        warn_report("insufficient sccb size to store full read scp info response");
>> 
>> WARNING: line over 80 characters
>> #115: FILE: target/s390x/cpu_features_def.inc.h:100:
>> +DEF_FEAT(EXTENDED_LENGTH_SCCB, "els", STFL, 140, "Extended-length SCCB facility")
>> 
>> total: 0 errors, 2 warnings, 76 lines checked
>> 
>> Patch 6/8 has style problems, please review.  If any of these errors
>> are false positives report them to the maintainer, see
>> CHECKPATCH in MAINTAINERS.
>> 7/8 Checking commit 6b87c5992768 (s390/kvm: header sync for diag318)
>> 8/8 Checking commit f8cb821134a7 (s390: guest support for diagnose 0x318)
>> ERROR: line over 90 characters
>> #226: FILE: target/s390x/cpu_features_def.inc.h:125:
>> +/* Features exposed via SCLP SCCB Facilities byte 134 (bit numbers relative to byte-134) */
>> 
>> WARNING: line over 80 characters
>> #227: FILE: target/s390x/cpu_features_def.inc.h:126:
>> +DEF_FEAT(DIAG_318, "diag_318", SCLP_FAC134, 0, "Control program name and version codes")
>> 
>> total: 1 errors, 1 warnings, 262 lines checked
>> 
>> Patch 8/8 has style problems, please review.  If any of these errors
>> are false positives report them to the maintainer, see
>> CHECKPATCH in MAINTAINERS.
>> 
>> === OUTPUT END ===
>> 
>> Test command exited with code: 1
>> 
>> 
>> The full log is available at
>> http://patchew.org/logs/20200515222032.18838-1-walling@linux.ibm.com/testing.checkpatch/?type=message.
>> ---
>> Email generated automatically by Patchew [https://patchew.org/].
>> Please send your feedback to patchew-devel@redhat.com
>> 
> 
> I'll have these fixed for next round, but there's not much I can do with
> regards to the styling in cpu_features_def.inc.h :/

They are perfectly fine :)

> 
> -- 
> Regards,
> Collin
> 
> Stay safe and stay healthy
> 

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

[Cornelia Huck]

On Mon, 18 May 2020 11:15:07 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 5/18/20 4:50 AM, Janosch Frank wrote:
> > On 5/16/20 12:20 AM, 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).
> >>
> >> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> >> ---
> >>  hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 49 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> >> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
> >> +                                    SCCBHeader *header)
> >> +{
> >> +    uint64_t current_len = sccb_addr + be16_to_cpu(header->length);
> >> +    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;  
> > 
> > Those are addresses not length indications and the names should reflect
> > that.  
> 
> True
> 
> > Also don't we need to use PAGE_SIZE - 1?
> >   
> 
> Technically we need to -1 on both sides since length denotes the size of
> the sccb in bytes, not the max address.
> 
> How about this:
> 
> s/current_len/sccb_max_addr
> s/allowed_len/sccb_boundary

+1, like the names.

> 
> -1 to sccb_max_addr
> 
> Change the check to: sccb_max_addr < sccb_boundary
> 
> ?
> 
> > I'm still trying to wake up, so take this with a grain of salt.
> >   
> 
> No worries. I appreciate the review nonetheless :)
> 
> >> +
> >> +    switch (code & SCLP_CMD_CODE_MASK) {
> >> +    default:
> >> +        if (current_len <= allowed_len) {
> >> +            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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
> >> +{
> >> +    int required_len = data_len + 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;
> >> +}  
> > 
> > Hm, from the function name alone I'd not have expected it to also set
> > the response code.
> >   
> 
> It also sets the required length in the header for an extended-length
> sccb. Perhaps this function name doesn't hold up well.
> 
> Does sccb_check_sufficient_len make more sense?

To me it does.

> 
> I think the same could be said of the boundary check function, which
> also sets the response code.
> 
> What about setting the response code outside the function, similar to
> what sclp_comand_code_valid does?

Whatever results in the least code churn to make it consistent ;)

> 
> >> +
> >>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
> >>  {
> >>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> >> @@ -66,6 +94,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
> >>      }
> >>  }
> >>  
> >> +/*
> >> + * The data length denotes the start of the struct to where the first
> >> + * CPU entry is to be allocated. This value also denotes the offset_cpu
> >> + * field.
> >> + */
> >> +static int get_read_scp_info_data_len(void)
> >> +{
> >> +    return offsetof(ReadInfo, entries);
> >> +}  
> > 
> > Not sure what the policy for this is, but maybe this can go into a
> > header file?
> > David and Conny will surely make that clear to me :)
> >   
> 
> Not sure either. If anything it might be a good candidate for an inline
> function.

If we don't process read info outside of this file, no need to move it
to a header. The compiler is probably also smart enough to inline it on
its own, I guess.

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

[Cornelia Huck]

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

On Mon, 18 May 2020 10:55:24 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 5/16/20 12:20 AM, Collin Walling 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                     | 21 ++++++++++++++++++++-
> >  include/hw/s390x/sclp.h             |  1 +
> >  target/s390x/cpu_features_def.inc.h |  1 +
> >  target/s390x/gen-features.c         |  1 +
> >  target/s390x/kvm.c                  |  4 ++++
> >  5 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> > index 755f5f3fab..bde4c5420e 100644
> > --- a/hw/s390x/sclp.c
> > +++ b/hw/s390x/sclp.c
> > @@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
> >      uint64_t allowed_len = (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 RSCPI and RSCPU and is

Nit: I had to stare at this for a bit before I figured out what the
acronyms refer to.

> > +         * 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;
> > +        }
> >      default:
> >          if (current_len <= allowed_len) {
> >              return true;

(...)

> > 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 69881a0da0..380fb81822 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2456,6 +2456,10 @@ 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 */
> > +    set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
> > +  
> 
> We need to fence this for secure guests as the SIDA is only 4k at the
> moment.
> 
> Do we need to take extra steps for migration safety?
> I guess this is only available with host-passthrough or -model?

What do you mean with '-model'? I think it can be added manually
everywhere? But I'm always a bit confused by cpu models.

> 
> >      /* strip of features that are not part of the maximum model */
> >      bitmap_and(model->features, model->features, model->def->full_feat,
> >                 S390_FEAT_MAX);
> >   
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Cornelia Huck]

On Fri, 15 May 2020 18:20:32 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> DIAGNOSE 0x318 (diag 318) allows the storage of diagnostic data that
> is collected by the firmware in the case of hardware/firmware service
> events.
> 
> The instruction is invoked in the Linux kernel and is handled,
> migrated, and reset (modified clear and load normal) by QEMU.
> KVM assists with the get/set of the relevent data via IOCTLs.
> 
> 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 diag 318
> 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.

I think it should be easy enough to wire it up for !kvm as well
(although I'm not sure how useful it would be there -- mostly for
seeing what a guest does with it, I guess.)

> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c          | 45 +++++++++++++++++++++++++++++
>  hw/s390x/sclp.c                     |  5 ++++
>  include/hw/s390x/s390-virtio-ccw.h  |  1 +
>  include/hw/s390x/sclp.h             |  3 ++
>  target/s390x/cpu.c                  | 23 +++++++++++++++
>  target/s390x/cpu.h                  |  4 +++
>  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-stub.c             | 10 +++++++
>  target/s390x/kvm.c                  | 40 +++++++++++++++++++++++++
>  target/s390x/kvm_s390x.h            |  2 ++
>  13 files changed, 139 insertions(+)
> 

(...)

> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index f2ccf0a06a..367a54c173 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -446,6 +446,29 @@ void s390_enable_css_support(S390CPU *cpu)
>          kvm_s390_enable_css_support(cpu);
>      }
>  }
> +
> +void s390_get_diag_318_info(uint64_t *info)
> +{
> +    if (kvm_enabled()) {
> +        kvm_s390_get_diag_318_info(info);
> +    }
> +}
> +
> +void s390_set_diag_318_info(uint64_t info)
> +{
> +    if (kvm_enabled()) {
> +        kvm_s390_set_diag_318_info(info);
> +    }
> +}
> +
> +bool s390_diag_318_is_allowed(void)
> +{
> +    if (kvm_enabled()) {

I would recommend not tying this explicitly to kvm -- assuming that the
feature checks should be enough.

> +        return s390_has_feat(S390_FEAT_DIAG_318) &&
> +               s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB);
> +    }
> +    return false;
> +}
>  #endif
>  
>  static gchar *s390_gdb_arch_name(CPUState *cs)

(...)

> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 380fb81822..5d7dc60c85 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
>  
> @@ -814,6 +815,28 @@ int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_low)
>      return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>  }
>  
> +int kvm_s390_get_diag_318_info(uint64_t *info)
> +{
> +    struct kvm_device_attr attr = {
> +        .group = KVM_S390_VM_MISC,
> +        .attr = KVM_S390_VM_MISC_DIAG_318,
> +        .addr = (uint64_t)info,
> +    };
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
> +}
> +
> +int kvm_s390_set_diag_318_info(uint64_t info)
> +{
> +    struct kvm_device_attr attr = {
> +        .group = KVM_S390_VM_MISC,
> +        .attr = KVM_S390_VM_MISC_DIAG_318,
> +        .addr = (uint64_t)&info,
> +    };
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
> +}
> +
>  /**
>   * kvm_s390_mem_op:
>   * @addr:      the logical start address in guest memory
> @@ -1601,6 +1624,14 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct kvm_run *run)
>      return -ENOENT;
>  }
>  
> +static void kvm_handle_diag_318(struct kvm_run *run)
> +{
> +    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
> +    uint64_t info = run->s.regs.gprs[reg];
> +
> +    kvm_s390_set_diag_318_info(info);

Follow the other diag handlers and rather call a common
handle_diag_318() function, which will in turn call
s390_set_diag_318_info()? While that feels like a bit of extra churn at
a glance, it allows non-kvm access to this diag more easily.

> +}
> +
>  #define DIAG_KVM_CODE_MASK 0x000000000000ffff
>  
>  static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
> @@ -1620,6 +1651,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:
> +        kvm_handle_diag_318(run);
> +        break;
>      case DIAG_KVM_HYPERCALL:
>          r = handle_hypercall(cpu, run);
>          break;
> @@ -2460,6 +2494,12 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>      /* Extended-Length SCCB is handled entirely within QEMU */
>      set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>  
> +    /* Allow diag 0x318 iff KVM supported and not in PV mode */

"iff supported by KVM" ?

> +    if (!s390_is_pv() && kvm_vm_check_attr(kvm_state,
> +        KVM_S390_VM_MISC, KVM_S390_VM_MISC_DIAG_318)) {
> +        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);

(...)

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

[Collin Walling]

On 5/20/20 7:30 AM, Cornelia Huck wrote:
> On Fri, 15 May 2020 18:20:32 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> DIAGNOSE 0x318 (diag 318) allows the storage of diagnostic data that
>> is collected by the firmware in the case of hardware/firmware service
>> events.
>>
>> The instruction is invoked in the Linux kernel and is handled,
>> migrated, and reset (modified clear and load normal) by QEMU.
>> KVM assists with the get/set of the relevent data via IOCTLs.
>>
>> 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 diag 318
>> 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.
> 
> I think it should be easy enough to wire it up for !kvm as well
> (although I'm not sure how useful it would be there -- mostly for
> seeing what a guest does with it, I guess.)
> 

I suppose we could. I'm working with the sync_regs idea that was
mentioned, and if it works the way I think it does, a lot the code on
the QEMU side will be disappearing.

Otherwise, I'll look into this. It should be rather harmless if QEMU
holds onto the data, but doesn't do anything with it.

>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c          | 45 +++++++++++++++++++++++++++++
>>  hw/s390x/sclp.c                     |  5 ++++
>>  include/hw/s390x/s390-virtio-ccw.h  |  1 +
>>  include/hw/s390x/sclp.h             |  3 ++
>>  target/s390x/cpu.c                  | 23 +++++++++++++++
>>  target/s390x/cpu.h                  |  4 +++
>>  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-stub.c             | 10 +++++++
>>  target/s390x/kvm.c                  | 40 +++++++++++++++++++++++++
>>  target/s390x/kvm_s390x.h            |  2 ++
>>  13 files changed, 139 insertions(+)
>>
> 
> (...)
> 
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index f2ccf0a06a..367a54c173 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -446,6 +446,29 @@ void s390_enable_css_support(S390CPU *cpu)
>>          kvm_s390_enable_css_support(cpu);
>>      }
>>  }
>> +
>> +void s390_get_diag_318_info(uint64_t *info)
>> +{
>> +    if (kvm_enabled()) {
>> +        kvm_s390_get_diag_318_info(info);
>> +    }
>> +}
>> +
>> +void s390_set_diag_318_info(uint64_t info)
>> +{
>> +    if (kvm_enabled()) {
>> +        kvm_s390_set_diag_318_info(info);
>> +    }
>> +}
>> +
>> +bool s390_diag_318_is_allowed(void)
>> +{
>> +    if (kvm_enabled()) {
> 
> I would recommend not tying this explicitly to kvm -- assuming that the
> feature checks should be enough.
> 

True. If I understand how the sync_regs approach works, then vmstate
stuff should be going away, and the reset code will also be handled in KVM.

Otherwise, I'll make the change.

>> +        return s390_has_feat(S390_FEAT_DIAG_318) &&
>> +               s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB);
>> +    }
>> +    return false;
>> +}
>>  #endif
>>  
>>  static gchar *s390_gdb_arch_name(CPUState *cs)
> 
> (...)
> 
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 380fb81822..5d7dc60c85 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
>>  
>> @@ -814,6 +815,28 @@ int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_low)
>>      return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>>  }
>>  
>> +int kvm_s390_get_diag_318_info(uint64_t *info)
>> +{
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_S390_VM_MISC,
>> +        .attr = KVM_S390_VM_MISC_DIAG_318,
>> +        .addr = (uint64_t)info,
>> +    };
>> +
>> +    return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
>> +}
>> +
>> +int kvm_s390_set_diag_318_info(uint64_t info)
>> +{
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_S390_VM_MISC,
>> +        .attr = KVM_S390_VM_MISC_DIAG_318,
>> +        .addr = (uint64_t)&info,
>> +    };
>> +
>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>> +}
>> +
>>  /**
>>   * kvm_s390_mem_op:
>>   * @addr:      the logical start address in guest memory
>> @@ -1601,6 +1624,14 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct kvm_run *run)
>>      return -ENOENT;
>>  }
>>  
>> +static void kvm_handle_diag_318(struct kvm_run *run)
>> +{
>> +    uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
>> +    uint64_t info = run->s.regs.gprs[reg];
>> +
>> +    kvm_s390_set_diag_318_info(info);
> 
> Follow the other diag handlers and rather call a common
> handle_diag_318() function, which will in turn call
> s390_set_diag_318_info()? While that feels like a bit of extra churn at
> a glance, it allows non-kvm access to this diag more easily.
> 

Can do.

>> +}
>> +
>>  #define DIAG_KVM_CODE_MASK 0x000000000000ffff
>>  
>>  static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
>> @@ -1620,6 +1651,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:
>> +        kvm_handle_diag_318(run);
>> +        break;
>>      case DIAG_KVM_HYPERCALL:
>>          r = handle_hypercall(cpu, run);
>>          break;
>> @@ -2460,6 +2494,12 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>      /* Extended-Length SCCB is handled entirely within QEMU */
>>      set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>>  
>> +    /* Allow diag 0x318 iff KVM supported and not in PV mode */
> 
> "iff supported by KVM" ?
>

That sounds a bit better :)


>> +    if (!s390_is_pv() && kvm_vm_check_attr(kvm_state,
>> +        KVM_S390_VM_MISC, KVM_S390_VM_MISC_DIAG_318)) {
>> +        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);
> 
> (...)
> 
> 

Thanks for the review!

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 8125 bytes --]

On 5/18/20 4:31 PM, Collin Walling wrote:
> On 5/18/20 4:55 AM, Janosch Frank wrote:
>> On 5/16/20 12:20 AM, Collin Walling 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                     | 21 ++++++++++++++++++++-
>>>  include/hw/s390x/sclp.h             |  1 +
>>>  target/s390x/cpu_features_def.inc.h |  1 +
>>>  target/s390x/gen-features.c         |  1 +
>>>  target/s390x/kvm.c                  |  4 ++++
>>>  5 files changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>> index 755f5f3fab..bde4c5420e 100644
>>> --- a/hw/s390x/sclp.c
>>> +++ b/hw/s390x/sclp.c
>>> @@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
>>>      uint64_t allowed_len = (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 RSCPI and RSCPU 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;
>>> +        }
>>>      default:
>>>          if (current_len <= allowed_len) {
>>>              return true;
>>> @@ -72,6 +84,10 @@ static bool sccb_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
>>>  
>>>      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;
>>> @@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>>   */
>>>  static int get_read_scp_info_data_len(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 */
>>> @@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>>      CPUEntry *entries_start = (void *)sccb + data_len;
>>>  
>>>      if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>>> +        warn_report("insufficient sccb size to store full read scp info response");
>>>          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 60db28351d..3548d65a69 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 69881a0da0..380fb81822 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2456,6 +2456,10 @@ 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 */
>>> +    set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>>> +
>>
>> We need to fence this for secure guests as the SIDA is only 4k at the
>> moment.
>>
> 
> I don't know much about the SE stuff, so I'll take your word for it.
> Should this follow the same fencing as diag 318 and simply check for pv
> mode?

So I had another look into it and I take everything back I said:
* The stfle bit is controlled by the UV, so it's not indicated to the
guest in PV mode
* If the guest tries to execute a long SCCB the UV will return a
boundary violation error on its own (well after a notification exit anyway).

Let's therefore throw in a comment:
For PV guests this is completely fenced by the Ultravisor as Service
Call error checking and STFLE interpretation are handled by SIE.


> 
>> Do we need to take extra steps for migration safety?
>> I guess this is only available with host-passthrough or -model?
>>
>>>      /* strip of features that are not part of the maximum model */
>>>      bitmap_and(model->features, model->features, model->def->full_feat,
>>>                 S390_FEAT_MAX);
>>>
>>
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 4557 bytes --]

On 5/19/20 3:19 PM, Cornelia Huck wrote:
> On Mon, 18 May 2020 11:15:07 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> On 5/18/20 4:50 AM, Janosch Frank wrote:
>>> On 5/16/20 12:20 AM, 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).
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>>  hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++-------
>>>>  1 file changed, 49 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>>> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
>>>> +                                    SCCBHeader *header)
>>>> +{
>>>> +    uint64_t current_len = sccb_addr + be16_to_cpu(header->length);
>>>> +    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;  
>>>
>>> Those are addresses not length indications and the names should reflect
>>> that.  
>>
>> True
>>
>>> Also don't we need to use PAGE_SIZE - 1?
>>>   
>>
>> Technically we need to -1 on both sides since length denotes the size of
>> the sccb in bytes, not the max address.
>>
>> How about this:
>>
>> s/current_len/sccb_max_addr
>> s/allowed_len/sccb_boundary
> 
> +1, like the names.
> 
>>
>> -1 to sccb_max_addr
>>
>> Change the check to: sccb_max_addr < sccb_boundary
>>
>> ?
>>
>>> I'm still trying to wake up, so take this with a grain of salt.
>>>   
>>
>> No worries. I appreciate the review nonetheless :)
>>
>>>> +
>>>> +    switch (code & SCLP_CMD_CODE_MASK) {
>>>> +    default:
>>>> +        if (current_len <= allowed_len) {
>>>> +            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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
>>>> +{
>>>> +    int required_len = data_len + 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;
>>>> +}  
>>>
>>> Hm, from the function name alone I'd not have expected it to also set
>>> the response code.
>>>   
>>
>> It also sets the required length in the header for an extended-length
>> sccb. Perhaps this function name doesn't hold up well.
>>
>> Does sccb_check_sufficient_len make more sense?
> 
> To me it does.
> 
>>
>> I think the same could be said of the boundary check function, which
>> also sets the response code.
>>
>> What about setting the response code outside the function, similar to
>> what sclp_comand_code_valid does?
> 
> Whatever results in the least code churn to make it consistent ;)
> 
>>
>>>> +
>>>>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>>>  {
>>>>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
>>>> @@ -66,6 +94,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>>>      }
>>>>  }
>>>>  
>>>> +/*
>>>> + * The data length denotes the start of the struct to where the first
>>>> + * CPU entry is to be allocated. This value also denotes the offset_cpu
>>>> + * field.
>>>> + */
>>>> +static int get_read_scp_info_data_len(void)
>>>> +{
>>>> +    return offsetof(ReadInfo, entries);
>>>> +}  
>>>
>>> Not sure what the policy for this is, but maybe this can go into a
>>> header file?
>>> David and Conny will surely make that clear to me :)
>>>   
>>
>> Not sure either. If anything it might be a good candidate for an inline
>> function.
> 
> If we don't process read info outside of this file, no need to move it
> to a header. The compiler is probably also smart enough to inline it on
> its own, I guess.
> 
> 

I'm also ok with the names and the rest



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Collin Walling]

On 5/25/20 6:50 AM, Janosch Frank wrote:
> On 5/18/20 4:31 PM, Collin Walling wrote:
>> On 5/18/20 4:55 AM, Janosch Frank wrote:
>>> On 5/16/20 12:20 AM, Collin Walling 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                     | 21 ++++++++++++++++++++-
>>>>  include/hw/s390x/sclp.h             |  1 +
>>>>  target/s390x/cpu_features_def.inc.h |  1 +
>>>>  target/s390x/gen-features.c         |  1 +
>>>>  target/s390x/kvm.c                  |  4 ++++
>>>>  5 files changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>>> index 755f5f3fab..bde4c5420e 100644
>>>> --- a/hw/s390x/sclp.c
>>>> +++ b/hw/s390x/sclp.c
>>>> @@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
>>>>      uint64_t allowed_len = (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 RSCPI and RSCPU 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;
>>>> +        }
>>>>      default:
>>>>          if (current_len <= allowed_len) {
>>>>              return true;
>>>> @@ -72,6 +84,10 @@ static bool sccb_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
>>>>  
>>>>      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;
>>>> @@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>>>   */
>>>>  static int get_read_scp_info_data_len(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 */
>>>> @@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>>>      CPUEntry *entries_start = (void *)sccb + data_len;
>>>>  
>>>>      if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>>>> +        warn_report("insufficient sccb size to store full read scp info response");
>>>>          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 60db28351d..3548d65a69 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 69881a0da0..380fb81822 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -2456,6 +2456,10 @@ 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 */
>>>> +    set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>>>> +
>>>
>>> We need to fence this for secure guests as the SIDA is only 4k at the
>>> moment.
>>>
>>
>> I don't know much about the SE stuff, so I'll take your word for it.
>> Should this follow the same fencing as diag 318 and simply check for pv
>> mode?
> 
> So I had another look into it and I take everything back I said:
> * The stfle bit is controlled by the UV, so it's not indicated to the
> guest in PV mode
> * If the guest tries to execute a long SCCB the UV will return a
> boundary violation error on its own (well after a notification exit anyway).
> 
> Let's therefore throw in a comment:
> For PV guests this is completely fenced by the Ultravisor as Service
> Call error checking and STFLE interpretation are handled by SIE.
> 
> 

Very cool. This is a much easier approach. Thanks for looking into it!

>>
>>> Do we need to take extra steps for migration safety?
>>> I guess this is only available with host-passthrough or -model?
>>>
>>>>      /* strip of features that are not part of the maximum model */
>>>>      bitmap_and(model->features, model->features, model->def->full_feat,
>>>>                 S390_FEAT_MAX);
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Regards,
Collin

Stay safe and stay healthy

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

[Thomas Huth]

On 16/05/2020 00.20, Collin Walling wrote:
> 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>
> ---
>  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 ede056b3ef..61e2e2839c 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);
> 

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

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

[Thomas Huth]

On 16/05/2020 00.20, Collin Walling wrote:
> 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>
> ---
>  hw/s390x/sclp.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 61e2e2839c..2bd618515e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -75,6 +75,11 @@ 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) + cpu_count * sizeof(CPUEntry))) {

You are using cpu_count here ...

> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +
>      /* CPU information */
>      prepare_cpu_entries(machine, read_info->entries, &cpu_count);

... but it is only initialized here.

Even if you replace the code in a later patch, please fix this here,
too, to maintain bisectability of the code.

>      read_info->entries_cpu = cpu_to_be16(cpu_count);
> @@ -83,12 +88,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 +134,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>      int cpu_count;
>  
> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {

dito!

> +        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);
>  
> -    if (be16_to_cpu(sccb->h.length) <
> -            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> -        return;
> -    }
> -
>      /* 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));

 Thanks,
  Thomas

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

[Thomas Huth]

On 16/05/2020 00.20, 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).
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
> +                                    SCCBHeader *header)
> +{
> +    uint64_t current_len = sccb_addr + be16_to_cpu(header->length);
> +    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> +
> +    switch (code & SCLP_CMD_CODE_MASK) {
> +    default:
> +        if (current_len <= allowed_len) {
> +            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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
> +{
> +    int required_len = data_len + 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,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>      }
>  }
>  
> +/*
> + * The data length denotes the start of the struct to where the first
> + * CPU entry is to be allocated. This value also denotes the offset_cpu
> + * field.
> + */
> +static int get_read_scp_info_data_len(void)
> +{
> +    return offsetof(ReadInfo, entries);
> +}
> +
>  /* Provide information about the configuration, CPUs and storage */
>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  {
> @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int cpu_count;
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
> +    int data_len = get_read_scp_info_data_len();
>  
> -    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);
> +    if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>          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(data_len);
>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
>  
>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
> @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
> +    int data_len = offsetof(ReadCpuInfo, entries);

Is there a reason for not using get_read_scp_info_data_len() here?

 Thomas

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

[Thomas Huth]

On 16/05/2020 00.20, Collin Walling wrote:
> The header 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>
> ---
>  hw/s390x/sclp.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

With the proper checks from the previous patch in place:

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

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

[Thomas Huth]

On 16/05/2020 00.20, Collin Walling wrote:
> 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>
> ---
>  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 5d6e98ae64..755f5f3fab 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -113,13 +113,14 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>      int data_len = get_read_scp_info_data_len();
> +    CPUEntry *entries_start = (void *)sccb + data_len;
>  
>      if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>          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(data_len);
>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
> 

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

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

[Collin Walling]

On 6/11/20 8:56 AM, Thomas Huth wrote:
> On 16/05/2020 00.20, 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).
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c | 57 ++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 49 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 2bd618515e..987699e3c4 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_has_valid_boundary(uint64_t sccb_addr, uint32_t code,
>> +                                    SCCBHeader *header)
>> +{
>> +    uint64_t current_len = sccb_addr + be16_to_cpu(header->length);
>> +    uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>> +
>> +    switch (code & SCLP_CMD_CODE_MASK) {
>> +    default:
>> +        if (current_len <= allowed_len) {
>> +            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_has_sufficient_len(SCCB *sccb, int num_cpus, int data_len)
>> +{
>> +    int required_len = data_len + 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,16 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>>      }
>>  }
>>  
>> +/*
>> + * The data length denotes the start of the struct to where the first
>> + * CPU entry is to be allocated. This value also denotes the offset_cpu
>> + * field.
>> + */
>> +static int get_read_scp_info_data_len(void)
>> +{
>> +    return offsetof(ReadInfo, entries);
>> +}
>> +
>>  /* Provide information about the configuration, CPUs and storage */
>>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>  {
>> @@ -74,16 +112,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>      int cpu_count;
>>      int rnsize, rnmax;
>>      IplParameterBlock *ipib = s390_ipl_get_iplb();
>> +    int data_len = get_read_scp_info_data_len();
>>  
>> -    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);
>> +    if (!sccb_has_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
>>          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(data_len);
>>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
>>  
>>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
>> @@ -132,16 +170,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>>  {
>>      MachineState *machine = MACHINE(qdev_get_machine());
>>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>> +    int data_len = offsetof(ReadCpuInfo, entries);
> 
> Is there a reason for not using get_read_scp_info_data_len() here?
> 
>  Thomas
> 
> 

That function is for Read SCP Info.

Read CPU Info does not face the complications that come with new
features intruding on the space used for CPU entries (thankfully), so
there's no need for a function to determine its data length.

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Collin Walling]

On 6/11/20 8:01 AM, Thomas Huth wrote:
> On 16/05/2020 00.20, Collin Walling wrote:
>> 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>
>> ---
>>  hw/s390x/sclp.c | 22 ++++++++++------------
>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 61e2e2839c..2bd618515e 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -75,6 +75,11 @@ 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) + cpu_count * sizeof(CPUEntry))) {
> 
> You are using cpu_count here ...
> 
>> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>> +        return;
>> +    }
>> +
>>      /* CPU information */
>>      prepare_cpu_entries(machine, read_info->entries, &cpu_count);
> 
> ... but it is only initialized here.
> 
> Even if you replace the code in a later patch, please fix this here,
> too, to maintain bisectability of the code.
> 
>>      read_info->entries_cpu = cpu_to_be16(cpu_count);
>> @@ -83,12 +88,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 +134,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>>      int cpu_count;
>>  
>> +    if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> 
> dito!
> 
>> +        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);
>>  
>> -    if (be16_to_cpu(sccb->h.length) <
>> -            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
>> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>> -        return;
>> -    }
>> -
>>      /* 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));
> 
>  Thanks,
>   Thomas
> 

I'll rework this patch. Thanks for the reviews!

-- 
Regards,
Collin

Stay safe and stay healthy